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

[Stephen Woodbridge]

Alan,

Going back to the original goal that is stated fairly well here:

> Reviving an old thread... it seems that several people were in
> support of this option, but some were concerned about the
> proliferation of exit() and the side-effects that they could create
> in some non-CGI environments. While I would agree that spawn-fcgi is
> flawed if it cannot restart aborted processes, especially in cases
> where a process is running out of memory, I also believe that we
> could try to be more exit-friendly in some non-fatal situations and
> this can be discussed further in the context of ticket #3099.

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.

My 2 cents anyway,
   -Steve W

On 11/23/2010 10:09 AM, Alan Boudreault wrote:
> Hi devs,
>
> Through my work on this task, I realized that the *Small* use is very
>  relative. What is considered a small alloc these days? Most of the
> time, the allocations are made dynamically, so it is hard to see if
> it's a small alloc or not. Currently, when I'm not sure if it's a
> small alloc or not, I use a macro that I define: MS_CHECK_ALLOC,
> which check the pointer, call msSetError and return a failure code.
> Of course, I have to check if this return code is properly checked if
> the function wasn't returning a failure code before. I think there
> will be a lot more MS_CHECK_ALLOC uses than our msSmall* funtions. At
> this point, I have 2 options:
>
> * Continue that way to allow MapServer to return an error message to
> the user as most as possible.
>
> * Replace msSmallMalloc by msMalloc and use it almost everywhere.
>
> regards,
>
> Alan
>
> On November 17, 2010 02:24:03 pm Lime, Steve D (DNR) wrote:
>
>> I guess I favor the explicit names (should msStrdup be
>> msSmallStrdup?),
>
>> less confusion. If I saw msMalloc() I would assume it's equivalent
>> to
>
>> malloc() in terms of use (like msFree/free) and that's not the
>> intention.
>
>> I don't think there's a need for a vote. The discussion has been
>> out in
>
>> the open and others have had the chance to weigh in. It would be
>> nice to
>
>> see this documented in the context of do's and don'ts related to
>> the
>
>> cleanup work Alan did. We don't want to go and reintroduce the vary
>> code
>
>> he worked so hard to clean up. Perhaps a wiki page?
>
>>
>
>> Steve
>
>>
>
>> -----Original Message-----
>
>> From: mapserver-dev-bounces at lists.osgeo.org
>
>> [mailto:mapserver-dev-bounces at lists.osgeo.org] On Behalf Of Daniel
>
>> Morissette Sent: Tuesday, November 16, 2010 2:46 PM
>
>> To: mapserver-dev at lists.osgeo.org
>
>> Subject: Re: [mapserver-dev] Ticket 3559 - malloc/calloc/realloc
>> return
>
>> values should always be checked
>
>>
>
>> Reviving an old thread... it seems that several people were in
>> support
>
>> of this option, but some were concerned about the proliferation of
>
>> exit() and the side-effects that they could create in some non-CGI
>
>> environments. While I would agree that spawn-fcgi is flawed if it
>> cannot
>
>> restart aborted processes, especially in cases where a process is
>
>> running out of memory, I also believe that we could try to be more
>
>> exit-friendly in some non-fatal situations and this can be
>> discussed
>
>> further in the context of ticket #3099.
>
>>
>
>> So for the short term, it would seem to make sense to introduce the
>> use
>
>> of msSmallMalloc(), msSmallCalloc() and msStrdup() which would
>> exit() on
>
>> out of memory after attempting to report the error through stderr,
>> since
>
>> there is not much we can do anyway when we're out of memory.
>
>>
>
>> With respect to better handling of exit() in other situations,
>> let's
>
>> save that for another thread.
>
>>
>
>> Do we need a PSC vote before we go ahead with msSmallMalloc(),
>
>> msSmallCalloc() and msStrdup()?
>
>>
>
>> What about using the names msMalloc() and msCalloc() instead and
>
>> properly documenting that they are intended for use with small
>
>> allocations only (as is done for GDAL's CPLMalloc family of
>> functions)?
>
>>
>
>> Daniel
>
>>
>
>> Frank Warmerdam wrote:
>
>>> strk wrote:
>
>>>> On: Thursday, October 07, 2010 9:42 AM, Frank Warmerdam wrote:
>
>>>>> I would like to propose an msSmallAlloc() function that
>>>>> behaves in a
>
>>>>> similar
>
>>>>> fashion, just writing an error to stderr in case of failure
>>>>> and
> calling
>
>>>>> exit(). Likewise, an msStrdup() with similar behavior.
>
>>>>
>
>>>> Better than nothing, but this will fail to be catchable by
>>>> mapscript
>
>>>> or the CGI (think XML or in-image exception).
>
>>>> It's very frustrating when you get a blank image and have
>
>>>> to resort to forcing text/html content type or even worst
>
>>>> reading server logs, and there are already lots of such cases.
>
>>>
>
>>> Strk,
>
>>>
>
>>> Well, I can assure you that in the situation where
>>> msSmallAlloc()
>
>>> fails there would be no way of actually preparing an error
>>> report
>
>>> and burning it into an image for "inimage" error reports. Things
>
>>> are already too desperate.
>
>>>
>
>>> It is in fact very doubtful that we could even allocate a new
>
>>> errorObj for the error stack if we called msSetError().
>
>>>
>
>>> To put all this in context, GDAL/OGR has always "just died" with
>>> an
>
>>> error message to stderr if small allocations fail, including when
>>> run
>
>>> in the context of applications like ArcGIS. This has never yet,
>>> as
>
>>> far as I recall, been reported as a problem.
>
>>>
>
>>> Best regards,
>
> --
>
> Alan Boudreault
>
> Mapgears
>
> http://www.mapgears.com
>
>
>
> _______________________________________________ mapserver-dev mailing
> list mapserver-dev at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapserver-dev