2

While working on a program which requires frequent memory allocation I came across behaviour I cannot explain. I've implemented a work around but I am curious to why my previous implementation didn't work. Here's the situation:

Memory reallocation of a pointer works

This may not be best practice (and if so please let me knwow) but I recall that realloc can allocate new memory if the pointer passed in is NULL. Below is an example where I read file data into a temporary buffer, then allocate appropriate size for *data and memcopy content

I have a file structure like so

typedef struct _my_file {
       int size;
       char *data;
}

And the mem reallocation and copy code like so:

// cycle through decompressed file until end is reached
while ((read_size = gzread(fh, buf, sizeof(buf))) != 0 && read_size != -1) {
        // allocate/reallocate memory to fit newly read buffer
        if ((tmp_data = realloc(file->data, sizeof(char *)*(file->size+read_size))) == (char *)NULL) {
                printf("Memory reallocation error for requested size %d.\n", file->size+read_size);
                // if memory was previous allocated but realloc failed this time, free memory!
                if (file->size > 0)
                        free(file->data);
                return FH_REALLOC_ERROR;
        }
        // update pointer to potentially new address (man realloc)
        file->data = tmp_data;
        // copy data from temporary buffer
        memcpy(file->data + file->size, buf, read_size);

        // update total read file size
        file->size += read_size;
}

Memory reallocation of pointer to pointer fails

However, here is where I'm confused. Using the same thought that reallocation of a NULL pointer will allocate new memory, I parse a string of arguments and for each argument I allocate a pointer to a pointer, then allocate a pointer that is pointed by that pointer to a pointer. Maybe code is easier to explain:

This is the structure:

typedef struct _arguments {
        unsigned short int options;    // options bitmap
        char **regexes;                // array of regexes
        unsigned int nregexes;         // number of regexes
        char *logmatch;                // log file match pattern
        unsigned int limit;            // log match limit
        char *argv0;                   // executable name
} arguments;

And the memory allocation code:

int i = 0;
int len;
char **tmp;
while (strcmp(argv[i+regindex], "-logs") != 0) {
        len = strlen(argv[i+regindex]);

        if((tmp = realloc(args->regexes, sizeof(char **)*(i+1))) == (char **)NULL) {
                printf("Cannot allocate memory for regex patterns array.\n");
                return -1;
        }
        args->regexes = tmp;
        tmp = NULL;

        if((args->regexes[i] = (char *)malloc(sizeof(char *)*(len+1))) == (char *)NULL) {
                printf("Cannot allocate memory for regex pattern.\n");
                return -1;
        }

        strcpy(args->regexes[i], argv[i+regindex]);

        i++;
}

When I compile and run this I get a run time error "realloc: invalid pointer "

I must be missing something obvious but after not accomplishing much trying to debug and searching for solutions online for 5 hours now, I just ran two loops, one counts the numbers of arguments and mallocs enough space for it, and the second loop allocates space for the arguments and strcpys it.

Any explanation to this behaviour is much appreciated! I really am curious to know why.

9
  • if ((tmp_data = realloc(file->data, sizeof(char *)*(file->size+read_size))) == (char *)NULL) {} make that if ((tmp_data = realloc(file->data, file->size+read_size)) == NULL) {} . You are aloocating an array of characters, not of pointers. And you make the same kind of error in the struct realloc. Commented Feb 13, 2013 at 20:08
  • 1
    Are you sure args->regexes is initialized before the realloc call ? Try printing it printf("%p\n", ...) Commented Feb 13, 2013 at 20:10
  • if (file->size > 0) free(file->data); return FH_REALLOC_ERROR; without setting file->data to NULL will leak memory, the same way as @cnicutar mentioned above. BTW: it will not leak, but the freed memory could be referenced or "doule freed()" by a next iteration, if any. Commented Feb 13, 2013 at 20:16
  • @wildplasser thank you for pointing that out. A bit surprised that part of the code was working fine. I also fixed the incorrect casting in the "regex parsing code (2nd heading in post above)". @cnicutar, absolutely correct. Apparently I had not initialised the pointer. args->regexes = NULL fixed it. And also @wildplassar I didn't know that, thanks for the tip. From now on I will also do '=NULL' after free(). Thanks! (now to figure out how to mark this as solved...new to stackoverflow as a user) Commented Feb 13, 2013 at 20:30
  • I also fixed the incorrect casting in the "regex parsing code (2nd heading in post above)" Correcting/clarifying what I meant, I fixed args->regexes[i] = (char *)malloc(sizeof(char *)*(len+1)) to args->regexes[i] = (char *)malloc(sizeof(char)*(len+1)) and similarly for the realloc. Commented Feb 13, 2013 at 20:42

1 Answer 1

1

First fragment:

// cycle through decompressed file until end is reached
while (1) {
        char **tmp_data;
        read_size = gzread(fh, buf, sizeof buf);
        if (read_size <= 0) break;

        // allocate/reallocate memory to fit newly read buffer
        tmp_data = realloc(file->data, (file->size+read_size) * sizeof *tmp_data );
        if ( !tmp_data ) {
                printf("Memory reallocation error for requested size %d.\n"
                      , file->size+read_size);

                if (file->data) {
                        free(file->data)
                        file->data = NULL;
                        file->size = 0;
                }
                return FH_REALLOC_ERROR;
        }

        file->data = tmp_data;
        // copy data from temporary buffer
        memcpy(file->data + file->size, buf, read_size);

        // update total read file size
        file->size += read_size;
}

Second fragment:

unsigned i; // BTW this variable is already present as args->nregexes;

for(i =0; strcmp(argv[i+regindex], "-logs"); i++) {
        char **tmp;

        tmp = realloc(args->regexes, (i+1) * sizeof *tmp ); 
        if (!tmp) {
                printf("Cannot allocate memory for regex patterns array.\n");
                return -1;
        }
        args->regexes = tmp;

        args->regexes[i] = strdup( argv[i+regindex] ); 
        if ( !args->regexes[i] ) {
                printf("Cannot allocate memory for regex pattern.\n");
                return -1;
        }
...
return 0;
}

A few notes:

  • the syntax ptr = malloc ( CNT * sizeof *ptr); is more robust than the sizeof(type) variant.
  • strdup() does exactly the same as your malloc+strcpy()
  • the for(;;) loop is less error prone than a while() loop with a loose i++; at the end of the loop body. (it also makes clear that the loopcondition is never checked)
  • to me if ( !ptr ) {} is easyer to read than if (ptr != NULL) {}
  • the casts are not needed and sometimes unwanted.
Sign up to request clarification or add additional context in comments.

1 Comment

Thank you for your advice. I found them very useful, and will use strdup from now on. I would up vote it if I could but got a box saying I need 15 reps. And you are right, why not use nregexes instead of creating another variable i. Will go through my code again for further cleaning up and optimization. Again, thank you!

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.