Getting a heap error when trying to free

Question

This smells like a heap corruption of some kind but I can not seem to find it. the problem occurs on string_utils_replace() when trying to run free(tmp_for_free). the function is supposed to replace every occurrence of "search" with "replace" in "string".

char* string_utils_part_of_string(char *string, int from, int to)
{
    int size_to_allocate = to - from + 1;
    char *result = (char*)malloc(sizeof(char) * size_to_allocate);

    strncpy(result, string + from, to - from);
    result[size_to_allocate - 1] = '\0';

    return result;
}

char* string_utils_replace(char *search, char *replace, char *string)
{
    char *end, *result = string, *tmp_for_free = NULL;
    int before_replace, after_replace;
    int size_search = strlen(search);
    int size_replace = strlen(replace);
    int size_string, size_find;
    int first_time = 1;

    char *find = strstr(string, search);

    if (find == NULL)
        return string_utils_copy_string(string);

    while (find != NULL)
    {
        tmp_for_free = result;

        size_string = strlen(result);
        size_find = strlen(find);

        before_replace = size_string - size_find;
        after_replace = before_replace + size_replace;

        end = string_utils_part_of_string(result, after_replace, size_string);
        result = string_utils_part_of_string(result, 0, before_replace);
        strcat(result, replace);
        strcat(result, end);

        // no memory leaks, hooray!
        free(end);
        if (first_time == 0)
            free(tmp_for_free);

        size_string = strlen(result);
        find = strstr(result, search);
        first_time = 0;
    }

    return result;
}

any ideas?

first time loop runs `tmp_for_free = result;` assigns undefined result storage to tmp_for_free.. — amdixon, Apr 28 '15 at 11:41
@Morb no, you don't need that, `free(NULL)` is perfectly valid. — Sourav Ghosh, Apr 28 '15 at 11:42
@amdixon - no, result is equal to string in the first run. check the first line in the function — ricebus, Apr 28 '15 at 11:43
I'm too lazy to check the code, but can you double check that you've allocated enough memory to support those `strcat()`s? — Sourav Ghosh, Apr 28 '15 at 11:49
well, this is probably it @SouravGhosh, I thought strcat() reallocs for you. — ricebus, Apr 28 '15 at 11:51
Do not use `strncpy()`. It is never the right tool for anything. Use `memcpy()` with the same arguments here. — chqrlie, Apr 28 '15 at 11:58

score 3 · Accepted Answer · edited Jun 20 '20 at 09:12

As per the man page of strcat(),

char *strcat(char *dest, const char *src);

[..] and the dest string must have enough space for the result. If dest is not large enough, program behavior is unpredictable;

In your string_utils_part_of_string() function, you did not allocate enough memory to the result to be able to hold the entire input, and later, you're trying to use the same pointer to store the whole input, through strcat(). This is creating the memory overrun which in turn invokes undefined behaviour.

Note: Please do not cast the return value of malloc() and family in C.

chqrlie · Answer 2 · 2015-04-28T12:03:10.967

You are causing a buffer overflow here:

    result = string_utils_part_of_string(result, 0, before_replace);
    strcat(result, replace);
    strcat(result, end);

result is allocated exactly before_replace+1 bytes and initialized with before_replace bytes from the beginning of string and a final '\0'. You cannot concatenate replace and end to this array, it is full already.

The logic in your function is convoluted. You should simplify it. For instance, you should first run a loop counting the number of occurrences of find in string, then allocate a buffer for the result, then run a second loop copying fragments of string and copies of replace.

You should also test if find is an empty string. strstr() will always find the empty string, causing your algorithm to loop endlessly.

Getting a heap error when trying to free

2 Answers2