String trimming causes memory leak?

Question

I'm curious what the proper way to trim a string is to ensure that no memory leak occurs. I guess this may really be a question based on exactly how free() works. I've included the code for my trim() function. See below.

int main()
{
    char* testStr1 = strdup("some string");
    char* testStr2 = strdup("   some string");
    char* testStr3 = strdup("some string     ");

    trim(&testStr1);
    trim(&testStr2);
    trim(&testStr3);

    free(testStr1); // no memory leak
    free(testStr2); // possible memory leak?
    free(testStr3); // possible memory leak?

    return 0;
}

int trim(char** pStr)
{
 if(pStr == NULL || *pStr == NULL)
  return FAILURE;
 char* str = *pStr;
 while(isspace(*str)) {
  (*pStr)++;
  str++;
 }

 if(*str == 0) {
  *pStr = str;
  return SUCCESS;
 }

 char *end = str + strlen(str) - 1;
 while(end > str && isspace(*end))
  end--;
 *(end+1) = 0;

 *pStr = str;
 return SUCCESS;
}

Answer 1

The pointer you pass to free needs to be exactly the same pointer you received from malloc (or calloc or realloc), not just a pointer into the region of memory that malloc returned. As such, your second string is the one that causes a problem. Your first and third are fine because the pointer you pass to free matches the one you received from malloc (via strdup).

What you're getting in that case, however, isn't really a memory leak -- it's undefined behavior.

Answer 2

Yes, this will cause a memory leak, but worse, it causes undefined behavior. Since trim modifies the pointer variables, main passes a pointer to free that was not returned by malloc. This is undefined behavior, and it will corrupt the heap on many implementations.

There are at least three correct ways to handle this.

1. Have trim allocate and return a new string, and make the caller responsible for freeing the new one, as well as the old (if needed):

char *trim(char *orig);
// ...
char *trimmed1 = trim(testStr1);
free(testStr1);
// ...
free(trimmed1);

2. Let the caller allocate a new string the same length (to be conservative), and pass both pointers in.

int trim(char *orig, char *new);
// ...
char *trimmed1 = malloc(strlen(testStr1) + 1);
trim(testStr1, trimmed1);
free(testStr1);
// ...
free(trimmed1);

3. Trim the string in place, shifting it left:

| | |t|r|im| | |\0|->
|t|r|i|m|\0|

int *trim(char *orig);
trim(testStr1);
// ...
free(testStr1);

Answer 3

This isn't really an answer to how free works, but I would do something along these lines:

char * trim_realloc(char * str) { char * p = str; char * e; char * ne; // new end char * r; size_t len;

// Since you put this level of error testing in your program
if (!str) {
   return str; // str is NULL
}

while (*p || isspace(*p) ) {
    p++;
}

len = strlen(p);
e = p + len;

ne = e;

while (ne > p) {
    if (isspace(*ne)) {
       *ne = 0;
       ne--;
    } else {
        break;
    }
}


if (p == str) {
   if (e != ne) {
       return realloc(str, len+1);  // only tail trim -- you could just return str here
   } else {
       return str; // no actual trim
   }
} else {
    r = strdup(p);
    free(str); // str is the head of the string, so that's what we have to free
    return r;
}

}

You should note my comment on the line with realloc Since I zeroed out trailing space anyway (and since many realloc implementations only worry about "is it big enough", not "is there too much extra space") you could have just let the buffer that your string lived in take up too much space at the end. It's still \0 terminated at the correct spot (unless there's bugs in my untested code, which there could be).

Other things you could do would be to just move the string to the beginning of the buffer and then trim the tail, so that:

"  cat   "

went through the steps:

"c cat " "ca cat " "catcat " "cat at " "cat t " "cat "

before you started trimming the tail.

Now, back to how free works -- free needs to be passed either NULL or a value that one of the heap allocation functions passed to you. Some heap allocation libraries are implemented so that when malloc allocates data the size of that data chunk is stored in the bytes just before the address that malloc returns, and when you call free the bytes just in front of that pointer are used to determine what the size of that memory chunk actually is. If you pass the something that was not returned by malloc (or calloc, or realloc, or similar) then free may look in the wrong place and use whatever it finds there as the size of the chunk you are freeing -- and nothing good comes of this.

Answer 4

You don't need an extra malloc/realloc/... in trim, like:

char *trim(char *s)
{
  while( isspace(*s) )
    memmove( s, s+1, strlen(s) );
  while( *s && isspace(s[strlen(s)-1]) )
    s[strlen(s)-1] = 0;
  return s;
}

Not fast but safe, free fails never for your examples, because s not changed. Only the s content can change.