[OGR] Seg fault in ~OGRKMLDataSource()

[Daniel Morissette]

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)

[Daniel Morissette]

I have rebuilt my GDAL in debug mode. Taking this bug.

[Daniel Morissette]

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);

 /* -------------------------------------------------------------------- */

[warmerdam]

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!

[Daniel Morissette]

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.

[Daniel Morissette]

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.

[hobu]

I think we should backport this one to 1.4.1 if we can.

[warmerdam]

Mateusz,

Can you back port this fix into the 1.4 branch please?

[Mateusz Łoskot]

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.

[warmerdam]

Excellent!