Memory leak in lwgeom_to_points

[dbaston]

==8742== 1,024 bytes in 1 blocks are definitely lost in loss record 3 of 4
==8742==    at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==8742==    by 0x4E5C2BB: lwcollection_reserve (lwcollection.c:180)
==8742==    by 0x4E5CB69: lwcollection_add_lwgeom (lwcollection.c:211)
==8742==    by 0x4E8668C: lwpoly_to_points (lwgeom_geos.c:1710)
==8742==    by 0x4E868A0: lwmpoly_to_points (lwgeom_geos.c:1768)
==8742==    by 0x406416: test_point_density (cu_algorithm.c:1014)
==8742==    by 0x52EAC99: ??? (in /usr/lib/libcunit.so.1.0.1)
==8742==    by 0x52EAF27: ??? (in /usr/lib/libcunit.so.1.0.1)
==8742==    by 0x52EB2A5: CU_run_all_tests (in /usr/lib/libcunit.so.1.0.1)
==8742==    by 0x40620E: main (cu_tester.c:163)

[pramsey]

Looks like it was just the test leaking. Should be fixed at r14509

[dbaston]

It's more than the test. On line 1782, lwgeom_release is leaving sub_mpt->geoms intact, and the reference isn't being given to anyone else. Adding a lwfree(sub_mpt->geoms) on the following line fixes it but somehow feels wrong. Is it?

What do you think about adding a lwmpoint_add_lwmpoint that would handle this situation?

[pramsey]

The answer might be a review of the use and semantics of lwgeom_release. i don't think it's widely used, so we can tweak it slightly. I think it's fair to say the semantic is: "release the container but leave the contained". for for a lienstring, it would leave the pointarray intact, and for a polygon it would leave the pointarrays intact, but drop the rings[] array. And for collections it would leave the sub-geometries intact but lose the geoms[]. But possibly it's being used somewhere outside that semantic box.

[dbaston]

Should we just fix the leak directly for this release?

[strk]

Fix in trunk, check with valgrind, mark for backport if everyone's happy

[pramsey]

Well, I tried extending the semantic of lwgeom_release very slightly, to also free the underlying array containing the geoms, but unfortunately the topology code is tied to the existing semantic. It's the only major user of lwgeom_release, almost nothing else does. Here's what I tried

void
lwgeom_release(LWGEOM *lwgeom)
{
	if ( ! lwgeom )
		lwerror("lwgeom_release: someone called on 0x0");

	LWDEBUGF(3, "releasing type %s", lwtype_name(lwgeom->type));

	if (lwgeom_is_collection(lwgeom)) 
	{
		LWCOLLECTION *lwcol = lwgeom_as_lwcollection(lwgeom);
		if (lwcol && lwcol->geoms) 
			lwfree(lwcol->geoms);
	}
	
	if (lwgeom_get_type(lwgeom) == POLYGONTYPE) 
	{
		LWPOLY *lwpoly = lwgeom_as_lwpoly(lwgeom);
		if (lwpoly && lwpoly->rings) 
			lwfree(lwpoly->rings);
	}

	/* Drop bounding box (always a copy) */
	if ( lwgeom->bbox )
	{
		LWDEBUGF(3, "lwgeom_release: releasing bbox. %p", lwgeom->bbox);
		lwfree(lwgeom->bbox);
	}
	lwfree(lwgeom);

}

I'm just going to do the single-point patch you proposed Dan, to fix this particular function. A library-wide fix on the semantics of cloning/freeing/releasing is beyond scope… really at any time at all.

[pramsey]

Point fix applied to trunk at r15640