Abort instead of unwinding out of an inconsistent BTreeMap::split_off - #161784

Closed
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety
Closed

Abort instead of unwinding out of an inconsistent BTreeMap::split_off#161784
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety

Conversation

@matthew-demidoff

@matthew-demidoffmatthew-demidoff commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Fixes#158165.

BTreeMap::split_off runs the caller's Ord/Borrow impl via search_node inside Root::split_off's descent. After the first move_suffix, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from #![forbid(unsafe_code)] on stable (reproducer on the issue).

Fix: guard the descent loop with an abort-on-panic PanicGuard, as btree::mem::replace already does. Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.

Revives the approach from #158710 (closed for inactivity); credit to @ostrowr for the report.

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 25, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

r? @nia-e

rustbot has assigned @nia-e.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: libs
  • libs expanded to 12 candidates
  • Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, nia-e

@rustbot

This comment has been minimized.

@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from de7b14a to 05c9135CompareAugust 25, 2026 21:12

@nia-enia-e left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments, but the overall approach seems good.

@rustbot author

View changes since this review

// unwinding through it double-frees the shared values (#158165). Abort
// instead, matching the panic-safety strategy used elsewhere in this
// module (see `mem::replace`).
struct PanicGuard;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use either a bare catch_unwind or a DropGuard?

// inconsistent. `get_or_insert_with` (rather than `get_or_insert`)
// avoids constructing a fresh `PanicGuard` on later iterations,
// which would drop immediately and abort.
guard.get_or_insert_with(|| PanicGuard);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're rearming the guard every loop iteration? hm. why not just arm it once before the loop and disarm after, or just envelop the entire loop in a catch_unwind?

@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

After the first move_suffix in Root::split_off, the source and result
trees share values through two temporarily invalid structures. If a
later key comparison panics, unwinding leaves the map with a stale
length over a partially detached tree, and consuming iteration then
double-frees the shared values, reachable from safe code.
Guard the descent loop with an abort-on-panic PanicGuard, armed before
the first move_suffix and forgotten once the borders are fixed, matching
the strategy already used in btree::mem::replace.
@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from 05c9135 to dc0341aCompareAugust 29, 2026 12:54
@rustbotrustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 29, 2026
@matthew-demidoff

Copy link
Copy Markdown
ContributorAuthor

Sorry for the noise - I botched a force-push from a shallow clone and this PR's head got detached, so I could not reopen it. Reopened as #161975 with your feedback addressed (guard armed once before the loop; the first search stays outside it).

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
rust-borsBot pushed a commit that referenced this pull request Aug 30, 2026
Rollup merge of #161975 - matthew-demidoff:btree-split-off-panic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixes#158165. Supersedes #161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to #158710 for the original approach.
r? @nia-e
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-libsRelevant to the library team, which will review and decide on the PR/issue.

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

3 participants

@matthew-demidoff@rustbot@nia-e
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Abort instead of unwinding out of an inconsistent BTreeMap::split_off - #161784

Closed
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety
Closed

Abort instead of unwinding out of an inconsistent BTreeMap::split_off#161784
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety

Conversation

@matthew-demidoff

@matthew-demidoffmatthew-demidoff commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Fixes#158165.

BTreeMap::split_off runs the caller's Ord/Borrow impl via search_node inside Root::split_off's descent. After the first move_suffix, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from #![forbid(unsafe_code)] on stable (reproducer on the issue).

Fix: guard the descent loop with an abort-on-panic PanicGuard, as btree::mem::replace already does. Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.

Revives the approach from #158710 (closed for inactivity); credit to @ostrowr for the report.

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 25, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

r? @nia-e

rustbot has assigned @nia-e.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: libs
  • libs expanded to 12 candidates
  • Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, nia-e

@rustbot

This comment has been minimized.

@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from de7b14a to 05c9135CompareAugust 25, 2026 21:12

@nia-enia-e left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments, but the overall approach seems good.

@rustbot author

View changes since this review

// unwinding through it double-frees the shared values (#158165). Abort
// instead, matching the panic-safety strategy used elsewhere in this
// module (see `mem::replace`).
struct PanicGuard;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use either a bare catch_unwind or a DropGuard?

// inconsistent. `get_or_insert_with` (rather than `get_or_insert`)
// avoids constructing a fresh `PanicGuard` on later iterations,
// which would drop immediately and abort.
guard.get_or_insert_with(|| PanicGuard);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're rearming the guard every loop iteration? hm. why not just arm it once before the loop and disarm after, or just envelop the entire loop in a catch_unwind?

@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

After the first move_suffix in Root::split_off, the source and result
trees share values through two temporarily invalid structures. If a
later key comparison panics, unwinding leaves the map with a stale
length over a partially detached tree, and consuming iteration then
double-frees the shared values, reachable from safe code.
Guard the descent loop with an abort-on-panic PanicGuard, armed before
the first move_suffix and forgotten once the borders are fixed, matching
the strategy already used in btree::mem::replace.
@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from 05c9135 to dc0341aCompareAugust 29, 2026 12:54
@rustbotrustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 29, 2026
@matthew-demidoff

Copy link
Copy Markdown
ContributorAuthor

Sorry for the noise - I botched a force-push from a shallow clone and this PR's head got detached, so I could not reopen it. Reopened as #161975 with your feedback addressed (guard armed once before the loop; the first search stays outside it).

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
rust-borsBot pushed a commit that referenced this pull request Aug 30, 2026
Rollup merge of #161975 - matthew-demidoff:btree-split-off-panic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixes#158165. Supersedes #161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to #158710 for the original approach.
r? @nia-e
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-libsRelevant to the library team, which will review and decide on the PR/issue.

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

3 participants

@matthew-demidoff@rustbot@nia-e
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Abort instead of unwinding out of an inconsistent BTreeMap::split_off - #161784

Closed
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety
Closed

Abort instead of unwinding out of an inconsistent BTreeMap::split_off#161784
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety

Conversation

@matthew-demidoff

@matthew-demidoffmatthew-demidoff commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Fixes#158165.

BTreeMap::split_off runs the caller's Ord/Borrow impl via search_node inside Root::split_off's descent. After the first move_suffix, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from #![forbid(unsafe_code)] on stable (reproducer on the issue).

Fix: guard the descent loop with an abort-on-panic PanicGuard, as btree::mem::replace already does. Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.

Revives the approach from #158710 (closed for inactivity); credit to @ostrowr for the report.

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 25, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

r? @nia-e

rustbot has assigned @nia-e.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: libs
  • libs expanded to 12 candidates
  • Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, nia-e

@rustbot

This comment has been minimized.

@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from de7b14a to 05c9135CompareAugust 25, 2026 21:12

@nia-enia-e left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments, but the overall approach seems good.

@rustbot author

View changes since this review

// unwinding through it double-frees the shared values (#158165). Abort
// instead, matching the panic-safety strategy used elsewhere in this
// module (see `mem::replace`).
struct PanicGuard;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use either a bare catch_unwind or a DropGuard?

// inconsistent. `get_or_insert_with` (rather than `get_or_insert`)
// avoids constructing a fresh `PanicGuard` on later iterations,
// which would drop immediately and abort.
guard.get_or_insert_with(|| PanicGuard);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're rearming the guard every loop iteration? hm. why not just arm it once before the loop and disarm after, or just envelop the entire loop in a catch_unwind?

@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

After the first move_suffix in Root::split_off, the source and result
trees share values through two temporarily invalid structures. If a
later key comparison panics, unwinding leaves the map with a stale
length over a partially detached tree, and consuming iteration then
double-frees the shared values, reachable from safe code.
Guard the descent loop with an abort-on-panic PanicGuard, armed before
the first move_suffix and forgotten once the borders are fixed, matching
the strategy already used in btree::mem::replace.
@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from 05c9135 to dc0341aCompareAugust 29, 2026 12:54
@rustbotrustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 29, 2026
@matthew-demidoff

Copy link
Copy Markdown
ContributorAuthor

Sorry for the noise - I botched a force-push from a shallow clone and this PR's head got detached, so I could not reopen it. Reopened as #161975 with your feedback addressed (guard armed once before the loop; the first search stays outside it).

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
rust-borsBot pushed a commit that referenced this pull request Aug 30, 2026
Rollup merge of #161975 - matthew-demidoff:btree-split-off-panic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixes#158165. Supersedes #161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to #158710 for the original approach.
r? @nia-e
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-libsRelevant to the library team, which will review and decide on the PR/issue.

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

3 participants

@matthew-demidoff@rustbot@nia-e
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Abort instead of unwinding out of an inconsistent BTreeMap::split_off - #161784

Closed
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety
Closed

Abort instead of unwinding out of an inconsistent BTreeMap::split_off#161784
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety

Conversation

@matthew-demidoff

@matthew-demidoffmatthew-demidoff commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Fixes#158165.

BTreeMap::split_off runs the caller's Ord/Borrow impl via search_node inside Root::split_off's descent. After the first move_suffix, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from #![forbid(unsafe_code)] on stable (reproducer on the issue).

Fix: guard the descent loop with an abort-on-panic PanicGuard, as btree::mem::replace already does. Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.

Revives the approach from #158710 (closed for inactivity); credit to @ostrowr for the report.

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 25, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

r? @nia-e

rustbot has assigned @nia-e.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: libs
  • libs expanded to 12 candidates
  • Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, nia-e

@rustbot

This comment has been minimized.

@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from de7b14a to 05c9135CompareAugust 25, 2026 21:12

@nia-enia-e left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments, but the overall approach seems good.

@rustbot author

View changes since this review

// unwinding through it double-frees the shared values (#158165). Abort
// instead, matching the panic-safety strategy used elsewhere in this
// module (see `mem::replace`).
struct PanicGuard;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use either a bare catch_unwind or a DropGuard?

// inconsistent. `get_or_insert_with` (rather than `get_or_insert`)
// avoids constructing a fresh `PanicGuard` on later iterations,
// which would drop immediately and abort.
guard.get_or_insert_with(|| PanicGuard);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're rearming the guard every loop iteration? hm. why not just arm it once before the loop and disarm after, or just envelop the entire loop in a catch_unwind?

@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

After the first move_suffix in Root::split_off, the source and result
trees share values through two temporarily invalid structures. If a
later key comparison panics, unwinding leaves the map with a stale
length over a partially detached tree, and consuming iteration then
double-frees the shared values, reachable from safe code.
Guard the descent loop with an abort-on-panic PanicGuard, armed before
the first move_suffix and forgotten once the borders are fixed, matching
the strategy already used in btree::mem::replace.
@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from 05c9135 to dc0341aCompareAugust 29, 2026 12:54
@rustbotrustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 29, 2026
@matthew-demidoff

Copy link
Copy Markdown
ContributorAuthor

Sorry for the noise - I botched a force-push from a shallow clone and this PR's head got detached, so I could not reopen it. Reopened as #161975 with your feedback addressed (guard armed once before the loop; the first search stays outside it).

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
rust-borsBot pushed a commit that referenced this pull request Aug 30, 2026
Rollup merge of #161975 - matthew-demidoff:btree-split-off-panic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixes#158165. Supersedes #161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to #158710 for the original approach.
r? @nia-e
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-libsRelevant to the library team, which will review and decide on the PR/issue.

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

3 participants

@matthew-demidoff@rustbot@nia-e
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Abort instead of unwinding out of an inconsistent BTreeMap::split_off - #161784

Closed
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety
Closed

Abort instead of unwinding out of an inconsistent BTreeMap::split_off#161784
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety

Conversation

@matthew-demidoff

@matthew-demidoffmatthew-demidoff commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Fixes#158165.

BTreeMap::split_off runs the caller's Ord/Borrow impl via search_node inside Root::split_off's descent. After the first move_suffix, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from #![forbid(unsafe_code)] on stable (reproducer on the issue).

Fix: guard the descent loop with an abort-on-panic PanicGuard, as btree::mem::replace already does. Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.

Revives the approach from #158710 (closed for inactivity); credit to @ostrowr for the report.

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 25, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

r? @nia-e

rustbot has assigned @nia-e.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: libs
  • libs expanded to 12 candidates
  • Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, nia-e

@rustbot

This comment has been minimized.

@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from de7b14a to 05c9135CompareAugust 25, 2026 21:12

@nia-enia-e left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments, but the overall approach seems good.

@rustbot author

View changes since this review

// unwinding through it double-frees the shared values (#158165). Abort
// instead, matching the panic-safety strategy used elsewhere in this
// module (see `mem::replace`).
struct PanicGuard;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use either a bare catch_unwind or a DropGuard?

// inconsistent. `get_or_insert_with` (rather than `get_or_insert`)
// avoids constructing a fresh `PanicGuard` on later iterations,
// which would drop immediately and abort.
guard.get_or_insert_with(|| PanicGuard);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're rearming the guard every loop iteration? hm. why not just arm it once before the loop and disarm after, or just envelop the entire loop in a catch_unwind?

@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

After the first move_suffix in Root::split_off, the source and result
trees share values through two temporarily invalid structures. If a
later key comparison panics, unwinding leaves the map with a stale
length over a partially detached tree, and consuming iteration then
double-frees the shared values, reachable from safe code.
Guard the descent loop with an abort-on-panic PanicGuard, armed before
the first move_suffix and forgotten once the borders are fixed, matching
the strategy already used in btree::mem::replace.
@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from 05c9135 to dc0341aCompareAugust 29, 2026 12:54
@rustbotrustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 29, 2026
@matthew-demidoff

Copy link
Copy Markdown
ContributorAuthor

Sorry for the noise - I botched a force-push from a shallow clone and this PR's head got detached, so I could not reopen it. Reopened as #161975 with your feedback addressed (guard armed once before the loop; the first search stays outside it).

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
rust-borsBot pushed a commit that referenced this pull request Aug 30, 2026
Rollup merge of #161975 - matthew-demidoff:btree-split-off-panic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixes#158165. Supersedes #161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to #158710 for the original approach.
r? @nia-e
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-libsRelevant to the library team, which will review and decide on the PR/issue.

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

3 participants

@matthew-demidoff@rustbot@nia-e
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Abort instead of unwinding out of an inconsistent BTreeMap::split_off - #161784

Closed
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety
Closed

Abort instead of unwinding out of an inconsistent BTreeMap::split_off#161784
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety

Conversation

@matthew-demidoff

@matthew-demidoffmatthew-demidoff commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Fixes#158165.

BTreeMap::split_off runs the caller's Ord/Borrow impl via search_node inside Root::split_off's descent. After the first move_suffix, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from #![forbid(unsafe_code)] on stable (reproducer on the issue).

Fix: guard the descent loop with an abort-on-panic PanicGuard, as btree::mem::replace already does. Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.

Revives the approach from #158710 (closed for inactivity); credit to @ostrowr for the report.

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 25, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

r? @nia-e

rustbot has assigned @nia-e.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: libs
  • libs expanded to 12 candidates
  • Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, nia-e

@rustbot

This comment has been minimized.

@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from de7b14a to 05c9135CompareAugust 25, 2026 21:12

@nia-enia-e left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments, but the overall approach seems good.

@rustbot author

View changes since this review

// unwinding through it double-frees the shared values (#158165). Abort
// instead, matching the panic-safety strategy used elsewhere in this
// module (see `mem::replace`).
struct PanicGuard;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use either a bare catch_unwind or a DropGuard?

// inconsistent. `get_or_insert_with` (rather than `get_or_insert`)
// avoids constructing a fresh `PanicGuard` on later iterations,
// which would drop immediately and abort.
guard.get_or_insert_with(|| PanicGuard);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're rearming the guard every loop iteration? hm. why not just arm it once before the loop and disarm after, or just envelop the entire loop in a catch_unwind?

@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

After the first move_suffix in Root::split_off, the source and result
trees share values through two temporarily invalid structures. If a
later key comparison panics, unwinding leaves the map with a stale
length over a partially detached tree, and consuming iteration then
double-frees the shared values, reachable from safe code.
Guard the descent loop with an abort-on-panic PanicGuard, armed before
the first move_suffix and forgotten once the borders are fixed, matching
the strategy already used in btree::mem::replace.
@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from 05c9135 to dc0341aCompareAugust 29, 2026 12:54
@rustbotrustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 29, 2026
@matthew-demidoff

Copy link
Copy Markdown
ContributorAuthor

Sorry for the noise - I botched a force-push from a shallow clone and this PR's head got detached, so I could not reopen it. Reopened as #161975 with your feedback addressed (guard armed once before the loop; the first search stays outside it).

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
rust-borsBot pushed a commit that referenced this pull request Aug 30, 2026
Rollup merge of #161975 - matthew-demidoff:btree-split-off-panic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixes#158165. Supersedes #161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to #158710 for the original approach.
r? @nia-e
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-libsRelevant to the library team, which will review and decide on the PR/issue.

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

3 participants

@matthew-demidoff@rustbot@nia-e
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Abort instead of unwinding out of an inconsistent BTreeMap::split_off - #161784

Closed
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety
Closed

Abort instead of unwinding out of an inconsistent BTreeMap::split_off#161784
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety

Conversation

@matthew-demidoff

@matthew-demidoffmatthew-demidoff commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Fixes#158165.

BTreeMap::split_off runs the caller's Ord/Borrow impl via search_node inside Root::split_off's descent. After the first move_suffix, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from #![forbid(unsafe_code)] on stable (reproducer on the issue).

Fix: guard the descent loop with an abort-on-panic PanicGuard, as btree::mem::replace already does. Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.

Revives the approach from #158710 (closed for inactivity); credit to @ostrowr for the report.

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 25, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

r? @nia-e

rustbot has assigned @nia-e.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: libs
  • libs expanded to 12 candidates
  • Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, nia-e

@rustbot

This comment has been minimized.

@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from de7b14a to 05c9135CompareAugust 25, 2026 21:12

@nia-enia-e left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments, but the overall approach seems good.

@rustbot author

View changes since this review

// unwinding through it double-frees the shared values (#158165). Abort
// instead, matching the panic-safety strategy used elsewhere in this
// module (see `mem::replace`).
struct PanicGuard;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use either a bare catch_unwind or a DropGuard?

// inconsistent. `get_or_insert_with` (rather than `get_or_insert`)
// avoids constructing a fresh `PanicGuard` on later iterations,
// which would drop immediately and abort.
guard.get_or_insert_with(|| PanicGuard);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're rearming the guard every loop iteration? hm. why not just arm it once before the loop and disarm after, or just envelop the entire loop in a catch_unwind?

@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

After the first move_suffix in Root::split_off, the source and result
trees share values through two temporarily invalid structures. If a
later key comparison panics, unwinding leaves the map with a stale
length over a partially detached tree, and consuming iteration then
double-frees the shared values, reachable from safe code.
Guard the descent loop with an abort-on-panic PanicGuard, armed before
the first move_suffix and forgotten once the borders are fixed, matching
the strategy already used in btree::mem::replace.
@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from 05c9135 to dc0341aCompareAugust 29, 2026 12:54
@rustbotrustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 29, 2026
@matthew-demidoff

Copy link
Copy Markdown
ContributorAuthor

Sorry for the noise - I botched a force-push from a shallow clone and this PR's head got detached, so I could not reopen it. Reopened as #161975 with your feedback addressed (guard armed once before the loop; the first search stays outside it).

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
rust-borsBot pushed a commit that referenced this pull request Aug 30, 2026
Rollup merge of #161975 - matthew-demidoff:btree-split-off-panic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixes#158165. Supersedes #161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to #158710 for the original approach.
r? @nia-e
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-libsRelevant to the library team, which will review and decide on the PR/issue.

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

3 participants

@matthew-demidoff@rustbot@nia-e
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Abort instead of unwinding out of an inconsistent BTreeMap::split_off - #161784

Closed
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety
Closed

Abort instead of unwinding out of an inconsistent BTreeMap::split_off#161784
matthew-demidoff wants to merge 1 commit into
rust-lang:mainfrom
matthew-demidoff:btree-split-off-panic-safety

Conversation

@matthew-demidoff

@matthew-demidoffmatthew-demidoff commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Fixes#158165.

BTreeMap::split_off runs the caller's Ord/Borrow impl via search_node inside Root::split_off's descent. After the first move_suffix, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from #![forbid(unsafe_code)] on stable (reproducer on the issue).

Fix: guard the descent loop with an abort-on-panic PanicGuard, as btree::mem::replace already does. Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.

Revives the approach from #158710 (closed for inactivity); credit to @ostrowr for the report.

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 25, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

r? @nia-e

rustbot has assigned @nia-e.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: libs
  • libs expanded to 12 candidates
  • Random selection from JohnTitor, Mark-Simulacrum, clarfonthey, nia-e

@rustbot

This comment has been minimized.

@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from de7b14a to 05c9135CompareAugust 25, 2026 21:12

@nia-enia-e left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some comments, but the overall approach seems good.

@rustbot author

View changes since this review

// unwinding through it double-frees the shared values (#158165). Abort
// instead, matching the panic-safety strategy used elsewhere in this
// module (see `mem::replace`).
struct PanicGuard;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use either a bare catch_unwind or a DropGuard?

// inconsistent. `get_or_insert_with` (rather than `get_or_insert`)
// avoids constructing a fresh `PanicGuard` on later iterations,
// which would drop immediately and abort.
guard.get_or_insert_with(|| PanicGuard);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're rearming the guard every loop iteration? hm. why not just arm it once before the loop and disarm after, or just envelop the entire loop in a catch_unwind?

@rustbotrustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2026
@rustbot

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

After the first move_suffix in Root::split_off, the source and result
trees share values through two temporarily invalid structures. If a
later key comparison panics, unwinding leaves the map with a stale
length over a partially detached tree, and consuming iteration then
double-frees the shared values, reachable from safe code.
Guard the descent loop with an abort-on-panic PanicGuard, armed before
the first move_suffix and forgotten once the borders are fixed, matching
the strategy already used in btree::mem::replace.
@matthew-demidoff
matthew-demidoffforce-pushed the btree-split-off-panic-safety branch from 05c9135 to dc0341aCompareAugust 29, 2026 12:54
@rustbotrustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 29, 2026
@matthew-demidoff

Copy link
Copy Markdown
ContributorAuthor

Sorry for the noise - I botched a force-push from a shallow clone and this PR's head got detached, so I could not reopen it. Reopened as #161975 with your feedback addressed (guard armed once before the loop; the first search stays outside it).

JonathanBrouwer added a commit to JonathanBrouwer/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 29, 2026
…anic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixesrust-lang#158165. Supersedes rust-lang#161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to rust-lang#158710 for the original approach.
r? @nia-e
rust-borsBot pushed a commit that referenced this pull request Aug 30, 2026
Rollup merge of #161975 - matthew-demidoff:btree-split-off-panic-safety, r=nia-e
Abort instead of unwinding out of an inconsistent BTreeMap::split_off
Fixes#158165. Supersedes #161784, which I had to abandon after a bad force-push from a shallow clone left its head detached.
`BTreeMap::split_off` runs the caller's `Ord`/`Borrow` impl via `search_node` inside `Root::split_off`'s descent. After the first `move_suffix`, the two roots alias the same values through structurally invalid trees until the borders are fixed; a comparator panic there unwinds with a stale length and double-frees on later iteration or drop. It is reachable from `#![forbid(unsafe_code)]` on stable (reproducer on the issue).
Guard the descent loop with an abort-on-panic `PanicGuard`, as `btree::mem::replace` already does. `catch_unwind` isnt available in `alloc` (no_std), and I kept an inline guard rather than the unstable `DropGuard`. It is armed once before the loop; the first `search_node` stays outside it, since a panic there can still unwind safely (nothing has moved yet).
Verified with Miri: the reproducer goes from a double-free to a clean abort, and normal multi-level splits are unaffected. Adds a happy-path regression test.
Credit to @ostrowr for the report and to #158710 for the original approach.
r? @nia-e
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-libsRelevant to the library team, which will review and decide on the PR/issue.

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

3 participants

@matthew-demidoff@rustbot@nia-e