CODEWITHSUNDEEP
cpointersmemory-leaksfreestrdup
JochenVdB
JochenVdB

Reputation: 31

multiple calls to strdup() with the same lvalue

Throughout the programs I inherited from my predecessors, there are functions of the following format:

    somefunc(some_type some_parameter, char ** msg)

In other words, the last parameter is a char **, which is used to return messages. That is: somefunc() will "change" msg. In some cases the changing in question is of the form:

    sprintf(txt,"some text. Not fixed but with a format and variables etc");
    LogWar("%s",txt);   //call to some logging function that uses txt
    *msg = strdup(txt);

I know that each call to strdup() should have a related call to free() to release the memory it allocated.

Since that memory is used to return something, it should obviously not be freed at the end of somefunc().

But then where?

If somefunc() is called multiple times with the same msg, then that pointer will move around, I presume. So the space allocated by the previous call will be lost, right?

Somewhere before the end of the program I should certainly free(*msg). (In this case *msg is the version that is used as parameter in the calls to somefunc().) But I think that call would only release the last allocated memory, not the memory allocated in earlier calls to somefunc(), right?

So, am I correct in saying that somefunc() should look like this:

    sprintf(txt,"some text. Not fixed like here, but actually with variables etc");
    LogWar("%s",txt);   //call to some logging function that uses txt
    free(*msg); //free up the memory that was previously assigned to msg, since we will be re-allocating it immediatly hereafter
    *msg = strdup(txt);

So with a free() before the strdup().

Am I correct?

Upvotes: 2

Views: 808

Answers (1)

unwind
unwind

Reputation: 399949

Yes, you're correct. Any old pointer returned from strdup() must be free()d before you overwrite it, or you will leak memory.

I'm sure you where being simple for clarity, but I would of course vote for something like this:

const char * set_error(char **msg, const char *text)
{
  free(*msg);
  *msg = strdup(text);
}

and then:

LogWar("%s",txt);   //call to some logging function that uses txt
set_error(msg, txt);

See how I used encapsulation to make this pretty important sequence more well-defined, and even named?

Upvotes: 1

Related Questions