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

clarify how write_bytes can lead to UB due to invalid values #99084

Merged
merged 4 commits into from
Jul 26, 2022

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 9, 2022

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 9, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 9, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 9, 2022
@5225225
Copy link
Contributor

5225225 commented Jul 9, 2022

I'd include a code sample as well, personally.

let mut value: u8 = 0;
let ptr: *mut bool = &mut value as *mut u8 as *mut bool;
ptr.write_bytes(42, 1);

Something like that. Not sure.

And/or

Additionally, note that changing *dst in this way can lead to undefined behavior later if the written bytes are not a valid representation of some T, but the act of writing the bytes is not undefined behavior.

That might be too redundant, but it is more explicit.

@5225225

This comment was marked as outdated.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 9, 2022

I'd include a code sample as well, personally.

That's fair, I was just lazy...

write_bytes doesn't write a T.

I agree with that comment but I have no idea if you are suggesting any docs change here.

@5225225
Copy link
Contributor

5225225 commented Jul 9, 2022

I agree with that comment but I have no idea if you are suggesting any docs change here.

Was replying to a deleted comment, I'll hide it.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 9, 2022

Ah... yeah @eggyal please don't delete comments, it's just confusing.

@eggyal
Copy link
Contributor

eggyal commented Jul 9, 2022

Apologies. I posted my comment before I saw #99084 (comment), which had already answered it; and then deleted within seconds not realising a reply was already being written... hadn't intended to cause all this confusion!

@saethlin
Copy link
Member

saethlin commented Jul 9, 2022

Does this also close #84184 ? Or will that issue just need an update?

@RalfJung
Copy link
Member Author

RalfJung commented Jul 9, 2022

I think that issue is basically rust-lang/unsafe-code-guidelines#84, so I don't think it is entirely resolved by this PR.

This PR carefully uses a local variable of type u8 and a pointer of type bool to avoid rust-lang/unsafe-code-guidelines#84. Not that I expect anyone to actually deduce this from the example, but I wanted to avoid making a statement on rust-lang/unsafe-code-guidelines#84 here. Or maybe that's just not possible? I don't think we are ready to decide about rust-lang/unsafe-code-guidelines#84 though.

@saethlin
Copy link
Member

You commented before that:

I think it should be valid, but I don't think we have an RFC or a lang-team decision saying that it is valid. So we can't really recommend using it in the docs currently.

So I don't understand how if the example that already exists is on shaky ground, we can justify adding a second one. I can never tell if the docs are normative or not, but if they are we've already got the cart miles ahead of the horse here. What is the point of an RFC or a lang-team decision if this pattern is already enshrined as valid because it is in the docs?

@RalfJung
Copy link
Member Author

The docs I am adding here do not use the potentially bad pattern.

@saethlin
Copy link
Member

saethlin commented Jul 10, 2022

As far as I can tell the question that's up in the air in rust-lang/unsafe-code-guidelines#84 is: Even if you don't "use" it, is setting a value to an invalid bit pattern in itself UB? If the docs are normative, this has already been decided and the answer is no.

Also, I'm not sure what it means to "use" a value. The reference defines "producing" a value but that's not the term you're using here and also that page in the reference has a big red warning that is is not exhaustive. Does taking a reference to a value use the value? It seems like the answer to that is already "no"... because the docs do it. Even though there has been a lot of discussion about this recently.

I understand what you intend for this example in the docs. My concern is that these docs already suggest that a whole bunch which seems to me is not settled has actually been decided, and this PR is adding to that situation.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 10, 2022

If the docs are normative, this has already been decided and the answer is no.

Which part of the existing docs are you referring to? And which part of my PR makes any commitments regarding rust-lang/unsafe-code-guidelines#84?

All I want to do here is answer rust-lang/unsafe-code-guidelines#330 (comment) without making any other commitments. You seem to be saying that is not possible? I don't understand why, since rust-lang/unsafe-code-guidelines#330 (comment) is independent of rust-lang/unsafe-code-guidelines#84.

@saethlin
Copy link
Member

saethlin commented Jul 10, 2022

Okay, phew. I think I've poorly communicated my concern here and I'm also re-familiarizing myself with the context. Sorry about that.

Which part of the existing docs are you referring to?

/// ```
/// use std::ptr;
///
/// let mut v = Box::new(0i32);
///
/// unsafe {
/// // Leaks the previously held value by overwriting the `Box<T>` with
/// // a null pointer.
/// ptr::write_bytes(&mut v as *mut Box<i32>, 0, 1);
/// }
///
/// // At this point, using or dropping `v` results in undefined behavior.
/// // drop(v); // ERROR
///
/// // Even leaking `v` "uses" it, and hence is undefined behavior.
/// // mem::forget(v); // ERROR
///
/// // In fact, `v` is invalid according to basic type layout invariants, so *any*
/// // operation touching it is undefined behavior.
/// // let v2 = v; // ERROR
///
/// unsafe {
/// // Let us instead put in a valid value
/// ptr::write(&mut v as *mut Box<i32>, Box::new(42i32));
/// }
///
/// // Now the box is fine
/// assert_eq!(*v, 42);
/// ```

And which part of my PR makes any commitments regarding rust-lang/unsafe-code-guidelines#84?

I do not think this PR makes any additional commitments. Sorry if I suggested that. I think we are already more committed by the example I linked above.

I'm concerned about the net effect of adding more documentation and more examples to write_bytes. I think that a user could be rightfully confused because there are two examples which seem to be demonstrating the same thing. I don't want readers to be guessing that there is some subtle distinction between these examples that has led to having two examples instead of just one. The things that stand out to me as different is Box vs bool and one creates a reference to an invalid value where the other doesn't. I'm not sure we want a reader of these docs to be guessing or combing through UCG issues to figure out what's different.

If I had my way, we'd delete the existing example that I linked, and just have the one you are trying to add with this PR.

Co-authored-by: Ben Kimock <kimockb@gmail.com>
@RalfJung
Copy link
Member Author

Which part of the existing docs are you referring to?

Ah, I see.
No reference actually gets invalidated in the time where the Box is bad, so this is somewhere on the edge of UCG#84. I would also be fine with removing that example 🤷

I think it was actually meant to demonstrate the part of the docs this is editing, and that the UB is indeed delayed, not at the time of the write.

@5225225
Copy link
Contributor

5225225 commented Jul 11, 2022

Yeah, I think that box example should go, it looks to be mostly answering the question of rust-lang/unsafe-code-guidelines#84 rather than showing how to zero some memory.

84 should be answered in the docs when it's resolved, but I don't think it needs to be answered here. (I'd probably put it under ptr.write or something, in any case that's future work and we can't do it now)

@RalfJung
Copy link
Member Author

We haven't heard from @m-ou-se , so maybe let's re-roll the reviewer dice.

r? rust-lang/T-libs-api

@RalfJung
Copy link
Member Author

Oh, did the syntax change?
@rustbot r? rust/lib-api

@RalfJung
Copy link
Member Author

Yet another attempt at guessing the right syntax
r? libs

@rust-highfive rust-highfive assigned thomcc and unassigned m-ou-se Jul 25, 2022
@thomcc
Copy link
Member

thomcc commented Jul 25, 2022

Do you think this needs API review? If so, filing an issue on https://github.com/rust-lang/libs-team/ is the best way to do that now.

Otherwise I'll take a look later today, at a glance it doesn't look like it changes API contracts, but I mention it just cuz you tried to reroll for libs-api first.

@RalfJung
Copy link
Member Author

Filing an issue elsewhere to request review for a rustc PR? I am confused.^^

This changes the doc comment of a stdlib function. It doesn't change the syntax or types of any public API, but it changes / clarifies its contract. I'll leave it to you to decide what kind of review that needs.

@thomcc
Copy link
Member

thomcc commented Jul 25, 2022

If it changes the contract you should make an ACP about it.

If you want, you can wait until I review and I can tell you if I think this is needed, though.

@RalfJung
Copy link
Member Author

RalfJung commented Jul 25, 2022

Oh. If that much process is needed for this kind of PR nowadays I will probably have to reduce the amount of those PRs I write. I often notice small-ish issues in our unsafe API documentation, and like to write quick clarifications. But I don't have the time for extra paperwork here -- sorry. I could file issues rather than submit PRs, but in my experience these either never get fixed, or end up need a few rounds of review due to a lot of subtle issues at play -- which takes a lot more time than writing a PR myself.

Also that says ACP is for changes to unstable APIs. This is a change to the documentation of a stable API (like most of my stdlib documentation PRs).

@thomcc
Copy link
Member

thomcc commented Jul 25, 2022

Also that says ACP is for changes to unstable APIs. This is a change to the documentation of a stable API (like most of my stdlib documentation PRs).

Right, I guess the assumption is that we don't make changes to stable APIs.


Either way, this doesn't actually seem to change anything. This is a clarification of an existing API, and this was already UB in the same way, no?

@RalfJung
Copy link
Member Author

We change the documentation of unsafe stable APIs all the time. It's very hard to get the wording just right, so this is a continuous process.

Either way, this doesn't actually seem to change anything. This is a clarification of an existing API, and this was already UB in the same way, no?

Whether or not this changes anything depends on how you interpret the documentation. ;) My interpretation is that this just says, in a clearer way, what we already meant before.

@thomcc
Copy link
Member

thomcc commented Jul 25, 2022

Whether or not this changes anything depends on how you interpret the documentation. ;) My interpretation is that this just says, in a clearer way, what we already meant before.

I agree. The other option would be T-libs-API FCP, but I don't think this is needed (if someone disagrees they can r- or back it out) -- It's not identical to what was promised before, but I think the previous one meant to say something closer to this. I don't think that needs signoff, at least not in this case.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 25, 2022

📌 Commit 1b3870e has been approved by thomcc

It is now in the queue for this repository.

@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 Jul 25, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 25, 2022
clarify how write_bytes can lead to UB due to invalid values

Fixes rust-lang/unsafe-code-guidelines#330

Cc `@5225225`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022
Rollup of 9 pull requests

Successful merges:

 - rust-lang#92390 (Constify a few `(Partial)Ord` impls)
 - rust-lang#97077 (Simplify some code that depend on Deref)
 - rust-lang#98710 (correct the output of a `capacity` method example)
 - rust-lang#99084 (clarify how write_bytes can lead to UB due to invalid values)
 - rust-lang#99178 (Lighten up const_prop_lint, reusing const_prop)
 - rust-lang#99673 (don't ICE on invalid dyn calls)
 - rust-lang#99703 (Expose size_hint() for TokenStream's iterator)
 - rust-lang#99709 (`Inherited` always has `TypeckResults` available)
 - rust-lang#99713 (Fix sidebar background)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit aeca079 into rust-lang:master Jul 26, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 26, 2022
@RalfJung RalfJung deleted the write_bytes branch July 26, 2022 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When using std::ptr::copy and similar methods, does the memory need to be valid for T?
9 participants