uber / h3-js

h3-js provides a JavaScript version of H3, a hexagon-based geospatial indexing system.
https://uber.github.io/h3
Apache License 2.0
875 stars 79 forks source link

Fix bug in freeing geo polygon memory #104

Closed nrabinowitz closed 4 years ago

nrabinowitz commented 4 years ago

Closes #103

This fixes some errors in freeing GeoPolygon memory, which could get the Emscripten virtual memory management into a bad state.

Explanation of the issue: We were expecting the GeoPolygon reference to Geofence to be a pointer, but it’s not, the struct is inlined into GeoPolygon. So I was trying to free the value at the geofence position, because I thought it was a pointer, but I’m actually getting the first value in the Geofence struct, which is numVerts, and trying to free that. C would probably segfault at this point, Emscripten just gets confused and ends up with undefined behavior in future calls.

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 217


Totals Coverage Status
Change from base Build 213: 0.0%
Covered Lines: 399
Relevant Lines: 399

💛 - Coveralls
nrabinowitz commented 4 years ago

Are there any ways to test for this class of bugs? For example is there an equivalent to Valgrind that can be used to check that the allocated/freed buffers match up?

I honestly don't know. The memory starts off clean, so all of the issues are deterministic. The original test case (the pair of polygons in #103) was deterministic, so I could test via script and determine whether the counts matched or not. The test I put into the test suite was deterministic too, but not in the way I'd like - if it mismanaged the memory, that test still passed, but 23 other subsequent tests failed with weird behavior. So I can reasonably assert that I fixed the original bug. But I doubt there's any Valgrind equivalent just for Emscripten-compiled code :/. I'm a bit worried here that I don't have a great way to assert that the holes arrays are freed correctly in this PR - I couldn't manage a failing test here, even though I can see that the previous code doesn't look correct.