Prevent String::retain from creating non-utf8 strings when abusing panic #78499
Conversation
|
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Won't this result in an empty string if you do |
SkiFire13
force-pushed
the
fix-string-retain
branch
from
0890596 to
92049e5
Compare
camelid
added
T-libs
|
Can you add a test that fails without this change, but passes with? |
|
A test for |
Done. I added it in the same function of the other tests for
There's already a test for that, which in fact initially failed in the CI. The reason I didn't catch it before opening the pull request is that I naively ran |
SkiFire13
force-pushed
the
fix-string-retain
branch
from
3860e50 to
1f6f917
Compare
|
Thanks! @bors r+ |
|
📌 Commit 1f6f917 has been approved by |
bors
added
S-waiting-on-bors
…as-schievink Rollup of 11 pull requests Successful merges: - rust-lang#75078 (Improve documentation for slice strip_* functions) - rust-lang#76138 (Explain fully qualified syntax for `Rc` and `Arc`) - rust-lang#78244 (Dogfood {exclusive,half-open} ranges in compiler (nfc)) - rust-lang#78422 (Do not ICE on invalid input) - rust-lang#78423 (rustc_span: improve bounds checks in byte_pos_to_line_and_col) - rust-lang#78431 (Prefer new associated numeric consts in float error messages) - rust-lang#78462 (Use unwrapDIPtr because the Scope may be null.) - rust-lang#78493 (Update cargo) - rust-lang#78499 (Prevent String::retain from creating non-utf8 strings when abusing panic) - rust-lang#78505 (Update Clippy - temporary_cstring_as_ptr deprecation) - rust-lang#78527 (Fix some more typos) Failed merges: r? `@ghost`
| unsafe { | ||
| self.vec.set_len(0); | ||
| } | ||
|
|
||
| while idx < len { | ||
| let ch = unsafe { self.get_unchecked(idx..len).chars().next().unwrap() }; |
There was a problem hiding this comment.
@RalfJung this was brought to my attention by @DJMcNab (the author of retain_more), as somewhat suspicious - it's calling get_unchecked on a &str obtained via auto-deref which is zero-length now. So we more or less expect this needs to go through a raw pointer directly or something similar.
There was a problem hiding this comment.
Good catch! I guess another fix would be using a DropGuard for setting the length at the end of the loop. This way it's not necessary to set the length to 0 anymore because its drop implementation would called even in case of panic.
There was a problem hiding this comment.
I've opened #82554, let me know what you think about it
…ness, r=m-ou-se Fix invalid slice access in String::retain As noted in rust-lang#78499, the previous fix was technically still unsound because it accessed elements of a slice outside its bounds (even though they were still inside the same allocation). This PR addresses that concern by switching to a dropguard approach.
…ness, r=m-ou-se Fix invalid slice access in String::retain As noted in rust-lang#78499, the previous fix was technically still unsound because it accessed elements of a slice outside its bounds (even though they were still inside the same allocation). This PR addresses that concern by switching to a dropguard approach.
Fixes #78498
The idea is the same as
Vec::drain, set the len to 0 so that nobody can observe the broken invariant if it escapes the function (in this case iffpanics)