Opened 20 years ago
Closed 16 years ago
#697 closed defect (fixed)
mapogr.cpp uses delete on GDAL allocated objects
Reported by: | warmerdam | Owned by: | mapserverbugs |
---|---|---|---|
Priority: | high | Milestone: | 5.2 release |
Component: | OGR Support | Version: | 4.3 |
Severity: | normal | Keywords: | |
Cc: |
Description (last modified by )
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.
Attachments (1)
Change History (9)
comment:2 by , 20 years ago
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.
comment:3 by , 20 years ago
Status: | new → assigned |
---|
(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,
comment:4 by , 19 years ago
Milestone: | → 4.4 release |
---|
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.
comment:5 by , 17 years ago
Description: | modified (diff) |
---|
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)...
comment:6 by , 17 years ago
Milestone: | 5.0 release → 5.2 release |
---|
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?
comment:7 by , 17 years ago
yes, we need at the very least real destructor methods in GDAL 1.5 for the Style stuff (and corresponding C API methods).
comment:8 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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.