4

I am trying to return an array of string from a function and then free the memory it used. The code is below:

int main(int argc, const char * argv[])
{
    for (int m = 0; m < 10000; m++) {
        char **data = dataTest();

        int i = 0;
        while(data[i]) {
            printf("%p ",data[i]);
            free(data[i]);
            i++;
        }
        printf(" address= %p.\n",data);
        free(data);
    }

    return 0;
}

Here is the function:

char **dataTest()
{
    char *row[] = {"this", "is", "a", "data", "string", NULL};
    char **str = row;
    char **dataReturn = (char **) malloc(sizeof(char *) * 6);

    int i = 0;
    while (*str) {
        dataReturn[i] = malloc(sizeof(char) * strlen(*str));
        strcpy(dataReturn[i++], *str);
        str++;
    }

    return dataReturn;
}

It runs well in the beginning, but soon the error occurs. Below is the result. The address goes wrong somehow and the malloc error happens. Anyone has met the same problem before?

0x100300030 0x100300040 0x100300050 0x100300060 0x100300070  address= 0x100300000.
0x100300030 0x100300040 0x100300050 0x100300060 0x100300070  address= 0x100300000.
0x100400030 0x100300030 0x100300040 0x100300050 0x100300060  address= 0x100400000.
testC(562,0x7fff73e71310) malloc: *** error for object 0x3000000000000:     
pointer being freed was not allocated
*** set a breakpoint in malloc_error_break to debug
0x100300060 0x100300070 0x100300030 0x100300040 0x100300050 0x3000000000000                 
Program ended with exit code: 9
Sean Bright
  • 118,630
  • 17
  • 138
  • 146
ButterLover
  • 137
  • 1
  • 2
  • 10
  • 1
    Please [see why not to cast](http://stackoverflow.com/q/605845/2173917) the return value of `malloc()` and family in `C`. – Sourav Ghosh Jul 13 '15 at 14:25
  • 3
    I think you just forgot to add one to the result of `strlen` for the null terminator. And BTW, `sizeof(char)` is always unnecessary, because it is 1 by definition. – Fred Larson Jul 13 '15 at 14:25
  • 1
    `printf(" address= %p.\n",data);` should be `printf(" address= %p.\n",(void *)data);`. That's one of few cases where you actually _should_ cast a pointer. As others have said: `char ** malloc(sizeof(char *) * 6);` should be: `malloc( sizeof *dataReturn * 6);`. It's easier to read and understand anyway – Elias Van Ootegem Jul 13 '15 at 14:33
  • For completeness on why `printf("%p", (void *) data)` needs the cast, quote from the standard: _(C11, 7.21.6.1p8 Formatted input/output functions) "p The argument shall be a pointer to void."_. Put simply: not passing a `void *` to `printf` here is UB – Elias Van Ootegem Jul 13 '15 at 14:38

1 Answers1

8

You need to add this to just before return dataReturn; in your dataTest function:

dataReturn[i] = NULL ;

otherwise your while (data[i]) {} will continue further than wanted.

And instead of:

dataReturn[i] = malloc( sizeof(char) * (strlen(*str)) );

write:

dataReturn[i] = malloc(strlen(*str) + 1);

in order to allocate space for the terminating zero.

BTW sizeof (char) is always 1.

Jabberwocky
  • 48,281
  • 17
  • 65
  • 115