Skip to content

Fix double-free when a comparator panics mid-split_off - #2

Draft
jonbaldie wants to merge 2 commits into
mainfrom
cursor/fix-btree-split-off-panic-safety-8d5d
Draft

Fix double-free when a comparator panics mid-split_off#2
jonbaldie wants to merge 2 commits into
mainfrom
cursor/fix-btree-split-off-panic-safety-8d5d

Conversation

@jonbaldie

@jonbaldie jonbaldie commented Aug 13, 2026

Copy link
Copy Markdown
Owner

r? Darksonn

Root::split_off currently interleaves search_node (which invokes the caller's Ord/Borrow impl and can panic) with move_suffix, which physically relocates key-value pairs into a temporary right-hand tree one level at a time. If the comparator panics on a level after the first move_suffix, unwind drops that right-hand tree while self.length is still the old, too-large value. IntoIter trusts that length and can double-free.

This is rust-lang#158165 (I-unsound). A comparator panic is allowed (e.g. RefCell::cmp); it must not become UB.

Approach

Previous attempts (rust-lang#158710, rust-lang#160901) aborted after the first move. That is sound, but it cannot host the issue's catch_unwind reproducer as a regression test (requested on rust-lang#158710), and it turns a recoverable RefCell panic into a process abort.

This change splits split_off into two passes:

  1. Walk root-to-leaf recording the split edge index at each level. This is the only pass that calls Ord. The tree is not mutated, so a panic leaves self intact.
  2. Replay those indices with move_suffix, then fix borders as before. Ord is not invoked here.

Validation

  • Standalone repro from the issue on rustc 1.83: len=12 reachable=6, then free(): double free detected.
  • Minimised: 12 keys (CAPACITY+1, min height 1), original insertion order, panic on cmp(6, 0). Height-0 (11 keys) already unwound cleanly.
  • Probe of comparisons during the panicking split_off: [(6, 6), (6, 0)] — first search at the root succeeds and moves the suffix; second search panics.
  • After the fix, the same repro against stage1 std recovers with len=12 reachable=12 and into_iter().nth_back(5) no longer double-frees.
  • ./x.py test library/alloc: 336 unit tests passed (including the two new regression tests), plus the rest of the alloc suite.

Fixes rust-lang#158165

AI/LLM disclosure

  • AI coding tools (including Cursor Grok) were used to help draft or modify code and this PR description.
  • I reviewed the complete change, understand the reasoning, and ran the reported local tests before submitting.
  • This submission is original work of authorship under the project CLA / contributor terms; AI output was not pasted unreviewed.
Open in Web Open in Cursor 

cursoragent and others added 2 commits August 13, 2026 19:16
A comparator that panics mid-split_off currently leaves the map with a
stale length after a suffix has already been moved into a temporary
right-hand tree. These tests assert that catching that panic must not
leave length and reachable structure disagreeing.

Co-authored-by: Jonathan Baldie <jonbaldie@users.noreply.github.com>
Root::split_off used to interleave Ord searches with move_suffix. A
panic in the comparator after the first move left the map with a stale
length and a partially detached tree, which consuming iteration could
then double-free.

All searches now complete before any key-value pair is relocated, so a
panic in Ord leaves the original map unchanged. Ordinary (non-panicking)
splits still walk the same edges in the same order.

Co-authored-by: Jonathan Baldie <jonbaldie@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BTreeMap::split_off is not panic-safe leading to a potential double-free

2 participants