CODEWITHSUNDEEP
c++cstringmemory-management
bobobobo
bobobobo

Reputation: 67224

Why does returning a malloc'd pointer here result in "HEAP CORRUPTION" on free?

note: please suppress comments about "C++? Use std::string!". This question is using C strings but is more about memory management than strings in general.

I have this function

char* strclone( char* src )
{
    char* dst = (char*)malloc(strlen(src+1));
    strcpy(dst,src);
    return dst;
}

The function should work to allocate a new pointer (in strclone), write the string in src to it, and return the address of the new string.

However, when the string is freed, some time later in the program:

str = strclone( some_str_variable );    
// ..code..
free( str ) ; //! ERROR!

The error reads:

Debug Error! Program: C:\... HEAP CORRUPTION DETECTED: after Normal block (#39713) at 0x090CC448. CRT detected that the application wrote to memory after end of heap buffer.

The error occurs at the line where I call free( str ) in the program. If I change the assignment of str to this:

str = (char*)malloc( strlen( some_string_variable ) +1);
strcpy( str, some_string_variable ) ;
//...
free( str ) ; //fine now

Then there is no bug, the program functions perfectly.

Why does the strclone function not work as expected?

Upvotes: 1

Views: 746

Answers (5)

Martin James
Martin James

Reputation: 24847

Try this:

malloc(strlen(src)+1);

Upvotes: 0

templatetypedef
templatetypedef

Reputation: 372724

I believe that the problem is that you've written

(char*)malloc(strlen(src+1));

Notice that in the call to strlen, you have written

strlen(src + 1)

instead of

strlen(src) + 1

This first line says "the length of the string starting one character past src," which is the length of the string minus one (or total garbage if the string is empty). The second one is the one you want - the length of the string plus one for the null terminator. If you use the first version, then in the line

strcpy(dst,src);

You will end up writing past the end of the buffer, leading to the dreaded Undefined Behavior. In your case, this was manifesting itself with a heap corruption error when you try to free the block, which makes sense because you have indeed corrupted the heap!

Try moving the +1 out of the parentheses and see if it fixes things.

Alternatively, most compilers ship with a nonstandard function called strdup that does exactly what your above function attempts to do. You might want to consider just using that instead.

Hope this helps!

Upvotes: 15

Oliver Charlesworth
Oliver Charlesworth

Reputation: 272467

strlen(src+1) is not the same as strlen(src)+1. So you are overwriting the bounds of your array by two elements when you do to the strcpy.

So I think the following is justified: C++? Use std::string!

Upvotes: 4

Amy
Amy

Reputation: 1904

The error happens because you are actually writing past the allocated memory. change your malloc line to this:

char* dst = (char*)malloc(strlen(src)+1);

Because in your original implementation, you're advancing the pointer by 1, and then you pass that to strlen(), which will give you the lenght of the string-1. Since you also need the NULL at the end, you're actually off-by-2.

Upvotes: 0

cyco130
cyco130

Reputation: 4934

I believe you should have typed

char* dst = (char*)malloc(strlen(src) + 1);

instead of

char* dst = (char*)malloc(strlen(src+1));

Upvotes: 0

Related Questions