Opened 17 years ago

Closed 17 years ago

#1535 closed defect (fixed)

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

Reported by: traianstanev Owned by: pka
Priority: normal Milestone: 1.4.2
Component: OGR_SF Version: 1.4.0
Severity: major Keywords: interlis
Cc: warmerdam

Description

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().

Change History (7)

comment:2 by warmerdam, 17 years ago

Cc: pka@… added
Milestone: 1.4.1

Pirmin,

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

comment:3 by warmerdam, 17 years ago

Priority: normalhigh
Severity: normalmajor

comment:4 by traianstanev, 17 years ago

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

comment:5 by warmerdam, 17 years ago

Cc: warmerdam added; pka@… removed
Owner: changed from warmerdam to pka

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.

comment:6 by warmerdam, 17 years ago

Keywords: interlis added
Milestone: 1.4.11.4.2
Priority: highnormal

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.

in reply to:  6 comment:7 by pka, 17 years ago

Status: newassigned

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]

comment:8 by pka, 17 years ago

Resolution: fixed
Status: assignedclosed

Backported to 1.4 branch

Note: See TracTickets for help on using tickets.