Memory leak with strings?

Question

I'm new to C, so this may be obvious, but I'm still not sure. Java took care of this for me ^^

I have a table of replacements, input string, and a function str_replace which does some work on the string. str_replace internally calls malloc to get space for the new string (it returns a newly allocated char*.

char* color_tags(char* s) {

    char* out = s;

    // in real, the table is much longer
    static char* table[4][2] = {
        {"<b>", BOLD},
        {"<u>", UNDERLINE},         
        {"</b>", BOLD_R},
        {"</u>", UNDERLINE_R},
    };


    for(int r=0; r<4; r++) {
        // here's what bothers me
        out = str_replace(table[r][0], table[r][1], out);
    }

    return out;
}

As you can see, the char* out is replaced by pointer to the new string, so the old string apparently ends up as a memory leak - if I don't understand it totally wrong.

What would be a better way for this?

Answer 1

The code leaks 3 strings in total: one after each of the iterations except the last one. The solution is to deallocate each of those strings after its use. The code may look like this:

for(int r=0; r<4; r++) {
    char* new_out = str_replace(table[r][0], table[r][1], out);
    if (r>0) {
        // out is an intermediate value which will never be used again, free it
        free(out);
    }
    out = new_out;
}

Answer 2

[This is more of a comment than an answer — abacabadabacaba has already posted the answer — but I hope it will clarify things a bit.]

I would argue that the memory leak is in this statement:

str_replace internally calls malloc to get space for the new string […]

[emphasis mine] Memory management is such a fundamental concern in C that if a function allocates memory that it itself doesn't de-allocate, then that is a major property of the function, and one that needs to be documented up-front, together with information about what the caller is supposed to do about it. It should not be considered "internal" to the function, and you shouldn't have the read the entirety of the function's source-code in order to determine it. It's enough to make me suspicious of the rest of the function (and indeed, a quick glance at that function is enough to notice a lot of issues: its parameter-types should be const char * rather than char *; it should check the return-value of malloc; it could be made more efficient by keeping track of the tail of new_subject, or cleaner by using strcat, instead of the current worst-of-both-worlds; etc.).

You didn't write str_replace originally, but you can modify your own version, so you should change its documentation from this:

Search and replace a string with another string , in a string

to something like this:

Creates and returns a copy of subject, but with all occurrences of the substring search replaced by replace. The returned string is newly allocated using malloc; the caller should use free.

(Your color_tags function will need similar documentation, since it too returns a newly-allocated string using malloc.)

That documentation in hand, there's a clear chain of "ownership": the caller of str_replace takes ownership of the string it returns. So color_tags has to call free for every string returned by str_replace, except the string that color_tags itself will return (which in turn will be "owned" by the caller of color_tags). Hence abacabadabacaba's answer.