[Qgis-developer] Inconsistant use of VSIMalloc() / malloc() and VSIFree() / free()

[Etienne Tourigny]

Hello,

On Sat, Sep 29, 2012 at 10:59 AM, Even Rouault
<even.rouault at mines-paris.org> wrote:
> Le samedi 29 septembre 2012 15:24:59, Etienne Tourigny a écrit :
>> Hi,
>>
>> my opinion is that VSIMalloc/VSIFree should be used mainly by the
>> gdal/ogr providers, and if needed elsewhere (e.g. when calling
>> gdal/ogr code)
>
> This is even not needed for code that interact with GDAL/OGR. GDAL/OGR
> functions that take memory buffers just works fine with whatever memory
> allocator you want (malloc, new[], etc...)
>
> The only requirement is that VSIFree( ) *must* be used to free a pointer
> allocated by GDAL, to make sure that the CRT used for free() matches the CRT
> used for malloc().
>
>> , but alloc/free only internally to a function/class.
>>
>> Basically, following Even's recommendations.
>>
>> for example, in QgsRasterDataProvider::readBlock :
>>   // TODO: replace VSIMalloc, it is GDAL function
>>   void * data = VSIMalloc( dataTypeSize( bandNo ) * width * height );
>> ...
>> return data;
>
> You didn't finish your sentence here. So I suppose you meant that the TODO
> should be implemented by replacing VSIMalloc() by malloc() ?

I was just quoting the code, where it says "TODO: replace VSIMalloc",
it probably means using malloc instead, because VSIMalloc is a GDAL
function

>
>>
>>
>> QgsRasterFileWriter makes much use of VSIMalloc/VSIFree but that's
>> probably ok, because its use seems to be limited to internal use and
>> passed to QgsRasterIterator, but freed locally
>>
>> However, QgsRasterFileWriter::writeImageRaster uses VSIMalloc()
>> followed by CPLFree() - is that ok?
>
> Yes, CPLFree(VSIMalloc()) is OK
>
> The difference between VSIMalloc() and CPLMalloc() is that CPLMalloc() calls
> internally VSIMalloc() and aborts if the allocation fails. Thus CPLMalloc()
> never returns a NULL pointer and is only meant for very small allocations that
> shouldn't fail. My personnal opinion on this is that it should be completely
> avoided and replaced by VSIMalloc() with a proper check of the return value.
>
> VSIFree() and CPLFree() are the same: in port/cpl_conv.h : "#define CPLFree
> VSIFree"
>
>>
>> Even - did you try to save a raster (using new raster save as dialog)
>> with your modifications?
>
> I didn't do any modification yet. With the memory allocation debugging enabled,
> QGIS just crashes when opening a raster, so I couldn't test saving ;-)
>
>>
>>
>> On the other hand, it could go the other way around - make sure that
>> all frees are using VSIFree() instead of free().
>
> That's one possibility, but I was wondering if there are other raster
> providers than GDAL. In that case, they are unlikely to allocate with
> VSIMalloc().

There is grass provider as well as wcs (but it uses GDAL also).
The Grass raster provider does not use any VSI stuff, but the generic
QgsRasterDataProvider class does use it.

Maybe Radim can comment better on how the VSI stuff is used outside of
gdal provider, notably in the src/core/raster files.

>>
>> Even - what are the fundamental advantages of using VSIMalloc()
>> instead of malloc()?
>
> Basically none, from the application's point of view when GDAL is compiled
> with default options.
>
> For the purpose of GDAL developement, VSIMalloc() / VSIFree() are interesting,
> because they can be instrumented to provide debugging and statistics
> facilities. For example, if you enable both DEBUG_VSIMALLOC and
> DEBUG_VSIMALLOC_STATS, you get the following report at the end of each process
> using GDAL :
>
> Current VSI memory usage        : 0 bytes
> Maximum VSI memory usage        : 23781 bytes
> Number of calls to VSIMalloc()  : 770
> Number of calls to VSICalloc()  : 0
> Number of calls to VSIRealloc() : 235
> Number of calls to VSIFree()    : 770
> VSIMalloc + VSICalloc - VSIFree : 0
>
> This is a cheap way of tracking most memory leaks with quite unnoticeable
> slowdown. When DEBUG_VSIMALLOC is enabled, there's also a check in VSIFree()
> to detect that the pointer passed is valid (i.e. allocated by VSIMalloc()) and
> to detect double-frees. A kind of poor man's "valgrind --leak-check=full" to
> sum it up ;-) For my developement activities in GDAL, I enable that all the
> time.
>
> Of course, other applications can benefit from that by using
> VSIMalloc()/VSIFree(). But you must be very carefull to correctly pair
> VSIMalloc() with VSIFree(), and malloc() with free() : any other combination
> will lead to chaos.

thanks for the info. In regard of this, it might be worth using this
for raster stuff (exclusively) with extra care.

My advice would be stick to VSIMalloc()/VSIFree() where it is for now,
and correct for free() errors.  And come policy must be established
and added to the guidelines.

cheers,
Etienne

>
>
> Even