Fix double free when a comparator panics mid-split_off#158710
Open
chatman-media wants to merge 1 commit into
Open
Fix double free when a comparator panics mid-split_off#158710chatman-media wants to merge 1 commit into
chatman-media wants to merge 1 commit into
Conversation
…Map::split_off split_off's loop interleaves search_node (which calls into user Ord/Borrow code and can panic) with move_suffix, which physically relocates key-value pairs from the left tree into the still-being-built right tree one level at a time. If the comparator panics after the first move_suffix has already run, the unwind leaves self with its old, too-large length but a tree that's missing the part that got moved into the right-hand root (which then gets dropped along with the panic). Iterating or dropping the map afterwards walks past the border into node slots that no longer own what they claim to, and can double free. Since search_node is the only failure point and it's pure (no mutation until move_suffix runs), a small drop guard that only gets armed once the first move actually happens turns the panic into a clean abort, which matches how mem::replace already handles the equivalent situation elsewhere in this module. Added a regression test for the ordinary multi-level path too, since get_or_insert vs get_or_insert_with is an easy thing to get backwards here (get_or_insert takes its arg by value, so it'll construct-and-immediately- drop a fresh guard on every loop iteration after the first).
Collaborator
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @Darksonn (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
Darksonn
reviewed
Jul 3, 2026
| let mut guard = None; | ||
|
|
||
| loop { | ||
| let mut split_edge = match left_node.search_node(key) { |
Member
There was a problem hiding this comment.
Just to help me understand: this call to search_node is the only line that actually calls into code that can panic, right? So for this to result in an abort, it has to fail on at least the second iteration and it has to fail here?
Darksonn
reviewed
Jul 3, 2026
| // unaffected; the abort itself can't be observed from within a single | ||
| // process. See the reproducer attached to the issue for the double free. | ||
| #[test] | ||
| fn test_split_off_multi_level_unaffected_by_panic_guard() { |
Member
There was a problem hiding this comment.
Let's also add the actual failing code from #158165 as a regression test.
Member
|
@rustbot author |
Collaborator
|
Reminder, once the PR becomes ready for a review, use |
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
3 participants
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.
Root::split_off's loop callssearch_node(which invokes the caller'sOrd/Borrowimpl and can panic) interleaved withmove_suffix, which physically relocates key-value pairs out of the left tree and into the new right-hand tree one level at a time. If the comparator panics on a level after the firstmove_suffixhas already run, the unwind leavesselfwith its old, too-largelengthbut a tree that's missing whatever already got moved intoright_root— andright_rootitself gets dropped along with the panic, freeing those values. Iterating or dropping the "recovered" map afterwards walks past the border into node slots that no longer own what they claim to, and double frees. Reproducer from #158165 (comparator that panics oncmp(6, 0), caught withcatch_unwind, then the map is dropped) crashes withmalloc(): double free detectedon stable/beta/nightly.search_nodeis pure — it's the only thing here that can fail, and nothing gets mutated untilmove_suffixruns — so a drop guard that only gets armed once the first move actually happens turns the panic into a clean abort instead. This is the same strategymem::replacealready uses a few lines away in this same module for an analogous "can't safely unwind out of this" situation.Worth flagging for review: my first pass at this used
guard.get_or_insert(AbortOnDrop), which takes its argument by value — so on every loop iteration after the first it would construct a freshAbortOnDrop, find theOptionalreadySome, and immediately drop (and thus abort on) the throwaway one, turning every ordinary multi-level split into a crash. Caught it by testing against the fullalloctestssuite before deciding this was ready;get_or_insert_withis the actual fix,get_or_insertwas silently wrong for stateful guard types like this. Added a regression test for exactly that (multi-level, non-panicking splits) alongside the existingsplit_offtests.Verified with
-Z build-stdagainst a patched localalloc: the double-free repro now aborts cleanly instead of continuing into the corrupted state, a panic on the very first comparison (before any move) still unwinds normally viacatch_unwindas before, and all 334 tests inalloctests(173 of them inbtree) pass, includingBTreeSet::split_offwhich goes through the sameRoot::split_off.