user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

move UnsafeCell-in-const check from interning to validation

Browse source
This commit is contained in:
Ralf Jung 2020-10-24 20:49:17 +02:00
parent d0a23e613d
commit 0e014be359
7 changed files with 74 additions and 53 deletions
Download patch file Download diff file Expand all files Collapse all files

17
compiler/rustc_mir/src/const_eval/eval_queries.rs
View file

@ -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;
}
}
};

6
compiler/rustc_mir/src/interpret/intern.rs
View file

@ -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

2
compiler/rustc_mir/src/interpret/mod.rs
View file

@ -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;

74
compiler/rustc_mir/src/interpret/validity.rs
View file

@ -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<T, PATH = ()> {
pub seen: FxHashSet<T>,
@ -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<PathElem>,
ref_tracking_for_consts:
Option<&'rt mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>>,
may_ref_to_static: bool,
ref_tracking: Option<&'rt mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>>,
/// `None` indicates this is not validating for CTFE (but for runtime).
ctfe_mode: Option<CtfeValidationMode>,
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<PathElem>,
ref_tracking_for_consts: Option<
&mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>,
>,
may_ref_to_static: bool,
ref_tracking: Option<&mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>>,
ctfe_mode: Option<CtfeValidationMode>,
) -> 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<PathElem>,
ref_tracking: &mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>,
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)
}
}

11
compiler/rustc_mir/src/transform/const_prop.rs
View file

@ -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;

4
src/test/ui/consts/miri_unleashed/mutable_references_err.rs
View file

@ -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;

13
src/test/ui/consts/miri_unleashed/mutable_references_err.stderr
View file

@ -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.<deref>
|
= 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 .<deref>.<dyn-downcast>.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`.