From 0e014be35971f19001bb58c1e9a904e9f596a052 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 24 Oct 2020 20:49:17 +0200 Subject: [PATCH] move UnsafeCell-in-const check from interning to validation --- .../rustc_mir/src/const_eval/eval_queries.rs | 17 +++-- compiler/rustc_mir/src/interpret/intern.rs | 6 +- compiler/rustc_mir/src/interpret/mod.rs | 2 +- compiler/rustc_mir/src/interpret/validity.rs | 74 +++++++++++-------- .../rustc_mir/src/transform/const_prop.rs | 11 +-- .../miri_unleashed/mutable_references_err.rs | 4 +- .../mutable_references_err.stderr | 13 +++- 7 files changed, 74 insertions(+), 53 deletions(-) diff --git a/compiler/rustc_mir/src/const_eval/eval_queries.rs b/compiler/rustc_mir/src/const_eval/eval_queries.rs index 2c92c8e2a68d..49cb284be8e6 100644 --- a/compiler/rustc_mir/src/const_eval/eval_queries.rs +++ b/compiler/rustc_mir/src/const_eval/eval_queries.rs @@ -1,8 +1,8 @@ use super::{CompileTimeEvalContext, CompileTimeInterpreter, ConstEvalErr, MemoryExtra}; use crate::interpret::eval_nullary_intrinsic; use crate::interpret::{ - intern_const_alloc_recursive, Allocation, ConstAlloc, ConstValue, GlobalId, Immediate, - InternKind, InterpCx, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking, Scalar, + intern_const_alloc_recursive, Allocation, ConstAlloc, ConstValue, CtfeValidationMode, GlobalId, + Immediate, InternKind, InterpCx, InterpResult, MPlaceTy, MemoryKind, OpTy, RefTracking, Scalar, ScalarMaybeUninit, StackPopCleanup, }; @@ -376,13 +376,14 @@ pub fn eval_to_allocation_raw_provider<'tcx>( // https://github.com/rust-lang/rust/issues/67465 is made if cid.promoted.is_none() { let mut ref_tracking = RefTracking::new(mplace); + let mut inner = false; while let Some((mplace, path)) = ref_tracking.todo.pop() { - ecx.const_validate_operand( - mplace.into(), - path, - &mut ref_tracking, - /*may_ref_to_static*/ ecx.memory.extra.can_access_statics, - )?; + let mode = match tcx.static_mutability(cid.instance.def_id()) { + Some(_) => CtfeValidationMode::Regular, // a `static` + None => CtfeValidationMode::Const { inner }, + }; + ecx.const_validate_operand(mplace.into(), path, &mut ref_tracking, mode)?; + inner = true; } } }; diff --git a/compiler/rustc_mir/src/interpret/intern.rs b/compiler/rustc_mir/src/interpret/intern.rs index 846ca1899002..53ce4deeff82 100644 --- a/compiler/rustc_mir/src/interpret/intern.rs +++ b/compiler/rustc_mir/src/interpret/intern.rs @@ -129,9 +129,7 @@ fn intern_shallow<'rt, 'mir, 'tcx, M: CompileTimeMachine<'mir, 'tcx>>( // See const_eval::machine::MemoryExtra::can_access_statics for why // immutability is so important. - // There are no sensible checks we can do here; grep for `mutable_memory_in_const` to - // find the checks we are doing elsewhere to avoid even getting here for memory - // that "wants" to be mutable. + // Validation will ensure that there is no `UnsafeCell` on an immutable allocation. alloc.mutability = Mutability::Not; }; // link the alloc id to the actual allocation @@ -176,7 +174,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: CompileTimeMachine<'mir, 'tcx>> ValueVisitor<'mir // they caused. It also helps us to find cases where const-checking // failed to prevent an `UnsafeCell` (but as `ignore_interior_mut_in_const` // shows that part is not airtight). - mutable_memory_in_const(self.ecx.tcx, "`UnsafeCell`"); + //mutable_memory_in_const(self.ecx.tcx, "`UnsafeCell`"); } // We are crossing over an `UnsafeCell`, we can mutate again. This means that // References we encounter inside here are interned as pointing to mutable diff --git a/compiler/rustc_mir/src/interpret/mod.rs b/compiler/rustc_mir/src/interpret/mod.rs index a931b0bbe977..072fe21fbcca 100644 --- a/compiler/rustc_mir/src/interpret/mod.rs +++ b/compiler/rustc_mir/src/interpret/mod.rs @@ -24,7 +24,7 @@ pub use self::machine::{compile_time_machine, AllocMap, Machine, MayLeak, StackP pub use self::memory::{AllocCheck, FnVal, Memory, MemoryKind}; pub use self::operand::{ImmTy, Immediate, OpTy, Operand}; pub use self::place::{MPlaceTy, MemPlace, MemPlaceMeta, Place, PlaceTy}; -pub use self::validity::RefTracking; +pub use self::validity::{RefTracking, CtfeValidationMode}; pub use self::visitor::{MutValueVisitor, ValueVisitor}; crate use self::intrinsics::eval_nullary_intrinsic; diff --git a/compiler/rustc_mir/src/interpret/validity.rs b/compiler/rustc_mir/src/interpret/validity.rs index c38f25564e8d..3026b6c6c493 100644 --- a/compiler/rustc_mir/src/interpret/validity.rs +++ b/compiler/rustc_mir/src/interpret/validity.rs @@ -113,6 +113,15 @@ pub enum PathElem { DynDowncast, } +/// Extra things to check for during validation of CTFE results. +pub enum CtfeValidationMode { + /// Regular validation, nothing special happening. + Regular, + /// Validation of a `const`. `inner` says if this is an inner, indirect allocation (as opposed + /// to the top-level const allocation). + Const { inner: bool }, +} + /// State for tracking recursive validation of references pub struct RefTracking { pub seen: FxHashSet, @@ -202,9 +211,9 @@ struct ValidityVisitor<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> { /// starts must not be changed! `visit_fields` and `visit_array` rely on /// this stack discipline. path: Vec, - ref_tracking_for_consts: - Option<&'rt mut RefTracking, Vec>>, - may_ref_to_static: bool, + ref_tracking: Option<&'rt mut RefTracking, Vec>>, + /// `None` indicates this is not validating for CTFE (but for runtime). + ctfe_mode: Option, ecx: &'rt InterpCx<'mir, 'tcx, M>, } @@ -418,7 +427,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' { "a dangling {} (use-after-free)", kind }, ); // Recursive checking - if let Some(ref mut ref_tracking) = self.ref_tracking_for_consts { + if let Some(ref mut ref_tracking) = self.ref_tracking { if let Some(ptr) = ptr { // not a ZST // Skip validation entirely for some external statics @@ -426,19 +435,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' if let Some(GlobalAlloc::Static(did)) = alloc_kind { assert!(!self.ecx.tcx.is_thread_local_static(did)); assert!(self.ecx.tcx.is_static(did)); - if self.may_ref_to_static { - // We skip checking other statics. These statics must be sound by - // themselves, and the only way to get broken statics here is by using - // unsafe code. - // The reasons we don't check other statics is twofold. For one, in all - // sound cases, the static was already validated on its own, and second, we - // trigger cycle errors if we try to compute the value of the other static - // and that static refers back to us. - // We might miss const-invalid data, - // but things are still sound otherwise (in particular re: consts - // referring to statics). - return Ok(()); - } else { + if matches!(self.ctfe_mode, Some(CtfeValidationMode::Const { .. })) { // See const_eval::machine::MemoryExtra::can_access_statics for why // this check is so important. // This check is reachable when the const just referenced the static, @@ -447,6 +444,17 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' { "a {} pointing to a static variable", kind } ); } + // We skip checking other statics. These statics must be sound by + // themselves, and the only way to get broken statics here is by using + // unsafe code. + // The reasons we don't check other statics is twofold. For one, in all + // sound cases, the static was already validated on its own, and second, we + // trigger cycle errors if we try to compute the value of the other static + // and that static refers back to us. + // We might miss const-invalid data, + // but things are still sound otherwise (in particular re: consts + // referring to statics). + return Ok(()); } } // Proceed recursively even for ZST, no reason to skip them! @@ -504,7 +512,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' let value = self.ecx.read_scalar(value)?; // NOTE: Keep this in sync with the array optimization for int/float // types below! - if self.ref_tracking_for_consts.is_some() { + if self.ctfe_mode.is_some() { // Integers/floats in CTFE: Must be scalar bits, pointers are dangerous let is_bits = value.check_init().map_or(false, |v| v.is_bits()); if !is_bits { @@ -723,6 +731,15 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> // Sanity check: `builtin_deref` does not know any pointers that are not primitive. assert!(op.layout.ty.builtin_deref(true).is_none()); + // Special check preventing `UnsafeCell` in constants + if let Some(def) = op.layout.ty.ty_adt_def() { + if matches!(self.ctfe_mode, Some(CtfeValidationMode::Const { inner: true })) + && Some(def.did) == self.ecx.tcx.lang_items().unsafe_cell_type() + { + throw_validation_failure!(self.path, { "`UnsafeCell` in a `const`" }); + } + } + // Recursively walk the value at its type. self.walk_value(op)?; @@ -814,7 +831,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> self.ecx, ptr, size, - /*allow_uninit_and_ptr*/ self.ref_tracking_for_consts.is_none(), + /*allow_uninit_and_ptr*/ self.ctfe_mode.is_none(), ) { // In the happy case, we needn't check anything else. Ok(()) => {} @@ -865,16 +882,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { &self, op: OpTy<'tcx, M::PointerTag>, path: Vec, - ref_tracking_for_consts: Option< - &mut RefTracking, Vec>, - >, - may_ref_to_static: bool, + ref_tracking: Option<&mut RefTracking, Vec>>, + ctfe_mode: Option, ) -> InterpResult<'tcx> { trace!("validate_operand_internal: {:?}, {:?}", *op, op.layout.ty); // Construct a visitor - let mut visitor = - ValidityVisitor { path, ref_tracking_for_consts, may_ref_to_static, ecx: self }; + let mut visitor = ValidityVisitor { path, ref_tracking, ctfe_mode, ecx: self }; // Try to cast to ptr *once* instead of all the time. let op = self.force_op_ptr(op).unwrap_or(op); @@ -902,16 +916,18 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { /// `ref_tracking` is used to record references that we encounter so that they /// can be checked recursively by an outside driving loop. /// - /// `may_ref_to_static` controls whether references are allowed to point to statics. + /// `constant` controls whether this must satisfy the rules for constants: + /// - no pointers to statics. + /// - no `UnsafeCell` or non-ZST `&mut`. #[inline(always)] pub fn const_validate_operand( &self, op: OpTy<'tcx, M::PointerTag>, path: Vec, ref_tracking: &mut RefTracking, Vec>, - may_ref_to_static: bool, + ctfe_mode: CtfeValidationMode, ) -> InterpResult<'tcx> { - self.validate_operand_internal(op, path, Some(ref_tracking), may_ref_to_static) + self.validate_operand_internal(op, path, Some(ref_tracking), Some(ctfe_mode)) } /// This function checks the data at `op` to be runtime-valid. @@ -919,6 +935,6 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { /// It will error if the bits at the destination do not match the ones described by the layout. #[inline(always)] pub fn validate_operand(&self, op: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx> { - self.validate_operand_internal(op, vec![], None, false) + self.validate_operand_internal(op, vec![], None, None) } } diff --git a/compiler/rustc_mir/src/transform/const_prop.rs b/compiler/rustc_mir/src/transform/const_prop.rs index 14b310cda939..7e5a13a01a7b 100644 --- a/compiler/rustc_mir/src/transform/const_prop.rs +++ b/compiler/rustc_mir/src/transform/const_prop.rs @@ -9,7 +9,6 @@ use rustc_hir::def::DefKind; use rustc_hir::HirId; use rustc_index::bit_set::BitSet; use rustc_index::vec::IndexVec; -use rustc_middle::mir::interpret::{InterpResult, Scalar}; use rustc_middle::mir::visit::{ MutVisitor, MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor, }; @@ -28,9 +27,10 @@ use rustc_trait_selection::traits; use crate::const_eval::ConstEvalErr; use crate::interpret::{ - self, compile_time_machine, truncate, AllocId, Allocation, ConstValue, Frame, ImmTy, Immediate, - InterpCx, LocalState, LocalValue, MemPlace, Memory, MemoryKind, OpTy, Operand as InterpOperand, - PlaceTy, Pointer, ScalarMaybeUninit, StackPopCleanup, + self, compile_time_machine, truncate, AllocId, Allocation, ConstValue, CtfeValidationMode, + Frame, ImmTy, Immediate, InterpCx, InterpResult, LocalState, LocalValue, MemPlace, Memory, + MemoryKind, OpTy, Operand as InterpOperand, PlaceTy, Pointer, Scalar, ScalarMaybeUninit, + StackPopCleanup, }; use crate::transform::MirPass; @@ -805,8 +805,9 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> { value, vec![], // FIXME: is ref tracking too expensive? + // FIXME: what is the point of ref tracking if we do not even check the tracked refs? &mut interpret::RefTracking::empty(), - /*may_ref_to_static*/ true, + CtfeValidationMode::Regular, ) { trace!("validation error, attempt failed: {:?}", e); return; diff --git a/src/test/ui/consts/miri_unleashed/mutable_references_err.rs b/src/test/ui/consts/miri_unleashed/mutable_references_err.rs index 06fb27bcff86..e9f6f9140e77 100644 --- a/src/test/ui/consts/miri_unleashed/mutable_references_err.rs +++ b/src/test/ui/consts/miri_unleashed/mutable_references_err.rs @@ -13,7 +13,7 @@ unsafe impl Sync for Meh {} // the following will never be ok! no interior mut behind consts, because // all allocs interned here will be marked immutable. -const MUH: Meh = Meh { //~ ERROR: mutable memory (`UnsafeCell`) is not allowed in constant +const MUH: Meh = Meh { //~ ERROR: it is undefined behavior to use this value x: &UnsafeCell::new(42), }; @@ -24,7 +24,7 @@ unsafe impl Sync for Synced {} // Make sure we also catch this behind a type-erased `dyn Trait` reference. const SNEAKY: &dyn Sync = &Synced { x: UnsafeCell::new(42) }; -//~^ ERROR: mutable memory (`UnsafeCell`) is not allowed in constant +//~^ ERROR: it is undefined behavior to use this value // Make sure we also catch mutable references. const BLUNT: &mut i32 = &mut 42; diff --git a/src/test/ui/consts/miri_unleashed/mutable_references_err.stderr b/src/test/ui/consts/miri_unleashed/mutable_references_err.stderr index 7647a9ff4f6e..b5b34642ed19 100644 --- a/src/test/ui/consts/miri_unleashed/mutable_references_err.stderr +++ b/src/test/ui/consts/miri_unleashed/mutable_references_err.stderr @@ -1,16 +1,20 @@ -error: mutable memory (`UnsafeCell`) is not allowed in constant +error[E0080]: it is undefined behavior to use this value --> $DIR/mutable_references_err.rs:16:1 | LL | / const MUH: Meh = Meh { LL | | x: &UnsafeCell::new(42), LL | | }; - | |__^ + | |__^ type validation failed: encountered `UnsafeCell` in a `const` at .x. + | + = 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. -error: mutable memory (`UnsafeCell`) is not allowed in constant +error[E0080]: it is undefined behavior to use this value --> $DIR/mutable_references_err.rs:26:1 | LL | const SNEAKY: &dyn Sync = &Synced { x: UnsafeCell::new(42) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered `UnsafeCell` in a `const` at ...x + | + = 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. error: mutable memory (`&mut`) is not allowed in constant --> $DIR/mutable_references_err.rs:30:1 @@ -38,3 +42,4 @@ LL | const BLUNT: &mut i32 = &mut 42; error: aborting due to 3 previous errors; 1 warning emitted +For more information about this error, try `rustc --explain E0080`.