string: don't unwind prematurely by nia-e · Pull Request #162286 · rust-lang/rust

nia-e · 2026-09-04T10:00:32Z

splice on a vec can technically unwind (since we still say handle_alloc_error may unwind in the future, and nightly users could trigger this via setting the alloc error hook) which would expose a string in an invalid state during replace_range. This adds an abort guard around it to ensure this is never triggered.

r? libs

Darksonn · 2026-09-04T10:45:57Z

-        // ignore-tidy-undocumented-unsafe
+        let guard = DropGuard::new((), |_| core::process::abort_immediate());
+        // SAFETY: We ensure that we're not replacing across a char boundary and
+        // that the new contents are valid UTF-8. We also protect against unwinds
+        // which may leave the string in an invalid state.
        unsafe { self.as_mut_vec() }.splice(checked_range, replace_with.bytes());
+        DropGuard::dismiss(guard);


Why does this warrant an immediate abort? Can't we just invoke reserve before the call to splice to ensure that splice does not need to reallocate?

View changes since the review

we could also do that, I wasn't sure if it may be a perf regression due to the duplicated checks. right now handle_alloc_error always aborts on stable anyway so I figured this is the safer call. I'm happy to change to the other & do a perf run just in case?

In general, I'd say we shouldn't turn panics into aborts without a good reason.

Perhaps splice already performs the same reserve call in a way that doesn't leave the string in an invalid state?

Looks like that's not the case. Let us definitely reserve memory up front.

nia-e · 2026-09-04T11:15:15Z

mildly scared that this could still be unsound since shrink is also allowed to error for no good reason, but I don't see a way to trigger UB from an erroring call to shrink.

@rustbot ready

nia-e · 2026-09-04T11:18:18Z

@bors try @rust-timer queue

string: don't unwind prematurely

Darksonn · 2026-09-04T11:41:25Z

Do we have a benchmark that uses String::splice to begin with?

nia-e · 2026-09-04T11:50:18Z

i thought we did in the string benchmarks, but. it's probably not that bad fwiw

rust-bors · 2026-09-04T12:16:57Z

☀️ Try build successful (CI)
Build commit: 96517d8 (96517d83fa42ba27064d360facb17a38e7f75670)
Base parent: b924f94 (b924f94129bce80f36dd9f08bb78dd5393fe196e)

…, r=Darksonn alloc: a bunch of safety comments Following up from rust-lang#160941. Triaging this is what found the errors in rust-lang#162285 & rust-lang#162286. More to come, but I didn't want to make the review effort too high on any single PR. r? libs

rust-timer · 2026-09-04T13:05:08Z

Finished benchmarking commit (96517d8): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -1.3%, secondary 2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.5%	[1.5%, 1.5%]	1
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-4.0%	[-4.0%, -4.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.3%	[-4.0%, 1.5%]	2

Cycles

Results (primary -3.0%, secondary 2.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-3.0%	[-3.0%, -3.0%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.0%	[-3.0%, -3.0%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 480.684s -> 479.082s (-0.33%)
Artifact size: 403.39 MiB -> 403.33 MiB (-0.01%)

Darksonn

LGTM!

@bors r+

View changes since this review

rust-bors · 2026-09-04T13:07:55Z

📌 Commit c66f150 has been approved by Darksonn

It is now in the queue for this repository.

…rksonn string: don't unwind prematurely `splice` on a vec can technically unwind (since we still say `handle_alloc_error` *may* unwind in the future, and nightly users could trigger this via setting the alloc error hook) which would expose a string in an invalid state during `replace_range`. This adds an abort guard around it to ensure this is never triggered. r? libs

…, r=Darksonn alloc: a bunch of safety comments Following up from rust-lang#160941. Triaging this is what found the errors in rust-lang#162285 & rust-lang#162286. More to come, but I didn't want to make the review effort too high on any single PR. r? libs

…rksonn string: don't unwind prematurely `splice` on a vec can technically unwind (since we still say `handle_alloc_error` *may* unwind in the future, and nightly users could trigger this via setting the alloc error hook) which would expose a string in an invalid state during `replace_range`. This adds an abort guard around it to ensure this is never triggered. r? libs

…, r=Darksonn alloc: a bunch of safety comments Following up from rust-lang#160941. Triaging this is what found the errors in rust-lang#162285 & rust-lang#162286. More to come, but I didn't want to make the review effort too high on any single PR. r? libs

Rollup of 25 pull requests Successful merges: - #159074 ([PAC] FnAbi, llvm.ptrauth.resign and Session API change (2/8)) - #159792 (A more readable debug map for IndexMaps) - #161895 (std::sys::pal::sgx: fix mismatched alloc/free alignment) - #161900 (bootstrap: Include feature-gated items in bootstrap tool docs) - #161940 (Promote `wasm32-wasip3` to a tier 2 target) - #162072 (Add new Tier-3 target: `powerpc64-sony-ps3`) - #162179 (type system const items via direct rhs) - #162277 (Introduce `rustc_middle::middel::resolve`) - #162285 (box: fixup map/try_map deallocate calls) - #162286 (string: don't unwind prematurely) - #162289 (alloc: a bunch of safety comments) - #162292 (Update `askama` version to `0.16.1`) - #160509 (Remove `RegionExt`; move methods to `Region` in `rustc_type_ir`) - #160906 (Suggest usize instead of placeholder type for array length constants) - #160936 (traits: Represent live alias arguments as bitsets) - #161400 (Improve diagnostics for references to closures) - #161656 (Suggest mutable references for FnMut closure arguments) - #161711 (Add more splat fn type tests) - #161786 (Make `tcx.def_id_partial_cmp` public) - #161953 (sanitizers: Implicitly disable mutually exclusive sanitizers) - #162155 (add suggestion for `rustc_allowed_through_unstable_modules` attribute) - #162212 (Implement `Rng` for `Box`) - #162246 (Fix incorrect meta span) - #162266 (std: fix typo) - #162291 (Add regression test from 1.98.1)

…rksonn string: don't unwind prematurely `splice` on a vec can technically unwind (since we still say `handle_alloc_error` *may* unwind in the future, and nightly users could trigger this via setting the alloc error hook) which would expose a string in an invalid state during `replace_range`. This adds an abort guard around it to ensure this is never triggered. r? libs

…, r=Darksonn alloc: a bunch of safety comments Following up from rust-lang#160941. Triaging this is what found the errors in rust-lang#162285 & rust-lang#162286. More to come, but I didn't want to make the review effort too high on any single PR. r? libs

Rollup of 25 pull requests Successful merges: - #159074 ([PAC] FnAbi, llvm.ptrauth.resign and Session API change (2/8)) - #159792 (A more readable debug map for IndexMaps) - #160745 (make closures act like MaybeDangling) - #161895 (std::sys::pal::sgx: fix mismatched alloc/free alignment) - #161940 (Promote `wasm32-wasip3` to a tier 2 target) - #162072 (Add new Tier-3 target: `powerpc64-sony-ps3`) - #162179 (type system const items via direct rhs) - #162277 (Introduce `rustc_middle::middel::resolve`) - #162285 (box: fixup map/try_map deallocate calls) - #162286 (string: don't unwind prematurely) - #162289 (alloc: a bunch of safety comments) - #162292 (Update `askama` version to `0.16.1`) - #160509 (Remove `RegionExt`; move methods to `Region` in `rustc_type_ir`) - #160906 (Suggest usize instead of placeholder type for array length constants) - #160936 (traits: Represent live alias arguments as bitsets) - #161400 (Improve diagnostics for references to closures) - #161656 (Suggest mutable references for FnMut closure arguments) - #161711 (Add more splat fn type tests) - #161786 (Make `tcx.def_id_partial_cmp` public) - #161953 (sanitizers: Implicitly disable mutually exclusive sanitizers) - #162155 (add suggestion for `rustc_allowed_through_unstable_modules` attribute) - #162212 (Implement `Rng` for `Box`) - #162246 (Fix incorrect meta span) - #162266 (std: fix typo) - #162291 (Add regression test from 1.98.1)

…rksonn string: don't unwind prematurely `splice` on a vec can technically unwind (since we still say `handle_alloc_error` *may* unwind in the future, and nightly users could trigger this via setting the alloc error hook) which would expose a string in an invalid state during `replace_range`. This adds an abort guard around it to ensure this is never triggered. r? libs

…, r=Darksonn alloc: a bunch of safety comments Following up from rust-lang#160941. Triaging this is what found the errors in rust-lang#162285 & rust-lang#162286. More to come, but I didn't want to make the review effort too high on any single PR. r? libs

…rksonn string: don't unwind prematurely `splice` on a vec can technically unwind (since we still say `handle_alloc_error` *may* unwind in the future, and nightly users could trigger this via setting the alloc error hook) which would expose a string in an invalid state during `replace_range`. This adds an abort guard around it to ensure this is never triggered. r? libs

…, r=Darksonn alloc: a bunch of safety comments Following up from rust-lang#160941. Triaging this is what found the errors in rust-lang#162285 & rust-lang#162286. More to come, but I didn't want to make the review effort too high on any single PR. r? libs

Rollup of 27 pull requests Successful merges: - #159074 ([PAC] FnAbi, llvm.ptrauth.resign and Session API change (2/8)) - #159792 (A more readable debug map for IndexMaps) - #160745 (make closures act like MaybeDangling) - #161940 (Promote `wasm32-wasip3` to a tier 2 target) - #162030 (Prevent `--test` to be used in `rustdoc-html` testsuite) - #162072 (Add new Tier-3 target: `powerpc64-sony-ps3`) - #162179 (type system const items via direct rhs) - #162262 (Avoid manually instantiating some binders in error reporting with `-Znext-solver`) - #162277 (Introduce `rustc_middle::middel::resolve`) - #162285 (box: fixup map/try_map deallocate calls) - #162286 (string: don't unwind prematurely) - #162289 (alloc: a bunch of safety comments) - #162290 (abby test DSL: AliasTyOutlivesViaEnv) - #162292 (Update `askama` version to `0.16.1`) - #160509 (Remove `RegionExt`; move methods to `Region` in `rustc_type_ir`) - #160906 (Suggest usize instead of placeholder type for array length constants) - #160936 (traits: Represent live alias arguments as bitsets) - #161400 (Improve diagnostics for references to closures) - #161656 (Suggest mutable references for FnMut closure arguments) - #161711 (Add more splat fn type tests) - #161786 (Make `tcx.def_id_partial_cmp` public) - #161953 (sanitizers: Implicitly disable mutually exclusive sanitizers) - #162155 (add suggestion for `rustc_allowed_through_unstable_modules` attribute) - #162212 (Implement `Rng` for `Box`) - #162246 (Fix incorrect meta span) - #162266 (std: fix typo) - #162291 (Add regression test from 1.98.1)

…rksonn string: don't unwind prematurely `splice` on a vec can technically unwind (since we still say `handle_alloc_error` *may* unwind in the future, and nightly users could trigger this via setting the alloc error hook) which would expose a string in an invalid state during `replace_range`. This adds an abort guard around it to ensure this is never triggered. r? libs

…, r=Darksonn alloc: a bunch of safety comments Following up from rust-lang#160941. Triaging this is what found the errors in rust-lang#162285 & rust-lang#162286. More to come, but I didn't want to make the review effort too high on any single PR. r? libs

…uwer Rollup of 25 pull requests Successful merges: - #162072 (Add new Tier-3 target: `powerpc64-sony-ps3`) - #162179 (type system const items via direct rhs) - #162255 (let people change rustc-dev-guide in-tree) - #162262 (Avoid manually instantiating some binders in error reporting with `-Znext-solver`) - #162277 (Introduce `rustc_middle::middel::resolve`) - #162285 (box: fixup map/try_map deallocate calls) - #162286 (string: don't unwind prematurely) - #162289 (alloc: a bunch of safety comments) - #162290 (abby test DSL: AliasTyOutlivesViaEnv) - #162292 (Update `askama` version to `0.16.1`) - #160509 (Remove `RegionExt`; move methods to `Region` in `rustc_type_ir`) - #160906 (Suggest usize instead of placeholder type for array length constants) - #160936 (traits: Represent live alias arguments as bitsets) - #161394 (fix `is_homogeneous_aggregate`: use unit size, not total size ) - #161400 (Improve diagnostics for references to closures) - #161656 (Suggest mutable references for FnMut closure arguments) - #161711 (Add more splat fn type tests) - #161786 (Make `tcx.def_id_partial_cmp` public) - #161953 (sanitizers: Implicitly disable mutually exclusive sanitizers) - #162155 (add suggestion for `rustc_allowed_through_unstable_modules` attribute) - #162212 (Implement `Rng` for `Box`) - #162246 (Fix incorrect meta span) - #162266 (std: fix typo) - #162291 (Add regression test from 1.98.1) - #162303 (Fix a minor spelling error in `library/std/src/sys/fs/unix.rs`)

string: don't unwind prematurely

8f1e147

rustbot assigned Darksonn Sep 4, 2026

rustbot 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 Sep 4, 2026

Darksonn requested changes Sep 4, 2026

View reviewed changes

rustbot 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 Sep 4, 2026

nia-e mentioned this pull request Sep 4, 2026

alloc: a bunch of safety comments #162289

Open

reserve ahead of time

22894e4

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

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 4, 2026

This comment has been minimized.

Sign in to view

rust-bors Bot pushed a commit that referenced this pull request Sep 4, 2026

Auto merge of #162286 - nia-e:string-splice-nounwind, r=<try>

96517d8

string: don't unwind prematurely

Darksonn reviewed Sep 4, 2026

View reviewed changes

Comment thread library/alloc/src/lib.rs Outdated

drop unnecessary feature gate

c66f150

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 4, 2026

Darksonn approved these changes Sep 4, 2026

View reviewed changes

rust-bors Bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 4, 2026

GuillaumeGomez mentioned this pull request Sep 4, 2026

Rollup of 25 pull requests #162297

Closed

GuillaumeGomez mentioned this pull request Sep 4, 2026

Rollup of 25 pull requests #162298

Closed

GuillaumeGomez mentioned this pull request Sep 4, 2026

Rollup of 25 pull requests #162300

Closed

GuillaumeGomez mentioned this pull request Sep 4, 2026

Rollup of 28 pull requests #162307

Closed

GuillaumeGomez mentioned this pull request Sep 4, 2026

Rollup of 27 pull requests #162308

Closed

JonathanBrouwer mentioned this pull request Sep 4, 2026

Rollup of 25 pull requests #162310

Open

Uh oh!

Uh oh!

Conversation

nia-e commented Sep 4, 2026

Uh oh!

Darksonn Sep 4, 2026 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nia-e Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Darksonn Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Darksonn Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

nia-e commented Sep 4, 2026

Uh oh!

nia-e commented Sep 4, 2026

Uh oh!

This comment has been minimized.

This comment has been minimized.

Darksonn commented Sep 4, 2026

Uh oh!

nia-e commented Sep 4, 2026

Uh oh!

Uh oh!

rust-bors Bot commented Sep 4, 2026

Uh oh!

This comment has been minimized.

rust-timer commented Sep 4, 2026

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Darksonn left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rust-bors Bot commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Darksonn Sep 4, 2026 •

edited by rustbot

Loading

Darksonn left a comment •

edited by rustbot

Loading