Mem leak in OGRGeometry::Intersects (and other)

[ftrastour]

In ogrgeometry.cpp, OGRGeometry::Intersects calls exportToGEOS for the two concerned geometries but deletes objects returned by these calls only if the two calls were successfull. If only one call returned 0, the other result is not freed.

  hThisGeosGeom = exportToGEOS();
    hOtherGeosGeom = poOtherGeom->exportToGEOS();
    if( hThisGeosGeom != NULL && hOtherGeosGeom != NULL )
    {
        bResult = GEOSOverlaps( hThisGeosGeom, hOtherGeosGeom );
        GEOSGeom_destroy( hThisGeosGeom );
        GEOSGeom_destroy( hOtherGeosGeom );
    }

Some other functions in this file are implemented in the same way : OGRGeometry::Overlaps, OGRGeometry *OGRGeometry::Intersection OGRGeometry::Union OGRGeometry::Difference OGRGeometry::SymDifference? OGRGeometry::Disjoint OGRGeometry::Touches OGRGeometry::Crosses OGRGeometry::Within OGRGeometry::Contains OGRGeometry::Overlaps

[rouault]

While I agree with you on theory, actually, I'm not sure this can happen in practice. The only way (in the #ifdef HAVE_GEOS case of course) that exportToGEOS() returns NULL is if exportToWkb() returns an error or if GEOSGeomFromWKB_buf() returns NULL. But from a quick look, exportToWkb() always return OGRERR_NONE, and I would be surprised that GEOSGeomFromWKB_buf() would fail then (perhaps in case of memory exhausion ?). So did you run in an actual case that caused a leak (would be interesting to detail it), or is it the result of your code review ?

[ftrastour]

Replying to rouault:

While I agree with you on theory, actually, I'm not sure this can happen in practice. The only way (in the #ifdef HAVE_GEOS case of course) that exportToGEOS() returns NULL is if exportToWkb() returns an error or if GEOSGeomFromWKB_buf() returns NULL. But from a quick look, exportToWkb() always return OGRERR_NONE, and I would be surprised that GEOSGeomFromWKB_buf() would fail then (perhaps in case of memory exhausion ?). So did you run in an actual case that caused a leak (would be interesting to detail it), or is it the result of your code review ?

Even, I have discovered this leak because in an application I used an OGRLinearRing instead of an OGRPolygon as spatial filter ( in a call to SetSpatialFilter? ).

When a spatial filter is set, GetNextFeature? calls Intersects to filter features;

In Intersects, exportToGEOS() fails for OGRLinearRing and I got the leak.

In my case ( a tile-based vector to raster application ) it was a big problem : very big leak + no feature filtering at all...

Perhaps the documentation of SetSpatialFilter? should specify that the filter has to be a polygon (perhaps this works with other kind of geometry).

[rouault]

Ah ok. I missed that OGRLinearRing::exportToWkb() actually returns OGRERR_UNSUPPORTED_OPERATION.

As far as SetSpatialFilter?, most of the time you will use a polygon, but also a linestring is possible. What works might depend on the OGR driver (some drivers will perhaps only consider the bounding box of the provided geometry). But as a rule of thumb, OGRLinearRing isn't really a "public" geometry type and must only be used to be embedded inside a polygon.

[rouault]

r23140 /trunk/gdal/ogr/ogrgeometry.cpp: Prevent memleaks in various OGR GEOS-based methods in some cases where exportToGEOS() returns NULL (#4264)