Fix double free when a comparator panics mid-split_off - #160901
Closed
sankalpsthakur wants to merge 1 commit into
Closed
Fix double free when a comparator panics mid-split_off#160901sankalpsthakur wants to merge 1 commit into
sankalpsthakur wants to merge 1 commit into
Conversation
Root::split_off interleaves Ord-invoking search_node with move_suffix. If the comparator panics after the first move, unwind left trees in an inconsistent state that double-frees on drop. Abort after the first move_suffix instead, matching the panic-safety pattern in btree/mem.rs. Fixes rust-lang#158165 Signed-off-by: Sankalp Thakur <sankalphimself@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @clarfonthey (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:
|
Collaborator
|
Contributor
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.
Summary
Revives the approach from closed #158710 for #158165 (
I-unsound/P-high).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.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 matches the panic-safety strategymem::replacealready uses inbtree/mem.rs.Uses
get_or_insert_with(notget_or_insert): the latter 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.Adds a regression test that ordinary multi-level (non-panicking) splits still work with the guard armed across levels. The abort path itself cannot be observed from within a single process; see the issue reproducer for the double-free.
Validation
main;split.rsstill matched the Fix double free when a comparator panics mid-split_off #158710 base (no intervening conflict).split.rs+map/tests.rs)../x.py test library/allocnot run locally (sparse clone / disk); relying on CIalloctestsand the prior PR's local verification (334 alloctests including 173 btree, plus-Z build-stdabort-vs-unwind checks described on Fix double free when a comparator panics mid-split_off #158710).Fixes #158165
AI/LLM disclosure
Made with Cursor