Ticket #1432 (closed defect: fixed)

Opened 1 year ago

Last modified 1 year ago

[OGR] Seg fault in ~OGRKMLDataSource()

Reported by: dmorissette Assigned to: mloskot
Priority: normal Milestone: 1.4.1
Component: OGR_SF Version: 1.4.0
Severity: normal Keywords:
Cc:

Description (Last modified by hobu) 

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

01/10/07 14:33:34 changed by dmorissette 

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

01/10/07 14:39:36 changed by dmorissette 

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

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

01/10/07 14:43:36 changed by 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!

01/10/07 14:53:55 changed by dmorissette 

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.

01/10/07 15:03:44 changed by dmorissette 

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.

03/25/07 20:32:57 changed by hobu

  • status changed from closed to reopened.
  • resolution deleted.
  • description changed.
  • milestone set to 1.4.1.

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

03/26/07 23:40:24 changed by warmerdam

  • priority changed from highest to normal.
  • status changed from reopened to new.
  • severity changed from blocker to normal.
  • owner changed from dmorissette to mloskot.

Mateusz,

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

04/02/07 11:01:43 changed by mloskot

  • status changed from new to 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.

04/02/07 11:15:01 changed by warmerdam

  • status changed from assigned to closed.
  • resolution set to fixed.

Excellent!