paulsmith / gogeos

Go library for spatial data operations and geometric algorithms (Go bindings for GEOS)
http://paulsmith.github.io/gogeos/
MIT License
280 stars 79 forks source link

Memory leak #29

Open denverquane opened 4 years ago

denverquane commented 4 years ago

The wkt encoding function leaks memory, as it does not free the C String allocated when encoding.

You can verify this with a simple example that encodes a polygon to a wkt string, and analyze the executable with Valgrind:

valgrind --leak-check=full --show-leak-kinds=all ./main
...
==26520== LEAK SUMMARY:
==26520==    definitely lost: 41,100 bytes in 100 blocks
==26520==    indirectly lost: 0 bytes in 0 blocks
==26520==      possibly lost: 3,552 bytes in 6 blocks
==26520==    still reachable: 250,616 bytes in 2,488 blocks
==26520==         suppressed: 0 bytes in 0 blocks

And here is an example of one of the lost memory records (evidently a trace to the wkt encoding):

==26520== 411 bytes in 1 blocks are definitely lost in loss record 228 of 363
==26520==    at 0x4C29F73: malloc (vg_replace_malloc.c:309)
==26520==    by 0x4E4BF34: (anonymous namespace)::gstrdup_s(char const*, unsigned long) (geos_ts_c.cpp:311)
==26520==    by 0x4E562CE: gstrdup (geos_ts_c.cpp:330)
==26520==    by 0x4E562CE: operator() (geos_ts_c.cpp:2516)
==26520==    by 0x4E562CE: execute<GEOSWKTWriter_write_r(GEOSContextHandle_t, geos::io::WKTWriter*, const geos::geom::Geometry*)::__lambda140, nullptr> (geos_ts_c.cpp:379)
==26520==    by 0x4E562CE: GEOSWKTWriter_write_r (geos_ts_c.cpp:2518)
==26520==    by 0x4CA94E: _cgo_d650cf0434ad_Cfunc_GEOSWKTWriter_write_r (cgo-gcc-prolog:3024)
==26520==    by 0x45B42F: runtime.asmcgocall (/usr/local/go/src/runtime/asm_amd64.s:655)
==26520==    by 0xC00018A137: ???
==26520==    by 0x51541F: ??? (in /home/***)

Compare this same test with just the fix provided in this PR:

valgrind --leak-check=full --show-leak-kinds=all ./main
...
==26894== LEAK SUMMARY:
==26894==    definitely lost: 0 bytes in 0 blocks
==26894==    indirectly lost: 0 bytes in 0 blocks
==26894==      possibly lost: 3,552 bytes in 6 blocks
==26894==    still reachable: 232,448 bytes in 2,302 blocks
==26894==         suppressed: 0 bytes in 0 blocks

Valgrind shouldn't typically be used for analyzing executables built with Go because of the GC, but in this context we can see that the version that doesn't free the C String certainly leaks memory (as it did with our deployment, and we traced it back to this function).