Reallocing an array of strings

Question

So I made this function that receives an unknown amount of strings and adds them into an array of strings.

char **receiveCode(int socket){
    int line = 0;
    size_t lines = 1;
    size_t size = 1;
    char** code = malloc(sizeof(char)*size);

    while(1){
        package_struct *aPackage = receivePackage(socket);
        if(aPackage->type=='F'){break;}
        size = size + aPackage->size;
        code = realloc(code, size);
        code[line] = malloc(sizeof(char)*aPackage->size);
        strcpy(code[line],aPackage->package);
        line++;
        lines++;
        free(aPackage);
    }
    code = realloc(code, size + 2);
    code[line] = malloc(sizeof(char)*3);
    code[lines]=NULL;
    return code;
}

Sometimes when I run this code I get the following error

* glibc detected ./pp: realloc(): invalid next size: 0x00007f0f88001220 **

Which, according to Valgrind, happens in that function.

Probably I am using too many mallocs and reallocs... not sure though.

Answer 1

Here's a version that uses strdup() to simplify allocation of memory for each new line of text. It also uses 'x' versions of memory allocation functions to simplify out-of-memory error handling (a somewhat common idiom, even if non-standard).

So all the complexity that really remains (which ends up being not too much) is in managing the growth of the array of string pointers. I think this makes it easier to separate handling each string from handling the array of pointers. The original code got these two areas confused.

// these variants allocate memory, but abort program on failure
//  for simplified error handling - you may need different
//  error handling, but often this is enough
//
//  Also, your platform may or may not already have these functions
//      simplified versions are in the example.

void* xmalloc( size_t size);
void* xrealloc(void* ptr, size_t size);
char* xstrdup(char const* s);


char** receiveCode(int socket){
    size_t lines = 0;
    char** code = xmalloc( (lines + 1) * sizeof(*code));

    *code = NULL;

    while(1){
        package_struct *aPackage = receivePackage(socket);
        if(aPackage->type=='F') {
            free(aPackage); // not 100% sure if this should happen here or not.
                            // Is a `package_struct` with type 'F' dynamically
                            // allocated or is a pointer to a static sentinel 
                            // returned in this case?
            break;
        }


        // why use `aPackage->size` when you use `strcpy()` to
        //  copy the string anyway? Just let `strdup()` handle the details
        //
        // If the string in the `pckage_struct` isn't really null terminated, 
        // then use `xstrndup(aPackage->package, aPackage->size);` or something
        // similar.

        char* line = xstrdup(aPackage->package);
        ++lines;

        // add another pointer to the `code` array
        code = xrealloc(code, (lines + 1) * sizeof(*code));
        code[lines-1] = line;
        code[lines] = NULL;

        free(aPackage);
    }

    return code;
}


void* xmalloc(size_t size)
{
    void* tmp = malloc(size);

    if (!tmp) {
        fprintf(stderr, "%s\n", "failed to allocate memory.\n";
        exit(EXIT_FAILURE);
    }

    return tmp;
}

void* xrealloc(void *ptr, size_t size)
{
    void* tmp = realloc(ptr, size);

    if (!tmp) {
        fprintf(stderr, "%s\n", "failed to allocate memory.\n";
        exit(EXIT_FAILURE);
    }

    return tmp;
}


char* xstrdup(char const* s)
{
    char* tmp = strdup(s);

    if (!tmp) {
        fprintf(stderr, "%s\n", "failed to allocate memory.\n";
        exit(EXIT_FAILURE);
    }

    return tmp;
}

Also, I think it should be clarified if aPackage->package is a string pointer or if it's the actual location of the char[] holding the string data (ie., should &aPackage->package be passed to strcpy()/xstrdup()?). If it really is a pointer, should it be freed before aPackage is?

Answer 2

I assume this is to allocate an array of char* :

code = realloc(code, size);

Should be

code = realloc(code, size * sizeof(char*));
// and this one too
code = realloc(code, size + 2 * sizeof(char*));

Also, you don't need this:

char** code = malloc(sizeof(char)*size);

If you call realloc(NULL, size) it's equivalent to malloc(size)

size_t size = 0;
char** code = NULL;
...
code = realloc(code, size * sizeof(char*));

Note: lines seems useless to me, in fact in the last two lines you overwrite the memory you just allocated since line==lines

Answer 3

I think the problem is this :

char** code = malloc(sizeof(char)*size);

It should be char * instead of char inside sizeof()

char** code = malloc(sizeof(char*)*size);

Since code is a pointer to string so allocate memory for pointers that is char*

Also there is same kind of problem in realloc