At Zed (https://zed.dev) we track panics in a centralized system so that we can identify and fix problems.
A major cause of panics at Zed is indexing into strings erroneously (we do a lot of string manipulation); usually due to bad utf8 offset handling, or occasionally just off by one or out-of-bounds.
The problem is that String when it panics includes a small fragment of the string. This can be useful locally for debugging, but for centralized panic tracking, we'd really like to not handle this stuff.
Is there appetite to either:
Add a flag to the standard library to suppress this.
Change the default panic messages to not include content.
For example
currently we see: "begin <= end (4 <= 3) when slicing test. I'd like this to become "byte index range starts before end (4..3)"
"byte index 2458645928158 is out of bounds of test". I'd like this to become "byte index 2458645928158 is out of bounds for string of length 4"
"byte index 1 is not a char boundary; it is inside 'ĂŁ' (bytes 0..2) of ĂŁchoo. I'd like this to become "byte index 1 is not at a utf-8 character boundary; the character spans from indexes 0..2)"
The alternative we can explore is trying to replace the strings on the way through the panic handler (or forbid indexing into a string - doesn't seem tenable); but I'd rather push this down to the standard library.
I assume this is about not wanting to handle PI? Couldn't your crash reporter filter this out on the client side with a regex, at least as a stop-gap solution?
byte index 1000 is out of bounds of aseirughaoirughaoerghaoerghaorvlijesr;oibgfhs;eroignvs;eorigjhvas;loijhvsoeruifpsioreuyhfgvosliueryhvglaiueyrhvfgliuhrlukvzyhrvlkusrdyhvlkzuyrsvhlkuyhfgbrlixuyvlikzurhfclsvkurdygvhskrerghaierughaslierughaseirughaergiuhaeriuhager
Echoing the whole string seems clearly wrong and unnecessary, so at least this one I think we should just do. Maybe make a PR for that part and we can nominate for libs/libs-api discussion? (Feel free to r? @scottmcm.)
The single USV, however, does seem particularly useful as friendliness to the developer, especially the new developer not yet used to how rust does indexing, so I'd be more reticent to remove the "inside 'ĂŁ'" part. (Removing the "of ĂŁchoo" part seems obvious, like the previous case.)
How much of a problem does the single USV cause? My instinct is that that wouldn't trigger the same GDPR-and-similar problems the full string could...
I assume this is about not wanting to handle PI? Couldn't your crash reporter filter this out on the client side with a regex, at least as a stop-gap solution?
We definitely can, but I'm a bit reluctant to as a permanent solution in case someone (like me) messes with the panic messages more.
Echoing the whole string seems clearly wrong and unnecessary, so at least this one I think we should just do. Maybe make a PR for that part and we can nominate for libs/libs-api discussion? (Feel free to r? @scottmcm.)
It's currently limited to 256 bytes; reducing the size would help (but it'd be better to get rid of it I think). For the range being backward, it's nothing to do with the string content.
The single USV, however, does seem particularly useful as friendliness to the developer, especially the new developer not yet used to how rust does indexing, so I'd be more reticent to remove the "inside 'ĂŁ'" part. (Removing the "of ĂŁchoo" part seems obvious, like the previous case.)
That seems like a reasonable compromise to me too.
So I think that would leave us with the following panic strings:
"byte index {} is out of bounds for string of length {}"
"byte range starts at {} but ends at {}"
"byte index {} inside multibyte character {:?} (bytes {}..{} of string)"
The first one is the main one where I think there's (maybe?) value in outputting the string content; but this does mirror what we do for slices already, so maybe it's OK to avoid it there too.
I'll send a PR and we can discuss more the trade-offs there
What kind of mechanism do you have in mind for having a “flag”?
That should be relatively easy to notice with an automated test though, shouldn’t it? Just run a test for a great variety of possible of out-of-bounds errors (including all existing flavors that are differentiated) in order to be able to be notified of any future changes in the panic message formatting, and have that test be executed whenever Zed upgrades to use a newer Rust toolchain.
Edit: Or perhaps a test that looks at Rust’s source and creates a git blame or sth, of the relevant section of source code (slice_error_fail and its helper methods, or maybe only slice_error_fail_rt). Those aren’t exactly fast-changing, so it should be low effort to keep up. While the latest change was a month ago and changed the panic messages, before that the code remained unchanged for a long time. 2 changes about 4 years ago (adding const support, and a small refactor of char boundaries), another change about 7 years ago (running rustfmt over the standard library), which the actual panic message output logic having remained unchanged for about 10 years.
What kind of mechanism do you have in mind for having a “flag”?
I was imagining something like the Cargo feature flags (though I don't actually know if you can apply those to the standard library). If not, we could also tie this to cfg!(debug_assertions).
My thought is including string content is a potential security/GDPR problem. I would agree it should not be included.
Potential security problem is my top-most concern (it seems more likely that developers will have API keys in their buffers than PII); but either way it's not great.
Luckily, although this is common in terms of the panics we get; it's very rare when divided by the total Zed user-base, and to my knowledge we've never actually seen anything that would be considered sensitive. I'd like to fix this before we do though
That should be relatively easy to notice with an automated test though, shouldn’t it? Just run a test for a great variety of possible of out-of-bounds errors (including all existing flavors that are differentiated) in order to be able to be notified of any future changes in the panic message formatting, and have that test be executed whenever Zed upgrades to use a newer Rust toolchain.
Working around the problem for just Zed seems to leave the whole ecosystem in a bad place (and the effort to build all that is - in my opinion - more work than fixing the problem in the right place).
There have been cases in the past where printing specific strings has been a way to trigger security vulnerabilities. Most commonly this includes terminal control characters like U+001B and U+009B, and Rust appears to print those both literally at present when a string operation produces a panic. (My browser shows U+001B visibly and has no visible rendering for U+009B, but I was able to copy-and-paste it.)
Normally this is considered a security problem in the terminal that's displaying the panic message, rather than in the program that writes the message, so technically speaking this isn't a vulnerability in Rust. I can imagine scenarios where it could be a problem, though (e.g. if you can put terminal control codes into a panic message you can embed links into it, and there are a number of ways that that could go wrong, especially given that there's a plausible chance that such a string could be attacker-controlled).
As such, even if we aren't removing the string fragment from the panic message, we should at least be Debug-escaping it to avoid it being misinterpreted by terminals that might display the message.
This is an interesting point. As a consequence, I’d pose it may be a good idea for the string to be Debug-printed (i.e. escaped) – rather than Display-printed as currently seems to be the case – thus behaving similar to e.q. what a failed assert_eq!("ab\u{1b}c\n\n\nd\u{9b}ef", "something else") would do (it also uses the Debug-printed representation of the string).
