-
Notifications
You must be signed in to change notification settings - Fork 445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Polygon to cells experimental fuzzer #800
base: master
Are you sure you want to change the base?
Polygon to cells experimental fuzzer #800
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that maxPolygonToCellsSize
doesn't allocate enough cells for the new modes in some cases. I've seen similar behavior here in benchmarks - this could be an issue that affects the old polygonToCells
as well.
I have a plan for a new maxPolygonToCellsSize
algo that I'll try out, this may fix the fuzzer errors.
Has this polygon been added as a test case to ensure we aren't dropping a lot of cells there? |
I'd rather add some simplified version, as the Antarctica polygon is too large to be easily added to a test file. I can add a new PR with more tests for this. |
Still seeing errors here with the new estimator. Is there an easy-ish way to get the input for the failure cases? |
I added a test case that demonstrates this. (If you run under valgrind it will show an issue.) It seems we do not actually test a polygon with a single vertex being passed in. If you do so with res=0, flags=2 (CONTAINMENT_OVERLAPPING), the estimate will be 0 but the function will try to write the buffer. |
src/h3lib/lib/polyfill.c
Outdated
@@ -418,6 +418,12 @@ void iterStepPolygonCompact(IterCellsPolygonCompact *iter) { | |||
iter->_started = true; | |||
} | |||
|
|||
if (iter->_polygon->geoloop.numVerts == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be changed to 0 or 1 returns nothing? @nrabinowitz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the case for 1 and 2 vertexes is equivalent - in both cases, we have input that cannot be a 2D polygon, and we either have to discard it or treat it like a shape. While it's obviously a pathological case, I think it would be reasonable to assume that If you have a one-vertex "polygon" and you use CONTAINMENT_OVERLAPPING
that you'd get the cell that contains the point. There's also a potential use case for line segments in the 2-vertex case. I'm also concerned that the estimate and the implementation don't line up.
Should I take a shot at a PR to fix this, which we can land before this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that seems like the expected behavior, if you have an idea for addressing please take a look.
I can help apply these tests to another branch to help validate.
This includes the fixes from #800, plus one more fix for estimating a 1-vertex polygon. Includes the following tests: 0-vertex polygon, all modes 1-vertex polygon, all modes 2-vertex polygon, all modes polygon with 0-vertex hole, all modes polygon with 1-vertex hole, all modes polygon with 2-vertex hole, all modes
Here is the hexdump of a new crash case for
|
Nick Rabinowitz seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
This adds new fuzzers for the
polygonToCellsExperimental
function, based on the existing functions. Since #796 (this PR is based on that branch) adds containment modes, this updates the existing fuzzers to exercise those options too.When I run the
fuzzerPolygonToCellsExperimental
, I see some runtime errors about misaligned addresses, which doesn't cause an issue on Apple M1 architecture but might indicate trouble on other architectures. When I runfuzzerPolygonToCellsExperimentalNoHoles
I get a crash aboutheap-buffer-overflow
aroundpolygonToCellsExperimental polyfill.c:646
(looks like the output exceeds the allocated output buffer in some case?)