Skip to content
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

Fix potential segfault in gridDisk due to invalid digit #498

Merged
merged 1 commit into from
Jul 15, 2021

Conversation

isaacbrodsky
Copy link
Collaborator

@isaacbrodsky isaacbrodsky commented Jul 15, 2021

h3NeighborRotations would index off the end of the NEW_DIGIT_II table (and others) if the input index had invalid digits in it. This can result in a crash.

Detected via AFL fuzzer and then narrowed down what was happening with UBSan. The segfault itself only reproduces with certain build configurations.

@coveralls
Copy link

coveralls commented Jul 15, 2021

Coverage Status

Coverage increased (+0.001%) to 98.993% when pulling 5089842 on isaacbrodsky:kring-segfault into 4ce808f on uber:master.

@@ -346,7 +346,10 @@ H3Index h3NeighborRotations(H3Index origin, Direction dir, int *rotations) {
} else {
Direction oldDigit = H3_GET_INDEX_DIGIT(out, r + 1);
Direction nextDir;
if (isResolutionClassIII(r + 1)) {
if (oldDigit == INVALID_DIGIT) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am wondering if expanding the size of the constant matrices like NEW_DIGIT_II and having an invalid detection at the end would be a bit more performant due to less branch code. But since this is already a branch it probably has only a small impact?

(So not a blocker, but it might be worthwhile to make a branch less version of this function since it is often in the deepest parts of looping code.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not exactly sure how invalid detection would work in this case without doing a check for the digit being 7 here, or calling h3IsValid.

Further optimizing this function is a good idea since it is performance critical to gridDisk.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So it's a hunch, but because of CPU pipelining, I think something very much like that would be faster.

Get rid of the isResolutionClassIII check by just having pointers set at the beginning of the loop that is toggled between NEW_DIGIT_II/NEW_ADJUSTMENT_II and NEW_DIGIT_III/NEW_ADJUSTMENT_III each time r-- is also run, eliminating that conditional check.

Then the INVALID_DIGIT check can be turned into something like:

// before the while loop
byte foundInvalidDigit = 0;

...

// immediately after `oldDigit` is declared
foundInvalidDigit |= oldDigit == INVALID_DIGIT;

... // the rest of the loop continues on, with the tables having extra (invalid) rows for the invalid digit

// while loop is done
if (foundInvalidDigit) return H3_NULL; // Could have returned this earlier, but no need to optimize for this case

I might also change up while (true) to while (r > -1) and put the final part of the top-level conditional outside of the loop, but that might not have any measurable impact on performance as currently there's an unconditional jump at the end of the loop and then a conditional jump, while this would reduce it to just a conditional jump, but unconditional jumps are cheap to schedule for the CPU.

@isaacbrodsky isaacbrodsky merged commit fda03e2 into uber:master Jul 15, 2021
@isaacbrodsky isaacbrodsky deleted the kring-segfault branch July 15, 2021 17:02
isaacbrodsky added a commit that referenced this pull request Jul 16, 2021
…500)

* Backport fix potential segfault in kRing due to invalid digit (#498)

* Backport CI config
mrdvt92 pushed a commit to mrdvt92/h3 that referenced this pull request Jun 19, 2022
…) (uber#500)

* Backport fix potential segfault in kRing due to invalid digit (uber#498)

* Backport CI config
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.

None yet

4 participants