fix: type-check membership and handle-table pages against their tree position (#141) - #158
Merged
Merged
Conversation
…position Closes #141 (I148). The crate's three radixes disagreed about a threat the repo tests everywhere else: a page whose XXH3 checksum is valid — it is non-cryptographic and publicly recomputable — but whose STRUCTURE is wrong. `freemap_tree`, `overflow` and `data_page` fail closed with a typed CorruptPage; `membership_index` and `handle_table` failed open. The check is positional, not merely "is this one of the membership types": `expected_type_at(level)` requires MembershipLeaf at level 0 and MembershipInterior above. A leaf reached one level too early has its ENTRY words read as child pointers, and an interior reached at level 0 has its child pointers read as values — checking the type against the position catches both. The sharpest instance is corruption AMPLIFICATION rather than a bad read. `free_subtree` took an interior's child pointers at the depth-1 boundary and pushed them into `freed` with no read and no validation, so a corrupt-but-checksummed pointer became a page id marked REUSABLE at commit — a live data page, or superblock slot 1, handed back to the allocator. One bad pointer turns into arbitrary overwriting later. Validating at level 0 means leaves are now read rather than freed sight-unseen; that read is the point, since a page you never read cannot be type-checked. Adversarial review found two holes in the first version of this fix, both confirmed and both closed here: * `insert_recursive` was still unguarded at level 0, and pushed `page` into `freed` BEFORE validating anything. Same amplification on the insert path, and worse: `delete` at least returned CorruptPage, while `insert` SUCCEEDED silently — COWing a foreign page, queueing the original for reclamation, and letting commit mark a live page reusable with nothing surfacing. Reachable via a corrupt child pointer or a corrupt packed outer-leaf value whose depth bits read as 0. The check now runs at the top of the frame, before the alloc/copy/push, matching `delete_recursive`. * The issue's fix direction names `HandleTable`+FLAG too, and the first version touched only membership_index. `HandleTable::find_leaf` descended with no check at all — only `recover_depth` validated. The handle table tags position with a FLAG byte rather than a distinct PageType, so both bytes are checked: `buf[0]` proves it is a handle-table page, `buf[1] == FLAG_INTERIOR` proves it belongs at this level. `iter_bounded_saturates_prefix_on_corrupt_max_depth` previously asserted that walking a self-referential interior at MAX_DEPTH SUCCEEDED — the fail-open behaviour this issue is about. It now asserts CorruptPage, and still covers the property it was written for: `next_base` saturates in the top-level frame before the recursion that now fails, so a regression to non-saturating arithmetic still panics it. Cost: freeing a subtree reads its leaves as well as its interiors — O(leaves) extra cache lookups on a tag-drop, an operation already O(n). Under cache pressure those reads can surface CacheFull where the old push-blind code could not fail; that is operational, not fatal, and inherent to having a guard here.
This was referenced Aug 5, 2026
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.
Closes #141 (I148). Stacked on #157.
The crate's radixes disagreed about a threat the repo tests everywhere else: a page whose XXH3 checksum is valid — it is non-cryptographic and publicly recomputable — but whose structure is wrong.
freemap_tree,overflow,data_pageCorruptPagemembership_index,handle_tableThe check is positional, not merely "is this one of the membership types".
expected_type_at(level)requiresMembershipLeafat level 0 andMembershipInteriorabove. A leaf reached one level too early has its entry words read as child pointers; an interior reached at level 0 has its child pointers read as values. Checking type against position catches both.The sharpest instance is corruption amplification, not a bad read
free_subtreetook an interior's child pointers at the depth-1 boundary and pushed them intofreedwith no read and no validation. A corrupt-but-checksummed pointer therefore became a page id marked reusable at commit — a live data page, or superblock slot 1, handed back to the allocator. One bad pointer turns into arbitrary overwriting later.Validating at level 0 means leaves are now read rather than freed sight-unseen. That read is the point: a page you never read cannot be type-checked.
Two holes the adversarial review found in the first version
Both confirmed by reproduction, both closed here:
insert_recursivewas still unguarded at level 0, and pushedpageintofreedbefore validating anything. Same amplification on the insert path, and worse:deleteat least returnedCorruptPage, whileinsertsucceeded silently — COWing a foreign page, queueing the original for reclamation, letting commit mark a live page reusable, with nothing surfacing. Reachable via a corrupt child pointer, or a corrupt packed outer-leaf value whose depth bits read as 0. The check now runs at the top of the frame, before the alloc/copy/push, matchingdelete_recursive.The issue names
HandleTable+FLAG too, and the first version touched onlymembership_index.HandleTable::find_leafdescended with no check at all — onlyrecover_depthvalidated. The handle table tags position with a FLAG byte rather than a distinctPageType, so both bytes are checked:buf[0]proves it is a handle-table page,buf[1] == FLAG_INTERIORproves it belongs at this level.One test changed meaning
iter_bounded_saturates_prefix_on_corrupt_max_depthpreviously asserted that walking a self-referential interior atMAX_DEPTHsucceeded — the fail-open behaviour this issue is about. It now assertsCorruptPage, and still covers the property it was written for:next_basesaturates in the top-level frame before the recursion that now fails, so a regression to non-saturating arithmetic still panics it.Cost
Freeing a subtree reads its leaves as well as its interiors — O(leaves) extra cache lookups on a tag-drop, an operation already O(n). Under cache pressure those reads can surface
CacheFullwhere the old push-blind code could not fail; that is operational, not fatal, and inherent to having a guard here at all.Verification
721 tests pass, clippy and fmt clean. Both new tests verified non-vacuous by removing their guard.