mapogr.cpp uses delete on GDAL allocated objects

[warmerdam]

Mladen Turk wrote:
> Hi all,
> 
> There is a memory leak in mapogr.cpp.
> 
> The reason is quite simple and obvious when using gdal as dll, and it
> involves operator delete on a memory that has been allocated using
> OGRSFDriverRegistrar::Open in a gdal dll, that has a different memory heap.
> 
> Not sure how to resolve this, but something like 
> 
> void GDALDelete(void *p)
> {
>    delete p;
> }
> 
> Inside the gdal itself should be called instead simply calling delete from
> mapserver or mapscript. 

Mladen,

In fact, this is generally worse than just a memory leak ... it is a heap
corruption.  Can you point out the particular places this is occuring?  Skimming
the mapogr.cpp I can see a number of questionable uses of delete likely to 
cause problems. 

There are "destructor" methods for most types in OGR.  For instance, 
the OGRFeature::DestroyFeature( poFeature ) static method can be used to 
destroy features within the GDAL DLL itself.

[fwarmerdam]

 
In a normal (old style) MapServer build on windows, the OGR code is
actually statically linked in as well as being in the DLL so this might
not cause any problems.  But as we move to using GDAL/OGR purely from the
DLL with no static libraries it becomes a more serious bug. 

[mturk@…]

It won't work for all cases like OGRSFDriverRegistrar, or any other classes 
then OGRFeature, or the one inherited from OGRFeature.

IMO something like OGRFeature::DestroyFeature should be added to GDAL, but it 
needs to be more generic.
For example:

OGRDelete(enum OGRclassType, void *pMem)
{
   switch (OGRclassType) {
      case OGRFeatureT:
          delete (OGRFeature *)pMem;
      break;
      case OGRSFDriverRegistrarT:
          delete (OGRSFDriverRegistrar *)pMem;
      break;
      ... etc
  }
}


This way both the destructor and memory will be freed.

[mturk@…]

PATCH for mapogr.cpp and gdal

[fwarmerdam]

(From a discussion on the gdal-dev list)

Mladen,

As I understand it this is an issue if you use GDAL as a DLL, but build
with static LIBC.  That means each DLL and the main program all have a 
seperate heap, and it is critical that delete be called from within
the same DLL as the allocation took place within.  

It can be avoided by building GDAL and the application with a runtime 
C library.  I think this is accomplished with the /MD switch. 

The other approach is to provide some sort of external destructor access
via exact function entry points.  I have done this already for many GDAL and
OGR objects.  In particular for GDAL it is usually sufficient to call GDALClose()
on a dataset handle to clean things up. 

In OGR there are more objects to worry about. Features, datasources, and
coordinate system objects.  I think I have specific destructors for each of
those.  If others I needed I can extend it.  I am not keen to add a generic
OGRDelete as you have suggested in your bug report. I prefer specific
destructors.  But certainly MapServer needs to be updated to use them.  In
fact, I would like to also rewrite the MapServer mapogr.cpp code to use the
OGR C API instead of the C++ API.  This would address the delete issues and
other C++ linkage fragility issues all in one pass.  Hopefully this can be
done in time for the MapServer 4.4 release. 

I will add this note to the mapserver bugzilla report as well for completeness.

Best regards,

[dmorissette]

Frank, how important is this problem?

Based on the comments above there seem to be two options:
1- Address this now for 4.4 by just fixing the delete calling the specific
destructors for each object in mapogr.cpp
2- Leave things as is in 4.4, mark the bug as FUTURE and try to switch to the
OGR C API in a later release?

I'll mark this bug for 4.4 for now, but will wait for your feedback before we
make a final call.

[hobu]

Updated to use OGR destructors instead of delete for many things in r6496. OGR doesn't have these for style-related stuff (would be nice to add for GDAL 1.5)...

[dmorissette]

Howard, if I'm not mistaken you kept this open because the other delete's will have to wait for GDAL 1.5 (and MapServer 5.2), right?

[hobu]

yes, we need at the very least real destructor methods in GDAL 1.5 for the Style stuff (and corresponding C API methods).

[dmorissette]

Fixed with the conversion of mapogr.cpp to use the C API in r7168 (see also #545).

Note: This is really fixed only if you use GDAL/OGR 1.5.0 or more recent which enables use of C API for everything. With older releases of GDAL we still use the C++ classes for style stuff, and there are still two instances of potentially problematic deletes on OGRStyleTool objects allocated by GDAL.