Memory leaks in liblwgeom ?

[strk]

Valgrind analisys of liblwgeom/cunit/cu_tester run:

==6914== HEAP SUMMARY:
==6914==     in use at exit: 69,592 bytes in 1,022 blocks
==6914==   total heap usage: 9,990 allocs, 8,968 frees, 772,317 bytes allocated

I'll attach full log.

[strk]

run of liblwgeom/cunit/cu_tester in r7570 with valgrind

[strk]

With r7578 I've fixed a few leaks which were in the unit tests themselves, but next big leak really comes from library, and is related to WKT errors during parsing. It is triggered by passing 'TIN(((0 1 2,3 4 5,6 7 8,9 10 11,0 1 2)))' to lwgeom_from_ewkt. The code will allocate a POINTARRAY and then bail out calling lwerror, failing to release the allocated POINARRAY.

[strk]

I belive the culprit here is lwgeom_clone taking no effort to properly set the referenced PTARRAY readonly flags, so if you clone an LWGEOM with fully-owned PTARRAY you'd end up with two versions pointing to the same PTARRAY and both marked as owning it (lw*_clone uses memcpy)

[strk]

Uhm, it's more complex than that. the memcpy doesn't even copy the POINTARRAY but keeps it by reference. We'd need to have a ptarray_clone doing shallow copy and ptarray_clone_deep to do what the current ptarray_clone does… confusing.

[strk]

*manually* copying the POINARRAY (but not the serialized_pointlist) seems to resolve the issue. Would still be worth cleaning things up so that ptarray_clone does this instead.

[strk]

patch ensuring base type clones do copy POINTARRAY memory (w/out serialized points)

[strk]

The attached patch implements proper cloning of POINTARRAY elements in lwpoint_clone, lwline_clone and lwpoly_clone. Serialized pointlist is _not_ copied but simply marked as READONLY. A more integrated approach might be porting the readonly/non-readonly (or better _owned_/_not_owned_ flags in the LWGEOM structure itself, but I'm not sure it'd save us much.

Conceptual and practical reviews of the patch are welcome. If we like the idea, it should be better documented that lwgeom_clone does not copy the _serialized_points_, not the POINTARRAY (as currently documented).

[pramsey]

You have shown this patch to affect your TIN case? Because I feel like I've tracked the TIN case to an altogether different piece of code, the roll-back code in the WKT parser which goes into effect when an error is caught. It correctly rolls back objects still on the parser stack, but at the time the error is forced for your case the ptarray has already been moved off the stack so it doesn't get freed.

This is not to say that the semantics of clone() might be wrong/off, they just don't seem germane to the original ticket issue.

[pramsey]

I think I have a fix for the actual ticket issue at r7587

[strk]

I confirm my _clone patch is not directly related to the _specific_ issue I was talking about in the first comments. Your patch is probably good for that. Since this ticket is generally summarized as "memory leaks in liblwgeom" I was tracking a different spot, the one that forced me commit r7579 as a partial rollback of some memory cleanup work done with r7578.

See how it seems impossible to release all memory in the function modified by r7579 unless (if things didn't change with your modifications).

[strk]

After r7590 the only leaks detected by valgrind runs of liblwgeom/cunit/cu_tester come from cu_homogenize:

definitely lost: 15,667 bytes in 346 blocks

I think we want these fixed before releasing 2.0

[strk]

Oh, another small but continuos leak seems to be in WKT parser, where 2 bytes get struped and are lost forever (for each WKT input parsed). Thi sis lwin_wkt_lex.l:96

[pramsey]

I think I have recovered the missing 2 bytes. r7591 I don't think we need to strdup that data because we never use it as a string later on, we just look at it to see if it has Z or M in it.

[strk]

Alright, definitely lost down to 15,601 bytes in 314 blocks as of r7591

[strk]

This seems to also leak on error:

lwgeom_from_ewkt("POLYHEDRALSURFACE(((0 1 2,3 4 5,6 7,0 1 2)))", PARSER_CHECK_NONE);

It leaks a POINARRAY allocated by wkt_parser_ptarray_new (lwin_wkt.c:259)

[strk]

As of r7592, definitely lost bytes are down to 9,441 bytes in 264 blocks

[pramsey]

OK, closed that one at r7593

[strk]

Great, and as of r7600 we have 2,240 bytes in 162 blocks definitely lost.

Robe, next one is for you: lwout_x3d.c:64 creates a temporary lwgeom using lwgeom_as_multi and never releases it.

[strk]

I'll do the lwout_x3d.c part, so I also fix the warnings…

[strk]

It turns out the lwout_x3d.c part is also related to lwgeom_clone doing a partial clone. See r7602

[strk]

Plan: rename ptarray_clone to ptarray_clone_deep and keep ptarray_clone as a shallow copy too (to conform with lwgeom_clone).

[strk]

ptarray_clone → ptarray_clone_deep done in r7506, time to refine my _clone patch to use a new ptarray_clone for shallow cloning

[strk]

I've added ptarray_clone (shallow) in r7608, togheter with unit test for lwgeom_clone and lwgeom_release round.

In r7609 I've cleaned up lwgeom_homogenize to use lwgeom_free safely.

Definitely lost now: 248 bytes in 8 blocks (x3d and lwcollection_extract)

lwcollection_extract problem seems to be that geometries from input collection are added to output collection w/out cloning. I think it'd be ok to clone, given our clones are shallow by default (no serialized pointlist copied)

[strk]

r7610 makes lwcollection_extract shallow clone the extracted components, taking lost bytes down to 48 bytes in 6 blocks

[strk]

definitely lost 16 bytes in 2 blocks as of r7611

[strk]

And… definitely lost: 0 bytes in 0 blocks in r7612 That's all, folks.

[mcayland]

Nice work strk!

[pramsey]

Indeed, I feel cleaner already. Thanks strk.