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

VecDeque: fix for stacked borrows #56161

Merged
merged 3 commits into from
Dec 13, 2018

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Nov 22, 2018

VecDeque violates a version of stacked borrows where creating a shared reference is not enough to make a location mutably accessible from raw pointers (and I think that is the version we want). There are two problems:

  • Creating a NonNull<T> from &mut T goes through &T (inferred for a _), then *const T, then NonNull<T>. That means in this stricter version of Stacked Borrows, we cannot actually write to such a NonNull because it was created from a shared reference! This PR fixes that by going from &mut T to *mut T to *const T.
  • VecDeque::drain creates the Drain struct by first creating a NonNull from self (which is an &mut VecDeque), and then calling self.buffer_as_mut_slice(). The latter reborrows self, asserting that self is currently the unique pointer to access this VecDeque, and hence invalidating the NonNull that was created earlier. This PR fixes that by instead using self.buffer_as_slice(), which only performs read accesses and creates only shared references, meaning the raw pointer (NonNull) remains valid.

It is possible that other methods on VecDeque do something similar, miri's test coverage of VecDeque is sparse to say the least.

Cc @nikomatsakis @gankro

@rust-highfive
Copy link
Collaborator

r? @KodrAus

(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 Nov 22, 2018
@RalfJung
Copy link
Member Author

Turns out just not using a mutable buffer, as suggested by @gnzlbg, is enough :)

Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

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

LGTM.

@bluss
Copy link
Member

bluss commented Nov 24, 2018

Since this is super tricky stuff, it would be awesome if the PR description was updated to correspond to the final state of the PR .

@RalfJung
Copy link
Member Author

@bluss Good point, done.

@@ -2808,14 +2808,14 @@ impl<T: ?Sized> fmt::Pointer for Unique<T> {
#[unstable(feature = "ptr_internals", issue = "0")]
impl<'a, T: ?Sized> From<&'a mut T> for Unique<T> {
fn from(reference: &'a mut T) -> Self {
Unique { pointer: NonZero(reference as _), _marker: PhantomData }
Unique { pointer: NonZero(reference as *mut T as *const T), _marker: PhantomData }
Copy link
Member

@bluss bluss Nov 24, 2018

Choose a reason for hiding this comment

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

If I understand you correctly, the _ here was filled in with &'a T, and that's no good?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.

Ideally we could make it infer *mut T for the _, that would be the safer choice.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 1, 2018

Rebased (because I want to test this together with #56165)

@bors

This comment has been minimized.

By going through a shared reference, we share the destination as read-only, meaning we can read but not write with the raw pointers
@RalfJung
Copy link
Member Author

RalfJung commented Dec 7, 2018

This PR is now awaiting review for 12 days.

Ping @KodrAus

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 7, 2018

r? me

@RalfJung
Copy link
Member Author

RalfJung commented Dec 7, 2018

r? @gnzlbg

@SimonSapin
Copy link
Contributor

These are really subtle rules, and I’m worried about what this means for unsafe code outside of the standard library that might similarly violate those rules.

This combination feels very dangerous:

  • Going from &mut T to &T is a sort of "assertion of immutability" that has very significant meaning for the soundness of unsafe code
  • This coercion can happen implicitly without as
  • It even happens in the middle of foo as _ where foo: &mut T in a context that expects *const T, without the &T type being named anywhere

@SimonSapin
Copy link
Contributor

I think I feel strongly enough about this that I’m opposed to landing this PR as-is. Making soundness rely on the non-existence of a shared reference is not acceptable IMO as long as such a shared reference can be created "invisibly" like this.

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 7, 2018

@SimonSapin I am not sure that I understand your argument correctly. Are you arguing that it is easy to assert immutability accidentally and therefore introduce undefined behavior ?

If so, I agree. But this code was doing so explicitly with as _ where a lot was going on independently of whether this PR is merged or not. This PR is more explicit and clear (e.g. as *mut T instead of as _) and undefined behavior free (EDIT: at least, with the current stacked borrows semantics).

There are a couple of ideas being floated around about how to prevent "asserting immutability by accident" (e.g. making as/coercion chains "atomic"), and one of these ideas already has an RFC (rust-lang/rfcs#2582). But more RFCs will be needed to at least reduce the degree by which this can happen by accident.

@SimonSapin
Copy link
Contributor

Yes, this is what I’m arguing.

We have here a combination of three things:

  1. The in the newly-proposed stacked borrows model, mutating through a raw pointer that was crated from a shared reference is not allowed.
  2. Converting from &mut T to *const T with x as _ silently goes through &T.
  3. We found one instance of 2. happening with mutation, violating the rules of 1.

It’s only with all three combined that there’s a problem. So we can fix the problem in any of three ways:

A. Not accepting this proposed model
B. Changing as so it doesn’t do that
C. Fixing the code in this one instance

I am opposed to doing only C. because that doen’t help with other potential instances of the same problem. This is made worse by 2. being, in my opinion, very unexpected. I claim to be fairly proficient at Rust, and I had no idea this was happening. x as *const T is also allowed, why would _ not be inferred to *const T in this case? I suspect this was only discovered through mechanical checking by miri, but it’s not practical to declare that all unsafe Rust code in the world must be checked by miri. (For example, that is not possible when FFI is involved.)

So I think we should fix as and not land (the ptr.rs part of) this PR.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 7, 2018

Going from &mut T to &T is a sort of "assertion of immutability" that has very significant meaning for the soundness of unsafe code

From my understanding, this was an explicit design goal from the start. Cc @eddyb @gankro @nikomatsakis

Not doing this kills a lot of useful optimizations, because it means that creating a shared reference gives license to anyone to mutate data (until a barrier is added). In particular, the change I am making here is a necessary requirement to ever solve #16515 for shared references.

mutating through a raw pointer that was crated from a shared reference is not allowed.

This was one of the first rules I ever learned about mutability in Rust: Though shall not mutate through shared references. Of course this has to include raw pointers created from shared references, otherwise the rule is useless (the compiler syntactically prevents direct assignment to a shared reference). This rule is the reason we have UnsafeCell. It is fundamental, and it is ancient.

It even happens in the middle of foo as _ where foo: &mut T in a context that expects *const T, without the &T type being named anywhere

That, I think, is a problem. We should change the coercion logic to only coerce to &T if needed.


The reason I would like to land this PR is that it is needed to move forward with experimenting with this model in miri. I do not see this as acknowledgement that these will ultimately be the rules. But as long as a core data structure like NonNull breaks them, we cannot even reasonably use miri to see which effect this stricter rule has in the wild.

When I previously talked about this with @nikomatsakis, he was open to changing the coercion rules in principle, but also said that coercion is a mess and in its current state maybe should not be touched, but rewritten. This PR is a way forward for experimentation with miri that is not blocked on fixing coercion. But I absolutely agree that we should block accepting this model and exploiting it in the compiler on fixing coercions. I am not sure what would be the best place to track this.

@eddyb
Copy link
Member

eddyb commented Dec 7, 2018

So is the problem here that x as _ infers the _ to &T instead of *const T?
If so, I agree it needs to be fixed. I think we shouldn't coerce the result of a cast if we can infer the cast to that coercion. We're only forced to do an extra coercion if the cast type is more explicit.

but also said that coercion is a mess and in its current state maybe should not be touched, but rewritten

I hope/suspect "rewritten" here doesn't mean just rewriting the librustc_typeck/check/coerce.rs file, since that wouldn't do much good, but rather integrating coercions with the trait system.

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 7, 2018

So is the problem here that x as _ infers the _ to &T instead of *const T?

The problem is that coercion chains are not atomic. x as _ does infers the _ to a &T, which is arguably wrong, but note that the as _ is not necessary here, x is a reference, and it can coerce to a raw pointer. This does still fail, because the coercion still makes it go through a reference.

I have no idea what as _ was trying to achieve here, but it doesn't really do anything. That is, this code needs changing no matter what (either to x as *mut T or just to x).

@gnzlbg
Copy link
Contributor

gnzlbg commented Dec 7, 2018

FWIW, I'm fine to changing this to x as *mut T for now, but in the future that should raise a warning because the as *mut T (or as _) are unnecessary.

@eddyb
Copy link
Member

eddyb commented Dec 7, 2018

Are let y: *mut T = x; and x as *mut T different, at all? That sounds very wrong.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 7, 2018

I opened #56604 for the coercion problem, let's continue the discussion there.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 7, 2018

@SimonSapin Beyond the issue above, I have added a not-yet-working test with a FIXME to miri and recorded this in the "missing validation tracking issue" at rust-lang/miri#551. Is that enough to remedy your concerns about this change?

libtest uses BTreeMap uses NonNull, so making cargo miri test work at all needs some kind of solution to this problem. I have no idea how long it will take to sort out the coercion problem, but I'm afraid it might take a while. That's why I'd like to land this.

I could change miri to be less strict about the "writing through raw pointers obtained from shared references" rule, but it seems strange to accept code that we definitely know we do not want to accept.

@RalfJung
Copy link
Member Author

RalfJung commented Dec 9, 2018

libtest uses BTreeMap uses NonNull, so making cargo miri test work at all needs some kind of solution to this problem.

Turns out that BTreeMap does not use the problematic parts of NonNull, so this PR doesn't actually block getting cargo miri test to work.

Still, seems likely that other code (besides VecDeque) will also run into these "bad" conversion functions.

@SimonSapin
Copy link
Contributor

The reason I would like to land this PR is that it is needed to move forward with experimenting with this model in miri.

Ok, landing this PR to unblock other work sounds acceptable, with the understanding that it is not "the" way to fix this kind of unsoundess (#56604 is) and that unsafe code outside of std does not need to be audited for this pattern.

@Centril
Copy link
Contributor

Centril commented Dec 11, 2018

@SimonSapin As a process note (not an opinion on #56604 or rust-lang/rfcs#2582), such an understanding depends on consensus within T-lang; Since this is a soundness fix, we should just land it with a FIXME(RalfJung) or something like that and revisit if and when rules change. Blocking it on anything else seems like linking things together that shouldn't be linked.

@Gankra
Copy link
Contributor

Gankra commented Dec 11, 2018

It’s not actually a soundness fix since nothing in codegen relies on the model this code violates

@RalfJung
Copy link
Member Author

the understanding that it is not "the" way to fix this kind of unsoundess (#56604 is) and that unsafe code outside of std does not need to be audited for this pattern.

Agreed.

Okay so can I get someone to r+ this? :D

@SimonSapin
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 12, 2018

📌 Commit feb775c 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 Dec 12, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Dec 12, 2018
…r=SimonSapin

VecDeque: fix for stacked borrows

`VecDeque` violates a version of stacked borrows where creating a shared reference is not enough to make a location *mutably accessible* from raw pointers (and I think that is the version we want).  There are two problems:

* Creating a `NonNull<T>` from `&mut T` goes through `&T` (inferred for a `_`), then `*const T`, then `NonNull<T>`. That means in this stricter version of Stacked Borrows, we cannot actually write to such a `NonNull` because it was created from a shared reference! This PR fixes that by going from `&mut T` to `*mut T` to `*const T`.
* `VecDeque::drain` creates the `Drain` struct by *first* creating a `NonNull` from `self` (which is an `&mut VecDeque`), and *then* calling `self.buffer_as_mut_slice()`. The latter reborrows `self`, asserting that `self` is currently the unique pointer to access this `VecDeque`, and hence invalidating the `NonNull` that was created earlier. This PR fixes that by instead using `self.buffer_as_slice()`, which only performs read accesses and creates only shared references, meaning the raw pointer (`NonNull`) remains valid.

It is possible that other methods on `VecDeque` do something similar, miri's test coverage of `VecDeque` is sparse to say the least.

Cc @nikomatsakis @gankro
@bors
Copy link
Contributor

bors commented Dec 13, 2018

⌛ Testing commit feb775c with merge 9fe5cb5...

bors added a commit that referenced this pull request Dec 13, 2018
VecDeque: fix for stacked borrows

`VecDeque` violates a version of stacked borrows where creating a shared reference is not enough to make a location *mutably accessible* from raw pointers (and I think that is the version we want).  There are two problems:

* Creating a `NonNull<T>` from `&mut T` goes through `&T` (inferred for a `_`), then `*const T`, then `NonNull<T>`. That means in this stricter version of Stacked Borrows, we cannot actually write to such a `NonNull` because it was created from a shared reference! This PR fixes that by going from `&mut T` to `*mut T` to `*const T`.
* `VecDeque::drain` creates the `Drain` struct by *first* creating a `NonNull` from `self` (which is an `&mut VecDeque`), and *then* calling `self.buffer_as_mut_slice()`. The latter reborrows `self`, asserting that `self` is currently the unique pointer to access this `VecDeque`, and hence invalidating the `NonNull` that was created earlier. This PR fixes that by instead using `self.buffer_as_slice()`, which only performs read accesses and creates only shared references, meaning the raw pointer (`NonNull`) remains valid.

It is possible that other methods on `VecDeque` do something similar, miri's test coverage of `VecDeque` is sparse to say the least.

Cc @nikomatsakis @gankro
@bors
Copy link
Contributor

bors commented Dec 13, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: SimonSapin
Pushing 9fe5cb5 to master...

@bors bors merged commit feb775c into rust-lang:master Dec 13, 2018
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #56161!

Tested on commit 9fe5cb5.
Direct link to PR: #56161

💔 rls on linux: test-pass → test-fail (cc @nrc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Dec 13, 2018
Tested on commit rust-lang/rust@9fe5cb5.
Direct link to PR: <rust-lang/rust#56161>

💔 rls on linux: test-pass → test-fail (cc @nrc @Xanewok, @rust-lang/infra).
@Xanewok
Copy link
Member

Xanewok commented Dec 13, 2018

@alexcrichton @kennytm this is intermittent - any possibility to retrigger the -tools CI job and change current toolstate without submitting a new PR?

(On a similar note, why do newest nightlies don't contain RLS or rustfmt? The toolstate was ok and the manifest tool was fixed from what I've seen - anything else blocking these from distribution?)

@RalfJung
Copy link
Member Author

why do newest nightlies don't contain RLS or rustfmt? The toolstate was ok and the manifest tool was fixed from what I've seen - anything else blocking these from distribution?

That would be #56667. There simply wasn't a new nightly since the tools were fixed.

@RalfJung RalfJung deleted the vecdeque-stacked-borrows branch December 13, 2018 14:21
@Xanewok
Copy link
Member

Xanewok commented Dec 13, 2018

Ah, so it's not because the nightlies are blocked on broken tools but there's a problem with distributing nightlies itself! Makes sense, thanks for the heads up.

EDIT: Or maybe not (it seems clippy managed to break just before the distribution). In any case, hopefully the next nightlies are okay.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.