Opened 17 years ago

Closed 14 years ago

#1540 closed defect (fixed)

Minor memory leaks in gmlreader.cpp

Reported by: rogerjames99 Owned by: chaitanya
Priority: normal Milestone: 1.7.0
Component: OGR_SF Version: 1.4.0
Severity: normal Keywords: ogr gml xerces
Cc: warmerdam, epifanio

Description (last modified by warmerdam)

I have fixed some minor memory leaks in gmlreader.cpp. The xerces runtime was not being cleaned up properly on exit so I have added a call to XMLPlatformUtils::Terminate(); in the GMLReader class destructor. Also some strings in GMLReader::SetupParser were not being freed, so I have enclosed them in std::auto_ptr(s). The relevant files modded against the 1.4.0 release are attached. These leaks are trivial but can bite someone making protracted use of the library without reloading.

Attachments (7)

gmlreader.cpp (27.8 KB ) - added by rogerjames99 17 years ago.
gmlreaderp.h (7.4 KB ) - added by rogerjames99 17 years ago.
gmlreader-minor-leak-fix.patch (3.0 KB ) - added by Mateusz Łoskot 16 years ago.
This is regular patch including fixes proposed in attached big gmlreaderp.h and gmlreader.cpp files. This patch is to help to review proposed changes.
gml-leaks-new-fix-mloskot.patch (4.0 KB ) - added by Mateusz Łoskot 16 years ago.
This is new improved version of proposed patch. The original version is buggy, because it's forbidden to use std::auto_ptr with buffers allocated by memory manager used in XMLString due to delete / delete[] mismatch.
test.cpp (1.8 KB ) - added by Mateusz Łoskot 16 years ago.
Simple test presenting the proposed patch in action. Running this test result in segmentation fault because internal state of the reader is broken. As Frank predicted, the solution is to track number of XML readers. This test uses road.gml copied to road1.gml and road2.gml. Attached Makefile is used to compile the test.
Makefile (386 bytes ) - added by Mateusz Łoskot 16 years ago.
Makefile used to build test.cpp program
road1.gml (1.3 KB ) - added by Mateusz Łoskot 16 years ago.
Sample GML file. Second copy 'road2.gml' is needed by the test.cpp program.

Download all attachments as: .zip

Change History (17)

by rogerjames99, 17 years ago

Attachment: gmlreader.cpp added

by rogerjames99, 17 years ago

Attachment: gmlreaderp.h added

comment:2 by warmerdam, 17 years ago

Cc: warmerdam added
Description: modified (diff)
Milestone: 1.5.0
Owner: changed from warmerdam to Mateusz Łoskot
Priority: lowestlow
Version: unspecified1.4.0

Mateusz,

I'd like you to review these changes and apply them if they seem appropriate.

Note, please ensure that the XMLPlatformUtils::Terminate() does not interfere with other GMLReader's that may be active at the same time. So try tests where two GML datastores are open at the same time. Close one, and then verify you can still read from the other.

I suspect the patch will have to be changed to keep track of how many GMLReader's are active, and only call the Terminate() when the last one is destroyed. Presumably that logic should also set m_bXercesInitialized to FALSE.

Since this is low priority and a bit risky as a change, I'd like to hold it to trunk, not the stable branch.

comment:3 by Mateusz Łoskot, 16 years ago

Status: newassigned

by Mateusz Łoskot, 16 years ago

This is regular patch including fixes proposed in attached big gmlreaderp.h and gmlreader.cpp files. This patch is to help to review proposed changes.

by Mateusz Łoskot, 16 years ago

This is new improved version of proposed patch. The original version is buggy, because it's forbidden to use std::auto_ptr with buffers allocated by memory manager used in XMLString due to delete / delete[] mismatch.

by Mateusz Łoskot, 16 years ago

Attachment: test.cpp added

Simple test presenting the proposed patch in action. Running this test result in segmentation fault because internal state of the reader is broken. As Frank predicted, the solution is to track number of XML readers. This test uses road.gml copied to road1.gml and road2.gml. Attached Makefile is used to compile the test.

by Mateusz Łoskot, 16 years ago

Attachment: Makefile added

Makefile used to build test.cpp program

by Mateusz Łoskot, 16 years ago

Attachment: road1.gml added

Sample GML file. Second copy 'road2.gml' is needed by the test.cpp program.

comment:4 by Mateusz Łoskot, 16 years ago

It's important to note that only first run of ./test program throws segmentation fault. Next, .gfs files are available and further executions do not crash.

comment:5 by Mateusz Łoskot, 16 years ago

Keywords: xerces added

comment:6 by warmerdam, 16 years ago

Milestone: 1.5.01.5.1

comment:7 by Mateusz Łoskot, 16 years ago

Summary: Minor memory leaks in gmlreader.cpp (fixed)Minor memory leaks in gmlreader.cpp

comment:8 by warmerdam, 14 years ago

Milestone: 1.5.41.7.0
Owner: changed from Mateusz Łoskot to chaitanya
Priority: lownormal
Status: assignednew

Chaitanya,

Please review proposed changes, and whether the driver has any memory leaks (via valgrind) before 1.7 release.

comment:9 by Even Rouault, 14 years ago

Cc: epifanio added

Chaitanya,

your commit in r18188 broke compilation when Xerces is not available.

libtool: compile:  g++ -g -O2 -Wall -I.. -I../.. -I/Users/sasha/gdal/port -I/Users/sasha/gdal/gcore -I/Users/sasha/gdal/alg -I/Users/sasha/gdal/ogr -I/Users/sasha/gdal/ogr/ogrsf_frmts -I/usr/local/include -DOGR_ENABLED -I/Users/sasha/gdal/port -I/Users/Shared/source/epylib/local/include -I/Library/Frameworks/UnixImageIO.framework/unix -I/Library/Frameworks/UnixImageIO.framework/unix/include -I/Users/Shared/source/epylib/local/include -I/Users/Shared/source/epylib/local/include -I -I/include -DHAVE_XERCES=0 -DHAVE_EXPAT -c gmlreader.cpp  -fno-common -DPIC -o ../o/.libs/gmlreader.o
gmlreader.cpp: In destructor 'virtual GMLReader::~GMLReader()':
gmlreader.cpp:140: error: 'XMLPlatformUtils' has not been declared
make[3]: *** [../o/gmlreader.lo] Error 1
make[2]: *** [gml-target] Error 2
make[1]: *** [sublibs] Error 2
make: *** [ogr-target] Error 2
MacBook-Pro-15-di-Massimo-Di-Stefano:gdal sasha$

XMLPlatformUtils::Terminate(); should be protected by #if HAVE_XERCES == 1

comment:10 by chaitanya, 14 years ago

Status: newassigned

Fixed in r18188 and r18190 in trunk.

comment:11 by chaitanya, 14 years ago

Resolution: fixed
Status: assignedclosed

Further testing on valgrind using the python interpreter with the autotest scripts ogr_gml_read.py and ogr_gml_geom.py and the attached test.cpp showed no memory leaks.

Note: See TracTickets for help on using tickets.