Fix out of bounds read in _baseCellToCCWrot60 for face == NUM_ICOSA_FACES (#978) - #1224
Merged
Merged
Conversation
…ACES The bounds check used > instead of >=, so a face index equal to NUM_ICOSA_FACES (20) passed validation and indexed one element past the end of faceIjkBaseCells[NUM_ICOSA_FACES][3][3][3]. Reproduced as an AddressSanitizer global-buffer-overflow at baseCells.c:882 under the sanitizer flags the repo CI already uses. Adds a regression test that aborts without the fix. Fixes uber#978
isaacbrodsky
approved these changes
Aug 20, 2026
Contributor
Author
|
Thanks for the review! Flagging the one red check:
Happy to rebase or push an empty commit if you would like the fuzzer job re-run, though that would dismiss the approval, so I would rather leave it to you. |
nrabinowitz
approved these changes
Aug 20, 2026
Collaborator
|
Thanks for the fix! The fuzzer issue does not look related, I think we can ignore for this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #978.
The bug
_baseCellToCCWrot60guards itsfaceargument with:The table it then indexes is declared as:
so valid indices are
0 .. NUM_ICOSA_FACES - 1.face == NUM_ICOSA_FACES(20) passes the check and the loop reads one whole[3][3][3]row past the end of the array.The sibling function
_faceIjkToBaseCelland friends use the correct>=form; this one is the odd one out.Reproduction
Built with the same sanitizer flags the repo CI already uses in
.github/workflows/test-linux.yml(-fsanitize=address -fno-sanitize-recover=address), calling_baseCellToCCWrot60(16, NUM_ICOSA_FACES):exit code 1. With the fix applied the same binary passes.
Note for reviewers: in a plain non-sanitizer build the added assertion happens to pass without the fix, because the out of bounds read is undefined behaviour that usually returns garbage that does not equal
INVALID_ROTATIONSanyway. It is an effective regression test specifically under the ASAN/MSAN/UBSAN/Valgrind jobs this repo already runs on every PR, which is where the bug is actually detectable.Change
>becomes>=in the bounds check.testBaseCellsInternal.calongside the existing invalid-face assertions.This is one of the checks called out in #430: "I did not port two of the checks in baseCells.c since I could not readily test them. They could be added in a future PR."
Testing
Full suite under ASAN:
100% tests passed, 0 tests failed out of 317.