views:

275

answers:

6

I have a function which is called multiple times during the program's execution. In said function, I have a dynamic character pointer which I resize numerous times.

My question is: do I need to free this pointer before the end of the function?

void functionName()
{
 char *variable = (char *) malloc(0);
    //variable is resized with realloc x number of times

 //should free be called here?
 return;
}

I should also note that I have tried to free the pointer, however, gdb gives me warnings when I do so.

+2  A: 

Yes, you have to free it or you'll leak the memory. Your code should look something like this:

void function(void) 
{
    char *variable = (char *)malloc(0);

    variable = realloc(variable, 10);
    // do something with new memory

    variable = realloc(variable, 20);
    // do something with more new memory

    free(variable);  // clean up
}

Calling malloc(0) is a little weird, I think.

Carl Norum
gdb is giving me: warning: Heap block at 003E2468 modified at 003E25C8 past requested size of 158
Tim Cooper
That warning sounds like you're overflowing your buffer at some point or otherwise doing bad things with the results of your `malloc()`/`realloc()` calls.
Carl Norum
It turns out I wasn't reallocating enough memory for the data I was trying to put in.Thanks for your help!
Tim Cooper
`x = realloc(x, amt)` is wrong. You should use `char *tmp = realloc(x, amt); if(tmp) { x = tmp; } else { /* x still valid, but not resized, handle error best as possible */ }` In the event that a resize was not possible, `realloc()` returns `NULL`, but leaves the pointer in it's argument unchanged. If you do `x = realloc(x, amt)` and it returns `NULL` but doesn't free `x` you have a memory leak.
Chris Lutz
`variable = realloc(variable, ...)` is a really bad idea. If `realloc()` fails, you get a memory leak. See the answers at http://stackoverflow.com/questions/1986538 for example
Alok
Safety first, as always.
Carl Norum
And of course if you're checking the return value of realloc, then you should also be checking the return value of malloc. Not so much for the case of malloc(0), but in general. It's not that rare to see code which doesn't bother checking malloc, hopefully for sound platform-specific reasons such as "I'm on a console, and I've proved my game cannot exhaust memory". In that case the "incorrect" realloc code is fine too.
Steve Jessop
A: 

From the man pages:

If size is 0, then malloc() returns either NULL, or a unique pointer value that can later be successfully passed to free().

So, I believe the answer is "yes" :).

yankle
A: 

Yes you need to call free() once to release the block of memory. You do not need to call free for the subsequent reallocs() that you are doing, even if those return a different address/pointer. The memory manager knows that the old block is not needed any longer and will free() it.

VoidPointer
A: 

You should be able to just call free(variable) at the end. If realloc ever has to move the data in order to resize it, it calls free internally, you do not need to worry about that.

Also, where you initialize variable, you could just set it to NULL instead of calling malloc; realloc will work just like malloc the first time.

ezod
A: 

Have a look at some of the answers I have given to a couple of questions in relation to memory management:

All of the above point out the obvious thing, for every malloc there is a free, if not you have a memory leak, so it is imperative that you free the memory when you are finished with your pointer variable that is mallocd.

Hope this helps, Best regards, Tom.

tommieb75
By the way it should be noted, someone today had that exact question titled 'C stdlib realloc issue'...
tommieb75
+2  A: 

A few points to make:

I can't see how you use realloc() in your code, but if you're using it like this, it's wrong:

variable = realloc(variable, amount);

When it's unable to allocate more memory, realloc() returns NULL but leaves the original pointer unaltered. In the above line, this means that variable is NULL and we've lost access to the memory it pointed to, but that memory hasn't been freed. The correct idiom is this:

void *tmp = realloc(variable, amount);
if(tmp)
  {
    // success! variable invalid, tmp has new allocated data
    variable = tmp;
  }
else
  {
    // failure! variable valid, but same size as before, handle error
  }

The reason you should use the second one is because, with realloc(), failure is bad, but is quite recoverable in many situations, unlike malloc() where failure usually means "stop everything and die."

This is a more contentious issue, but it is questionable whether or not you should cast the return value of malloc() and realloc() like you do. Consider:

// functionally identical in C
char *a = malloc(10);
char *b = (char *)malloc(10);

In C++, the cast must be made, because in C++ void * cannot be implicitly converted to another pointer type. (I think this is a language mistake, but it's not my place to judge.) If your code is C++, you should be using new and delete anyway. If your code is C but needs to compile with C++ compilers (for some inane reason), you have no choice but to cast. If you don't need to compile C code with C++ compilers (which is similar to having to run Ruby code in a Python interpreter), continue to the points below, which are why I think you shouldn't cast.

  1. In C89, if a function is used without being declared, it will be implicitly declared as returning an int. If, say, we forgot to #include <stdlib.h> and we called malloc(), the version without a cast would cause a compiler error (implicit casts from int to char * aren't allowed), while the version with the cast would (wrongly) tell the compiler "I know this sounds crazy, but cast it anyway." Most compilers will give you a warning for implicit (or incompatable) declarations of built-in functions like malloc(), but the cast does make it harder to find.

  2. Say you have some data:

    float *array = (float *)malloc(10 * sizeof(float));
    

    Later, you discover that you need more precision on your data, and have to make this a double array. In the above line, you need to change no more than 3 different places:

    double *array = (double *)malloc(10 * sizeof(double));
    

    If, on the other hand, you had written:

    float *array = malloc(10 * sizeof *array);
    

    You would only need to change float to double in 1 place. Furthermore, always using sizeof *obj instead of sizeof(type) and never using casts means that a later call to realloc() can work without any changes, while using casts and explicit type names would require finding anywhere you called realloc and change the casts and sizeofs. Also, if you forget, and do this:

    double *array = (float *)malloc(10 * sizeof(float));
    

    On most platforms, array will now only be an array of 5 elements, assuming the alignment isn't off and the compiler doesn't complain that you're assigning a float * to a double *. Some consider the warning the compiler issues to be helpful, as it points out potentially incorrect lines. However, if we avoid sizeof(type) and avoid casting, we can see that the lines won't be incorrect, so having the compiler draw attention to them is wasting time we could be using to program.

Chris Lutz
As an addition, most of the people who feel the need to cast the result of malloc usually don't feel the need to cast the argument to free() as well. Usually a sure sign of plain Cargo Cult without real understanding.
Secure
@Secure: more likely a sign of using C++, where there is an implicit cast from char* to void* but not the other way. Personally I think that *if* you use `sizeof(type)` in the malloc, *then* you should cast to type* for safety. In the case you're initializing a pointer variable, it's no safety at all, but in the case of assignment it is. But as Chris points out, you probably shouldn't be using `sizeof(type)` in the malloc to start with. It's better to base the size on the pointer to which the result is going to be allocated, then leave out the cast.
Steve Jessop
@secure:what's cargo cult?
tommieb75
@Secure: After reading this definition by wikipedia, that is a bad assumption to make! don't assume all programmers who use a cast when using malloc mean that they are part of a cargo cult which is what you are implying. It does help in an assignment statement to tell what is the type that you are malloc'ing and makes it legible. Incidentally, if you were to pass that code through the lint program, it will complain noisily!!! i.e without a cast using malloc! Lint is right, in fact, it should never have been separated from the compiler but I digress!
tommieb75
@Secure - I agree with @tommieb75. Casting isn't cargo cult programming, it's caused by the (inane) idea that C code should compile on C++ compilers.
Chris Lutz
Yes, you're right, I've realized my mistake after having no more net access to edit it. Let me carefully modify my statement and say that it may be a sign of misunderstanding when malloc is casted but realloc and calloc are not.
Secure