fix str mutating through a ptr derived from &self by RalfJung · Pull Request #58200 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation9 Commits4 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Found by Miri: In get_unchecked_mut (also used by the checked variants internally) uses str::as_ptr to create a mutable reference, but as_ptr takes &self. This means the mutable references we return here got created from a shared reference, which violates the shared-references-are-read-only discipline!

For this by using a newly introduced as_mut_ptr instead.

r? @Kimundi

(rust_highfive has picked a reviewer for you, use r? to override)

let len = self.end - self.start;
super::from_utf8_unchecked_mut(slice::from_raw_parts_mut(ptr as *mut u8, len))
super::from_utf8_unchecked_mut(slice::from_raw_parts_mut(ptr, len))

Choose a reason for hiding this comment

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

Notice that this raw ptr cast here is the "canary" that gave away that the old code was wrong -- it was actually casting *const u8 to *mut u8, which should not have been necessary.

/// modified in a way that it remains valid UTF-8.
///
/// [`u8`]: primitive.u8.html
#[unstable(feature = "str_as_mut_ptr", issue = "0")]

Choose a reason for hiding this comment

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

r+ with a tracking issue

Choose a reason for hiding this comment

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

I opened a tracking issue in #58215, please check if that looks all right.

📌 Commit a996f2c has been approved by SimonSapin

bors 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

Feb 6, 2019

Turns out there was another bad use of as_ptr, in str::split_at_mut. I fixed that as well and reviewed the remaining uses in str/mod.rs.

@SimonSapin could you review?

RalfJung added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

labels

Feb 7, 2019

📌 Commit 66c894e has been approved by SimonSapin

bors 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

Feb 7, 2019

Centril added a commit to Centril/rust that referenced this pull request

Feb 13, 2019

fix str mutating through a ptr derived from &self

For this by using a newly introduced as_mut_ptr instead.

Centril added a commit to Centril/rust that referenced this pull request

Feb 13, 2019

fix str mutating through a ptr derived from &self

For this by using a newly introduced as_mut_ptr instead.

bors added a commit that referenced this pull request

Feb 13, 2019

Rollup of 13 pull requests

Successful merges:

#57693 (Doc rewording)
#57815 (Speed up the fast path for assert_eq! and assert_ne!)
#58034 (Stabilize the time_checked_add feature)
#58057 (Stabilize linker-plugin based LTO (aka cross-language LTO))
#58137 (Cleanup: rename node_id_to_type(_opt))
#58166 (allow shorthand syntax for deprecation reason)
#58196 (Add specific feature gate error for const-unstable features)
#58200 (fix str mutating through a ptr derived from &self)
#58273 (Rename rustc_errors dependency in rust 2018 crates)
#58289 (impl iter() for dyn Error)
#58387 (Disallow auto trait alias syntax)
#58404 (use Ubuntu keyserver for CloudABI ports)
#58405 (Remove some dead code from libcore)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request

Feb 13, 2019

fix str mutating through a ptr derived from &self

For this by using a newly introduced as_mut_ptr instead.

bors added a commit that referenced this pull request

Feb 13, 2019

Rollup of 12 pull requests

Successful merges:

#57693 (Doc rewording)
#57815 (Speed up the fast path for assert_eq! and assert_ne!)
#58034 (Stabilize the time_checked_add feature)
#58057 (Stabilize linker-plugin based LTO (aka cross-language LTO))
#58137 (Cleanup: rename node_id_to_type(_opt))
#58166 (allow shorthand syntax for deprecation reason)
#58200 (fix str mutating through a ptr derived from &self)
#58273 (Rename rustc_errors dependency in rust 2018 crates)
#58289 (impl iter() for dyn Error)
#58387 (Disallow auto trait alias syntax)
#58404 (use Ubuntu keyserver for CloudABI ports)
#58405 (Remove some dead code from libcore)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request

May 14, 2019

as_ptr returns a read-only pointer

Add comments to as_ptr methods to warn that these are read-only pointers, and writing to them is UB.

It was pointed out that CStr does not even have an as_mut_ptr. I originally was going to add one, but there is no method at all that would mutate a CStr. Was that a deliberate choice or should I add an as_mut_ptr (similar to what I did for str)?

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.