strdup() memory leak even after free()

Question

I've never needed to use strdup(stringp) with strsep(&stringp_copy, token) together until recently and I think it was causing a memory leak.

(strdup() has always free'd just fine before.)

I fixed the leak, and I think I understand how, but I just can't figure out why I needed to.

Original code (summarized):

const char *message = "From: username\nMessage: basic message\n";
char *message_copy, *line, *field_name;
int colon_position;
message_copy = strdup(message);

while(line = strsep(&message_copy, "\n")) {
  printf(line);
  char *colon = strchr(line, ':');
  if (colon != NULL) {
    colon_position = colon - line;
    strncpy(field_name, line, colon_position);
    printf("%s\n", field_name);
  }
}

free(message_copy);

New code that doesn't leak:

const char *message = "From: username\nMessage: basic message\n";
char *message_copy, *freeable_message_copy, *line, *field_name;
int colon_position;
freeable_message_copy = message_copy = strdup(message);

while(line = strsep(&message_copy, "\n")) {
  printf(line);
  char *colon = strchr(line, ':');
  if (colon != NULL) {
    colon_position = colon - line;
    strncpy(field_name, line, colon_position);
    printf("%s\n", field_name);
  }
}

free(freeable_message_copy);

How is the message_copy pointer being overwritten in the first code? or is it?

Answer 1

The function strsep() takes a pointer to the original string (message_copy) and modifies it to return a new pointer to the 'next' token

const char *message = "From: username\nMessage: basic message\n";
char *message_copy, *original_copy;
//here you have allocated new memory, a duplicate of message
message_copy = original_copy = strdup(message);

Print out the pointer here,

printf("copy %p, original %p\n", message_copy, original_copy);

Note that as you use strsep(), you are modifying the message_copy,

char* token;
//here you modify message_copy
while(token = strsep(&message_copy, "\n")) {
    printf("%s\n", token);
}

This illustrates the changed message_copy, while original_copy is unchanged,

printf("copy %p, original %p\n", message_copy, original_copy);

Since message_copy does not point to the original strdup() result this would be wrong,

free(message_copy);

But keep around the original strdup() result, and this free works

//original_copy points to the results of strdup
free(original_copy);

Answer 2

Because strsep() modifies the message_copy argument, you were trying to free a pointer that was not returned by malloc() et al. This would generate complaints from some malloc() libraries, and from valgrind. It is also undefined behaviour, usually leading to crashes in short order (but crashes in code at an inconvenient location unrelated to the code that did the damage).

In fact, your loop iterates until message_copy is set to NULL, so you were freeing NULL, which is defined and safe behaviour, but it is also a no-op. It did not free the pointer allocated via strdup().

Summary:

• Only free pointers returned by the memory allocators.
• Do not free pointers into the middle or end of a block returned by the memory allocators.

Answer 3

Have a read of the strsep man page here.

In short the strsep function will modify the original character pointer that's passed into the function, overwriting each occurrance of the delimiter with a \0, and the original character pointer is then updated to point past the \0.

Your second version doesn't leak, as you created a temporary pointer to point to the start of the original char pointer returned from strdup(), so the memory was freed correctly, as you called free() with the original char pointer instead of the updated one that strsep() had modified.