From bf5e6ebdd3e1d54d210ff0285058ffa0979b9baa Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Oct 2018 16:06:50 +0200 Subject: [PATCH 01/19] miri validity: make recursive ref checking optional --- src/librustc_lint/builtin.rs | 9 +- src/librustc_mir/interpret/mod.rs | 2 + src/librustc_mir/interpret/validity.rs | 197 +++++++++++++++---------- 3 files changed, 121 insertions(+), 87 deletions(-) diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index f04e5496ce443..8dd1e2b53cdcb 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -1558,15 +1558,12 @@ fn validate_const<'a, 'tcx>( let ecx = ::rustc_mir::const_eval::mk_eval_cx(tcx, gid.instance, param_env).unwrap(); let result = (|| { let op = ecx.const_to_op(constant)?; - let mut todo = vec![(op, Vec::new())]; - let mut seen = FxHashSet(); - seen.insert(op); - while let Some((op, mut path)) = todo.pop() { + let mut ref_tracking = ::rustc_mir::interpret::RefTracking::new(op); + while let Some((op, mut path)) = ref_tracking.todo.pop() { ecx.validate_operand( op, &mut path, - &mut seen, - &mut todo, + Some(&mut ref_tracking), )?; } Ok(()) diff --git a/src/librustc_mir/interpret/mod.rs b/src/librustc_mir/interpret/mod.rs index b840af193b64a..9e0efaa9c78ef 100644 --- a/src/librustc_mir/interpret/mod.rs +++ b/src/librustc_mir/interpret/mod.rs @@ -35,3 +35,5 @@ pub use self::memory::{Memory, MemoryKind}; pub use self::machine::Machine; pub use self::operand::{ScalarMaybeUndef, Value, ValTy, Operand, OpTy}; + +pub use self::validity::RefTracking; diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index c5238d24cf7ed..fd952abf273ae 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -11,7 +11,7 @@ use std::fmt::Write; use syntax_pos::symbol::Symbol; -use rustc::ty::layout::{self, Size, Primitive}; +use rustc::ty::layout::{self, Size}; use rustc::ty::{self, Ty}; use rustc_data_structures::fx::FxHashSet; use rustc::mir::interpret::{ @@ -19,7 +19,7 @@ use rustc::mir::interpret::{ }; use super::{ - OpTy, Machine, EvalContext, ScalarMaybeUndef + OpTy, MPlaceTy, Machine, EvalContext, ScalarMaybeUndef }; macro_rules! validation_failure{ @@ -63,6 +63,23 @@ pub enum PathElem { Tag, } +/// State for tracking recursive validation of references +pub struct RefTracking<'tcx> { + pub seen: FxHashSet<(OpTy<'tcx>)>, + pub todo: Vec<(OpTy<'tcx>, Vec)>, +} + +impl<'tcx> RefTracking<'tcx> { + pub fn new(op: OpTy<'tcx>) -> Self { + let mut ref_tracking = RefTracking { + seen: FxHashSet(), + todo: vec![(op, Vec::new())], + }; + ref_tracking.seen.insert(op); + ref_tracking + } +} + // Adding a Deref and making a copy of the path to be put into the queue // always go together. This one does it with only new allocation. fn path_clone_and_deref(path: &Vec) -> Vec { @@ -205,6 +222,41 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> } } + /// Validate a reference, potentially recursively. `place` is assumed to already be + /// dereferenced, i.e. it describes the target. + fn validate_ref( + &self, + place: MPlaceTy<'tcx>, + path: &mut Vec, + ref_tracking: Option<&mut RefTracking<'tcx>>, + ) -> EvalResult<'tcx> { + // Skip recursion for some external statics + if let Scalar::Ptr(ptr) = place.ptr { + let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id); + if let Some(AllocType::Static(did)) = alloc_kind { + // statics from other crates are already checked. + // they might be checked at a different type, but for now we want + // to avoid recursing too deeply. + // extern statics cannot be validated as they have no body. + if !did.is_local() || self.tcx.is_foreign_item(did) { + return Ok(()); + } + } + } + // Check if we have encountered this pointer+layout combination + // before. Proceed recursively even for integer pointers, no + // reason to skip them! They are valid for some ZST, but not for others + // (e.g. `!` is a ZST). + let op = place.into(); + if let Some(ref_tracking) = ref_tracking { + if ref_tracking.seen.insert(op) { + trace!("Recursing below ptr {:#?}", *op); + ref_tracking.todo.push((op, path_clone_and_deref(path))); + } + } + Ok(()) + } + /// This function checks the data at `op`. /// It will error if the bits at the destination do not match the ones described by the layout. /// The `path` may be pushed to, but the part that is present when the function @@ -213,8 +265,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> &self, dest: OpTy<'tcx>, path: &mut Vec, - seen: &mut FxHashSet<(OpTy<'tcx>)>, - todo: &mut Vec<(OpTy<'tcx>, Vec)>, + mut ref_tracking: Option<&mut RefTracking<'tcx>>, ) -> EvalResult<'tcx> { trace!("validate_operand: {:?}, {:#?}", *dest, dest.layout); @@ -273,7 +324,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // Validate all fields match dest.layout.fields { - // primitives are unions with zero fields + // Primitives appear as Union with 0 fields -- except for fat pointers. // We still check `layout.fields`, not `layout.abi`, because `layout.abi` // is `Scalar` for newtypes around scalars, but we want to descend through the // fields to get a proper `path`. @@ -302,32 +353,61 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> }; let scalar = value.to_scalar_or_undef(); self.validate_scalar(scalar, size, scalar_layout, &path, dest.layout.ty)?; - if scalar_layout.value == Primitive::Pointer { - // ignore integer pointers, we can't reason about the final hardware - if let Scalar::Ptr(ptr) = scalar.not_undef()? { - let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id); - if let Some(AllocType::Static(did)) = alloc_kind { - // statics from other crates are already checked. - // extern statics cannot be validated as they have no body. - if !did.is_local() || self.tcx.is_foreign_item(did) { - return Ok(()); - } - } - if value.layout.ty.builtin_deref(false).is_some() { - let ptr_op = self.ref_to_mplace(value)?.into(); - // we have not encountered this pointer+layout combination - // before. - if seen.insert(ptr_op) { - trace!("Recursing below ptr {:#?}", *value); - todo.push((ptr_op, path_clone_and_deref(path))); - } - } - } + // Recursively check *safe* references + if dest.layout.ty.builtin_deref(true).is_some() && + !dest.layout.ty.is_unsafe_ptr() + { + self.validate_ref(self.ref_to_mplace(value)?, path, ref_tracking)?; } }, _ => bug!("bad abi for FieldPlacement::Union(0): {:#?}", dest.layout.abi), } } + layout::FieldPlacement::Arbitrary { .. } + if dest.layout.ty.builtin_deref(true).is_some() => + { + // This is a fat pointer. + let ptr = match self.read_value(dest.into()) + .and_then(|val| self.ref_to_mplace(val)) + { + Ok(ptr) => ptr, + Err(_) => + return validation_failure!( + "undefined location or metadata in fat pointer", path + ), + }; + // check metadata early, for better diagnostics + match self.tcx.struct_tail(ptr.layout.ty).sty { + ty::Dynamic(..) => { + match ptr.extra.unwrap().to_ptr() { + Ok(_) => {}, + Err(_) => + return validation_failure!( + "non-pointer vtable in fat pointer", path + ), + } + // FIXME: More checks for the vtable. + } + ty::Slice(..) | ty::Str => { + match ptr.extra.unwrap().to_usize(self) { + Ok(_) => {}, + Err(_) => + return validation_failure!( + "non-integer slice length in fat pointer", path + ), + } + } + _ => + bug!("Unexpected unsized type tail: {:?}", + self.tcx.struct_tail(ptr.layout.ty) + ), + } + // for safe ptrs, recursively check it + if !dest.layout.ty.is_unsafe_ptr() { + self.validate_ref(ptr, path, ref_tracking)?; + } + } + // Compound data structures layout::FieldPlacement::Union(_) => { // We can't check unions, their bits are allowed to be anything. // The fields don't need to correspond to any bit pattern of the union's fields. @@ -411,7 +491,11 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> for (i, field) in self.mplace_array_fields(dest)?.enumerate() { let field = field?; path.push(PathElem::ArrayElem(i)); - self.validate_operand(field.into(), path, seen, todo)?; + self.validate_operand( + field.into(), + path, + ref_tracking.as_mut().map(|r| &mut **r) + )?; path.truncate(path_len); } } @@ -421,60 +505,11 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // An empty array. Nothing to do. } layout::FieldPlacement::Arbitrary { ref offsets, .. } => { - // Fat pointers are treated like pointers, not aggregates. - if dest.layout.ty.builtin_deref(true).is_some() { - // This is a fat pointer. - let ptr = match self.read_value(dest.into()) - .and_then(|val| self.ref_to_mplace(val)) - { - Ok(ptr) => ptr, - Err(_) => - return validation_failure!( - "undefined location or metadata in fat pointer", path - ), - }; - // check metadata early, for better diagnostics - match self.tcx.struct_tail(ptr.layout.ty).sty { - ty::Dynamic(..) => { - match ptr.extra.unwrap().to_ptr() { - Ok(_) => {}, - Err(_) => - return validation_failure!( - "non-pointer vtable in fat pointer", path - ), - } - // FIXME: More checks for the vtable. - } - ty::Slice(..) | ty::Str => { - match ptr.extra.unwrap().to_usize(self) { - Ok(_) => {}, - Err(_) => - return validation_failure!( - "non-integer slice length in fat pointer", path - ), - } - } - _ => - bug!("Unexpected unsized type tail: {:?}", - self.tcx.struct_tail(ptr.layout.ty) - ), - } - // for safe ptrs, recursively check it - if !dest.layout.ty.is_unsafe_ptr() { - let ptr = ptr.into(); - if seen.insert(ptr) { - trace!("Recursing below fat ptr {:?}", ptr); - todo.push((ptr, path_clone_and_deref(path))); - } - } - } else { - // Not a pointer, perform regular aggregate handling below - for i in 0..offsets.len() { - let field = self.operand_field(dest, i as u64)?; - path.push(self.aggregate_field_path_elem(dest.layout.ty, variant, i)); - self.validate_operand(field, path, seen, todo)?; - path.truncate(path_len); - } + for i in 0..offsets.len() { + let field = self.operand_field(dest, i as u64)?; + path.push(self.aggregate_field_path_elem(dest.layout.ty, variant, i)); + self.validate_operand(field, path, ref_tracking.as_mut().map(|r| &mut **r))?; + path.truncate(path_len); } } } From ff5a245f52813d4d876f283b0dffa3674e9210b1 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Oct 2018 17:02:58 +0200 Subject: [PATCH 02/19] check that entire ref is in-bounds before recursing; add macro for validation msgs on error --- src/librustc_mir/interpret/memory.rs | 15 ++- src/librustc_mir/interpret/validity.rs | 116 ++++++------------ src/test/ui/consts/const-eval/ub-uninhabit.rs | 8 +- .../ui/consts/const-eval/ub-uninhabit.stderr | 16 ++- .../ui/consts/const-eval/ub-usize-in-ref.rs | 4 +- .../consts/const-eval/ub-usize-in-ref.stderr | 11 ++ src/test/ui/union-ub-fat-ptr.stderr | 12 +- 7 files changed, 86 insertions(+), 96 deletions(-) create mode 100644 src/test/ui/consts/const-eval/ub-usize-in-ref.stderr diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 222d1164667d3..1b75982a83a57 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -284,7 +284,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { /// If you want to check bounds before doing a memory access, be sure to /// check the pointer one past the end of your access, then everything will /// work out exactly. - pub fn check_bounds(&self, ptr: Pointer, access: bool) -> EvalResult<'tcx> { + pub fn check_bounds_ptr(&self, ptr: Pointer, access: bool) -> EvalResult<'tcx> { let alloc = self.get(ptr.alloc_id)?; let allocation_size = alloc.bytes.len() as u64; if ptr.offset.bytes() > allocation_size { @@ -296,6 +296,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { } Ok(()) } + + /// Check if the memory range beginning at `ptr` and of size `Size` is "in-bounds". + #[inline(always)] + pub fn check_bounds(&self, ptr: Pointer, size: Size, access: bool) -> EvalResult<'tcx> { + // if ptr.offset is in bounds, then so is ptr (because offset checks for overflow) + self.check_bounds_ptr(ptr.offset(size, &*self)?, access) + } } /// Allocation accessors @@ -524,8 +531,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { ) -> EvalResult<'tcx, &[u8]> { assert_ne!(size.bytes(), 0, "0-sized accesses should never even get a `Pointer`"); self.check_align(ptr.into(), align)?; - // if ptr.offset is in bounds, then so is ptr (because offset checks for overflow) - self.check_bounds(ptr.offset(size, &*self)?, true)?; + self.check_bounds(ptr, size, true)?; if check_defined_and_ptr { self.check_defined(ptr, size)?; @@ -569,8 +575,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { ) -> EvalResult<'tcx, &mut [u8]> { assert_ne!(size.bytes(), 0, "0-sized accesses should never even get a `Pointer`"); self.check_align(ptr.into(), align)?; - // if ptr.offset is in bounds, then so is ptr (because offset checks for overflow) - self.check_bounds(ptr.offset(size, &self)?, true)?; + self.check_bounds(ptr, size, true)?; self.mark_definedness(ptr, size, true)?; self.clear_relocations(ptr, size)?; diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index fd952abf273ae..44faa45d19113 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -22,7 +22,7 @@ use super::{ OpTy, MPlaceTy, Machine, EvalContext, ScalarMaybeUndef }; -macro_rules! validation_failure{ +macro_rules! validation_failure { ($what:expr, $where:expr, $details:expr) => {{ let where_ = path_format($where); let where_ = if where_.is_empty() { @@ -49,6 +49,15 @@ macro_rules! validation_failure{ }}; } +macro_rules! try_validation { + ($e:expr, $what:expr, $where:expr) => {{ + match $e { + Ok(x) => x, + Err(_) => return validation_failure!($what, $where), + } + }} +} + /// We want to show a nice path to the invalid field for diagnotsics, /// but avoid string operations in the happy case where no error happens. /// So we track a `Vec` where `PathElem` contains all the data we @@ -230,8 +239,14 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> path: &mut Vec, ref_tracking: Option<&mut RefTracking<'tcx>>, ) -> EvalResult<'tcx> { - // Skip recursion for some external statics - if let Scalar::Ptr(ptr) = place.ptr { + // Before we do anything else, make sure this is entirely in-bounds. + if !place.layout.is_zst() { + let ptr = try_validation!(place.ptr.to_ptr(), + "integer pointer in non-ZST reference", path); + let size = self.size_and_align_of(place.extra, place.layout)?.0; + try_validation!(self.memory.check_bounds(ptr, size, false), + "dangling reference (not entirely in bounds)", path); + // Skip recursion for some external statics let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id); if let Some(AllocType::Static(did)) = alloc_kind { // statics from other crates are already checked. @@ -257,7 +272,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> Ok(()) } - /// This function checks the data at `op`. + /// This function checks the data at `op`. `op` is assumed to cover valid memory if it + /// is an indirect operand. /// It will error if the bits at the destination do not match the ones described by the layout. /// The `path` may be pushed to, but the part that is present when the function /// starts must not be changed! @@ -305,13 +321,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> let dest = match dest.layout.ty.sty { ty::Dynamic(..) => { let dest = dest.to_mem_place(); // immediate trait objects are not a thing - match self.unpack_dyn_trait(dest) { - Ok(res) => res.1.into(), - Err(_) => - return validation_failure!( - "invalid vtable in fat pointer", path - ), - } + try_validation!(self.unpack_dyn_trait(dest), + "invalid vtable in fat pointer", path).1.into() } _ => dest }; @@ -337,20 +348,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // expectation. layout::Abi::Scalar(ref scalar_layout) => { let size = scalar_layout.value.size(self); - let value = match self.read_value(dest) { - Ok(val) => val, - Err(err) => match err.kind { - EvalErrorKind::PointerOutOfBounds { .. } | - EvalErrorKind::ReadUndefBytes(_) => - return validation_failure!( - "uninitialized or out-of-bounds memory", path - ), - _ => - return validation_failure!( - "unrepresentable data", path - ), - } - }; + let value = try_validation!(self.read_value(dest), + "uninitialized or unrepresentable data", path); let scalar = value.to_scalar_or_undef(); self.validate_scalar(scalar, size, scalar_layout, &path, dest.layout.ty)?; // Recursively check *safe* references @@ -367,35 +366,24 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> if dest.layout.ty.builtin_deref(true).is_some() => { // This is a fat pointer. - let ptr = match self.read_value(dest.into()) - .and_then(|val| self.ref_to_mplace(val)) - { - Ok(ptr) => ptr, - Err(_) => - return validation_failure!( - "undefined location or metadata in fat pointer", path - ), - }; + let ptr = try_validation!(self.read_value(dest.into()), + "undefined location in fat pointer", path); + let ptr = try_validation!(self.ref_to_mplace(ptr), + "undefined metadata in fat pointer", path); // check metadata early, for better diagnostics match self.tcx.struct_tail(ptr.layout.ty).sty { ty::Dynamic(..) => { - match ptr.extra.unwrap().to_ptr() { - Ok(_) => {}, - Err(_) => - return validation_failure!( - "non-pointer vtable in fat pointer", path - ), - } + let vtable = try_validation!(ptr.extra.unwrap().to_ptr(), + "non-pointer vtable in fat pointer", path); + try_validation!(self.read_drop_type_from_vtable(vtable), + "invalid drop fn in vtable", path); + try_validation!(self.read_size_and_align_from_vtable(vtable), + "invalid size or align in vtable", path); // FIXME: More checks for the vtable. } ty::Slice(..) | ty::Str => { - match ptr.extra.unwrap().to_usize(self) { - Ok(_) => {}, - Err(_) => - return validation_failure!( - "non-integer slice length in fat pointer", path - ), - } + try_validation!(ptr.extra.unwrap().to_usize(self), + "non-integer slice length in fat pointer", path); } _ => bug!("Unexpected unsized type tail: {:?}", @@ -418,23 +406,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> match dest.layout.ty.sty { // Special handling for strings to verify UTF-8 ty::Str => { - match self.read_str(dest) { - Ok(_) => {}, - Err(err) => match err.kind { - EvalErrorKind::PointerOutOfBounds { .. } | - EvalErrorKind::ReadUndefBytes(_) => - // The error here looks slightly different than it does - // for slices, because we do not report the index into the - // str at which we are OOB. - return validation_failure!( - "uninitialized or out-of-bounds memory", path - ), - _ => - return validation_failure!( - "non-UTF-8 data in str", path - ), - } - } + try_validation!(self.read_str(dest), + "uninitialized or non-UTF-8 data in str", path); } // Special handling for arrays/slices of builtin integer types ty::Array(tys, ..) | ty::Slice(tys) if { @@ -470,18 +443,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> "undefined bytes", path ) }, - EvalErrorKind::PointerOutOfBounds { allocation_size, .. } => { - // If the array access is out-of-bounds, the first - // undefined access is the after the end of the array. - let i = (allocation_size.bytes() * ty_size) as usize; - path.push(PathElem::ArrayElem(i)); - }, - _ => (), + // Other errors shouldn't be possible + _ => return Err(err), } - - return validation_failure!( - "uninitialized or out-of-bounds memory", path - ) } } }, diff --git a/src/test/ui/consts/const-eval/ub-uninhabit.rs b/src/test/ui/consts/const-eval/ub-uninhabit.rs index a5e341524bc73..0f58c84c10b5e 100644 --- a/src/test/ui/consts/const-eval/ub-uninhabit.rs +++ b/src/test/ui/consts/const-eval/ub-uninhabit.rs @@ -9,14 +9,18 @@ // except according to those terms. union Foo { - a: u8, + a: usize, b: Bar, + c: &'static Bar, } #[derive(Copy, Clone)] enum Bar {} -const BAD_BAD_BAD: Bar = unsafe { Foo { a: 1 }.b}; +const BAD_BAD_BAD: Bar = unsafe { Foo { a: 1 }.b }; +//~^ ERROR this constant likely exhibits undefined behavior + +const BAD_BAD_REF: &Bar = unsafe { Foo { a: 1 }.c }; //~^ ERROR this constant likely exhibits undefined behavior fn main() { diff --git a/src/test/ui/consts/const-eval/ub-uninhabit.stderr b/src/test/ui/consts/const-eval/ub-uninhabit.stderr index 623b98dc4531b..95cf8865c8de2 100644 --- a/src/test/ui/consts/const-eval/ub-uninhabit.stderr +++ b/src/test/ui/consts/const-eval/ub-uninhabit.stderr @@ -1,11 +1,19 @@ error[E0080]: this constant likely exhibits undefined behavior - --> $DIR/ub-uninhabit.rs:19:1 + --> $DIR/ub-uninhabit.rs:20:1 | -LL | const BAD_BAD_BAD: Bar = unsafe { Foo { a: 1 }.b}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of an uninhabited type +LL | const BAD_BAD_BAD: Bar = unsafe { Foo { a: 1 }.b }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of an uninhabited type | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior -error: aborting due to previous error +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-uninhabit.rs:23:1 + | +LL | const BAD_BAD_REF: &Bar = unsafe { Foo { a: 1 }.c }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of an uninhabited type at . + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior + +error: aborting due to 2 previous errors For more information about this error, try `rustc --explain E0080`. diff --git a/src/test/ui/consts/const-eval/ub-usize-in-ref.rs b/src/test/ui/consts/const-eval/ub-usize-in-ref.rs index aaff2f233815b..6ea758ea9fe66 100644 --- a/src/test/ui/consts/const-eval/ub-usize-in-ref.rs +++ b/src/test/ui/consts/const-eval/ub-usize-in-ref.rs @@ -8,15 +8,13 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -// compile-pass - union Foo { a: &'static u8, b: usize, } -// This might point to an invalid address, but that's the user's problem const USIZE_AS_STATIC_REF: &'static u8 = unsafe { Foo { b: 1337 }.a}; +//~^ ERROR this constant likely exhibits undefined behavior fn main() { } diff --git a/src/test/ui/consts/const-eval/ub-usize-in-ref.stderr b/src/test/ui/consts/const-eval/ub-usize-in-ref.stderr new file mode 100644 index 0000000000000..55bc1e50aac00 --- /dev/null +++ b/src/test/ui/consts/const-eval/ub-usize-in-ref.stderr @@ -0,0 +1,11 @@ +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-usize-in-ref.rs:16:1 + | +LL | const USIZE_AS_STATIC_REF: &'static u8 = unsafe { Foo { b: 1337 }.a}; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered integer pointer in non-ZST reference + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0080`. diff --git a/src/test/ui/union-ub-fat-ptr.stderr b/src/test/ui/union-ub-fat-ptr.stderr index 5d817dce205b1..08aaa40e2f3c9 100644 --- a/src/test/ui/union-ub-fat-ptr.stderr +++ b/src/test/ui/union-ub-fat-ptr.stderr @@ -2,7 +2,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:87:1 | LL | const B: &str = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.str}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized or out-of-bounds memory at . + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling reference (not entirely in bounds) | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -26,7 +26,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:99:1 | LL | const B2: &[u8] = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.slice}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized or out-of-bounds memory at .[1] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling reference (not entirely in bounds) | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -42,7 +42,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:106:1 | LL | const D: &Trait = unsafe { DynTransmute { repr: DynRepr { ptr: &92, vtable: &3 } }.rust}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid vtable in fat pointer at . + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop fn in vtable | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -50,7 +50,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:109:1 | LL | const E: &Trait = unsafe { DynTransmute { repr2: DynRepr2 { ptr: &92, vtable: &3 } }.rust}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid vtable in fat pointer at . + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered invalid drop fn in vtable | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -98,7 +98,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:132:1 | LL | const J1: &str = unsafe { SliceTransmute { slice: &[0xFF] }.str }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered non-UTF-8 data in str at . + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized or non-UTF-8 data in str at . | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -106,7 +106,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:135:1 | LL | const J2: &MyStr = unsafe { SliceTransmute { slice: &[0xFF] }.my_str }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered non-UTF-8 data in str at ..0 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized or non-UTF-8 data in str at ..0 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior From f65d3b5491a8f675c3c5bb892c3e97ce4d4f9798 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Oct 2018 18:07:40 +0200 Subject: [PATCH 03/19] switch validation of scalars to be type-driven This does not actually regress anything. It would regress NonNull, but we didn't handle that correctly previously either. --- src/librustc_mir/interpret/validity.rs | 162 +++++++----------- ...nst-pointer-values-in-various-types.stderr | 14 +- .../ui/consts/const-eval/transmute-const.rs | 1 - .../consts/const-eval/transmute-const.stderr | 2 +- src/test/ui/consts/const-eval/ub-enum.stderr | 2 +- .../{ub-usize-in-ref.rs => ub-ref.rs} | 18 +- src/test/ui/consts/const-eval/ub-ref.stderr | 27 +++ .../consts/const-eval/ub-usize-in-ref.stderr | 11 -- .../ui/consts/const-eval/union-ice.stderr | 2 +- src/test/ui/consts/const-eval/union-ub.rs | 3 +- src/test/ui/consts/const-eval/union-ub.stderr | 2 +- src/test/ui/union-ub-fat-ptr.stderr | 8 +- 12 files changed, 117 insertions(+), 135 deletions(-) rename src/test/ui/consts/const-eval/{ub-usize-in-ref.rs => ub-ref.rs} (58%) create mode 100644 src/test/ui/consts/const-eval/ub-ref.stderr delete mode 100644 src/test/ui/consts/const-eval/ub-usize-in-ref.stderr diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 44faa45d19113..a5e4231e7ab4e 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -15,7 +15,7 @@ use rustc::ty::layout::{self, Size}; use rustc::ty::{self, Ty}; use rustc_data_structures::fx::FxHashSet; use rustc::mir::interpret::{ - Scalar, AllocType, EvalResult, EvalErrorKind, PointerArithmetic + Scalar, AllocType, EvalResult, EvalErrorKind }; use super::{ @@ -50,6 +50,13 @@ macro_rules! validation_failure { } macro_rules! try_validation { + ($e:expr, $what:expr, $where:expr, $details:expr) => {{ + match $e { + Ok(x) => x, + Err(_) => return validation_failure!($what, $where, $details), + } + }}; + ($e:expr, $what:expr, $where:expr) => {{ match $e { Ok(x) => x, @@ -121,114 +128,65 @@ fn path_format(path: &Vec) -> String { out } +fn scalar_format(value: ScalarMaybeUndef) -> String { + match value { + ScalarMaybeUndef::Undef => + "uninitialized bytes".to_owned(), + ScalarMaybeUndef::Scalar(Scalar::Ptr(_)) => + "a pointer".to_owned(), + ScalarMaybeUndef::Scalar(Scalar::Bits { bits, .. }) => + bits.to_string(), + } +} + impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { fn validate_scalar( &self, value: ScalarMaybeUndef, size: Size, - scalar: &layout::Scalar, path: &Vec, ty: Ty, ) -> EvalResult<'tcx> { - trace!("validate scalar: {:#?}, {:#?}, {:#?}, {}", value, size, scalar, ty); - let (lo, hi) = scalar.valid_range.clone().into_inner(); - - let value = match value { - ScalarMaybeUndef::Scalar(scalar) => scalar, - ScalarMaybeUndef::Undef => return validation_failure!("undefined bytes", path), - }; - - let bits = match value { - Scalar::Bits { bits, size: value_size } => { - assert_eq!(value_size as u64, size.bytes()); - bits - }, - Scalar::Ptr(_) => { - match ty.sty { - ty::Bool | - ty::Char | - ty::Float(_) | - ty::Int(_) | - ty::Uint(_) => { - return validation_failure!( - "a pointer", - path, - format!("the type {}", ty.sty) - ); - } - ty::RawPtr(_) | - ty::Ref(_, _, _) | - ty::FnPtr(_) => {} - _ => { unreachable!(); } - } + trace!("validate scalar: {:#?}, {:#?}, {}", value, size, ty); - let ptr_size = self.pointer_size(); - let ptr_max = u128::max_value() >> (128 - ptr_size.bits()); - return if lo > hi { - if lo - hi == 1 { - // no gap, all values are ok - Ok(()) - } else if hi < ptr_max || lo > 1 { - let max = u128::max_value() >> (128 - size.bits()); - validation_failure!( - "pointer", - path, - format!("something in the range {:?} or {:?}", 0..=lo, hi..=max) - ) - } else { - Ok(()) - } - } else if hi < ptr_max || lo > 1 { - validation_failure!( - "pointer", - path, - format!("something in the range {:?}", scalar.valid_range) - ) - } else { - Ok(()) - }; - }, - }; - - // char gets a special treatment, because its number space is not contiguous so `TyLayout` - // has no special checks for chars + // Go over all the primitive types match ty.sty { + ty::Bool => { + try_validation!(value.to_bool(), + scalar_format(value), path, "a boolean"); + }, ty::Char => { - debug_assert_eq!(size.bytes(), 4); - if ::std::char::from_u32(bits as u32).is_none() { - return validation_failure!( - "character", - path, - "a valid unicode codepoint" - ); - } + try_validation!(value.to_char(), + scalar_format(value), path, "a valid unicode codepoint"); + }, + ty::Float(_) | ty::Int(_) | ty::Uint(_) => { + // Must be scalar bits + try_validation!(value.to_bits(size), + scalar_format(value), path, "initialized plain bits"); } - _ => {}, - } - - use std::ops::RangeInclusive; - let in_range = |bound: RangeInclusive| bound.contains(&bits); - if lo > hi { - if in_range(0..=hi) || in_range(lo..=u128::max_value()) { - Ok(()) - } else { - validation_failure!( - bits, - path, - format!("something in the range {:?} or {:?}", ..=hi, lo..) - ) + ty::RawPtr(_) => { + // Anything but undef goes + try_validation!(value.not_undef(), + scalar_format(value), path, "a raw pointer"); + }, + ty::Ref(..) => { + // This is checked by the recursive reference handling, nothing to do here. + debug_assert!(ty.builtin_deref(true).is_some() && !ty.is_unsafe_ptr()); } - } else { - if in_range(scalar.valid_range.clone()) { - Ok(()) - } else { - validation_failure!( - bits, - path, - format!("something in the range {:?}", scalar.valid_range) - ) + ty::FnPtr(_sig) => { + let ptr = try_validation!(value.to_ptr(), + scalar_format(value), path, "a pointer"); + let _fn = try_validation!(self.memory.get_fn(ptr), + scalar_format(value), path, "a function pointer"); + // TODO: Check if the signature matches + } + ty::FnDef(..) => { + // This is a zero-sized type with all relevant data sitting in the type. + // There is nothing to validate. } + _ => bug!("Unexpected primitive type {}", ty) } + Ok(()) } /// Validate a reference, potentially recursively. `place` is assumed to already be @@ -240,10 +198,12 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> ref_tracking: Option<&mut RefTracking<'tcx>>, ) -> EvalResult<'tcx> { // Before we do anything else, make sure this is entirely in-bounds. + let (size, align) = self.size_and_align_of(place.extra, place.layout)?; + try_validation!(self.memory.check_align(place.ptr, align), + "unaligned reference", path); if !place.layout.is_zst() { let ptr = try_validation!(place.ptr.to_ptr(), "integer pointer in non-ZST reference", path); - let size = self.size_and_align_of(place.extra, place.layout)?.0; try_validation!(self.memory.check_bounds(ptr, size, false), "dangling reference (not entirely in bounds)", path); // Skip recursion for some external statics @@ -277,6 +237,10 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> /// It will error if the bits at the destination do not match the ones described by the layout. /// The `path` may be pushed to, but the part that is present when the function /// starts must not be changed! + /// + /// `ref_tracking` can be None to avoid recursive checking below references. + /// This also toggles between "run-time" (no recursion) and "compile-time" (with recursion) + /// validation (e.g., pointer values are fine in integers at runtime). pub fn validate_operand( &self, dest: OpTy<'tcx>, @@ -346,12 +310,11 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> return validation_failure!("a value of an uninhabited type", path), // check that the scalar is a valid pointer or that its bit range matches the // expectation. - layout::Abi::Scalar(ref scalar_layout) => { - let size = scalar_layout.value.size(self); + layout::Abi::Scalar(_) => { let value = try_validation!(self.read_value(dest), "uninitialized or unrepresentable data", path); let scalar = value.to_scalar_or_undef(); - self.validate_scalar(scalar, size, scalar_layout, &path, dest.layout.ty)?; + self.validate_scalar(scalar, dest.layout.size, &path, dest.layout.ty)?; // Recursively check *safe* references if dest.layout.ty.builtin_deref(true).is_some() && !dest.layout.ty.is_unsafe_ptr() @@ -365,7 +328,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> layout::FieldPlacement::Arbitrary { .. } if dest.layout.ty.builtin_deref(true).is_some() => { - // This is a fat pointer. + // This is a fat pointer. We also check fat raw pointers, their metadata must + // be valid! let ptr = try_validation!(self.read_value(dest.into()), "undefined location in fat pointer", path); let ptr = try_validation!(self.ref_to_mplace(ptr), diff --git a/src/test/ui/consts/const-eval/const-pointer-values-in-various-types.stderr b/src/test/ui/consts/const-eval/const-pointer-values-in-various-types.stderr index 751f4113fbf60..7be9345b6b423 100644 --- a/src/test/ui/consts/const-eval/const-pointer-values-in-various-types.stderr +++ b/src/test/ui/consts/const-eval/const-pointer-values-in-various-types.stderr @@ -2,7 +2,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/const-pointer-values-in-various-types.rs:24:5 | LL | const I32_REF_USIZE_UNION: usize = unsafe { Nonsense { int_32_ref: &3 }.u }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected the type usize + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain bits | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -36,7 +36,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/const-pointer-values-in-various-types.rs:36:5 | LL | const I32_REF_U64_UNION: u64 = unsafe { Nonsense { int_32_ref: &3 }.uint_64 }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected the type u64 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain bits | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -74,7 +74,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/const-pointer-values-in-various-types.rs:51:5 | LL | const I32_REF_I64_UNION: i64 = unsafe { Nonsense { int_32_ref: &3 }.int_64 }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected the type i64 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain bits | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -96,7 +96,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/const-pointer-values-in-various-types.rs:60:5 | LL | const I32_REF_F64_UNION: f64 = unsafe { Nonsense { int_32_ref: &3 }.float_64 }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected the type f64 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain bits | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -144,7 +144,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/const-pointer-values-in-various-types.rs:78:5 | LL | const STR_U64_UNION: u64 = unsafe { Nonsense { stringy: "3" }.uint_64 }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected the type u64 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain bits | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -184,7 +184,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/const-pointer-values-in-various-types.rs:93:5 | LL | const STR_I64_UNION: i64 = unsafe { Nonsense { stringy: "3" }.int_64 }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected the type i64 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain bits | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -208,7 +208,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/const-pointer-values-in-various-types.rs:102:5 | LL | const STR_F64_UNION: f64 = unsafe { Nonsense { stringy: "3" }.float_64 }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected the type f64 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain bits | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior diff --git a/src/test/ui/consts/const-eval/transmute-const.rs b/src/test/ui/consts/const-eval/transmute-const.rs index a585a4404adf6..477e7119ba937 100644 --- a/src/test/ui/consts/const-eval/transmute-const.rs +++ b/src/test/ui/consts/const-eval/transmute-const.rs @@ -14,6 +14,5 @@ use std::mem; static FOO: bool = unsafe { mem::transmute(3u8) }; //~^ ERROR this static likely exhibits undefined behavior -//~^^ type validation failed: encountered 3, but expected something in the range 0..=1 fn main() {} diff --git a/src/test/ui/consts/const-eval/transmute-const.stderr b/src/test/ui/consts/const-eval/transmute-const.stderr index c9beca7aa30ac..2e82c0963c377 100644 --- a/src/test/ui/consts/const-eval/transmute-const.stderr +++ b/src/test/ui/consts/const-eval/transmute-const.stderr @@ -2,7 +2,7 @@ error[E0080]: this static likely exhibits undefined behavior --> $DIR/transmute-const.rs:15:1 | LL | static FOO: bool = unsafe { mem::transmute(3u8) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3, but expected something in the range 0..=1 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3, but expected a boolean | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior diff --git a/src/test/ui/consts/const-eval/ub-enum.stderr b/src/test/ui/consts/const-eval/ub-enum.stderr index 572d08ddfeebd..20105a46c1db0 100644 --- a/src/test/ui/consts/const-eval/ub-enum.stderr +++ b/src/test/ui/consts/const-eval/ub-enum.stderr @@ -18,7 +18,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/ub-enum.rs:45:1 | LL | const BAD_ENUM_CHAR : Option<(char, char)> = Some(('x', unsafe { TransmuteChar { a: !0 }.b })); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered character at .Some.0.1, but expected a valid unicode codepoint + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 4294967295 at .Some.0.1, but expected a valid unicode codepoint | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior diff --git a/src/test/ui/consts/const-eval/ub-usize-in-ref.rs b/src/test/ui/consts/const-eval/ub-ref.rs similarity index 58% rename from src/test/ui/consts/const-eval/ub-usize-in-ref.rs rename to src/test/ui/consts/const-eval/ub-ref.rs index 6ea758ea9fe66..e010152513c15 100644 --- a/src/test/ui/consts/const-eval/ub-usize-in-ref.rs +++ b/src/test/ui/consts/const-eval/ub-ref.rs @@ -8,13 +8,17 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -union Foo { - a: &'static u8, - b: usize, -} +#![feature(const_transmute)] -const USIZE_AS_STATIC_REF: &'static u8 = unsafe { Foo { b: 1337 }.a}; +use std::mem; + +const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) }; +//~^ ERROR this constant likely exhibits undefined behavior + +const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; +//~^ ERROR this constant likely exhibits undefined behavior + +const USIZE_AS_REF: &'static u8 = unsafe { mem::transmute(1337usize) }; //~^ ERROR this constant likely exhibits undefined behavior -fn main() { -} +fn main() {} diff --git a/src/test/ui/consts/const-eval/ub-ref.stderr b/src/test/ui/consts/const-eval/ub-ref.stderr new file mode 100644 index 0000000000000..b7989e22afabe --- /dev/null +++ b/src/test/ui/consts/const-eval/ub-ref.stderr @@ -0,0 +1,27 @@ +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-ref.rs:15:1 + | +LL | const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered unaligned reference + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior + +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-ref.rs:18:1 + | +LL | const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain bits + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior + +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-ref.rs:21:1 + | +LL | const USIZE_AS_REF: &'static u8 = unsafe { mem::transmute(1337usize) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered integer pointer in non-ZST reference + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0080`. diff --git a/src/test/ui/consts/const-eval/ub-usize-in-ref.stderr b/src/test/ui/consts/const-eval/ub-usize-in-ref.stderr deleted file mode 100644 index 55bc1e50aac00..0000000000000 --- a/src/test/ui/consts/const-eval/ub-usize-in-ref.stderr +++ /dev/null @@ -1,11 +0,0 @@ -error[E0080]: this constant likely exhibits undefined behavior - --> $DIR/ub-usize-in-ref.rs:16:1 - | -LL | const USIZE_AS_STATIC_REF: &'static u8 = unsafe { Foo { b: 1337 }.a}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered integer pointer in non-ZST reference - | - = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior - -error: aborting due to previous error - -For more information about this error, try `rustc --explain E0080`. diff --git a/src/test/ui/consts/const-eval/union-ice.stderr b/src/test/ui/consts/const-eval/union-ice.stderr index ec51802681e0d..4484dd6a14740 100644 --- a/src/test/ui/consts/const-eval/union-ice.stderr +++ b/src/test/ui/consts/const-eval/union-ice.stderr @@ -13,7 +13,7 @@ LL | / const FIELD_PATH: Struct = Struct { //~ ERROR this constant likely exhibi LL | | a: 42, LL | | b: unsafe { UNION.field3 }, LL | | }; - | |__^ type validation failed: encountered undefined bytes at .b + | |__^ type validation failed: encountered uninitialized bytes at .b, but expected initialized plain bits | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior diff --git a/src/test/ui/consts/const-eval/union-ub.rs b/src/test/ui/consts/const-eval/union-ub.rs index db36764c4a306..86b3bdaa6b79e 100644 --- a/src/test/ui/consts/const-eval/union-ub.rs +++ b/src/test/ui/consts/const-eval/union-ub.rs @@ -41,5 +41,4 @@ const BAD_BOOL: bool = unsafe { DummyUnion { u8: 42 }.bool}; const BAD_UNION: Foo = unsafe { Bar { u8: 42 }.foo }; -fn main() { -} +fn main() {} diff --git a/src/test/ui/consts/const-eval/union-ub.stderr b/src/test/ui/consts/const-eval/union-ub.stderr index 2a04dae337b27..5da7e1a4dbf60 100644 --- a/src/test/ui/consts/const-eval/union-ub.stderr +++ b/src/test/ui/consts/const-eval/union-ub.stderr @@ -2,7 +2,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub.rs:36:1 | LL | const BAD_BOOL: bool = unsafe { DummyUnion { u8: 42 }.bool}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 42, but expected something in the range 0..=1 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 42, but expected a boolean | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior diff --git a/src/test/ui/union-ub-fat-ptr.stderr b/src/test/ui/union-ub-fat-ptr.stderr index 08aaa40e2f3c9..0471ef075f809 100644 --- a/src/test/ui/union-ub-fat-ptr.stderr +++ b/src/test/ui/union-ub-fat-ptr.stderr @@ -66,7 +66,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:116:1 | LL | const G: &Trait = &unsafe { BoolTransmute { val: 3 }.bl }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ., but expected something in the range 0..=1 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ., but expected a boolean | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -74,7 +74,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:119:1 | LL | const H: &[bool] = &[unsafe { BoolTransmute { val: 3 }.bl }]; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at .[0], but expected something in the range 0..=1 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at .[0], but expected a boolean | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -82,7 +82,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:125:1 | LL | const I2: &MySliceBool = &MySlice(unsafe { BoolTransmute { val: 3 }.bl }, [false]); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ..0, but expected something in the range 0..=1 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ..0, but expected a boolean | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -90,7 +90,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:128:1 | LL | const I3: &MySliceBool = &MySlice(true, [unsafe { BoolTransmute { val: 3 }.bl }]); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ..1[0], but expected something in the range 0..=1 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ..1[0], but expected a boolean | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior From 0a2fae6cb72a19e690b74bd57a67b9b36deef81c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Oct 2018 19:17:30 +0200 Subject: [PATCH 04/19] fix validating arrays of ZSTs Fixes #54751 --- src/librustc_mir/interpret/place.rs | 12 ++++++++++ src/librustc_mir/interpret/validity.rs | 13 ++++++----- src/test/ui/consts/const-eval/ub-uninhabit.rs | 15 +++++++------ .../ui/consts/const-eval/ub-uninhabit.stderr | 22 +++++++++++++------ 4 files changed, 43 insertions(+), 19 deletions(-) diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index 8878e5ca83f41..f8e1e9e55d530 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -131,6 +131,18 @@ impl MemPlace { } impl<'tcx> MPlaceTy<'tcx> { + /// Produces a MemPlace that works for ZST but nothing else + #[inline] + pub fn dangling(layout: TyLayout<'tcx>, cx: impl HasDataLayout) -> Self { + MPlaceTy { + mplace: MemPlace::from_scalar_ptr( + Scalar::from_uint(layout.align.abi(), cx.pointer_size()), + layout.align + ), + layout + } + } + #[inline] fn from_aligned_ptr(ptr: Pointer, layout: TyLayout<'tcx>) -> Self { MPlaceTy { mplace: MemPlace::from_ptr(ptr, layout.align), layout } diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index a5e4231e7ab4e..23f09cf108e5a 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -365,8 +365,14 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // The fields don't need to correspond to any bit pattern of the union's fields. // See https://github.com/rust-lang/rust/issues/32836#issuecomment-406875389 }, - layout::FieldPlacement::Array { stride, .. } if !dest.layout.is_zst() => { - let dest = dest.to_mem_place(); // non-ZST array/slice/str cannot be immediate + layout::FieldPlacement::Array { stride, .. } => { + let dest = if dest.layout.is_zst() { + // it's a ZST, the memory content cannot matter + MPlaceTy::dangling(dest.layout, self) + } else { + // non-ZST array/slice/str cannot be immediate + dest.to_mem_place() + }; match dest.layout.ty.sty { // Special handling for strings to verify UTF-8 ty::Str => { @@ -429,9 +435,6 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> } } }, - layout::FieldPlacement::Array { .. } => { - // An empty array. Nothing to do. - } layout::FieldPlacement::Arbitrary { ref offsets, .. } => { for i in 0..offsets.len() { let field = self.operand_field(dest, i as u64)?; diff --git a/src/test/ui/consts/const-eval/ub-uninhabit.rs b/src/test/ui/consts/const-eval/ub-uninhabit.rs index 0f58c84c10b5e..99305beee5281 100644 --- a/src/test/ui/consts/const-eval/ub-uninhabit.rs +++ b/src/test/ui/consts/const-eval/ub-uninhabit.rs @@ -8,19 +8,20 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -union Foo { - a: usize, - b: Bar, - c: &'static Bar, -} +#![feature(const_transmute)] + +use std::mem; #[derive(Copy, Clone)] enum Bar {} -const BAD_BAD_BAD: Bar = unsafe { Foo { a: 1 }.b }; +const BAD_BAD_BAD: Bar = unsafe { mem::transmute(()) }; +//~^ ERROR this constant likely exhibits undefined behavior + +const BAD_BAD_REF: &Bar = unsafe { mem::transmute(1usize) }; //~^ ERROR this constant likely exhibits undefined behavior -const BAD_BAD_REF: &Bar = unsafe { Foo { a: 1 }.c }; +const BAD_BAD_ARRAY: [Bar; 1] = unsafe { mem::transmute(()) }; //~^ ERROR this constant likely exhibits undefined behavior fn main() { diff --git a/src/test/ui/consts/const-eval/ub-uninhabit.stderr b/src/test/ui/consts/const-eval/ub-uninhabit.stderr index 95cf8865c8de2..136d5f2919946 100644 --- a/src/test/ui/consts/const-eval/ub-uninhabit.stderr +++ b/src/test/ui/consts/const-eval/ub-uninhabit.stderr @@ -1,19 +1,27 @@ error[E0080]: this constant likely exhibits undefined behavior - --> $DIR/ub-uninhabit.rs:20:1 + --> $DIR/ub-uninhabit.rs:18:1 | -LL | const BAD_BAD_BAD: Bar = unsafe { Foo { a: 1 }.b }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of an uninhabited type +LL | const BAD_BAD_BAD: Bar = unsafe { mem::transmute(()) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of an uninhabited type | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: this constant likely exhibits undefined behavior - --> $DIR/ub-uninhabit.rs:23:1 + --> $DIR/ub-uninhabit.rs:21:1 | -LL | const BAD_BAD_REF: &Bar = unsafe { Foo { a: 1 }.c }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of an uninhabited type at . +LL | const BAD_BAD_REF: &Bar = unsafe { mem::transmute(1usize) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of an uninhabited type at . | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior -error: aborting due to 2 previous errors +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-uninhabit.rs:24:1 + | +LL | const BAD_BAD_ARRAY: [Bar; 1] = unsafe { mem::transmute(()) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a value of an uninhabited type at [0] + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior + +error: aborting due to 3 previous errors For more information about this error, try `rustc --explain E0080`. From 69a320f40d802060c27d022e98e91bba0a4cc537 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Oct 2018 20:05:12 +0200 Subject: [PATCH 05/19] also validate everything that has a Scalar layout, to catch NonNull --- src/librustc_mir/interpret/validity.rs | 94 +++++++++++++++++-- .../consts/const-eval/transmute-const.stderr | 2 +- src/test/ui/consts/const-eval/ub-enum.stderr | 2 +- src/test/ui/consts/const-eval/ub-nonnull.rs | 25 +++++ .../ui/consts/const-eval/ub-nonnull.stderr | 27 ++++++ .../ui/consts/const-eval/union-ice.stderr | 2 +- src/test/ui/consts/const-eval/union-ub.stderr | 2 +- src/test/ui/union-ub-fat-ptr.stderr | 8 +- 8 files changed, 146 insertions(+), 16 deletions(-) create mode 100644 src/test/ui/consts/const-eval/ub-nonnull.rs create mode 100644 src/test/ui/consts/const-eval/ub-nonnull.stderr diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 23f09cf108e5a..a174645b7ef5f 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -140,14 +140,15 @@ fn scalar_format(value: ScalarMaybeUndef) -> String { } impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { - fn validate_scalar( + /// Make sure that `value` is valid for `ty` + fn validate_scalar_type( &self, value: ScalarMaybeUndef, size: Size, path: &Vec, ty: Ty, ) -> EvalResult<'tcx> { - trace!("validate scalar: {:#?}, {:#?}, {}", value, size, ty); + trace!("validate scalar by type: {:#?}, {:#?}, {}", value, size, ty); // Go over all the primitive types match ty.sty { @@ -189,6 +190,62 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> Ok(()) } + /// Make sure that `value` matches the + fn validate_scalar_layout( + &self, + value: ScalarMaybeUndef, + size: Size, + path: &Vec, + layout: &layout::Scalar, + ) -> EvalResult<'tcx> { + trace!("validate scalar by layout: {:#?}, {:#?}, {:#?}", value, size, layout); + let (lo, hi) = layout.valid_range.clone().into_inner(); + if lo == u128::min_value() && hi == u128::max_value() { + // Nothing to check + return Ok(()); + } + // At least one value is excluded. Get the bits. + let value = try_validation!(value.not_undef(), + scalar_format(value), path, format!("something in the range {:?}", layout.valid_range)); + let bits = match value { + Scalar::Ptr(_) => { + // Comparing a ptr with a range is not meaningfully possible. + // In principle, *if* the pointer is inbonds, we could exclude NULL, but + // that does not seem worth it. + return Ok(()); + } + Scalar::Bits { bits, size: value_size } => { + assert_eq!(value_size as u64, size.bytes()); + bits + } + }; + // Now compare. This is slightly subtle because this is a special "wrap-around" range. + use std::ops::RangeInclusive; + let in_range = |bound: RangeInclusive| bound.contains(&bits); + if lo > hi { + // wrapping around + if in_range(0..=hi) || in_range(lo..=u128::max_value()) { + Ok(()) + } else { + validation_failure!( + bits, + path, + format!("something in the range {:?} or {:?}", 0..=hi, lo..=u128::max_value()) + ) + } + } else { + if in_range(layout.valid_range.clone()) { + Ok(()) + } else { + validation_failure!( + bits, + path, + format!("something in the range {:?}", layout.valid_range) + ) + } + } + } + /// Validate a reference, potentially recursively. `place` is assumed to already be /// dereferenced, i.e. it describes the target. fn validate_ref( @@ -297,6 +354,22 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // Remember the length, in case we need to truncate let path_len = path.len(); + // If this is a scalar, validate the scalar layout. + // Things can be aggregates and have scalar layout at the same time, and that + // is very relevant for `NonNull` and similar structs: We need to validate them + // at their scalar layout *before* descending into their fields. + match dest.layout.abi { + layout::Abi::Uninhabited => + return validation_failure!("a value of an uninhabited type", path), + layout::Abi::Scalar(ref layout) => { + let value = try_validation!(self.read_scalar(dest), + "uninitialized or unrepresentable data", path); + self.validate_scalar_layout(value, dest.layout.size, &path, layout)?; + } + // FIXME: Should we do something for ScalarPair? Vector? + _ => {} + } + // Validate all fields match dest.layout.fields { // Primitives appear as Union with 0 fields -- except for fat pointers. @@ -305,21 +378,26 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // fields to get a proper `path`. layout::FieldPlacement::Union(0) => { match dest.layout.abi { - // nothing to do, whatever the pointer points to, it is never going to be read - layout::Abi::Uninhabited => - return validation_failure!("a value of an uninhabited type", path), // check that the scalar is a valid pointer or that its bit range matches the // expectation. layout::Abi::Scalar(_) => { let value = try_validation!(self.read_value(dest), "uninitialized or unrepresentable data", path); - let scalar = value.to_scalar_or_undef(); - self.validate_scalar(scalar, dest.layout.size, &path, dest.layout.ty)?; + self.validate_scalar_type( + value.to_scalar_or_undef(), + dest.layout.size, + &path, + dest.layout.ty + )?; // Recursively check *safe* references if dest.layout.ty.builtin_deref(true).is_some() && !dest.layout.ty.is_unsafe_ptr() { - self.validate_ref(self.ref_to_mplace(value)?, path, ref_tracking)?; + self.validate_ref( + self.ref_to_mplace(value)?, + path, + ref_tracking + )?; } }, _ => bug!("bad abi for FieldPlacement::Union(0): {:#?}", dest.layout.abi), diff --git a/src/test/ui/consts/const-eval/transmute-const.stderr b/src/test/ui/consts/const-eval/transmute-const.stderr index 2e82c0963c377..c9beca7aa30ac 100644 --- a/src/test/ui/consts/const-eval/transmute-const.stderr +++ b/src/test/ui/consts/const-eval/transmute-const.stderr @@ -2,7 +2,7 @@ error[E0080]: this static likely exhibits undefined behavior --> $DIR/transmute-const.rs:15:1 | LL | static FOO: bool = unsafe { mem::transmute(3u8) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3, but expected a boolean + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3, but expected something in the range 0..=1 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior diff --git a/src/test/ui/consts/const-eval/ub-enum.stderr b/src/test/ui/consts/const-eval/ub-enum.stderr index 20105a46c1db0..243343c94b065 100644 --- a/src/test/ui/consts/const-eval/ub-enum.stderr +++ b/src/test/ui/consts/const-eval/ub-enum.stderr @@ -18,7 +18,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/ub-enum.rs:45:1 | LL | const BAD_ENUM_CHAR : Option<(char, char)> = Some(('x', unsafe { TransmuteChar { a: !0 }.b })); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 4294967295 at .Some.0.1, but expected a valid unicode codepoint + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 4294967295 at .Some.0.1, but expected something in the range 0..=1114111 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior diff --git a/src/test/ui/consts/const-eval/ub-nonnull.rs b/src/test/ui/consts/const-eval/ub-nonnull.rs new file mode 100644 index 0000000000000..2b07eee3ccb46 --- /dev/null +++ b/src/test/ui/consts/const-eval/ub-nonnull.rs @@ -0,0 +1,25 @@ +// Copyright 2018 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(const_transmute)] + +use std::mem; +use std::ptr::NonNull; +use std::num::{NonZeroU8, NonZeroUsize}; + +const NULL_PTR: NonNull = unsafe { mem::transmute(0usize) }; +//~^ ERROR this constant likely exhibits undefined behavior + +const NULL_U8: NonZeroU8 = unsafe { mem::transmute(0u8) }; +//~^ ERROR this constant likely exhibits undefined behavior +const NULL_USIZE: NonZeroUsize = unsafe { mem::transmute(0usize) }; +//~^ ERROR this constant likely exhibits undefined behavior + +fn main() {} diff --git a/src/test/ui/consts/const-eval/ub-nonnull.stderr b/src/test/ui/consts/const-eval/ub-nonnull.stderr new file mode 100644 index 0000000000000..5ebec560da6e8 --- /dev/null +++ b/src/test/ui/consts/const-eval/ub-nonnull.stderr @@ -0,0 +1,27 @@ +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-nonnull.rs:17:1 + | +LL | const NULL_PTR: NonNull = unsafe { mem::transmute(0usize) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something in the range 1..=18446744073709551615 + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior + +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-nonnull.rs:20:1 + | +LL | const NULL_U8: NonZeroU8 = unsafe { mem::transmute(0u8) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something in the range 1..=255 + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior + +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-nonnull.rs:22:1 + | +LL | const NULL_USIZE: NonZeroUsize = unsafe { mem::transmute(0usize) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something in the range 1..=18446744073709551615 + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0080`. diff --git a/src/test/ui/consts/const-eval/union-ice.stderr b/src/test/ui/consts/const-eval/union-ice.stderr index 4484dd6a14740..2848bc62aee96 100644 --- a/src/test/ui/consts/const-eval/union-ice.stderr +++ b/src/test/ui/consts/const-eval/union-ice.stderr @@ -13,7 +13,7 @@ LL | / const FIELD_PATH: Struct = Struct { //~ ERROR this constant likely exhibi LL | | a: 42, LL | | b: unsafe { UNION.field3 }, LL | | }; - | |__^ type validation failed: encountered uninitialized bytes at .b, but expected initialized plain bits + | |__^ type validation failed: encountered uninitialized bytes at .b, but expected something in the range 0..=18446744073709551615 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior diff --git a/src/test/ui/consts/const-eval/union-ub.stderr b/src/test/ui/consts/const-eval/union-ub.stderr index 5da7e1a4dbf60..2a04dae337b27 100644 --- a/src/test/ui/consts/const-eval/union-ub.stderr +++ b/src/test/ui/consts/const-eval/union-ub.stderr @@ -2,7 +2,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub.rs:36:1 | LL | const BAD_BOOL: bool = unsafe { DummyUnion { u8: 42 }.bool}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 42, but expected a boolean + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 42, but expected something in the range 0..=1 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior diff --git a/src/test/ui/union-ub-fat-ptr.stderr b/src/test/ui/union-ub-fat-ptr.stderr index 0471ef075f809..08aaa40e2f3c9 100644 --- a/src/test/ui/union-ub-fat-ptr.stderr +++ b/src/test/ui/union-ub-fat-ptr.stderr @@ -66,7 +66,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:116:1 | LL | const G: &Trait = &unsafe { BoolTransmute { val: 3 }.bl }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ., but expected a boolean + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ., but expected something in the range 0..=1 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -74,7 +74,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:119:1 | LL | const H: &[bool] = &[unsafe { BoolTransmute { val: 3 }.bl }]; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at .[0], but expected a boolean + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at .[0], but expected something in the range 0..=1 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -82,7 +82,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:125:1 | LL | const I2: &MySliceBool = &MySlice(unsafe { BoolTransmute { val: 3 }.bl }, [false]); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ..0, but expected a boolean + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ..0, but expected something in the range 0..=1 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -90,7 +90,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:128:1 | LL | const I3: &MySliceBool = &MySlice(true, [unsafe { BoolTransmute { val: 3 }.bl }]); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ..1[0], but expected a boolean + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 3 at ..1[0], but expected something in the range 0..=1 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior From 13bdc1673dbd6e0d67e2a238f56aafcffd0285b7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Oct 2018 20:07:56 +0200 Subject: [PATCH 06/19] move a test to a better place --- src/test/ui/{ => consts/const-eval}/union-ub-fat-ptr.rs | 0 src/test/ui/{ => consts/const-eval}/union-ub-fat-ptr.stderr | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename src/test/ui/{ => consts/const-eval}/union-ub-fat-ptr.rs (100%) rename src/test/ui/{ => consts/const-eval}/union-ub-fat-ptr.stderr (100%) diff --git a/src/test/ui/union-ub-fat-ptr.rs b/src/test/ui/consts/const-eval/union-ub-fat-ptr.rs similarity index 100% rename from src/test/ui/union-ub-fat-ptr.rs rename to src/test/ui/consts/const-eval/union-ub-fat-ptr.rs diff --git a/src/test/ui/union-ub-fat-ptr.stderr b/src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr similarity index 100% rename from src/test/ui/union-ub-fat-ptr.stderr rename to src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr From d2b9b1de0529efe839a9fa8440d9f0aceed60661 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Oct 2018 20:20:14 +0200 Subject: [PATCH 07/19] add machine option to validate things on every copy --- src/librustc_lint/builtin.rs | 1 + src/librustc_mir/const_eval.rs | 1 + src/librustc_mir/interpret/machine.rs | 3 +++ src/librustc_mir/interpret/place.rs | 22 +++++++++++++++++++--- src/librustc_mir/interpret/validity.rs | 21 +++++++++++++++------ 5 files changed, 39 insertions(+), 9 deletions(-) diff --git a/src/librustc_lint/builtin.rs b/src/librustc_lint/builtin.rs index 8dd1e2b53cdcb..d18edf22dc10c 100644 --- a/src/librustc_lint/builtin.rs +++ b/src/librustc_lint/builtin.rs @@ -1564,6 +1564,7 @@ fn validate_const<'a, 'tcx>( op, &mut path, Some(&mut ref_tracking), + /* const_mode */ true, )?; } Ok(()) diff --git a/src/librustc_mir/const_eval.rs b/src/librustc_mir/const_eval.rs index 3b1eba51aaf45..0c669b9ec31a7 100644 --- a/src/librustc_mir/const_eval.rs +++ b/src/librustc_mir/const_eval.rs @@ -274,6 +274,7 @@ impl<'a, 'mir, 'tcx> interpret::Machine<'a, 'mir, 'tcx> type MemoryKinds = !; const MUT_STATIC_KIND: Option = None; // no mutating of statics allowed + const ENFORCE_VALIDITY: bool = false; // for now, we don't fn find_fn( ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>, diff --git a/src/librustc_mir/interpret/machine.rs b/src/librustc_mir/interpret/machine.rs index 1eb0280409527..f90a7efce47b3 100644 --- a/src/librustc_mir/interpret/machine.rs +++ b/src/librustc_mir/interpret/machine.rs @@ -33,6 +33,9 @@ pub trait Machine<'a, 'mir, 'tcx>: Sized { /// The memory kind to use for mutated statics -- or None if those are not supported. const MUT_STATIC_KIND: Option; + /// Whether to enforce the validity invariant + const ENFORCE_VALIDITY: bool; + /// Called before a basic block terminator is executed. /// You can use this to detect endlessly running programs. fn before_terminator(ecx: &mut EvalContext<'a, 'mir, 'tcx, Self>) -> EvalResult<'tcx>; diff --git a/src/librustc_mir/interpret/place.rs b/src/librustc_mir/interpret/place.rs index f8e1e9e55d530..b75ceb61febb7 100644 --- a/src/librustc_mir/interpret/place.rs +++ b/src/librustc_mir/interpret/place.rs @@ -567,6 +567,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> dest: PlaceTy<'tcx>, ) -> EvalResult<'tcx> { trace!("write_value: {:?} <- {:?}", *dest, src_val); + // Check that the value actually is okay for that type + if M::ENFORCE_VALIDITY { + // Something changed somewhere, better make sure it matches the type! + let op = OpTy { op: Operand::Immediate(src_val), layout: dest.layout }; + self.validate_operand(op, &mut vec![], None, /*const_mode*/false)?; + } + // See if we can avoid an allocation. This is the counterpart to `try_read_value`, // but not factored as a separate function. let mplace = match dest.place { @@ -588,7 +595,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> self.write_value_to_mplace(src_val, dest) } - /// Write a value to memory + /// Write a value to memory. This does NOT do validation, so you better had already + /// done that before calling this! fn write_value_to_mplace( &mut self, value: Value, @@ -652,12 +660,18 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> }; // Slow path, this does not fit into an immediate. Just memcpy. trace!("copy_op: {:?} <- {:?}", *dest, *src); - let (dest_ptr, dest_align) = self.force_allocation(dest)?.to_scalar_ptr_align(); + let dest = self.force_allocation(dest)?; + let (dest_ptr, dest_align) = dest.to_scalar_ptr_align(); self.memory.copy( src_ptr, src_align, dest_ptr, dest_align, src.layout.size, false - ) + )?; + if M::ENFORCE_VALIDITY { + // Something changed somewhere, better make sure it matches the type! + self.validate_operand(dest.into(), &mut vec![], None, /*const_mode*/false)?; + } + Ok(()) } /// Make sure that a place is in memory, and return where it is. @@ -680,6 +694,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // that has different alignment than the outer field. let local_layout = self.layout_of_local(frame, local)?; let ptr = self.allocate(local_layout, MemoryKind::Stack)?; + // We don't have to validate as we can assume the local + // was already valid for its type. self.write_value_to_mplace(value, ptr)?; let mplace = ptr.mplace; // Update the local diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index a174645b7ef5f..5c8aeb2d82ae6 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -147,6 +147,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> size: Size, path: &Vec, ty: Ty, + const_mode: bool, ) -> EvalResult<'tcx> { trace!("validate scalar by type: {:#?}, {:#?}, {}", value, size, ty); @@ -160,12 +161,12 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> try_validation!(value.to_char(), scalar_format(value), path, "a valid unicode codepoint"); }, - ty::Float(_) | ty::Int(_) | ty::Uint(_) => { - // Must be scalar bits + ty::Float(_) | ty::Int(_) | ty::Uint(_) if const_mode => { + // Integers/floats in CTFE: Must be scalar bits try_validation!(value.to_bits(size), scalar_format(value), path, "initialized plain bits"); } - ty::RawPtr(_) => { + ty::Float(_) | ty::Int(_) | ty::Uint(_) | ty::RawPtr(_) => { // Anything but undef goes try_validation!(value.not_undef(), scalar_format(value), path, "a raw pointer"); @@ -303,6 +304,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> dest: OpTy<'tcx>, path: &mut Vec, mut ref_tracking: Option<&mut RefTracking<'tcx>>, + const_mode: bool, ) -> EvalResult<'tcx> { trace!("validate_operand: {:?}, {:#?}", *dest, dest.layout); @@ -387,7 +389,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> value.to_scalar_or_undef(), dest.layout.size, &path, - dest.layout.ty + dest.layout.ty, + const_mode, )?; // Recursively check *safe* references if dest.layout.ty.builtin_deref(true).is_some() && @@ -506,7 +509,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> self.validate_operand( field.into(), path, - ref_tracking.as_mut().map(|r| &mut **r) + ref_tracking.as_mut().map(|r| &mut **r), + const_mode, )?; path.truncate(path_len); } @@ -517,7 +521,12 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> for i in 0..offsets.len() { let field = self.operand_field(dest, i as u64)?; path.push(self.aggregate_field_path_elem(dest.layout.ty, variant, i)); - self.validate_operand(field, path, ref_tracking.as_mut().map(|r| &mut **r))?; + self.validate_operand( + field, + path, + ref_tracking.as_mut().map(|r| &mut **r), + const_mode, + )?; path.truncate(path_len); } } From 616cb6356fbb7553058940732c5030f537fdf394 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Oct 2018 21:16:35 +0200 Subject: [PATCH 08/19] miri engine: also check return type before calling function --- src/librustc/ich/impls_ty.rs | 4 ++++ src/librustc/mir/interpret/error.rs | 8 +++++++- src/librustc/ty/structural_impls.rs | 4 ++++ src/librustc_mir/interpret/eval_context.rs | 5 +++++ src/librustc_mir/interpret/terminator.rs | 16 +++++++++++++++- src/librustc_mir/transform/const_prop.rs | 1 + 6 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/librustc/ich/impls_ty.rs b/src/librustc/ich/impls_ty.rs index 3ff0034fbbee7..e83b514c69190 100644 --- a/src/librustc/ich/impls_ty.rs +++ b/src/librustc/ich/impls_ty.rs @@ -560,6 +560,10 @@ for ::mir::interpret::EvalErrorKind<'gcx, O> { a.hash_stable(hcx, hasher); b.hash_stable(hcx, hasher) }, + FunctionRetMismatch(a, b) => { + a.hash_stable(hcx, hasher); + b.hash_stable(hcx, hasher) + }, NoMirFor(ref s) => s.hash_stable(hcx, hasher), UnterminatedCString(ptr) => ptr.hash_stable(hcx, hasher), PointerOutOfBounds { diff --git a/src/librustc/mir/interpret/error.rs b/src/librustc/mir/interpret/error.rs index 566673857b975..fe466e247c917 100644 --- a/src/librustc/mir/interpret/error.rs +++ b/src/librustc/mir/interpret/error.rs @@ -186,6 +186,7 @@ pub enum EvalErrorKind<'tcx, O> { FunctionAbiMismatch(Abi, Abi), FunctionArgMismatch(Ty<'tcx>, Ty<'tcx>), + FunctionRetMismatch(Ty<'tcx>, Ty<'tcx>), FunctionArgCountMismatch, NoMirFor(String), UnterminatedCString(Pointer), @@ -294,7 +295,8 @@ impl<'tcx, O> EvalErrorKind<'tcx, O> { use self::EvalErrorKind::*; match *self { MachineError(ref inner) => inner, - FunctionAbiMismatch(..) | FunctionArgMismatch(..) | FunctionArgCountMismatch => + FunctionAbiMismatch(..) | FunctionArgMismatch(..) | FunctionRetMismatch(..) + | FunctionArgCountMismatch => "tried to call a function through a function pointer of incompatible type", InvalidMemoryAccess => "tried to access memory through an invalid pointer", @@ -470,6 +472,10 @@ impl<'tcx, O: fmt::Debug> fmt::Debug for EvalErrorKind<'tcx, O> { write!(f, "tried to call a function with argument of type {:?} \ passing data of type {:?}", callee_ty, caller_ty), + FunctionRetMismatch(caller_ty, callee_ty) => + write!(f, "tried to call a function with return type {:?} \ + passing return place of type {:?}", + callee_ty, caller_ty), FunctionArgCountMismatch => write!(f, "tried to call a function with incorrect number of arguments"), BoundsCheck { ref len, ref index } => diff --git a/src/librustc/ty/structural_impls.rs b/src/librustc/ty/structural_impls.rs index 83a9491cf4673..3f62883c6a57b 100644 --- a/src/librustc/ty/structural_impls.rs +++ b/src/librustc/ty/structural_impls.rs @@ -492,6 +492,10 @@ impl<'a, 'tcx, O: Lift<'tcx>> Lift<'tcx> for interpret::EvalErrorKind<'a, O> { tcx.lift(&a)?, tcx.lift(&b)?, ), + FunctionRetMismatch(a, b) => FunctionRetMismatch( + tcx.lift(&a)?, + tcx.lift(&b)?, + ), FunctionArgCountMismatch => FunctionArgCountMismatch, NoMirFor(ref s) => NoMirFor(s.clone()), UnterminatedCString(ptr) => UnterminatedCString(ptr), diff --git a/src/librustc_mir/interpret/eval_context.rs b/src/librustc_mir/interpret/eval_context.rs index ff059e7d1853b..e15a721731e87 100644 --- a/src/librustc_mir/interpret/eval_context.rs +++ b/src/librustc_mir/interpret/eval_context.rs @@ -231,6 +231,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc /// Mark a storage as live, killing the previous content and returning it. /// Remember to deallocate that! pub fn storage_live(&mut self, local: mir::Local) -> EvalResult<'tcx, LocalValue> { + assert!(local != mir::RETURN_PLACE, "Cannot make return place live"); trace!("{:?} is now live", local); let layout = self.layout_of_local(self.cur_frame(), local)?; @@ -242,6 +243,7 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc /// Returns the old value of the local. /// Remember to deallocate that! pub fn storage_dead(&mut self, local: mir::Local) -> LocalValue { + assert!(local != mir::RETURN_PLACE, "Cannot make return place dead"); trace!("{:?} is now dead", local); mem::replace(&mut self.frame_mut().locals[local], LocalValue::Dead) @@ -446,6 +448,9 @@ impl<'a, 'mir, 'tcx: 'mir, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tc let dummy = LocalValue::Live(Operand::Immediate(Value::Scalar(ScalarMaybeUndef::Undef))); let mut locals = IndexVec::from_elem(dummy, &mir.local_decls); + // Return place is handled specially by the `eval_place` functions, and the + // entry in `locals` should never be used. Make it dead, to be sure. + locals[mir::RETURN_PLACE] = LocalValue::Dead; // Now mark those locals as dead that we do not want to initialize match self.tcx.describe_def(instance.def_id()) { // statics and constants don't have `Storage*` statements, no need to look for them diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index c7ed69e0cb66d..e82d60af639fc 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -287,7 +287,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> let return_place = match dest { Some(place) => *place, - None => Place::null(&self), + None => Place::null(&self), // any access will error. good! }; self.push_stack_frame( instance, @@ -373,6 +373,20 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> trace!("Caller has too many args over"); return err!(FunctionArgCountMismatch); } + // Don't forget to check the return type! + if let Some(caller_ret) = dest { + let callee_ret = self.eval_place(&mir::Place::Local(mir::RETURN_PLACE))?; + if !Self::check_argument_compat(caller_ret.layout, callee_ret.layout) { + return err!(FunctionRetMismatch( + caller_ret.layout.ty, callee_ret.layout.ty + )); + } + } else { + // FIXME: The caller thinks this function cannot return. How do + // we verify that the callee agrees? + // On the plus side, the the callee every writes to its return place, + // that will be detected as UB (because we set that to NULL above). + } Ok(()) })(); match res { diff --git a/src/librustc_mir/transform/const_prop.rs b/src/librustc_mir/transform/const_prop.rs index 70d50d589d1a0..626baf207eebc 100644 --- a/src/librustc_mir/transform/const_prop.rs +++ b/src/librustc_mir/transform/const_prop.rs @@ -154,6 +154,7 @@ impl<'a, 'mir, 'tcx> ConstPropagator<'a, 'mir, 'tcx> { // FIXME: figure out the rules and start linting | FunctionAbiMismatch(..) | FunctionArgMismatch(..) + | FunctionRetMismatch(..) | FunctionArgCountMismatch // fine at runtime, might be a register address or sth | ReadBytesAsPointer From 654d9ff618ed95720cf9a3c49cc33e0a6c6ff158 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 2 Oct 2018 22:13:33 +0200 Subject: [PATCH 09/19] do not look at refs to external statics at all --- src/librustc_mir/interpret/validity.rs | 37 +++++++++++++++----------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 5c8aeb2d82ae6..9aa7ec3bb0751 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -254,8 +254,25 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> place: MPlaceTy<'tcx>, path: &mut Vec, ref_tracking: Option<&mut RefTracking<'tcx>>, + const_mode: bool, ) -> EvalResult<'tcx> { - // Before we do anything else, make sure this is entirely in-bounds. + if const_mode { + // Skip validation entirely for some external statics + if let Scalar::Ptr(ptr) = place.ptr { + let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id); + if let Some(AllocType::Static(did)) = alloc_kind { + // `extern static` cannot be validated as they have no body. + // They are not even properly aligned. + // Statics from other crates are already checked. + // They might be checked at a different type, but for now we want + // to avoid recursing too deeply. This is not sound! + if !did.is_local() || self.tcx.is_foreign_item(did) { + return Ok(()); + } + } + } + } + // Make sure this is non-NULL, aligned and entirely in-bounds. let (size, align) = self.size_and_align_of(place.extra, place.layout)?; try_validation!(self.memory.check_align(place.ptr, align), "unaligned reference", path); @@ -264,24 +281,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> "integer pointer in non-ZST reference", path); try_validation!(self.memory.check_bounds(ptr, size, false), "dangling reference (not entirely in bounds)", path); - // Skip recursion for some external statics - let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id); - if let Some(AllocType::Static(did)) = alloc_kind { - // statics from other crates are already checked. - // they might be checked at a different type, but for now we want - // to avoid recursing too deeply. - // extern statics cannot be validated as they have no body. - if !did.is_local() || self.tcx.is_foreign_item(did) { - return Ok(()); - } - } } // Check if we have encountered this pointer+layout combination // before. Proceed recursively even for integer pointers, no // reason to skip them! They are valid for some ZST, but not for others // (e.g. `!` is a ZST). - let op = place.into(); if let Some(ref_tracking) = ref_tracking { + let op = place.into(); if ref_tracking.seen.insert(op) { trace!("Recursing below ptr {:#?}", *op); ref_tracking.todo.push((op, path_clone_and_deref(path))); @@ -399,7 +405,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> self.validate_ref( self.ref_to_mplace(value)?, path, - ref_tracking + ref_tracking, + const_mode, )?; } }, @@ -437,7 +444,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> } // for safe ptrs, recursively check it if !dest.layout.ty.is_unsafe_ptr() { - self.validate_ref(ptr, path, ref_tracking)?; + self.validate_ref(ptr, path, ref_tracking, const_mode)?; } } // Compound data structures From 9bb4bcd7703366161be2cab416a04077e164aecf Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 3 Oct 2018 08:45:16 +0200 Subject: [PATCH 10/19] tidy up --- src/librustc_mir/interpret/validity.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 9aa7ec3bb0751..d7025f9d6b58e 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -180,7 +180,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> scalar_format(value), path, "a pointer"); let _fn = try_validation!(self.memory.get_fn(ptr), scalar_format(value), path, "a function pointer"); - // TODO: Check if the signature matches + // FIXME: Check if the signature matches } ty::FnDef(..) => { // This is a zero-sized type with all relevant data sitting in the type. From 22c1a0acc84e8081bc53e3725e0fc944f932aa56 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 3 Oct 2018 08:59:27 +0200 Subject: [PATCH 11/19] For now, accept all data for integer types when not in const mode We'll try ruling out undef later --- src/librustc_mir/interpret/validity.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index d7025f9d6b58e..219135e3ad833 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -162,14 +162,18 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> scalar_format(value), path, "a valid unicode codepoint"); }, ty::Float(_) | ty::Int(_) | ty::Uint(_) if const_mode => { - // Integers/floats in CTFE: Must be scalar bits + // Integers/floats in CTFE: Must be scalar bits, pointers are dangerous try_validation!(value.to_bits(size), scalar_format(value), path, "initialized plain bits"); } ty::Float(_) | ty::Int(_) | ty::Uint(_) | ty::RawPtr(_) => { - // Anything but undef goes - try_validation!(value.not_undef(), - scalar_format(value), path, "a raw pointer"); + if const_mode { + // Anything but undef goes + try_validation!(value.not_undef(), + scalar_format(value), path, "a raw pointer"); + } else { + // At run-time, for now, we accept *anything* for these types. + } }, ty::Ref(..) => { // This is checked by the recursive reference handling, nothing to do here. @@ -182,10 +186,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> scalar_format(value), path, "a function pointer"); // FIXME: Check if the signature matches } - ty::FnDef(..) => { - // This is a zero-sized type with all relevant data sitting in the type. - // There is nothing to validate. - } + // This should be all + ty::Never => bug!("Uninhabited type should have been catched earlier"), _ => bug!("Unexpected primitive type {}", ty) } Ok(()) From 95593331bfac293478582c2945b7106225500743 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 3 Oct 2018 09:06:35 +0200 Subject: [PATCH 12/19] add some tests with constants that better be valid --- src/test/ui/consts/const-eval/valid-const.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 src/test/ui/consts/const-eval/valid-const.rs diff --git a/src/test/ui/consts/const-eval/valid-const.rs b/src/test/ui/consts/const-eval/valid-const.rs new file mode 100644 index 0000000000000..a195e12defcea --- /dev/null +++ b/src/test/ui/consts/const-eval/valid-const.rs @@ -0,0 +1,18 @@ +// compile-pass + +// Some constants that *are* valid +#![feature(const_transmute)] + +use std::mem; +use std::ptr::NonNull; +use std::num::{NonZeroU8, NonZeroUsize}; + +const NON_NULL_PTR1: NonNull = unsafe { mem::transmute(1usize) }; +const NON_NULL_PTR2: NonNull = unsafe { mem::transmute(&0) }; + +const NON_NULL_U8: NonZeroU8 = unsafe { mem::transmute(1u8) }; +const NON_NULL_USIZE: NonZeroUsize = unsafe { mem::transmute(1usize) }; + +const UNIT: () = (); + +fn main() {} From 322017b2bc578216a62a297de84b98eb7143923e Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 3 Oct 2018 11:38:16 +0200 Subject: [PATCH 13/19] unify handling of thin and fat pointers by moving primitive type handling out of aggregate handling Also, make enum variant handling a bit nicer --- src/librustc_mir/interpret/validity.rs | 400 +++++++++--------- .../ui/consts/const-eval/ub-nonnull.stderr | 6 +- src/test/ui/consts/const-eval/ub-ref.rs | 3 + src/test/ui/consts/const-eval/ub-ref.stderr | 12 +- .../ui/consts/const-eval/union-ice.stderr | 2 +- .../consts/const-eval/union-ub-fat-ptr.stderr | 4 +- 6 files changed, 225 insertions(+), 202 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 219135e3ad833..2aa6b07df90d7 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -11,15 +11,15 @@ use std::fmt::Write; use syntax_pos::symbol::Symbol; -use rustc::ty::layout::{self, Size}; -use rustc::ty::{self, Ty}; +use rustc::ty::layout::{self, Size, Align, TyLayout}; +use rustc::ty; use rustc_data_structures::fx::FxHashSet; use rustc::mir::interpret::{ Scalar, AllocType, EvalResult, EvalErrorKind }; use super::{ - OpTy, MPlaceTy, Machine, EvalContext, ScalarMaybeUndef + ValTy, OpTy, MPlaceTy, Machine, EvalContext, ScalarMaybeUndef }; macro_rules! validation_failure { @@ -140,55 +140,137 @@ fn scalar_format(value: ScalarMaybeUndef) -> String { } impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> { - /// Make sure that `value` is valid for `ty` - fn validate_scalar_type( + /// Make sure that `value` is valid for `ty`, *assuming* `ty` is a primitive type. + fn validate_primitive_type( &self, - value: ScalarMaybeUndef, - size: Size, + value: ValTy<'tcx>, path: &Vec, - ty: Ty, + ref_tracking: Option<&mut RefTracking<'tcx>>, const_mode: bool, ) -> EvalResult<'tcx> { - trace!("validate scalar by type: {:#?}, {:#?}, {}", value, size, ty); + trace!("validate scalar by type: {:#?}, {:#?}, {}", + *value, value.layout.size, value.layout.ty); // Go over all the primitive types - match ty.sty { + match value.layout.ty.sty { ty::Bool => { + let value = value.to_scalar_or_undef(); try_validation!(value.to_bool(), scalar_format(value), path, "a boolean"); }, ty::Char => { + let value = value.to_scalar_or_undef(); try_validation!(value.to_char(), scalar_format(value), path, "a valid unicode codepoint"); }, - ty::Float(_) | ty::Int(_) | ty::Uint(_) if const_mode => { - // Integers/floats in CTFE: Must be scalar bits, pointers are dangerous - try_validation!(value.to_bits(size), - scalar_format(value), path, "initialized plain bits"); - } - ty::Float(_) | ty::Int(_) | ty::Uint(_) | ty::RawPtr(_) => { + ty::Float(_) | ty::Int(_) | ty::Uint(_) => { + let size = value.layout.size; + let value = value.to_scalar_or_undef(); if const_mode { - // Anything but undef goes - try_validation!(value.not_undef(), - scalar_format(value), path, "a raw pointer"); + // Integers/floats in CTFE: Must be scalar bits, pointers are dangerous + try_validation!(value.to_bits(size), + scalar_format(value), path, "initialized plain bits"); } else { - // At run-time, for now, we accept *anything* for these types. + // At run-time, for now, we accept *anything* for these types, including + // undef. We should fix that, but let's start low. + } + } + ty::RawPtr(..) | ty::Ref(..) => { + // Handle fat pointers. We also check fat raw pointers, + // their metadata must be valid! + // This also checks that the ptr itself is initialized, which + // seems reasonable even for raw pointers. + let place = try_validation!(self.ref_to_mplace(value), + "undefined data in pointer", path); + // Check metadata early, for better diagnostics + if place.layout.is_unsized() { + match self.tcx.struct_tail(place.layout.ty).sty { + ty::Dynamic(..) => { + let vtable = try_validation!(place.extra.unwrap().to_ptr(), + "non-pointer vtable in fat pointer", path); + try_validation!(self.read_drop_type_from_vtable(vtable), + "invalid drop fn in vtable", path); + try_validation!(self.read_size_and_align_from_vtable(vtable), + "invalid size or align in vtable", path); + // FIXME: More checks for the vtable. + } + ty::Slice(..) | ty::Str => { + try_validation!(place.extra.unwrap().to_usize(self), + "non-integer slice length in fat pointer", path); + } + ty::Foreign(..) => { + // Unsized, but not fat. + } + _ => + bug!("Unexpected unsized type tail: {:?}", + self.tcx.struct_tail(place.layout.ty) + ), + } + } + // for safe ptrs, recursively check + if let ty::Ref(..) = value.layout.ty.sty { + if const_mode { + // Skip validation entirely for some external statics + if let Scalar::Ptr(ptr) = place.ptr { + let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id); + if let Some(AllocType::Static(did)) = alloc_kind { + // `extern static` cannot be validated as they have no body. + // They are not even properly aligned. + // Statics from other crates are already checked. + // They might be checked at a different type, but for now we want + // to avoid recursing too deeply. This is not sound! + if !did.is_local() || self.tcx.is_foreign_item(did) { + return Ok(()); + } + } + } + } + // Make sure this is non-NULL and aligned + let (size, align) = self.size_and_align_of(place.extra, place.layout)?; + match self.memory.check_align(place.ptr, align) { + Ok(_) => {}, + Err(err) => match err.kind { + EvalErrorKind::InvalidNullPointerUsage => + return validation_failure!("NULL reference", path), + EvalErrorKind::AlignmentCheckFailed { .. } => + return validation_failure!("unaligned reference", path), + _ => + return validation_failure!( + "dangling (deallocated) reference", path + ), + } + } + // non-ZST also have to be dereferencable + if !place.layout.is_zst() { + let ptr = try_validation!(place.ptr.to_ptr(), + "integer pointer in non-ZST reference", path); + try_validation!(self.memory.check_bounds(ptr, size, false), + "dangling (not entirely in bounds) reference", path); + } + if let Some(ref_tracking) = ref_tracking { + // Check if we have encountered this pointer+layout combination + // before. Proceed recursively even for integer pointers, no + // reason to skip them! They are (recursively) valid for some ZST, + // but not for others (e.g. `!` is a ZST). + let op = place.into(); + if ref_tracking.seen.insert(op) { + trace!("Recursing below ptr {:#?}", *op); + ref_tracking.todo.push((op, path_clone_and_deref(path))); + } + } } - }, - ty::Ref(..) => { - // This is checked by the recursive reference handling, nothing to do here. - debug_assert!(ty.builtin_deref(true).is_some() && !ty.is_unsafe_ptr()); } ty::FnPtr(_sig) => { + let value = value.to_scalar_or_undef(); let ptr = try_validation!(value.to_ptr(), scalar_format(value), path, "a pointer"); let _fn = try_validation!(self.memory.get_fn(ptr), scalar_format(value), path, "a function pointer"); // FIXME: Check if the signature matches } - // This should be all + // This should be all the primitive types ty::Never => bug!("Uninhabited type should have been catched earlier"), - _ => bug!("Unexpected primitive type {}", ty) + _ => bug!("Unexpected primitive type {}", value.layout.ty) } Ok(()) } @@ -203,7 +285,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> ) -> EvalResult<'tcx> { trace!("validate scalar by layout: {:#?}, {:#?}, {:#?}", value, size, layout); let (lo, hi) = layout.valid_range.clone().into_inner(); - if lo == u128::min_value() && hi == u128::max_value() { + let max_hi = u128::max_value() >> (128 - size.bits()); // as big as the size fits + assert!(hi <= max_hi); + if lo == 0 && hi == max_hi { // Nothing to check return Ok(()); } @@ -211,11 +295,33 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> let value = try_validation!(value.not_undef(), scalar_format(value), path, format!("something in the range {:?}", layout.valid_range)); let bits = match value { - Scalar::Ptr(_) => { - // Comparing a ptr with a range is not meaningfully possible. - // In principle, *if* the pointer is inbonds, we could exclude NULL, but - // that does not seem worth it. - return Ok(()); + Scalar::Ptr(ptr) => { + if lo == 1 && hi == max_hi { + // only NULL is not allowed. + // We can call `check_align` to check non-NULL-ness, but have to also look + // for function pointers. + let non_null = + self.memory.check_align( + Scalar::Ptr(ptr), Align::from_bytes(1, 1).unwrap() + ).is_ok() || + self.memory.get_fn(ptr).is_ok(); + if !non_null { + // could be NULL + return validation_failure!("a potentially NULL pointer", path); + } + return Ok(()); + } else { + // Conservatively, we reject, because the pointer *could* have this + // value. + return validation_failure!( + "a pointer", + path, + format!( + "something that cannot possibly be outside the (wrapping) range {:?}", + layout.valid_range + ) + ); + } } Scalar::Bits { bits, size: value_size } => { assert_eq!(value_size as u64, size.bytes()); @@ -227,13 +333,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> let in_range = |bound: RangeInclusive| bound.contains(&bits); if lo > hi { // wrapping around - if in_range(0..=hi) || in_range(lo..=u128::max_value()) { + if in_range(0..=hi) || in_range(lo..=max_hi) { Ok(()) } else { validation_failure!( bits, path, - format!("something in the range {:?} or {:?}", 0..=hi, lo..=u128::max_value()) + format!("something in the range {:?} or {:?}", 0..=hi, lo..=max_hi) ) } } else { @@ -243,59 +349,14 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> validation_failure!( bits, path, - format!("something in the range {:?}", layout.valid_range) - ) - } - } - } - - /// Validate a reference, potentially recursively. `place` is assumed to already be - /// dereferenced, i.e. it describes the target. - fn validate_ref( - &self, - place: MPlaceTy<'tcx>, - path: &mut Vec, - ref_tracking: Option<&mut RefTracking<'tcx>>, - const_mode: bool, - ) -> EvalResult<'tcx> { - if const_mode { - // Skip validation entirely for some external statics - if let Scalar::Ptr(ptr) = place.ptr { - let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id); - if let Some(AllocType::Static(did)) = alloc_kind { - // `extern static` cannot be validated as they have no body. - // They are not even properly aligned. - // Statics from other crates are already checked. - // They might be checked at a different type, but for now we want - // to avoid recursing too deeply. This is not sound! - if !did.is_local() || self.tcx.is_foreign_item(did) { - return Ok(()); + if hi == max_hi { + format!("something greater or equal to {}", lo) + } else { + format!("something in the range {:?}", layout.valid_range) } - } - } - } - // Make sure this is non-NULL, aligned and entirely in-bounds. - let (size, align) = self.size_and_align_of(place.extra, place.layout)?; - try_validation!(self.memory.check_align(place.ptr, align), - "unaligned reference", path); - if !place.layout.is_zst() { - let ptr = try_validation!(place.ptr.to_ptr(), - "integer pointer in non-ZST reference", path); - try_validation!(self.memory.check_bounds(ptr, size, false), - "dangling reference (not entirely in bounds)", path); - } - // Check if we have encountered this pointer+layout combination - // before. Proceed recursively even for integer pointers, no - // reason to skip them! They are valid for some ZST, but not for others - // (e.g. `!` is a ZST). - if let Some(ref_tracking) = ref_tracking { - let op = place.into(); - if ref_tracking.seen.insert(op) { - trace!("Recursing below ptr {:#?}", *op); - ref_tracking.todo.push((op, path_clone_and_deref(path))); + ) } } - Ok(()) } /// This function checks the data at `op`. `op` is assumed to cover valid memory if it @@ -316,10 +377,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> ) -> EvalResult<'tcx> { trace!("validate_operand: {:?}, {:#?}", *dest, dest.layout); - // Find the right variant. We have to handle this as a prelude, not via - // proper recursion with the new inner layout, to be able to later nicely - // print the field names of the enum field that is being accessed. - let (variant, dest) = match dest.layout.variants { + // If this is a multi-variant layout, we have find the right one and proceed with that. + // (No good reasoning to make this recursion, but it is equivalent to that.) + let dest = match dest.layout.variants { layout::Variants::NicheFilling { .. } | layout::Variants::Tagged { .. } => { let variant = match self.read_discriminant(dest) { @@ -335,34 +395,28 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> ), } }; - let inner_dest = self.operand_downcast(dest, variant)?; // Put the variant projection onto the path, as a field path.push(PathElem::Field(dest.layout.ty .ty_adt_def() .unwrap() .variants[variant].name)); + // Proceed with this variant + let dest = self.operand_downcast(dest, variant)?; trace!("variant layout: {:#?}", dest.layout); - (variant, inner_dest) + dest }, - layout::Variants::Single { index } => { - // Pre-processing for trait objects: Treat them at their real type. - // (We do not do this for slices and strings: For slices it is not needed, - // `mplace_array_fields` does the right thing, and for strings there is no - // real type that would show the actual length.) - let dest = match dest.layout.ty.sty { - ty::Dynamic(..) => { - let dest = dest.to_mem_place(); // immediate trait objects are not a thing - try_validation!(self.unpack_dyn_trait(dest), - "invalid vtable in fat pointer", path).1.into() - } - _ => dest - }; - (index, dest) - } + layout::Variants::Single { .. } => dest, }; - // Remember the length, in case we need to truncate - let path_len = path.len(); + // First thing, find the real type: + // If it is a trait object, switch to the actual type that was used to create it. + let dest = match dest.layout.ty.sty { + ty::Dynamic(..) => { + let dest = dest.to_mem_place(); // immediate trait objects are not a thing + self.unpack_dyn_trait(dest)?.1.into() + }, + _ => dest + }; // If this is a scalar, validate the scalar layout. // Things can be aggregates and have scalar layout at the same time, and that @@ -380,81 +434,49 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> _ => {} } - // Validate all fields - match dest.layout.fields { + // Check primitive types. We do this after checking the scalar layout, + // just to have that done as well. Primitives can have varying layout, + // so we check them separately and before aggregate handling. + // It is CRITICAL that we get this check right, or we might be + // validating the wrong thing! + let primitive = match dest.layout.fields { // Primitives appear as Union with 0 fields -- except for fat pointers. - // We still check `layout.fields`, not `layout.abi`, because `layout.abi` - // is `Scalar` for newtypes around scalars, but we want to descend through the - // fields to get a proper `path`. - layout::FieldPlacement::Union(0) => { - match dest.layout.abi { - // check that the scalar is a valid pointer or that its bit range matches the - // expectation. - layout::Abi::Scalar(_) => { - let value = try_validation!(self.read_value(dest), - "uninitialized or unrepresentable data", path); - self.validate_scalar_type( - value.to_scalar_or_undef(), - dest.layout.size, - &path, - dest.layout.ty, - const_mode, - )?; - // Recursively check *safe* references - if dest.layout.ty.builtin_deref(true).is_some() && - !dest.layout.ty.is_unsafe_ptr() - { - self.validate_ref( - self.ref_to_mplace(value)?, - path, - ref_tracking, - const_mode, - )?; - } - }, - _ => bug!("bad abi for FieldPlacement::Union(0): {:#?}", dest.layout.abi), - } - } - layout::FieldPlacement::Arbitrary { .. } - if dest.layout.ty.builtin_deref(true).is_some() => - { - // This is a fat pointer. We also check fat raw pointers, their metadata must - // be valid! - let ptr = try_validation!(self.read_value(dest.into()), - "undefined location in fat pointer", path); - let ptr = try_validation!(self.ref_to_mplace(ptr), - "undefined metadata in fat pointer", path); - // check metadata early, for better diagnostics - match self.tcx.struct_tail(ptr.layout.ty).sty { - ty::Dynamic(..) => { - let vtable = try_validation!(ptr.extra.unwrap().to_ptr(), - "non-pointer vtable in fat pointer", path); - try_validation!(self.read_drop_type_from_vtable(vtable), - "invalid drop fn in vtable", path); - try_validation!(self.read_size_and_align_from_vtable(vtable), - "invalid size or align in vtable", path); - // FIXME: More checks for the vtable. - } - ty::Slice(..) | ty::Str => { - try_validation!(ptr.extra.unwrap().to_usize(self), - "non-integer slice length in fat pointer", path); - } - _ => - bug!("Unexpected unsized type tail: {:?}", - self.tcx.struct_tail(ptr.layout.ty) - ), - } - // for safe ptrs, recursively check it - if !dest.layout.ty.is_unsafe_ptr() { - self.validate_ref(ptr, path, ref_tracking, const_mode)?; - } - } - // Compound data structures - layout::FieldPlacement::Union(_) => { + layout::FieldPlacement::Union(0) => true, + _ => dest.layout.ty.builtin_deref(true).is_some(), + }; + if primitive { + let value = try_validation!(self.read_value(dest), + "uninitialized or unrepresentable data", path); + return self.validate_primitive_type( + value, + &path, + ref_tracking, + const_mode, + ); + } + + // Validate all fields of compound data structures + let path_len = path.len(); // Remember the length, in case we need to truncate + match dest.layout.fields { + layout::FieldPlacement::Union(..) => { // We can't check unions, their bits are allowed to be anything. // The fields don't need to correspond to any bit pattern of the union's fields. // See https://github.com/rust-lang/rust/issues/32836#issuecomment-406875389 }, + layout::FieldPlacement::Arbitrary { ref offsets, .. } => { + // Go look at all the fields + for i in 0..offsets.len() { + let field = self.operand_field(dest, i as u64)?; + path.push(self.aggregate_field_path_elem(dest.layout, i)); + self.validate_operand( + field, + path, + ref_tracking.as_mut().map(|r| &mut **r), + const_mode, + )?; + path.truncate(path_len); + } + } layout::FieldPlacement::Array { stride, .. } => { let dest = if dest.layout.is_zst() { // it's a ZST, the memory content cannot matter @@ -526,25 +548,12 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> } } }, - layout::FieldPlacement::Arbitrary { ref offsets, .. } => { - for i in 0..offsets.len() { - let field = self.operand_field(dest, i as u64)?; - path.push(self.aggregate_field_path_elem(dest.layout.ty, variant, i)); - self.validate_operand( - field, - path, - ref_tracking.as_mut().map(|r| &mut **r), - const_mode, - )?; - path.truncate(path_len); - } - } } Ok(()) } - fn aggregate_field_path_elem(&self, ty: Ty<'tcx>, variant: usize, field: usize) -> PathElem { - match ty.sty { + fn aggregate_field_path_elem(&self, layout: TyLayout<'tcx>, field: usize) -> PathElem { + match layout.ty.sty { // generators and closures. ty::Closure(def_id, _) | ty::Generator(def_id, _, _) => { let node_id = self.tcx.hir.as_local_node_id(def_id).unwrap(); @@ -557,7 +566,10 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // enums ty::Adt(def, ..) if def.is_enum() => { - let variant = &def.variants[variant]; + let variant = match layout.variants { + layout::Variants::Single { index } => &def.variants[index], + _ => bug!("aggregate_field_path_elem: got enum but not in a specific variant"), + }; PathElem::Field(variant.fields[field].ident.name) } @@ -565,7 +577,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> ty::Adt(def, _) => PathElem::Field(def.non_enum_variant().fields[field].ident.name), // nothing else has an aggregate layout - _ => bug!("aggregate_field_path_elem: got non-aggregate type {:?}", ty), + _ => bug!("aggregate_field_path_elem: got non-aggregate type {:?}", layout.ty), } } } diff --git a/src/test/ui/consts/const-eval/ub-nonnull.stderr b/src/test/ui/consts/const-eval/ub-nonnull.stderr index 5ebec560da6e8..8d1ca885b5aba 100644 --- a/src/test/ui/consts/const-eval/ub-nonnull.stderr +++ b/src/test/ui/consts/const-eval/ub-nonnull.stderr @@ -2,7 +2,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/ub-nonnull.rs:17:1 | LL | const NULL_PTR: NonNull = unsafe { mem::transmute(0usize) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something in the range 1..=18446744073709551615 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something greater or equal to 1 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -10,7 +10,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/ub-nonnull.rs:20:1 | LL | const NULL_U8: NonZeroU8 = unsafe { mem::transmute(0u8) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something in the range 1..=255 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something greater or equal to 1 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -18,7 +18,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/ub-nonnull.rs:22:1 | LL | const NULL_USIZE: NonZeroUsize = unsafe { mem::transmute(0usize) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something in the range 1..=18446744073709551615 + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something greater or equal to 1 | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior diff --git a/src/test/ui/consts/const-eval/ub-ref.rs b/src/test/ui/consts/const-eval/ub-ref.rs index e010152513c15..7ee13f20dd2d9 100644 --- a/src/test/ui/consts/const-eval/ub-ref.rs +++ b/src/test/ui/consts/const-eval/ub-ref.rs @@ -15,6 +15,9 @@ use std::mem; const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) }; //~^ ERROR this constant likely exhibits undefined behavior +const NULL: &u16 = unsafe { mem::transmute(0usize) }; +//~^ ERROR this constant likely exhibits undefined behavior + const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; //~^ ERROR this constant likely exhibits undefined behavior diff --git a/src/test/ui/consts/const-eval/ub-ref.stderr b/src/test/ui/consts/const-eval/ub-ref.stderr index b7989e22afabe..9907c780d2ccb 100644 --- a/src/test/ui/consts/const-eval/ub-ref.stderr +++ b/src/test/ui/consts/const-eval/ub-ref.stderr @@ -9,19 +9,27 @@ LL | const UNALIGNED: &u16 = unsafe { mem::transmute(&[0u8; 4]) }; error[E0080]: this constant likely exhibits undefined behavior --> $DIR/ub-ref.rs:18:1 | +LL | const NULL: &u16 = unsafe { mem::transmute(0usize) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0, but expected something greater or equal to 1 + | + = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior + +error[E0080]: this constant likely exhibits undefined behavior + --> $DIR/ub-ref.rs:21:1 + | LL | const REF_AS_USIZE: usize = unsafe { mem::transmute(&0) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain bits | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior error[E0080]: this constant likely exhibits undefined behavior - --> $DIR/ub-ref.rs:21:1 + --> $DIR/ub-ref.rs:24:1 | LL | const USIZE_AS_REF: &'static u8 = unsafe { mem::transmute(1337usize) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered integer pointer in non-ZST reference | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior -error: aborting due to 3 previous errors +error: aborting due to 4 previous errors For more information about this error, try `rustc --explain E0080`. diff --git a/src/test/ui/consts/const-eval/union-ice.stderr b/src/test/ui/consts/const-eval/union-ice.stderr index 2848bc62aee96..4484dd6a14740 100644 --- a/src/test/ui/consts/const-eval/union-ice.stderr +++ b/src/test/ui/consts/const-eval/union-ice.stderr @@ -13,7 +13,7 @@ LL | / const FIELD_PATH: Struct = Struct { //~ ERROR this constant likely exhibi LL | | a: 42, LL | | b: unsafe { UNION.field3 }, LL | | }; - | |__^ type validation failed: encountered uninitialized bytes at .b, but expected something in the range 0..=18446744073709551615 + | |__^ type validation failed: encountered uninitialized bytes at .b, but expected initialized plain bits | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior diff --git a/src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr b/src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr index 08aaa40e2f3c9..c4632ffe30974 100644 --- a/src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr +++ b/src/test/ui/consts/const-eval/union-ub-fat-ptr.stderr @@ -2,7 +2,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:87:1 | LL | const B: &str = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.str}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling reference (not entirely in bounds) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling (not entirely in bounds) reference | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior @@ -26,7 +26,7 @@ error[E0080]: this constant likely exhibits undefined behavior --> $DIR/union-ub-fat-ptr.rs:99:1 | LL | const B2: &[u8] = unsafe { SliceTransmute { repr: SliceRepr { ptr: &42, len: 999 } }.slice}; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling reference (not entirely in bounds) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered dangling (not entirely in bounds) reference | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior From e09e3c898cbc371e8c3f9e6f1be5417b30daa24a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 3 Oct 2018 12:34:10 +0200 Subject: [PATCH 14/19] fix nits and handling of extern static --- src/librustc_mir/interpret/memory.rs | 37 +++++++++++++++--------- src/librustc_mir/interpret/terminator.rs | 2 +- src/librustc_mir/interpret/validity.rs | 29 +++++++++---------- src/test/ui/issues/issue-14227.rs | 4 +-- src/test/ui/issues/issue-14227.stderr | 6 ++-- 5 files changed, 42 insertions(+), 36 deletions(-) diff --git a/src/librustc_mir/interpret/memory.rs b/src/librustc_mir/interpret/memory.rs index 1b75982a83a57..5437c8ababc27 100644 --- a/src/librustc_mir/interpret/memory.rs +++ b/src/librustc_mir/interpret/memory.rs @@ -19,7 +19,7 @@ use std::collections::VecDeque; use std::ptr; -use rustc::ty::{self, Instance, query::TyCtxtAt}; +use rustc::ty::{self, Instance, ParamEnv, query::TyCtxtAt}; use rustc::ty::layout::{self, Align, TargetDataLayout, Size, HasDataLayout}; use rustc::mir::interpret::{Pointer, AllocId, Allocation, ConstValue, GlobalId, EvalResult, Scalar, EvalErrorKind, AllocType, PointerArithmetic, @@ -235,7 +235,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { // Check non-NULL/Undef, extract offset let (offset, alloc_align) = match ptr { Scalar::Ptr(ptr) => { - let (size, align) = self.get_size_and_align(ptr.alloc_id)?; + let (size, align) = self.get_size_and_align(ptr.alloc_id); // check this is not NULL -- which we can ensure only if this is in-bounds // of some (potentially dead) allocation. if ptr.offset > size { @@ -359,19 +359,28 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> { } } - pub fn get_size_and_align(&self, id: AllocId) -> EvalResult<'tcx, (Size, Align)> { - Ok(match self.get(id) { - Ok(alloc) => (Size::from_bytes(alloc.bytes.len() as u64), alloc.align), - Err(err) => match err.kind { - EvalErrorKind::DanglingPointerDeref => - // This should be in the dead allocation map - *self.dead_alloc_map.get(&id).expect( - "allocation missing in dead_alloc_map" - ), - // E.g. a function ptr allocation - _ => return Err(err) + pub fn get_size_and_align(&self, id: AllocId) -> (Size, Align) { + if let Ok(alloc) = self.get(id) { + return (Size::from_bytes(alloc.bytes.len() as u64), alloc.align); + } + // Could also be a fn ptr or extern static + match self.tcx.alloc_map.lock().get(id) { + Some(AllocType::Function(..)) => (Size::ZERO, Align::from_bytes(1, 1).unwrap()), + Some(AllocType::Static(did)) => { + // The only way `get` couldnÄt have worked here is if this is an extern static + assert!(self.tcx.is_foreign_item(did)); + // Use size and align of the type + let ty = self.tcx.type_of(did); + let layout = self.tcx.layout_of(ParamEnv::empty().and(ty)).unwrap(); + (layout.size, layout.align) } - }) + _ => { + // Must be a deallocated pointer + *self.dead_alloc_map.get(&id).expect( + "allocation missing in dead_alloc_map" + ) + } + } } pub fn get_mut( diff --git a/src/librustc_mir/interpret/terminator.rs b/src/librustc_mir/interpret/terminator.rs index e82d60af639fc..862f61df227be 100644 --- a/src/librustc_mir/interpret/terminator.rs +++ b/src/librustc_mir/interpret/terminator.rs @@ -384,7 +384,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> } else { // FIXME: The caller thinks this function cannot return. How do // we verify that the callee agrees? - // On the plus side, the the callee every writes to its return place, + // On the plus side, the the callee ever writes to its return place, // that will be detected as UB (because we set that to NULL above). } Ok(()) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 2aa6b07df90d7..f0a51000cd875 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -209,22 +209,6 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> } // for safe ptrs, recursively check if let ty::Ref(..) = value.layout.ty.sty { - if const_mode { - // Skip validation entirely for some external statics - if let Scalar::Ptr(ptr) = place.ptr { - let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id); - if let Some(AllocType::Static(did)) = alloc_kind { - // `extern static` cannot be validated as they have no body. - // They are not even properly aligned. - // Statics from other crates are already checked. - // They might be checked at a different type, but for now we want - // to avoid recursing too deeply. This is not sound! - if !did.is_local() || self.tcx.is_foreign_item(did) { - return Ok(()); - } - } - } - } // Make sure this is non-NULL and aligned let (size, align) = self.size_and_align_of(place.extra, place.layout)?; match self.memory.check_align(place.ptr, align) { @@ -244,6 +228,19 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> if !place.layout.is_zst() { let ptr = try_validation!(place.ptr.to_ptr(), "integer pointer in non-ZST reference", path); + if const_mode { + // Skip validation entirely for some external statics + let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id); + if let Some(AllocType::Static(did)) = alloc_kind { + // `extern static` cannot be validated as they have no body. + // FIXME: Statics from other crates are also skipped. + // They might be checked at a different type, but for now we + // want to avoid recursing too deeply. This is not sound! + if !did.is_local() || self.tcx.is_foreign_item(did) { + return Ok(()); + } + } + } try_validation!(self.memory.check_bounds(ptr, size, false), "dangling (not entirely in bounds) reference", path); } diff --git a/src/test/ui/issues/issue-14227.rs b/src/test/ui/issues/issue-14227.rs index 250e78ce24640..857db50edbbc1 100644 --- a/src/test/ui/issues/issue-14227.rs +++ b/src/test/ui/issues/issue-14227.rs @@ -11,9 +11,9 @@ #![allow(safe_extern_statics, warnings)] extern { - pub static symbol: (); + pub static symbol: u32; } -static CRASH: () = symbol; +static CRASH: u32 = symbol; //~^ ERROR could not evaluate static initializer //~| tried to read from foreign (extern) static diff --git a/src/test/ui/issues/issue-14227.stderr b/src/test/ui/issues/issue-14227.stderr index f5f39465b187b..dc6c72d8a7256 100644 --- a/src/test/ui/issues/issue-14227.stderr +++ b/src/test/ui/issues/issue-14227.stderr @@ -1,8 +1,8 @@ error[E0080]: could not evaluate static initializer - --> $DIR/issue-14227.rs:16:20 + --> $DIR/issue-14227.rs:16:21 | -LL | static CRASH: () = symbol; - | ^^^^^^ tried to read from foreign (extern) static +LL | static CRASH: u32 = symbol; + | ^^^^^^ tried to read from foreign (extern) static error: aborting due to previous error From fcf6b5c79bdc69f0daa894605d42c271336af8a6 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 4 Oct 2018 20:04:16 +0200 Subject: [PATCH 15/19] add fixme for potential perf optimization --- src/librustc_mir/interpret/validity.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index f0a51000cd875..9e833598ef117 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -419,6 +419,9 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // Things can be aggregates and have scalar layout at the same time, and that // is very relevant for `NonNull` and similar structs: We need to validate them // at their scalar layout *before* descending into their fields. + // FIXME: We could avoid some redundant checks here. For newtypes wrapping + // scalars, we do the same check on every "level" (e.g. first we check + // MyNewtype and then the scalar in there). match dest.layout.abi { layout::Abi::Uninhabited => return validation_failure!("a value of an uninhabited type", path), From db1663d5984b084acbd62290ed13014e2b7a7ed3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 8 Oct 2018 14:45:46 +0200 Subject: [PATCH 16/19] update miri --- src/tools/miri | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri b/src/tools/miri index e8f6973e2d40a..cc275c63a90d4 160000 --- a/src/tools/miri +++ b/src/tools/miri @@ -1 +1 @@ -Subproject commit e8f6973e2d40ab39e30cdbe0cf8e77a72c867d4f +Subproject commit cc275c63a90d4bea394e76607b2e10611eb1be36 From 6899af82fd80bd83a5e580c72c1655ac3d7a86d3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 8 Oct 2018 13:41:16 +0200 Subject: [PATCH 17/19] box is also a primitive type --- src/librustc_mir/interpret/validity.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 9e833598ef117..0aa2e70804301 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -152,7 +152,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> *value, value.layout.size, value.layout.ty); // Go over all the primitive types - match value.layout.ty.sty { + let ty = value.layout.ty; + match ty.sty { ty::Bool => { let value = value.to_scalar_or_undef(); try_validation!(value.to_bool(), @@ -175,7 +176,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // undef. We should fix that, but let's start low. } } - ty::RawPtr(..) | ty::Ref(..) => { + _ if ty.is_box() || ty.is_region_ptr() || ty.is_unsafe_ptr() => { // Handle fat pointers. We also check fat raw pointers, // their metadata must be valid! // This also checks that the ptr itself is initialized, which @@ -184,7 +185,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> "undefined data in pointer", path); // Check metadata early, for better diagnostics if place.layout.is_unsized() { - match self.tcx.struct_tail(place.layout.ty).sty { + let tail = self.tcx.struct_tail(place.layout.ty); + match tail.sty { ty::Dynamic(..) => { let vtable = try_validation!(place.extra.unwrap().to_ptr(), "non-pointer vtable in fat pointer", path); @@ -202,13 +204,11 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> // Unsized, but not fat. } _ => - bug!("Unexpected unsized type tail: {:?}", - self.tcx.struct_tail(place.layout.ty) - ), + bug!("Unexpected unsized type tail: {:?}", tail), } } // for safe ptrs, recursively check - if let ty::Ref(..) = value.layout.ty.sty { + if !ty.is_unsafe_ptr() { // Make sure this is non-NULL and aligned let (size, align) = self.size_and_align_of(place.extra, place.layout)?; match self.memory.check_align(place.ptr, align) { From 976880aa8493cec9e16b38c24f3113f1bbd91b88 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 8 Oct 2018 14:21:41 +0200 Subject: [PATCH 18/19] dont fail when validating non-local closures --- src/librustc_mir/interpret/validity.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 0aa2e70804301..3be710ef39888 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -207,7 +207,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> bug!("Unexpected unsized type tail: {:?}", tail), } } - // for safe ptrs, recursively check + // for safe ptrs, also check the ptr values itself if !ty.is_unsafe_ptr() { // Make sure this is non-NULL and aligned let (size, align) = self.size_and_align_of(place.extra, place.layout)?; @@ -556,9 +556,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> match layout.ty.sty { // generators and closures. ty::Closure(def_id, _) | ty::Generator(def_id, _, _) => { - let node_id = self.tcx.hir.as_local_node_id(def_id).unwrap(); - let freevar = self.tcx.with_freevars(node_id, |fv| fv[field]); - PathElem::ClosureVar(self.tcx.hir.name(freevar.var_id())) + if let Some(node_id) = self.tcx.hir.as_local_node_id(def_id) { + let freevar = self.tcx.with_freevars(node_id, |fv| fv[field]); + PathElem::ClosureVar(self.tcx.hir.name(freevar.var_id())) + } else { + // The closure is not local, so we cannot get the name + PathElem::ClosureVar(Symbol::intern(&field.to_string())) + } } // tuples From fe96f8235ca4929652950d62341d3de83527de71 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 8 Oct 2018 14:34:27 +0200 Subject: [PATCH 19/19] validity: check dynamic size, not static also less verbose logging --- src/librustc_mir/interpret/validity.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 3be710ef39888..f481238bd5ba7 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -148,9 +148,6 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> ref_tracking: Option<&mut RefTracking<'tcx>>, const_mode: bool, ) -> EvalResult<'tcx> { - trace!("validate scalar by type: {:#?}, {:#?}, {}", - *value, value.layout.size, value.layout.ty); - // Go over all the primitive types let ty = value.layout.ty; match ty.sty { @@ -225,7 +222,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> } } // non-ZST also have to be dereferencable - if !place.layout.is_zst() { + if size != Size::ZERO { let ptr = try_validation!(place.ptr.to_ptr(), "integer pointer in non-ZST reference", path); if const_mode { @@ -280,7 +277,6 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> path: &Vec, layout: &layout::Scalar, ) -> EvalResult<'tcx> { - trace!("validate scalar by layout: {:#?}, {:#?}, {:#?}", value, size, layout); let (lo, hi) = layout.valid_range.clone().into_inner(); let max_hi = u128::max_value() >> (128 - size.bits()); // as big as the size fits assert!(hi <= max_hi); @@ -372,7 +368,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> mut ref_tracking: Option<&mut RefTracking<'tcx>>, const_mode: bool, ) -> EvalResult<'tcx> { - trace!("validate_operand: {:?}, {:#?}", *dest, dest.layout); + trace!("validate_operand: {:?}, {:?}", *dest, dest.layout.ty); // If this is a multi-variant layout, we have find the right one and proceed with that. // (No good reasoning to make this recursion, but it is equivalent to that.)