Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#2349 closed defect (fixed)

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

Reported by: titan Owned by: Even 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 (1)

ticket_2349_test_1.gml (1.1 KB) - added by titan 12 years ago.
A GML file that reproduces the problem with ogrinfo

Download all attachments as: .zip

Change History (7)

comment:1 Changed 12 years ago by titan

Cc: titan added; kkassil@… removed

comment:2 Changed 12 years ago by warmerdam

Milestone: 1.5.2
Status: newassigned

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.

Changed 12 years ago by titan

Attachment: ticket_2349_test_1.gml added

A GML file that reproduces the problem with ogrinfo

comment:3 Changed 12 years ago 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

comment:4 Changed 11 years ago by Even Rouault

Cc: warmerdam added
Owner: changed from warmerdam to Even Rouault
Status: assignednew

comment:5 Changed 11 years ago by Even Rouault

Resolution: fixed
Status: newclosed

Titan, thanks for your precise bug report !

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

comment:6 Changed 11 years ago by titan

Thank you, Rouault, for applying a fix!

Note: See TracTickets for help on using tickets.