[mapserver-dev] Ticket 3559 - malloc/calloc/realloc return values should always be checked

[Stephen Woodbridge]

On 11/23/2010 1:49 PM, Daniel Morissette wrote:
> Stephen Woodbridge wrote:
>>
>> Forget about "small" being the criteria, and look at whether or not the
>> code can recover and clean up from a failed allocation attempt. If you
>> can not clean up ie: free up intermediate allocations on error returns
>> then you will have memory leaks on any error and I'm not sure which is
>> worse the leaks or  the exit because leaks are just a slow death that
>> might impact other processes as the current consumes leaked memory and
>> an exit just causes problems for the offending application.
>>
>
> I agree with you: I'm not sure which one is better between slow leaks
> from improper cleanup on errors, or a brute force exit. I would add that
> another factor to consider is to try to avoid unnecessarily increasing
> code complexity with conditional code to try to recover from out of
> memory situations where there is not much we can do anyway.
>
> So I'd second what Steve suggests and do a proper cleanup when possible,
> otherwise just go with msSmallAlloc/exit if the code cannot easily do a
> proper cleanup, even in cases where the allocation may not clearly meet
> the "small" criteria.

I just had to clean a bunch of old code I that I wrote 10 years ago - 
yikes was the horrid! Any this was on of the issues I had to deal with 
and what I did to keep things as clean and simple as possible was to 
deal all variables that held dynamically allocated objects like below. I 
susspect Alan is already doing something similar to this.

#define ERROR_IF_NULL(a, b) if (!(a)) goto b
#define FREE_IF(a) if(a) free(a)

TYPE var1 = NULL;
TYPE var2 = NULL;

....

var1 = (TYPE *) malloc(sizeof(TYPE));
ERROR_IF_NULL(var1, label);

...

var2 = (TYPE *) malloc(sizeof(TYPE));
ERROR_IF_NULL(var2, label);


return OK;

label:
   FREE_IF(var1);
   FREE_IF(var2);
   return ERROR;

This keeps the code fairly clean and the same scheme can be adapted to 
more complex objects. Obviously you can think up cases that this will 
not work on, but a general strategy you should be able to map those 
cases into problems that can be solved in a similar manner.

I totally agree on not adding to code complexity. If you can not do 
something simple like this to clean up it is probably better to exit 
than generate a slow leak which is hard to track down if it only happens 
during errors.

Maybe someone knows if for the fast-cgi case, is there a possibility to 
to do a longjump to a function the gracefully restarts the fast-cgi 
process instead of doing an exit? If this is the case then we could wrap 
exit and abort calls as msExit() or msAbort() and have these handle it 
appropriately for the context we are running it.

-Steve W