Skip to content

Remove string content from panics#153677

Open
ConradIrwin wants to merge 2 commits intorust-lang:mainfrom
ConradIrwin:sanitary-string-panics
Open

Remove string content from panics#153677
ConradIrwin wants to merge 2 commits intorust-lang:mainfrom
ConradIrwin:sanitary-string-panics

Conversation

@ConradIrwin
Copy link
Contributor

String content can be useful for debugging panics, particularly when you are working on a small codebase where you can infer more about the path taken through your program based on the content of the string.

In production deployments that upload crashes for centralized debugging, string content poses a risk to user privacy. The string can accidentally contain information that the user is not expecting to share with the development team.

On balance it seems like the risks outweigh the benefits. It is easy to add a dbg!() statement to gather more information in development; but comparatively tricky to ensure panics are sanitized by every rust app that monitors their production deployments.

Ref https://internals.rust-lang.org/t/stop-including-string-content-in-index-panics/24067

r? @scottmcm

@rustbot 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 Mar 10, 2026
@rust-log-analyzer

This comment has been minimized.


/// Copied from core::str::raw::slice_error_fail
#[inline(never)]
fn slice_error_fail(s: &Wtf8, begin: usize, end: usize) -> ! {

This comment was marked as resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: unused variable: `s`
     --> library/core/src/wtf8.rs:488:21
      |
  488 | fn slice_error_fail(s: &Wtf8, begin: usize, end: usize) -> ! {
      |                     ^ help: if this is intentional, prefix it with an underscore: `_s`

@ConradIrwin

// SAFETY: is_code_point_boundary checks that the index is valid
unsafe { slice_unchecked(self, range.start, range.end) }
} else {
slice_error_fail(self, range.start, range.end)

This comment was marked as resolved.

// SAFETY: is_code_point_boundary checks that the index is valid
unsafe { slice_unchecked(self, range.start, self.len()) }
} else {
slice_error_fail(self, range.start, self.len())

This comment was marked as resolved.

// SAFETY: is_code_point_boundary checks that the index is valid
unsafe { slice_unchecked(self, 0, range.end) }
} else {
slice_error_fail(self, 0, range.end)

This comment was marked as resolved.

String content can be useful for debugging panics, particularly when you
are working on a small codebase where you can infer more about the path
taken through your program based on the content of the string.

In production deployments that upload crashes for centralized debugging,
string content poses a risk to user privacy. The string can accidentally
contain information that the user is not expecting to share with the
development team.

On balance it seems like the risks outweigh the benefits. It is easy to
add a `dbg!()` statement to gather more information in development; but
comparatively tricky to ensure panics are sanitized by every rust app
that monitors their production deployments.

Ref https://internals.rust-lang.org/t/stop-including-string-content-in-index-panics/24067
@ConradIrwin ConradIrwin force-pushed the sanitary-string-panics branch from 534370d to 05ac9d4 Compare March 11, 2026 01:36
Comment on lines 487 to 500
#[inline(never)]
fn slice_error_fail(s: &Wtf8, begin: usize, end: usize) -> ! {
assert!(begin <= end);
panic!("index {begin} and/or {end} in `{s:?}` do not lie on character boundary");
let len = s.len();
if end > len {
panic!("end byte index {end} is out of bounds for string of length {len}");
}
if begin > end {
panic!("byte range starts at {begin} but ends at {end}");
}
if !s.is_code_point_boundary(begin) {
panic!("byte index {begin} is not a code point boundary");
}
panic!("byte index {end} is not a code point boundary");
}

This comment was marked as resolved.

@rust-log-analyzer

This comment has been minimized.

@TKanX
Copy link
Contributor

TKanX commented Mar 11, 2026

The job aarch64-gnu-llvm-20-1 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Executing "/scripts/stage_2_test_set1.sh"
+ /scripts/stage_2_test_set1.sh
PR_CI_JOB set; skipping tidy
+ '[' 1 == 1 ']'
+ echo 'PR_CI_JOB set; skipping tidy'
+ SKIP_TIDY='--skip tidy'
+ ../x.py --stage 2 test --skip tidy --skip compiler --skip src
##[group]Building bootstrap
    Finished `dev` profile [unoptimized] target(s) in 0.04s
##[endgroup]
downloading https://static.rust-lang.org/dist/2026-03-05/rustfmt-nightly-aarch64-unknown-linux-gnu.tar.xz
---
Saved the actual run.stderr to `/checkout/obj/build/aarch64-unknown-linux-gnu/test/ui/intrinsics/const-eval-select-backtrace-std/const-eval-select-backtrace-std.run.stderr`
diff of run.stderr:

1 
2 thread 'main' ($TID) panicked at $DIR/const-eval-select-backtrace-std.rs:6:8:
- start byte index 1 is out of bounds of ``
+ start byte index 1 is out of bounds for string of length 0
4 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
5 


The actual run.stderr differed from the expected run.stderr

@ConradIrwin use --bless

ConradIrwin added a commit to zed-industries/zed that referenced this pull request Mar 11, 2026
String panics are unfortunately common at Zed, and doubly unfortunately
they may incidentally include the contents of a user's buffer.

Although this hasn't happened yet (to my knowledge), I don't want to be
in the position of having received sensitive information this way.

See also rust-lang/rust#153677
ConradIrwin added a commit to zed-industries/zed that referenced this pull request Mar 11, 2026
String panics are unfortunately common at Zed, and doubly unfortunately
they may incidentally include the contents of a user's buffer.

Although this hasn't happened yet (to my knowledge), I don't want to be
in the position of having received sensitive information this way.

See also rust-lang/rust#153677
ConradIrwin added a commit to zed-industries/zed that referenced this pull request Mar 11, 2026
String panics are unfortunately common at Zed, and doubly unfortunately
they may incidentally include the contents of a user's buffer.

Although this hasn't happened yet (to my knowledge), I don't want to be
in the position of having received sensitive information this way.

See also rust-lang/rust#153677
Co-authored-by: TKanX <124454788+TKanX@users.noreply.github.com>
ConradIrwin added a commit to zed-industries/zed that referenced this pull request Mar 11, 2026
String panics are a non-trivial percentage of the crashes we see at Zed,
and
doubly unfortunately they may incidentally include the contents of a
user's
buffer.

Although this hasn't happened yet (to my knowledge), I don't want to be
in the position of having received sensitive information this way.

See also rust-lang/rust#153677

Release Notes:

- N/A
@ConradIrwin
Copy link
Contributor Author

Thanks @TKanX!

@scottmcm
Copy link
Member

Hello libs and libs-api folks! Not sure which team would be responsible for this, so nominating for both.

I think that not reflecting more that one USV in the panic message makes sense, like how indexing a vec doesn't show the elements of the vec. But wanted to run it by teams as a user-visible change.

How do you feel about it?

@scottmcm scottmcm added I-libs-nominated Nominated for discussion during a libs team meeting. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Mar 11, 2026
let ch = s[floor..ceil].chars().next().unwrap();
panic!(
"start byte index {begin} is not a char boundary; it is inside {ch:?} (bytes {range:?}) of `{s_trunc}`{ellipsis}"
"start byte index {begin} is not a char boundary; it is inside {ch:?} (bytes {range:?} of string)"
Copy link
Member

@RalfJung RalfJung Mar 11, 2026

Choose a reason for hiding this comment

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

Suggested change
"start byte index {begin} is not a char boundary; it is inside {ch:?} (bytes {range:?} of string)"
"start byte index {begin} is not a char boundary; it is inside {ch:?} (bytes {range:?})"

"of string" sounds odd to me -- is it needed? (Same for similar messages)

let ch = s[floor..ceil].chars().next().unwrap();
panic!(
"end byte index {end} is not a char boundary; it is inside {ch:?} (bytes {range:?}) of `{s_trunc}`{ellipsis}"
"end byte index {end} is not a char boundary; it is inside {ch:?} (bytes {range:?} of string)"
Copy link
Contributor

Choose a reason for hiding this comment

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

"of string" sounds odd to me -- is it needed? (Same for similar messages)
@RalfJung

Suggested change
"end byte index {end} is not a char boundary; it is inside {ch:?} (bytes {range:?} of string)"
"end byte index {end} is not a char boundary; it is inside {ch:?} (bytes {range:?})"

Copy link
Contributor

Choose a reason for hiding this comment

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

1, 2, 6 describe the type so not redundant right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

I-libs-api-nominated Nominated for discussion during a libs-api team meeting. I-libs-nominated Nominated for discussion during a libs team meeting. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants