Ticket #2349 (closed defect: fixed)

Opened 2 months ago

Last modified 2 months ago

Memory bug in OGRGMLLayer::GetNextFeature() when using attribute filter

Reported by: titan Assigned to: rouault
Priority: normal Milestone: 1.5.2
Component: OGR_SF Version: 1.5.1
Severity: normal Keywords: GML memory delete heap poGeom attribute filter
Cc: titan, warmerdam

Description

This pertains to the GML driver in OGR, when the user sets an attribute filter, then iterates over the features that match this query. A simple use case (pseudo C code): 

OGR_L_ResetReading(layer);
OGR_L_SetAttributeFilter(layer, "height > 200");
for( feature = OGR_L_GetNextFeature(layer); feature;
     feature = OGR_L_GetNextFeature(layer) )
{
}

There is an apparent double-delete bug in OGRGMLLayer::GetNextFeature().

The scenario begins in the while loop, when poGeom is constructed (ogrgmllayer.cpp:156). Let's then imagine that the attribute query fails. poOGRFeature is deleted but poGeom is not. As we begin the next iteration, we delete poGeom but do not mark it NULL.

At this point, if the second feature also fails the query, we will attempt to delete the first poGeom again on the third iteration.

The obvious patch, which I am currently testing, is to mark poGeom NULL when it is deleted (line 127). Maybe there is another way, considering the different cases where poGeom needs to be deleted.

Attachments

ticket_2349_test_1.gml (1.1 kB) - added by titan on 05/07/08 13:22:17.
A GML file that reproduces the problem with ogrinfo

Change History

05/07/08 10:59:50 changed by titan

  • cc changed from kkassil@gallium.com to titan.

05/07/08 12:49:04 changed by warmerdam

  • status changed from new to assigned.
  • milestone set to 1.5.2.

I have tried running valgrind on: 

  ogrinfo -ro mx_caps.gml mx_caps -where 'Capital = "Veracruz"'

and it ran fine without any reports of double frees, etc. So, I believe you will need to provide supporting information on how to reproduce the problem, including a data file. Ideally it would be demonstrated via ogrinfo.

05/07/08 13:22:17 changed by titan

  • attachment ticket_2349_test_1.gml added.

A GML file that reproduces the problem with ogrinfo

05/07/08 13:33:29 changed by titan

Thanks for looking into this.

I've found a minimal input that demonstrates the problem. Layer MyPolyline contains one feature. This feature does not satisfy the where clause: 

  ogrinfo -ro ticket_2349_test_1.gml MyPolyline -where 'height > 300'

Just before the double-free, the while loop begins to decode the next layer (MyPoint), and realizes it's not part of this layer (ogrgmllayer.cpp:143). 

        if( poGMLFeature->GetClass() != poFClass )
            continue;

On the next iteration, it crashes: 

    while( TRUE )
    {
    ...
        if( poGeom != NULL )
            delete poGeom;

I'm using Visual Studio 2005 debug heap but I hope this will allow you to reproduce it with valgrind.

Cheers

05/21/08 13:57:25 changed by rouault

  • status changed from assigned to new.
  • owner changed from warmerdam to rouault.
  • cc changed from titan to titan, warmerdam.

05/21/08 14:01:51 changed by rouault

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

Titan, thanks for your precise bug report !

Fixed in trunk in r14501 and in branches/1.5 in r14502

05/21/08 14:09:37 changed by titan

Thank you, Rouault, for applying a fix!