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.

Sign up for GitHub

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

Jump to bottom

fix str mutating through a ptr derived from &self #58200

Merged
merged 4 commits into from
Feb 13, 2019
Merged

fix str mutating through a ptr derived from &self #58200

merged 4 commits into from
Feb 13, 2019
+23 −7

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Feb 5, 2019

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.

@rust-highfive
Copy link
Collaborator

r? @Kimundi

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 5, 2019
RalfJung
RalfJung commented Feb 6, 2019
View reviewed changes
src/libcore/str/mod.rs
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))
Copy link
Member Author

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.

@RalfJung RalfJung mentioned this pull request Feb 6, 2019
Merged
SimonSapin
SimonSapin approved these changes Feb 6, 2019
View reviewed changes
src/libcore/str/mod.rs Outdated
/// modified in a way that it remains valid UTF-8.
///
/// [`u8`]: primitive.u8.html
#[unstable(feature = "str_as_mut_ptr", issue = "0")]
Copy link
Contributor

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

Copy link
Member Author

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.

@RalfJung RalfJung mentioned this pull request Feb 6, 2019
Closed
@RalfJung
Copy link
Member Author

RalfJung commented Feb 6, 2019

@bors r=SimonSapin

@bors
Copy link
Contributor

bors commented Feb 6, 2019

📌 Commit a996f2c has been approved by SimonSapin

@bors 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
@RalfJung
Copy link
Member Author

RalfJung commented Feb 7, 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 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
@SimonSapin
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 7, 2019

📌 Commit 66c894e has been approved by SimonSapin

@bors 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
@Centril
Rollup merge of rust-lang#58200 - RalfJung:str-as-mut-ptr, r=SimonSapin
17ca18c 
fix str mutating through a ptr derived from &self

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.
@Centril Centril mentioned this pull request Feb 13, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Feb 13, 2019
@Centril
Rollup merge of rust-lang#58200 - RalfJung:str-as-mut-ptr, r=SimonSapin
414f7ec 
fix str mutating through a ptr derived from &self

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.
@Centril Centril mentioned this pull request Feb 13, 2019
Closed
bors added a commit that referenced this pull request Feb 13, 2019
@bors
Auto merge of #58413 - Centril:rollup, r=Centril
2837b99 
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
@Centril
Rollup merge of rust-lang#58200 - RalfJung:str-as-mut-ptr, r=SimonSapin
ecb6503 
fix str mutating through a ptr derived from &self

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.
@Centril Centril mentioned this pull request Feb 13, 2019
Merged
bors added a commit that referenced this pull request Feb 13, 2019
@bors
Auto merge of #58415 - Centril:rollup, r=Centril
827a141 
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
@bors bors merged commit 66c894e into rust-lang:master Feb 13, 2019
@RalfJung RalfJung deleted the str-as-mut-ptr branch February 17, 2019 11:06
@RalfJung RalfJung mentioned this pull request May 1, 2019
Merged
Centril added a commit to Centril/rust that referenced this pull request May 14, 2019
@Centril
Rollup merge of rust-lang#60443 - RalfJung:as_ptr, r=SimonSapin
088c994 
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](https://internals.rust-lang.org/t/as-ptr-vs-as-mut-ptr/9940) 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`](rust-lang#58200))?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimonSapin SimonSapin SimonSapin approved these changes

Assignees

@Kimundi Kimundi

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RalfJung @rust-highfive @bors @SimonSapin @Kimundi