Why am I getting a malloc: double free error with realloc()?

Tags: c malloc
Question!

I've tried to write a string replace function in C, which works on a char *, which has been allocated using malloc(). It's a little different in that it will find and replace strings, rather than characters in the starting string.

It's trivial to do if the search and replace strings are the same length (or the replace string is shorter than the search string), since I have enough space allocated. If I try to use realloc(), I get an error that tells me I am doing a double free - which I don't see how I am, since I am only using realloc().

Perhaps a little code will help:

void strrep(char *input, char *search, char *replace) {
    int searchLen = strlen(search);
    int replaceLen = strlen(replace);
    int delta = replaceLen - searchLen;
    char *find = input;

    while (find = strstr(find, search)) {

        if (delta > 0) {
            realloc(input, strlen(input) + delta);
            find = strstr(input, search);            
        }

        memmove(find + replaceLen, find + searchLen, strlen(input) - (find - input));
        memmove(find, replace, replaceLen);
    }
}

The program works, until I try to realloc() in an instance where the replaced string will be longer than the initial string. (It still kind of works, it just spits out errors as well as the result).

If it helps, the calling code looks like:

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

void strrep(char *input, char *search, char *replace);

int main(void) {
    char *input = malloc(81);

    while ((fgets(input, 81, stdin)) != NULL) {
        strrep(input, "Noel", "Christmas");
    }
}


Answers

Someone else apologized for being late to the party - two and a half months ago. Oh well, I spend quite a lot of time doing software archaeology.

I'm interested that no-one has commented explicitly on the memory leak in the original design, or the off-by-one error. And it was observing the memory leak that tells me exactly why you are getting the double-free error (because, to be precise, you are freeing the same memory multiple times - and you are doing so after trampling over the already freed memory).

Before conducting the analysis, I'll agree with those who say your interface is less than stellar; however, if you dealt with the memory leak/trampling issues and documented the 'must be allocated memory' requirement, it could be 'OK'.

What are the problems? Well, you pass a buffer to realloc(), and realloc() returns you a new pointer to the area you should use - and you ignore that return value. Consequently, realloc() has probably freed the original memory, and then you pass it the same pointer again, and it complains that you're freeing the same memory twice because you pass the original value to it again. This not only leaks memory, but means that you are continuing to use the original space -- and John Downey's shot in the dark points out that you are misusing realloc(), but doesn't emphasize how severely you are doing so. There's also an off-by-one error because you do not allocate enough space for the NUL '\0' that terminates the string.

The memory leak occurs because you do not provide a mechanism to tell the caller about the last value of the string. Because you kept trampling over the original string plus the space after it, it looks like the code worked, but if your calling code freed the space, it too would get a double-free error, or it might get a core dump or equivalent because the memory control information is completely scrambled.

Your code also doesn't protect against indefinite growth -- consider replacing 'Noel' with 'Joyeux Noel'. Every time, you would add 7 characters, but you'd find another Noel in the replaced text, and expand it, and so on and so forth. My fixup (below) does not address this issue - the simple solution is probably to check whether the search string appears in the replace string; an alternative is to skip over the replace string and continue the search after it. The second has some non-trivial coding issues to address.

So, my suggested revision of your called function is:

char *strrep(char *input, char *search, char *replace) {
    int searchLen = strlen(search);
    int replaceLen = strlen(replace);
    int delta = replaceLen - searchLen;
    char *find = input;

    while ((find = strstr(find, search)) != 0) {
        if (delta > 0) {
            input = realloc(input, strlen(input) + delta + 1);
            find = strstr(input, search);            
        }

        memmove(find + replaceLen, find + searchLen, strlen(input) + 1 - (find - input));
        memmove(find, replace, replaceLen);
    }

    return(input);
}

This code does not detect memory allocation errors - and probably crashes (but if not, leaks memory) if realloc() fails. See Steve Maguire's 'Writing Solid Code' book for an extensive discussion of memory management issues.



First off, sorry I'm late to the party. This is my first stackoverflow answer. :)

As has been pointed out, when realloc() is called, you can potentially change the pointer to the memory being reallocated. When this happens, the argument "string" becomes invalid. Even if you reassign it, the change goes out of scope once the function ends.

To answer the OP, realloc() returns a pointer to the newly-reallocated memory. The return value needs to be stored somewhere. Generally, you would do this:

data *foo = malloc(SIZE * sizeof(data));
data *bar = realloc(foo, NEWSIZE * sizeof(data));

/* Test bar for safety before blowing away foo */
if (bar != NULL)
{
   foo = bar;
   bar = NULL;
}
else
{
   fprintf(stderr, "Crap. Memory error.\n");
   free(foo);
   exit(-1);
}

As TyBoer points out, you guys can't change the value of the pointer being passed in as the input to this function. You can assign whatever you want, but the change will go out of scope at the end of the function. In the following block, "input" may or may not be an invalid pointer once the function completes:

void foobar(char *input, int newlength)
{
   /* Here, I ignore my own advice to save space. Check your return values! */
   input = realloc(input, newlength * sizeof(char));
}

Mark tries to work around this by returning the new pointer as the output of the function. If you do that, the onus is on the caller to never again use the pointer he used for input. If it matches the return value, then you have two pointers to the same spot and only need to call free() on one of them. If they don't match, the input pointer now points to memory that may or may not be owned by the process. Dereferencing it could cause a segmentation fault.

You could use a double pointer for the input, like this:

void foobar(char **input, int newlength)
{
   *input = realloc(*input, newlength * sizeof(char));
}

If the caller has a duplicate of the input pointer somewhere, that duplicate still might be invalid now.

I think the cleanest solution here is to avoid using realloc() when trying to modify the function caller's input. Just malloc() a new buffer, return that, and let the caller decide whether or not to free the old text. This has the added benefit of letting the caller keep the original string!

By : Tryke


realloc is strange, complicated and should only be used when dealing with lots of memory lots of times per second. i.e. - where it actually makes your code faster.

I have seen code where

realloc(bytes, smallerSize);

was used and worked to resize the buffer, making it smaller. Worked about a million times, then for some reason realloc decided that even if you were shortening the buffer, it would give you a nice new copy. So you crash in a random place 1/2 a second after the bad stuff happened.

Always use the return value of realloc.



This video can help you solving your question :)
By: admin