Opened 17 years ago
Closed 17 years ago
#1432 closed defect (fixed)
[OGR] Seg fault in ~OGRKMLDataSource()
Reported by: | Daniel Morissette | Owned by: | Mateusz Łoskot |
---|---|---|---|
Priority: | normal | Milestone: | 1.4.1 |
Component: | OGR_SF | Version: | 1.4.0 |
Severity: | normal | Keywords: | |
Cc: |
Description (last modified by )
With GDAL/OGR 1.4.0, I get a seg fault (possibly a double free?) in OGRKMLDataSource::~OGRKMLDataSource() when I run the following command: ogr2ogr -f KML -dsco NameField=NAME_E /tmp/test.kml ./data/all_4326/ fedlimit park popplace If I remove the "-dsco ..." then the command runs fine. I don't have GDAL compiled in debug mode right now so I cannot investigate and fix right away, but I thought I would report this here now in case someone else has already found and fixed the error. GDB reports that the crash is in VSIFree() so I suspect a double free. Valgrind reports the following which suggests that it may be the CSLDestroy(papszCreateOptions) in OGRKMLDataSource::~OGRKMLDataSource() that causes the error: ==22792== Invalid free() / delete / delete[] ==22792== at 0x401CFCF: free (vg_replace_malloc.c:235) ==22792== by 0x41D4BF4: VSIFree (in /media/hda5/data-ubuntu/opt/fgs1/fgs-dev/built/gdal/lib/libgdal.so.1.10.0) ==22792== by 0x427714A: OGRKMLDataSource::~OGRKMLDataSource() (in /media/hda5/data-ubuntu/opt/fgs1/fgs-dev/built/gdal/lib/libgdal.so.1.10.0) ==22792== by 0x8049CD4: main (in /media/hda5/data-ubuntu/opt/fgs1/fgs/bin/ogr2ogr) ==22792== Address 0x4BDD13A is 10 bytes inside a block of size 17 alloc'd ==22792== at 0x401C422: malloc (vg_replace_malloc.c:149) ==22792== by 0x4991FEF: strdup (in /lib/tls/i686/cmov/libc-2.3.6.so) ==22792== by 0x41D4C18: VSIStrdup (in /media/hda5/data-ubuntu/opt/fgs1/fgs-dev/built/gdal/lib/libgdal.so.1.10.0) ==22792== by 0x41C5ADE: CPLStrdup (in /media/hda5/data-ubuntu/opt/fgs1/fgs-dev/built/gdal/lib/libgdal.so.1.10.0) ==22792== by 0x41CE5A4: CSLAddString (in /media/hda5/data-ubuntu/opt/fgs1/fgs-dev/built/gdal/lib/libgdal.so.1.10.0) ==22792== by 0x804A0DE: main (in /media/hda5/data-ubuntu/opt/fgs1/fgs/bin/ogr2ogr)
Change History (9)
comment:2 by , 17 years ago
I was wrong on my initial guess, the the source of the crash is the CPLFree(pszNameField) in ~OGRKMLDataSource(). pszNameField is set in OGRKMLDataSource::Create() by calling CSLFetchNameValue() which returns a const char * that should not be freed by the caller. Removing the CPLFree(pszNameField) and making pszNameField a const char * (instead of char *) solves the issue: Index: ogr_kml.h =================================================================== RCS file: /cvs/maptools/cvsroot/gdal/ogr/ogrsf_frmts/kml/ogr_kml.h,v retrieving revision 1.3 diff -u -r1.3 ogr_kml.h --- ogr_kml.h 27 Jul 2006 19:53:01 -0000 1.3 +++ ogr_kml.h 10 Jan 2007 18:35:25 -0000 @@ -101,7 +101,7 @@ OGRKMLLayer *TranslateKMLSchema( KMLFeatureClass * ); //The name of the field to use for - char *pszNameField; + const char *pszNameField; char **papszCreateOptions; Index: ogrkmldatasource.cpp =================================================================== RCS file: /cvs/maptools/cvsroot/gdal/ogr/ogrsf_frmts/kml/ogrkmldatasource.cpp,v retrieving revision 1.4 diff -u -r1.4 ogrkmldatasource.cpp --- ogrkmldatasource.cpp 4 Aug 2006 19:36:03 -0000 1.4 +++ ogrkmldatasource.cpp 10 Jan 2007 18:35:25 -0000 @@ -72,7 +72,6 @@ CSLDestroy( papszCreateOptions ); CPLFree( pszName ); - CPLFree( pszNameField ); for( int i = 0; i < nLayers; i++ ) { @@ -160,7 +159,7 @@ return FALSE; } - pszNameField = (char *)CSLFetchNameValue(papszOptions, "NameField"); + pszNameField = CSLFetchNameValue(papszOptions, "NameField"); CPLDebug("KML", "Using the field '%s' for name element", pszNameField); /* -------------------------------------------------------------------- */
comment:3 by , 17 years ago
Daniel, The caller of OGRKMLDataSouce::Create() is not required to keep the papszOptions around after the call, so I think the method should be taking a copy of the fieldname with CPLStrdup() instead of the change you made. Thanks for finding and digging into this!
comment:4 by , 17 years ago
Good point Frank. I had already committed the fix above to CVS when I read your last comment, so I have removed my revisions using cvs admin and will use CPLStrdup() as you suggest.
comment:5 by , 17 years ago
Fixed in CVS head. This would be a good candidate to backport to 1.4.x but there doesn't appear to be a 1.4.0 branch in CVS.
comment:6 by , 17 years ago
Description: | modified (diff) |
---|---|
Milestone: | → 1.4.1 |
Resolution: | fixed |
Status: | closed → reopened |
I think we should backport this one to 1.4.1 if we can.
comment:7 by , 17 years ago
Owner: | changed from | to
---|---|
Priority: | highest → normal |
Severity: | blocker → normal |
Status: | reopened → new |
Mateusz,
Can you back port this fix into the 1.4 branch please?
comment:8 by , 17 years ago
Status: | new → assigned |
---|
Frank,
As I see, the fix submitted by Daniel in r10591 is already there, in ogrkmldatasource.cpp:153 AFAIR, the branches/1.4 was created after Daniel applied the fix.
Note:
See TracTickets
for help on using tickets.