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

Extend check for UnsafeCell in consts to cover unions #90383

Merged
merged 1 commit into from
Jan 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion compiler/rustc_const_eval/src/interpret/validity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use rustc_middle::mir::interpret::InterpError;
use rustc_middle::ty;
use rustc_middle::ty::layout::{LayoutOf, TyAndLayout};
use rustc_span::symbol::{sym, Symbol};
use rustc_span::DUMMY_SP;
use rustc_target::abi::{Abi, Scalar as ScalarAbi, Size, VariantIdx, Variants, WrappingRange};

use std::hash::Hash;
Expand Down Expand Up @@ -736,9 +737,15 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
#[inline(always)]
fn visit_union(
&mut self,
_op: &OpTy<'tcx, M::PointerTag>,
op: &OpTy<'tcx, M::PointerTag>,
_fields: NonZeroUsize,
) -> InterpResult<'tcx> {
// Special check preventing `UnsafeCell` inside unions in the inner part of constants.
if matches!(self.ctfe_mode, Some(CtfeValidationMode::Const { inner: true, .. })) {
tmiasko marked this conversation as resolved.
Show resolved Hide resolved
if !op.layout.ty.is_freeze(self.ecx.tcx.at(DUMMY_SP), self.ecx.param_env) {
throw_validation_failure!(self.path, { "`UnsafeCell` in a `const`" });
}
}
Ok(())
}

Expand Down
24 changes: 24 additions & 0 deletions src/test/ui/consts/invalid-union.32bit.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
error[E0080]: it is undefined behavior to use this value
--> $DIR/invalid-union.rs:41:1
|
LL | fn main() {
| ^^^^^^^^^ type validation failed at .<deref>.y.<enum-variant(B)>.0: encountered `UnsafeCell` in a `const`
|
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
= note: the raw bytes of the constant (size: 4, align: 4) {
╾─alloc7──╼ │ ╾──╼
}

error: erroneous constant used
--> $DIR/invalid-union.rs:42:25
|
LL | let _: &'static _ = &C;
| ^^ referenced constant has errors
|
= note: `#[deny(const_err)]` on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #71800 <https://github.com/rust-lang/rust/issues/71800>

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0080`.
24 changes: 24 additions & 0 deletions src/test/ui/consts/invalid-union.64bit.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
error[E0080]: it is undefined behavior to use this value
--> $DIR/invalid-union.rs:41:1
|
LL | fn main() {
| ^^^^^^^^^ type validation failed at .<deref>.y.<enum-variant(B)>.0: encountered `UnsafeCell` in a `const`
|
= note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
= note: the raw bytes of the constant (size: 8, align: 8) {
╾───────alloc7────────╼ │ ╾──────╼
}

error: erroneous constant used
--> $DIR/invalid-union.rs:42:25
|
LL | let _: &'static _ = &C;
| ^^ referenced constant has errors
|
= note: `#[deny(const_err)]` on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #71800 <https://github.com/rust-lang/rust/issues/71800>

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0080`.
44 changes: 44 additions & 0 deletions src/test/ui/consts/invalid-union.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Check that constants with interior mutability inside unions are rejected
// during validation.
//
// Note that this test case relies on undefined behaviour to construct a
// constant with interior mutability that is "invisible" to the static checks.
// If for some reason this approach no longer works, it is should be fine to
// remove the test case.
Copy link
Member

Choose a reason for hiding this comment

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

Wow, what a test. :D

That said, which part of this test causes UB? With a repr, I think changing the tag of an enum via raw ptr manipulation is totally fine. It seems promotion analysis is making some IMO unfounded assumptions if it thinks the tag cannot change here.

Copy link
Member

Choose a reason for hiding this comment

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

Oh wait, you are writing through a shared ref. That is more tricky... though the exact scope of UnsafeCell inside enums is not decided yet, so this is not as clear-cut as one might think at first sight. For example, for an Option<Cell<NonZeroI32>>, mutating the discriminant through a shared ref is almost certainly going to be allowed.
Cc rust-lang/unsafe-code-guidelines#204 rust-lang/unsafe-code-guidelines#236

Copy link
Member

@BoxyUwU BoxyUwU Oct 29, 2021

Choose a reason for hiding this comment

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

For example, for an Option<Cell<NonZeroI32>>, mutating the discriminant through a shared ref is almost certainly going to be allowed.

was this a typo? I thought this specifically wasnt allowed, we made UnsafeCell inhibit niches, i.e. size_of::<Option<UnsafeCell<NonZeroU32>>>() == 8

edit: oh no I just checked and it doesnt apply when the unsafecell is newtyped..? That seems bad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test case creates a reference to S::x field which has a u32 type, so no interior mutability here. The const qualification assumes that resulting pointer cannot be used to access the other parts of a local. The assumption is violated here by offsetting the pointer and overwriting an adjacent tag.

edit: oh no I just checked and it doesnt apply when the unsafecell is newtyped..? That seems bad

Good catch. I think we already have an open issue for niche hiding not working exactly as intended.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right UnsafeCell masks niches these days (except when newtpyed but that is a bug: #87341).

Test case creates a reference to S::x field which has a u32 type, so no interior mutability here. The const qualification assumes that resulting pointer cannot be used to access the other parts of a local. The assumption is violated here by offsetting the pointer and overwriting an adjacent tag.

Ah I see. So this makes certain assumptions about the aliasing model, but this particular case is probably fine -- however, I am still a bit worried that promotion analysis might make more assumptions than we can actually uphold. In this case we are creating an &T where T: Freeze. But if we ever do anything value-based here (e.g., &None::<T> where T might be !Freeze), that would be sketchy.

//
// build-fail
// stderr-per-bitwidth
#![feature(const_mut_refs)]
#![feature(const_ptr_offset)]
#![feature(untagged_unions)]
use std::cell::Cell;

#[repr(C)]
struct S {
tmiasko marked this conversation as resolved.
Show resolved Hide resolved
x: u32,
y: E,
}

#[repr(u32)]
enum E {
A,
B(U)
}

union U {
cell: Cell<u32>,
}

const C: S = {
let s = S { x: 0, y: E::A };
// Go through an &u32 reference which is definitely not allowed to mutate anything.
let p = &s.x as *const u32 as *mut u32;
tmiasko marked this conversation as resolved.
Show resolved Hide resolved
// Change enum tag to E::B.
unsafe { *p.add(1) = 1 };
s
};

fn main() { //~ ERROR it is undefined behavior to use this value
let _: &'static _ = &C; //~ ERROR erroneous constant used
//~^ WARN this was previously accepted
}