Skip to content

Fix undefined behavior in polygonToCells test#636

Merged
isaacbrodsky merged 4 commits into
uber:masterfrom
isaacbrodsky:fix-polygon-undefined-test
Aug 16, 2022
Merged

Fix undefined behavior in polygonToCells test#636
isaacbrodsky merged 4 commits into
uber:masterfrom
isaacbrodsky:fix-polygon-undefined-test

Conversation

@isaacbrodsky
Copy link
Copy Markdown
Collaborator

@isaacbrodsky isaacbrodsky commented Aug 8, 2022

  • Fixes a test that had an invalid read. This was found by running the tests with ENABLE_FUZZERS, which enabled ASAN etc. We don't run this in CI but perhaps should.
  • Added a couple tests that exercise what happens when non-finite values are passed into polygonToCells. We previously had some casts from double to int64_t that were unsafe if the value was not finite because that cast would be undefined behavior.

Edit: happy to break this out into two PRs if that's easier.

Edit 2: This does not block #633

@coveralls
Copy link
Copy Markdown

coveralls commented Aug 8, 2022

Coverage Status

Coverage increased (+0.2%) to 98.863% when pulling 9378b6c on isaacbrodsky:fix-polygon-undefined-test into 6c77803 on uber:master.

@isaacbrodsky isaacbrodsky force-pushed the fix-polygon-undefined-test branch from 6b2608f to ead4005 Compare August 8, 2022 18:19
// missed branch in coverage
LatLng verts[] = {{0, 0}, {1, 0.5}, {0, 1}};
GeoLoop geoloop = {.numVerts = 4, .verts = verts};
GeoLoop geoloop = {.numVerts = 3, .verts = verts};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

Comment thread src/h3lib/lib/bbox.c Outdated
Comment thread src/h3lib/lib/bbox.c Outdated
Comment thread src/apps/testapps/testPolygonToCells.c Outdated
Comment thread src/apps/testapps/testPolygonToCells.c Outdated
@isaacbrodsky isaacbrodsky merged commit f6669a7 into uber:master Aug 16, 2022
@isaacbrodsky isaacbrodsky deleted the fix-polygon-undefined-test branch August 16, 2022 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants