Segmentation Fault when writing to a string [duplicate]

Question

I am trying to write an in-place reverse function and have followed online code pretty much exactly, yet running the following program throws a bus error. Am I passing the wrong kind of argument to reverse()?

void reverse(char *str) {
    char * end = str;
    char tmp;
    if (str) {
        while (*end) {
            ++end;
        }
        --end;
        while (str < end) {
            tmp = *str;
            *str++ = *end;
            *end-- = tmp;
        }
    }
}

int main() {
    char *s = "sample";
    reverse(s);
    printf("%s\n");
    return 1;
}

Zellner

I'm fairly certain you meant printf("%s\n", s);. – Ethnarch

You should have written const char *s = "sample";. Got it? Do you know why string constants are called constants? No? Better google it. – Nautilus

Answer 1

To know what is happening, you have to understand the memory layout of a C program.

char *s = "sample";    // Here the "sample" string is placed in 
                        // the read only memory of the Initialized Data segment. 

Here, you cannot modify the data. "s" is a pointer to a char const("sample") and you are trying to modify the char const. That is why you are getting the bus error error.

                        |Stack frame of main()          |
                        |char *s                        |
                        |-------------------------------|
                        |Stack frame of reverse()       |
                        |char *end                      |
                        |char tmp                       |
                        |                               |
                        |-------------------------------|
                        |                               |
                        |                               |
                        |                               |
                        |                               |
                        |                               |
                        |-------------------------------|
                        |                               |
                        |           HEAP                |
                        |                               |
                        |-------------------------------|
                        |                               |
                        |   UNINITIALIZED DATA (BSS)    |
                        |                               |
                        |-------------------------------|
                        |                               |
                        |      INITIALIZED DATA         |
                        |                               |
                        |"sample"   |                   |
                        |           |                   |
                        |(Read Only)| (Read/Write)      |
                        |-------------------------------|
                        |   Text or Code Segment        |
                        |                               |
                        |-------------------------------|

UPDATE Below post is not related to your question. But if you know where are the memory allocated for all the variables in C, then you can code better. The below program gives a better understanding of the memory layout of a C Program. I have not included the command line arguments, function argument and return values of function in the diagram. People who want to update this post can add the command line arguments, function argument and return values of function to the diagram.

|Stack frame of main()              |               
|local_To_Main                      |
|                                   |   #include <stdio.h>
|-----------------------------------|   #include <stdlib.h>
|Stack frame of function1()         |   int gVariable1 = 100;
|local_To_Function1                 |   int gVariable2;
|iptr                               |   char cstring[10] = "Hello";
|     \               STACK         |   char* cptr = "Hello World";
|------\---------------|------------|   void function1(void)
|       \             \|/           |   {
|        \                          |       static int j = 5;
|         \                         |       int local_To_Function1;
|          \                 ^      |       int *iptr;
|           \                |      |       iptr = (int *) malloc(sizeof(int));
|------------\---------------|------|       free(iptr);
|   HEAP      \       ---           |   }
|              \---> |int|          |   
|                     ---           |   int main(void)
|-----------------------------------|   {
|                                   |       static int i;
|   UNINITIALIZED DATA (BSS)        |       int local_To_Main;
|gVariable2(initialized to 0)       |   
|i (initialized to 0)               |
|-----------------------------------|       function1();
|                                   |       return 0;
|      INITIALIZED DATA             |   }
|                                   |
|"Hello World"  |gVariable1 =100    |
|       ^       |cstring="Hello"    |
|       |       |j=5                |
|       |---<---<---- cptr          |
|(Read Only)    | (Read/Write)      |
|-----------------------------------|
|   Text or Code Segment            |
|                                   |
|-----------------------------------|

Jaredjarek

Answer 2

You might want to change

char *s = "sample";  //Pointer to string literal

to

char s[] = "sample";  // mutable copy of string literal

It is undefined behavior to try and modify the string literals, they should be used as const char *.

---

Probably unrelated, but a suggestion,

When you do something like *end--, you might want to put paranthesis so that it does what you think it does.

The above can be

(*end)--

or

 *(end--)

And you need a good grasp of the precedence rules to be sure what you want is what is happening.

Cannonball

UB in C as well, so this answer is correct about why the crash. But the original code is correct in terms of the precedence; the dereference operator has a lower precedence than the postfix increment/decrement. – Astrogate

@Astrogate thanks, Im more familiar with C++, so wasnt sure – Cannonball

Answer 3

Your reverse function is perfectly correct. Just a few thing with your main function part:

• as KarthikT said, s should be char[] not char* because changing literal is undefined.

• in your printf function, you forgot to put s as a parameter.

• return 0 is successful. return 1 is error.

So the new main should be like:

int main() {
    char s[] = "sample";
    reverse(s);
    printf("%s\n", s);
    return 0;
}

Output:

elpmas

Festination

Answer 4

char *str="sample";

Here sample is stored in the read-only memory, so you can never make modifications on such literals.

In order to do manipulation, you have to store the literals in read-write memory. The below code can solve your problem.

#include<stdio.h>
void reverse(char *str) 
{
    char temp,*end;
    for(end=str;*end;end++);
    end--; 
    for(;str<end;temp=*str,*(str++)=*end,*(end--)=temp);
}

int main() 
{
    char str[]="sample"; //stored in read-write memory
    reverse(str);
    printf("%s\n",str);
return 0;
}

Output:

elpmas

Interviewer