Fix a TrieRef slice underflow when its key crosses a node boundary - #71
Open
imlvts wants to merge 1 commit into
Open
Fix a TrieRef slice underflow when its key crosses a node boundary#71imlvts wants to merge 1 commit into
imlvts wants to merge 1 commit into
Conversation
`TrieRef::new_with_key_and_path_in` descends one step before handing the
rest of the walk to `node_along_path`, and assumed the child it found
was reached at or beyond the end of `node_key`. In debug that
assumption was a `debug_assert!(consumed_byte_cnt >= node_key_len)`; in
release it was `&path[consumed_byte_cnt - node_key_len..]`, an unchecked
subtraction on `usize`.
The assumption does not hold. `create_path` materialises an empty node
part-way along a key, so `node_get_child` finds a child *inside*
`node_key` rather than past it, and the release build computed `2 - 6`
and indexed a slice at 18446744073709551612. From the public API:
map.create_path(&[2, 2]);
let mut wz = map.write_zipper_at_path(&[2, 2]);
wz.descend_to(&[2, 1, 3, 1]);
wz.val_at(&[1, 1]); // panic
When the child is reached inside `node_key`, what remains to walk is
`node_key[consumed..] ++ path`, which is a slice of neither. It does not
need to be: `node_along_path` below already walks a whole key from a
node and crosses boundaries itself, so that case now falls through to
it, exactly as the "no child at all" case already did. Both call sites
had the same code and both are fixed. This was the single largest panic
site the differential fuzzer found, 37% of a sample.
Regression test: `trie_ref_key_crossing_a_node_boundary`, with nothing
below the empty node and with values below it, through a write zipper
and through `trie_ref_at_path`. Fails before this change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BZmoASqM5FUuzvJeJaYQjR
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.
TrieRef::new_with_key_and_path_indescends one step before handing the rest of the walk tonode_along_path, and assumed the child it found was reached at or beyond the end ofnode_key. In debug that assumption was adebug_assert!(consumed_byte_cnt >= node_key_len); in release it was&path[consumed_byte_cnt - node_key_len..], an unchecked subtraction onusize.The assumption does not hold.
create_pathmaterialises an empty node part-way along a key, sonode_get_childfinds a child insidenode_keyrather than past it, and the release build computed2 - 6and indexed a slice at 18446744073709551612. From the public API:When the child is reached inside
node_key, what remains to walk isnode_key[consumed..] ++ path, which is a slice of neither. It does not need to be:node_along_pathbelow already walks a whole key from a node and crosses boundaries itself, so that case now falls through to it, exactly as the "no child at all" case already did. Both call sites had the same code and both are fixed. This was the single largest panic site the differential fuzzer found, 37% of a sample.Regression test:
trie_ref_key_crossing_a_node_boundary, with nothing below the empty node and with values below it, through a write zipper and throughtrie_ref_at_path. Fails before this change.