Why free caused a Segmentation fault in my code?

Question

I try to implement a simple program that returns a reversed copy of string. Everything went well except a Segmentation fault at the end!! It I comment the last free(), nothing looks wrong but I know it is not right.

#include <stdio.h>
#include <stdlib.h>

void revStr(char *str, char *newStr, int *idx)
{
    if( *str != '\0')
    {
        *idx = *idx+1;
        revStr(str+1, newStr, idx);
        printf("char=%c int=%d\n", *str,*idx);
        newStr[*idx]=*str;
        *idx = *idx+1;
    }
    else if (*idx !=0)
    {
        printf("End Size=%d\n",*idx);
        newStr = (char*)malloc( sizeof(char) * (*idx) );
        *idx=0;
    }
}

int main(void) {
    int idx =0;
    char * in = "This is a test string.";
    char * out;

    revStr(in, out, &idx);

    printf("Out:%s\n", out);
    free(out);

    return 0;
}

char=. int=0
char=g int=1
char=n int=2
char=i int=3
char=r int=4
char=t int=5
char=s int=6
char=  int=7
char=t int=8
char=s int=9
char=e int=10
char=t int=11
char=  int=12
char=a int=13
char=  int=14
char=s int=15
char=i int=16
char=  int=17
char=s int=18
char=i int=19
char=h int=20
char=T int=21
Out:.gnirts tset a si sihT
Segmentation fault (core dumped)

I tried to figure out from couple of question but in vain. Can someone help??

Answer 1

In your main() function you pass your revStr() function an argument out of type char *.

out is empty and points to nothing.

So when your revStr() function tries to append/change whatever out is pointing to (nothing in this case) with the line

newStr[*idx] = *str;

It leads to a segmentation fault.

Another problem is that as a result of out being empty, the call

free(out);

becomes dangerous and leads to undefined behaviour.

In order to fix this you can allocate memory for out with malloc().

size_t in_size = strlen(in);
char *out = malloc(in_size + 1);

// allocate memory for string
// and +1 for NUL terminator 

Also you should add a NUL terminator \0 to the end of out your string signify the end.

out[in_size] = '\0';

Note:

When allocating memory for out, it is best to do it inside main(). Calling malloc() multiple times inside a recursive function is probably not something you want.

Answer 2

Thanks to @RetiredNinja for an useful reference. I figured out my problem and even found myself made more unaware mistakes. Since no one provides a correct and complete answer, I answer my question.

First of all, this is my personal practice on recursion. If you are looking for a good example on reversing string. This one is definitely NOT!!

First mistake. When I declared the out pointer, I thought it is a null pointer.

char * out;
............
............
free(out);

As matter of fact, it is NOT. It is initialized with platform-dependent default value. This explains why a free() call on it will cause segmentation fault. It is not dynamically allocated, therefore can not be freed.

Second mistake. According to the idea of "passed by (copy of) values". When the pointer out was passed as parameter newStr, the pointer newStr holds same address value as out. However the address value in out remained unchanged when I changed the value in newStr to a newly allocated memory address from malloc(). My code never utilized the dynamically allocated memory. The pointer to it lost after the recursive call creating it ended.

Third and an unattempted mistake. Despite not utilizing the dynamically allocated memory, my code is still able to print out another string stored in another memory segmentation. This is because my platform(gcc on ubuntu18) doesn't enforce a strict prohibition on variables boundary. The declaration char * out; only allocated a memory space to hold a char and a pointer to it. My code accidentally accessed the memory space beyond the declared char. This is considered unsafe and may cause segmentation fault right away on other strict platforms.

Answer 3

first of all you have to write malloc (memory allocation to out), to do this you need to count the length of the string, then there is a mistake in revStr, you have to subtract at the end of the function, and finally you have to free allocated memory: so you have to write:

#include <stdio.h>
#include <stdlib.h>

void revStr(char *str, char *newStr, int *idx)
{
    if( *str != '\0')
    {
        *idx = *idx+1;
        revStr(str+1, newStr, idx);
        *idx = *idx-1;
        printf("char=%c int=%d\n", *str,*idx);
        newStr[*idx]=*str;
    }

}

int main(void) {
    int idx =0;
    char * in = "This is a test string.";
    char * out;

    while (in[idx])
        idx++;
    out = (char *) malloc (sizeof (char) * (idx + 1));
    if (!out) //if there is no enough memory we go to this exception
        return(12);
    out [idx] = 0;

    idx = 0;

    revStr(in, out, &idx);

    printf("Out:%s\n", out);
    if (out)
        free(out);

    return 0;
}

And the output will be:

>char=. int=21
char=g int=20
char=n int=19
char=i int=18
char=r int=17
char=t int=16
char=s int=15
char=  int=14
char=t int=13
char=s int=12
char=e int=11
char=t int=10
char=  int=9
char=a int=8
char=  int=7
char=s int=6
char=i int=5
char=  int=4
char=s int=3
char=i int=2
char=h int=1
char=T int=0
Out:This is a test string.