OGR Interlis driver GetNextFeature() returns a pointer to a feature that it holds on to

[traianstanev]

GetNextFeature? is supposed to return a new feature that the caller will then destroy using OGRFeature::DestroyFeature?. In the Interlis 2 driver, GetNextFeature? returns a pointer to an OGRFeature that is part of a collection of features that the layer holds on to between subsequent reads. When the caller destroys the feature, the original feature is freed. If one then calls ResetReading? on the layer, it returns pointers to freed memory. The fix that worked for me is as follows:

OGRFeature *OGRILI2Layer::GetNextFeature?() {

if (listFeatureIt != listFeature.end())

return (*(listFeatureIt++))->Clone();

return NULL;

}

I added the call to Clone().

[warmerdam]

Pirmin,

Traian's analysis looks quite right though I haven't looked into the code. What do you think?

[traianstanev]

Not sure, but it looks like the same problem exists with Interlis 1 layers. I have not confirmed this experimentally, since I am unable to read my Interlis 1 files for other reasons (something about java).

[warmerdam]

Reassigning to Pirmin who now has commit access.

However, Pirmin won't be available to work on this before 1.4.1 release, so I'm going to take care of applying Traian's fix for 1.4.1 if it looks ok.

[warmerdam]

I haved changed the code in trunk and 1.4 branch to do the Clone() for ogrili1layer.cpp and ogrili2layer.cpp. I was only able to test the ili1 case since I don't seem to have the java stuff setup properly for the ili2.

Turning over to Pirmin for a closer review before milestone 1.4.2.

I have also observed that:

• The driver(s) leak substantially (I can provide valgrind output if needed). I am dubious that the features are ever properly destroyed since otherwise even ogrinfo and ogr2ogr would have been crashing on double deletes.
• spatial filtering is not applied by GetNextFeature().
• attribute filtering is not applied by GetNextFeature().
• It appears the GetExtents() (at least in one of the implementations?) is implemented but does nothing. It is better let GetExtents() fall through to the default implementation if you don't want to provide an optimized version. Doing nothing doesn't count as optimization.

Drivers are responsible for spatial and attribute filtering. Failure to do so is a bug, not just a limitation. A construction something like the following can be used to test a feature against the current spatial and attribute filters.

        if( (m_poFilterGeom == NULL
             || FilterGeometry( poFeature->GetGeometryRef() ) )
            && (m_poAttrQuery == NULL
                || m_poAttrQuery->Evaluate( poFeature )) )
            return poFeature;

Ideally I'd like this all dealt with by 1.4.2.

[pka]

Replying to warmerdam:

I haved changed the code in trunk and 1.4 branch to do the Clone() for ogrili1layer.cpp and ogrili2layer.cpp. I was only able to test the ili1 case since I don't seem to have the java stuff setup properly for the ili2. Turning over to Pirmin for a closer review before milestone 1.4.2.

All my tests pass, thanks.

I have also observed that: * The driver(s) leak substantially (I can provide valgrind output if needed). I am dubious that the features are ever properly destroyed since otherwise even ogrinfo and ogr2ogr would have been crashing on double deletes.

Could you provide me the valgrind output?

* spatial filtering is not applied by GetNextFeature(). * attribute filtering is not applied by GetNextFeature(). * It appears the GetExtents() (at least in one of the implementations?) is implemented but does nothing. It is better let GetExtents() fall through to the default implementation if you don't want to provide an optimized version. Doing nothing doesn't count as optimization.

Implemented in changeset [11251]

[pka]

Backported to 1.4 branch