[mapserver-dev] Ticket 3559 - malloc/calloc/realloc return values
should always be checked
Alan Boudreault
aboudreault at mapgears.com
Tue Nov 23 12:29:42 EST 2010
Well,
I guess I'll continue the same way I was going. If I can return failure codes
properly, I wont use the msSmall* functions. Sometime its a mess to do proper
cleanup without making the code ugly. In example: mapgraticule.c:
msGraticuleLayerGetIntersectionPoints(). Allocations are dynamic, in 4
diffenrent scopes and are all conditional. For those cases, I'll either use
our alloc functions or return without cleanup. It's better than nothing
considering there was nothing before. I might consider the Cristiano approach
though, but I'd would prefer not using it to not confuse other devs if they
add/remove/modify code in the future.
thanks,
Alan
On November 23, 2010 11:51:55 am Cristiano Sumariva wrote:
> If you going to check the code maybe a simple way to recover from
> unavailable memory resource without memory leak should be to add a integer
> state marker variable and use a simple goto statement to jump at a switch
> that will clean up all allocations.
>
> Just add the cases in inverse( higher value to lower value ) order to
> clean from last to first allocation at end of function body.
> At each succesful allocation, increment state then check it. If failed jump
> to switch.
>
> You can increase the variable by a ammount of 10 to let free slots for code
> update so you will not need to rewrite the case entries at an small code
> change.
>
> I agree that small is relative. If the libc was not able to allocate it is
> because no more memory available. Fragmentation would fail on large blocks
> only.
>
> Hope it help you make Mapserver better.
>
> 2010/11/23 Alan Boudreault <aboudreault at mapgears.com>
>
> > 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
--
Alan Boudreault
Mapgears
http://www.mapgears.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.osgeo.org/pipermail/mapserver-dev/attachments/20101123/b88a767d/attachment-0001.html
More information about the mapserver-dev
mailing list