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

Promotion creates invalid constants that are never used in invalid ways #67534

Closed
oli-obk opened this issue Dec 22, 2019 · 17 comments · Fixed by #78363
Closed

Promotion creates invalid constants that are never used in invalid ways #67534

oli-obk opened this issue Dec 22, 2019 · 17 comments · Fixed by #78363
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Dec 22, 2019

Namely, we can have promoteds with interior mutable types but in an immutable allocation.
#67000 makes this explicit by introducing the ignore_interior_mut_in_const_validation field on the const interner.

We should instead rewrite such promoteds to just contain the parts of the promoted that is accessed. So

let x: &'static i32 = &[(Cell::new(42), 1), Cell::new(42), 2)][runtime_index].1;

currently produces

const PROMOTED: &[(Cell<i32>, i32); 2] = &[(Cell::new(42), 1), Cell::new(42), 2)];
let x: &'static i32 = &PROMOTED[runtime_index].1;

And we should be producing

const PROMOTED: &[i32; 2] = &[1, 2];
let x: &'static i32 = &PROMOTED[runtime_index];

This is definitely a nontrivial transformation, especially since it needs to be done on the generic promoted and not the computed final value (generic e.g. if we had T::CELL_VALUE instead of Cell::new(42)).
The reason it needs to be done on the MIR is that we also need to change the projection for obtaining the value of x, which we have no control over if we did it on the final constant.

@RalfJung
Copy link
Member

Also see this Zulip thread.

I am still not 100% of the soundness of this -- the UB in code like this relies on subtle and unstable details of current Stacked Borrows. In other words, assuming that what we currently do is sound makes some very strong statement about Rust semantics that @rust-lang/lang never signed off.

How hard would it be to make rustc reject such (IMO) broken promoteds, and use crater to determine how widely this pattern is used? The examples you showed so far are all extremely niche.

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 22, 2019

How hard would it be to make rustc reject such (IMO) broken promoteds, and use crater to determine how widely this pattern is used? The examples you showed so far are all extremely niche.

I think it would be fine, but I feel like the motivation to do so is weak. This is completely sane from a user perspective, and feels like the zero cost magic that Rust gives us.

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 22, 2019

this is the more realistic example that I would expect to see out there.

@RalfJung
Copy link
Member

This is completely sane from a user perspective,

I can't see anything sane about this from any perspective.^^ "This seems fine" is how we got ourselves in lots of trouble with promotion...

I feel like the motivation to do so is weak

The burden of motivation is on whoever writes an RFC to add support for this to rustc, IMO. That this didn't happen is an accident.

@RalfJung
Copy link
Member

I also don't think "this is unsound in all but our most complex Rust semantics" is a pretty strong motivation for not doing this. ;)

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 22, 2019

The burden of motivation is on whoever writes an RFC to add support for this to rustc, IMO. That this didn't happen is an accident.

This didn't happen by accident, this is entirely by design afaict (cc @eddyb). Though I'm not sure how much of that was RFCed. Implicit promotion is definitely not our proudest moment in RFCing and stabilizing.

@RalfJung
Copy link
Member

This didn't happen by accident, this is entirely by design afaict (cc @eddyb).

In that design, what was the soundness argument? Because as far as I can see, one can trivially break soundness of this scheme by doing some pointer arithmetic outside the bounds of the reference one gets. Yes, Stacked Borrows currently disallows this, but (a) Stacked Borrows didn't exist yet when this was designed, so that cannot have been the argument, and (b) this is one of the Stacked Borrows properties that people semi-regularly run into and complain about (e.g. here and here).

@RalfJung RalfJung added A-const-eval Area: Constant evaluation (MIR interpretation) I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Dec 23, 2019
@eddyb
Copy link
Member

eddyb commented Jan 16, 2020

What is being proposed, in terms of potentially breaking changes?
If you replace the i with a constant, then the question becomes about the difference between:

  • &CONST.1
  • &{CONST.1}

It's unreasonable to assume that you can access any other fields through the latter, so do we want to force people through that?

I'm bringing this up because it's easy for us to treat them like equivalent, or a lot easier than the runtime indexing case, at least.
But it is a can of worms that we've opened up, I agree with @RalfJung on that.


I think the reason for the original behavior (before the runtime indexing special-case, and even before MIR) was that accessing a field always cleared "qualifications" that didn't apply to the field type, even if in the borrow itself - that is, &A.1 and &(A.1 + B.1) had similar treatment of all the .1 field accesses, despite the + copying out the values from the fields (which the borrow doesn't do).

AFAICT, this was kept during the move the MIR, and only slightly tweaked later.
We do require A in &A.1 has no destructor, so we could do the same for interior mutability.
All it would take (for promotion, not sure about const-checking) is commenting this out:

// This allows borrowing fields which don't have
// `HasMutInterior`, from a type that does, e.g.:
// `let _: &'static _ = &(Cell::new(1), 2).1;`
let mut place_projection = &place.projection[..];
// FIXME(eddyb) use a forward loop instead of a reverse one.
while let [proj_base @ .., elem] = place_projection {
// FIXME(eddyb) this is probably excessive, with
// the exception of `union` member accesses.
let ty =
Place::ty_from(&place.local, proj_base, *self.body, self.tcx)
.projection_ty(self.tcx, elem)
.ty;
if ty.is_freeze(self.tcx, self.param_env, DUMMY_SP) {
has_mut_interior = false;
break;
}
place_projection = proj_base;
}

@RalfJung
Copy link
Member

It's unreasonable to assume that you can access any other fields through the latter

Not sure which part of our semantics you are basing that on, other than Stacked Borrows. Not being able to access stuff "outside" of where the pointer points to is a frequent issue with raw pointers.

@eddyb
Copy link
Member

eddyb commented Jan 19, 2020

@RalfJung "the latter" refers to &{CONST.1}, which borrows a temporary copy of the .1 field - even if it were &{x.1}, there's none of the other fields of x left around, so raw pointer hacks wouldn't ever work (on top of being UB because they access outside the allocation of the temporary copy).

@RalfJung
Copy link
Member

Oh I see... because that evaluated {CONST.1} to a value, and then creates a temporary with that and a pointer to it? Makes sense.

@jonas-schievink jonas-schievink added C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 8, 2020
@Elinvynia Elinvynia added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 9, 2020
@Dylan-DPC-zz
Copy link

Marking it as p-high based on the wg-prioritisation discussion

@Dylan-DPC-zz Dylan-DPC-zz added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 12, 2020
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 13, 2020

I was doing such a good job of ignoring this issue, and now it's P-high 😆

We can't really write a forward compat lint for this, as the user may not be using the promoted in a 'static way, so if we warn them we'd be warning on perfectly fine code. So the only path forward is to remove the snippet linked above and run crater.

@RalfJung
Copy link
Member

Here's another horrible thing we could do: we could change the type of the promoted to replace all UnsafeCell<T> by T...

Or does this also work for custom struct constructors, not just with tuples like in the example?

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 12, 2020

Yea this will also work for your own structs. We can wrap the entire thing in a MaybeUninit :D

@RalfJung
Copy link
Member

That does nothing with UnsafeCell...

@RalfJung
Copy link
Member

let x: &'static i32 = &[(Cell::new(42), 1), Cell::new(42), 2)][runtime_index].1;

FWIW, this code is currently not even accepted inside a function, I think? It only works inside another const/static, because there we still permit promotion of arbitrary const fn.

@bors bors closed this as completed in c0bfe34 Dec 9, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 25, 2020
validate promoteds

Turn on const-value validation for promoteds. This is made possible now that rust-lang#67534 is resolved.

I don't think this is a breaking change. We don't promote any unsafe operation any more (since rust-lang#77526 landed). We *do* promote `const fn` calls under some circumstances (in `const`/`static` initializers), but union field access and similar operations are not allowed in `const fn`. So now is a perfect time to add this check. :D

r? `@oli-obk`
Fixes rust-lang#67465
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation (MIR interpretation) 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants