Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

String::retain allows safely creating invalid (non-utf8) strings when abusing panic #78498

Closed
SkiFire13 opened this issue Oct 28, 2020 · 1 comment · Fixed by #78499
Closed
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@SkiFire13
Copy link
Contributor

While String::retain executes it may temporarily leave the String in an inconsistent state, in particular it may contain invalid utf8. This is safe because it restores this invariant before returning, but the caller may skip this by panicing inside the closure and catching the unwind it outside. This allows to create Strings that are not utf8, breaking the library invariant without using unsafe.

For example the following will panic at the final assertion, while I would expect it to never fail when s has type String:

let mut s = "0è0".to_string();
let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
    let mut count = 0;
    s.retain(|_| {
        count += 1;
        match count {
            1 => false,
            2 => true,
            _ => panic!(),
        }
    });
}));
assert!(std::str::from_utf8(s.as_bytes()).is_ok()); // This will fail
@SkiFire13 SkiFire13 added the C-bug Category: This is a bug. label Oct 28, 2020
@jonas-schievink jonas-schievink added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 28, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 28, 2020
@camelid camelid added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Oct 28, 2020
@camelid
Copy link
Member

camelid commented Oct 28, 2020

Assigning P-high and removing I-prioritize as discussed in the prioritization working group.

@camelid camelid added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 28, 2020
@bors bors closed this as completed in 48c4afb Oct 30, 2020
rodrimati1992 added a commit to rodrimati1992/abi_stable_crates that referenced this issue Dec 21, 2020
These methods copied their implementation from the standard library,
which had memory safety bugs discovered in
rust-lang/rust#60977 and rust-lang/rust#78498 .

This bug was reported in #44 .

Added adapted tests from std which test these bugs.
rodrimati1992 added a commit to rodrimati1992/abi_stable_crates that referenced this issue Dec 22, 2020
* Fixed String::retain, RVec::retain. Bumped patch version to 0.9.1 .

These methods copied their implementation from the standard library,
which had memory safety bugs discovered in
rust-lang/rust#60977 and rust-lang/rust#78498 .

This bug was reported in #44 .

Added adapted tests from std which test these bugs.

* Updated changelog for patch
rodrimati1992 added a commit to rodrimati1992/abi_stable_crates that referenced this issue Nov 22, 2022
* Fixed String::retain, RVec::retain. Bumped patch version to 0.9.1 .

These methods copied their implementation from the standard library,
which had memory safety bugs discovered in
rust-lang/rust#60977 and rust-lang/rust#78498 .

This bug was reported in #44 .

Added adapted tests from std which test these bugs.

* Updated changelog for patch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants