geos::operation::valid::IsValidOp::isValid() leak

[sgillies]

Output of valgrind:

==14421== 84 bytes in 3 blocks are definitely lost in loss record 59 of 110
==14421==    at 0x4021B59: operator new(unsigned) (vg_replace_malloc.c:163)
==14421==    by 0x5069BB5: geos::operation::valid::IsValidOp::checkConsistentArea(geos::geomgraph::GeometryGraph*) (IsValidOp.cpp:301)
==14421==    by 0x506AC20: geos::operation::valid::IsValidOp::checkValid(geos::geom::Polygon const*) (IsValidOp.cpp:191)
==14421==    by 0x506B0B5: geos::operation::valid::IsValidOp::checkValid(geos::geom::Geometry const*) (IsValidOp.cpp:124)
==14421==    by 0x506B448: geos::operation::valid::IsValidOp::isValid() (IsValidOp.cpp:83)
==14421==    by 0x4F72FB5: GEOSisValid (geos_c.cpp:363)
==14421==    by 0x4F66A16: ffi_call_SYSV (sysv.S:60)
==14421==    by 0x4F668A3: ffi_call (ffi.c:221)
==14421==    by 0x4F61203: _CallProc (callproc.c:668)
==14421==    by 0x4F5AC99: CFuncPtr_call (_ctypes.c:3362)
==14421==    by 0x8058BB6: PyObject_Call (abstract.c:1795)
==14421==    by 0x80B5C02: PyEval_EvalFrame (ceval.c:3776)

[denisem]

Here's what was causing the leak:

• GEOSisValid calls IsValidOp::isValid() and IsValidOp::getValidationError().
• Both of these methods call IsValidOp::checkValid(const Geometry *g)
• The first time IsValidOp::checkValid is called, isChecked is false and validErr is NULL. An error is found, a new TopologyValidationError object is created and assigned to validErr, but isChecked is still false.
• Subsequent calls to IsValidOp::checkValid, isChecked is still false, validErr is assigned NULL, losing the previous TopologyValidationError object. A new TVE object is later allocated and assigned to validErr, isChecked is still false.

The fix:

• at the end of IsValidOp::checkValid(const Geometry *g), set isChecked to true. validErr is either still NULL (if no error was found) or it may point to a TVE object.

I did not originally download the svn trunk version, so I can't create a proper patch, but here is my diff from the geos-3.0.0 version that I downloaded in Sept. The change is on line 129.

[denise@localhost valid]$ diff -C5 IsValidOp.cpp.orig IsValidOp.cpp
*** IsValidOp.cpp.orig  2008-10-22 14:02:31.000000000 -0400
--- IsValidOp.cpp       2008-10-23 09:54:46.000000000 -0400
***************
*** 124,133 ****
--- 124,134 ----
        else if (typeid(*g)==typeid(Polygon)) checkValid((Polygon*)g);
        else if (typeid(*g)==typeid(MultiPolygon)) checkValid((MultiPolygon*)g);
        else if ((gc=dynamic_cast<const GeometryCollection *>(g)))
                checkValid(gc);
        else throw util::UnsupportedOperationException();
+       isChecked=true;
  }

  /*
   * Checks validity of a Point.
   */

[warmerdam]

Denise, do you happen to have a small test program to demonstrate the problem and the fix? I tried running some of the unit tests in 3.0 branch and they don't seem to demonstrate the leak.

[denisem]

Test program is attached, and an invalid polygon in WKT.

Compile: gcc test_issue169.cpp -lgdal -o test_issue169

Test command (with valgrind): valgrind --leak-check=full ./test_issue169 test_polygon.wkt

[warmerdam]

Denise,

Thanks, that was exactly what I needed. I have applied the patch in 3.0 branch (r2209) and trunk (r2210).