Skip to content

Commit

Permalink
fix some cases of unexpected exceptions leaving validation
Browse files Browse the repository at this point in the history
  • Loading branch information
RalfJung committed Mar 8, 2020
1 parent 938f852 commit 4971d03
Show file tree
Hide file tree
Showing 13 changed files with 122 additions and 63 deletions.
13 changes: 8 additions & 5 deletions src/librustc/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,7 @@ fn print_backtrace(backtrace: &mut Backtrace) {
eprintln!("\n\nAn error occurred in miri:\n{:?}", backtrace);
}

impl From<ErrorHandled> for InterpErrorInfo<'tcx> {
impl From<ErrorHandled> for InterpErrorInfo<'_> {
fn from(err: ErrorHandled) -> Self {
match err {
ErrorHandled::Reported => err_inval!(ReferencedConstant),
Expand Down Expand Up @@ -291,7 +291,7 @@ pub enum InvalidProgramInfo<'tcx> {
Layout(layout::LayoutError<'tcx>),
}

impl fmt::Debug for InvalidProgramInfo<'tcx> {
impl fmt::Debug for InvalidProgramInfo<'_> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
use InvalidProgramInfo::*;
match self {
Expand All @@ -304,7 +304,7 @@ impl fmt::Debug for InvalidProgramInfo<'tcx> {
}

/// Error information for when the program caused Undefined Behavior.
pub enum UndefinedBehaviorInfo {
pub enum UndefinedBehaviorInfo<'tcx> {
/// Free-form case. Only for errors that are never caught!
Ub(String),
/// Free-form case for experimental UB. Only for errors that are never caught!
Expand All @@ -321,9 +321,11 @@ pub enum UndefinedBehaviorInfo {
RemainderByZero,
/// Overflowing inbounds pointer arithmetic.
PointerArithOverflow,
/// Invalid metadata in a wide pointer (using `str` to avoid allocations).
InvalidMeta(&'tcx str),
}

impl fmt::Debug for UndefinedBehaviorInfo {
impl fmt::Debug for UndefinedBehaviorInfo<'_> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
use UndefinedBehaviorInfo::*;
match self {
Expand All @@ -338,6 +340,7 @@ impl fmt::Debug for UndefinedBehaviorInfo {
DivisionByZero => write!(f, "dividing by zero"),
RemainderByZero => write!(f, "calculating the remainder with a divisor of zero"),
PointerArithOverflow => write!(f, "overflowing in-bounds pointer arithmetic"),
InvalidMeta(msg) => write!(f, "invalid metadata in wide pointer: {}", msg),
}
}
}
Expand Down Expand Up @@ -577,7 +580,7 @@ impl fmt::Debug for ResourceExhaustionInfo {

pub enum InterpError<'tcx> {
/// The program caused undefined behavior.
UndefinedBehavior(UndefinedBehaviorInfo),
UndefinedBehavior(UndefinedBehaviorInfo<'tcx>),
/// The program did something the interpreter does not support (some of these *might* be UB
/// but the interpreter is not sure).
Unsupported(UnsupportedOpInfo<'tcx>),
Expand Down
7 changes: 6 additions & 1 deletion src/librustc_mir/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,12 @@ fn validate_and_turn_into_const<'tcx>(
if cid.promoted.is_none() {
let mut ref_tracking = RefTracking::new(mplace);
while let Some((mplace, path)) = ref_tracking.todo.pop() {
ecx.validate_operand(mplace.into(), path, Some(&mut ref_tracking))?;
ecx.const_validate_operand(
mplace.into(),
path,
&mut ref_tracking,
/*may_ref_to_static*/ is_static,
)?;
}
}
// Now that we validated, turn this into a proper constant.
Expand Down
12 changes: 3 additions & 9 deletions src/librustc_mir/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -457,10 +457,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

// Check if this brought us over the size limit.
if size.bytes() >= self.tcx.data_layout().obj_size_bound() {
throw_ub_format!(
"wide pointer metadata contains invalid information: \
total size is bigger than largest supported object"
);
throw_ub!(InvalidMeta("total size is bigger than largest supported object"));
}
Ok(Some((size, align)))
}
Expand All @@ -476,10 +473,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {

// Make sure the slice is not too big.
let size = elem.size.checked_mul(len, &*self.tcx).ok_or_else(|| {
err_ub_format!(
"invalid slice: \
total size is bigger than largest supported object"
)
err_ub!(InvalidMeta("slice is bigger than largest supported object"))
})?;
Ok(Some((size, elem.align.abi)))
}
Expand Down Expand Up @@ -685,7 +679,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// invariant -- that is, unless a function somehow has a ptr to
// its return place... but the way MIR is currently generated, the
// return place is always a local and then this cannot happen.
self.validate_operand(self.place_to_op(return_place)?, vec![], None)?;
self.validate_operand(self.place_to_op(return_place)?)?;
}
} else {
// Uh, that shouldn't happen... the function did not intend to return
Expand Down
8 changes: 4 additions & 4 deletions src/librustc_mir/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -689,7 +689,7 @@ where

if M::enforce_validity(self) {
// Data got changed, better make sure it matches the type!
self.validate_operand(self.place_to_op(dest)?, vec![], None)?;
self.validate_operand(self.place_to_op(dest)?)?;
}

Ok(())
Expand All @@ -706,7 +706,7 @@ where

if M::enforce_validity(self) {
// Data got changed, better make sure it matches the type!
self.validate_operand(dest.into(), vec![], None)?;
self.validate_operand(dest.into())?;
}

Ok(())
Expand Down Expand Up @@ -843,7 +843,7 @@ where

if M::enforce_validity(self) {
// Data got changed, better make sure it matches the type!
self.validate_operand(self.place_to_op(dest)?, vec![], None)?;
self.validate_operand(self.place_to_op(dest)?)?;
}

Ok(())
Expand Down Expand Up @@ -951,7 +951,7 @@ where

if M::enforce_validity(self) {
// Data got changed, better make sure it matches the type!
self.validate_operand(dest.into(), vec![], None)?;
self.validate_operand(dest.into())?;
}

Ok(())
Expand Down
64 changes: 50 additions & 14 deletions src/librustc_mir/interpret/validity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ struct ValidityVisitor<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> {
path: Vec<PathElem>,
ref_tracking_for_consts:
Option<&'rt mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>>,
may_ref_to_static: bool,
ecx: &'rt InterpCx<'mir, 'tcx, M>,
}

Expand Down Expand Up @@ -324,9 +325,17 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M
self.check_wide_ptr_meta(place.meta, place.layout)?;
}
// Make sure this is dereferenceable and all.
let (size, align) = self
.ecx
.size_and_align_of(place.meta, place.layout)?
let size_and_align = match self.ecx.size_and_align_of(place.meta, place.layout) {
Ok(res) => res,
Err(err) => match err.kind {
err_ub!(InvalidMeta(msg)) => throw_validation_failure!(
format_args!("invalid {} metadata: {}", kind, msg),
self.path
),
_ => bug!("Unexpected error during ptr size_and_align_of: {}", err),
},
};
let (size, align) = size_and_align
// for the purpose of validity, consider foreign types to have
// alignment and size determined by the layout (size will be 0,
// alignment should take attributes into account).
Expand Down Expand Up @@ -387,6 +396,12 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M
if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
return Ok(());
}
if !self.may_ref_to_static && self.ecx.tcx.is_static(did) {
throw_validation_failure!(
format_args!("a {} pointing to a static variable", kind),
self.path
);
}
}
}
// Proceed recursively even for ZST, no reason to skip them!
Expand Down Expand Up @@ -781,26 +796,20 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
}

impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
/// 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.
///
/// `ref_tracking_for_consts` 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) and various other const
/// specific validation checks.
pub fn validate_operand(
fn validate_operand_internal(
&self,
op: OpTy<'tcx, M::PointerTag>,
path: Vec<PathElem>,
ref_tracking_for_consts: Option<
&mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>,
>,
may_ref_to_static: bool,
) -> InterpResult<'tcx> {
trace!("validate_operand: {:?}, {:?}", *op, op.layout.ty);
trace!("validate_operand_internal: {:?}, {:?}", *op, op.layout.ty);

// Construct a visitor
let mut visitor = ValidityVisitor { path, ref_tracking_for_consts, ecx: self };
let mut visitor =
ValidityVisitor { path, ref_tracking_for_consts, may_ref_to_static, ecx: self };

// Try to cast to ptr *once* instead of all the time.
let op = self.force_op_ptr(op).unwrap_or(op);
Expand All @@ -815,4 +824,31 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Err(err) => Err(err),
}
}

/// This function checks the data at `op` to be const-valid.
/// `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.
///
/// `ref_tracking` is used to record references that we encounter so that they
/// can be checked recursively by an outside driving loop.
///
/// `may_ref_to_static` controls whether references are allowed to point to statics.
#[inline(always)]
pub fn const_validate_operand(
&self,
op: OpTy<'tcx, M::PointerTag>,
path: Vec<PathElem>,
ref_tracking: &mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>,
may_ref_to_static: bool,
) -> InterpResult<'tcx> {
self.validate_operand_internal(op, path, Some(ref_tracking), may_ref_to_static)
}

/// This function checks the data at `op` to be runtime-valid.
/// `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.
#[inline(always)]
pub fn validate_operand(&self, op: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx> {
self.validate_operand_internal(op, vec![], None, false)
}
}
5 changes: 3 additions & 2 deletions src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -637,11 +637,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
source_info: SourceInfo,
) {
trace!("attepting to replace {:?} with {:?}", rval, value);
if let Err(e) = self.ecx.validate_operand(
if let Err(e) = self.ecx.const_validate_operand(
value,
vec![],
// FIXME: is ref tracking too expensive?
Some(&mut interpret::RefTracking::empty()),
&mut interpret::RefTracking::empty(),
/*may_ref_to_static*/ true,
) {
trace!("validation error, attempt failed: {:?}", e);
return;
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/consts/const-eval/dangling.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use std::{mem, usize};
const TEST: () = { unsafe { //~ NOTE
let slice: *const [u8] = mem::transmute((1usize, usize::MAX));
let _val = &*slice; //~ ERROR: any use of this value will cause an error
//~^ NOTE: total size is bigger than largest supported object
//~^ NOTE: slice is bigger than largest supported object
//~^^ on by default
} };

Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/consts/const-eval/dangling.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ error: any use of this value will cause an error
LL | / const TEST: () = { unsafe {
LL | | let slice: *const [u8] = mem::transmute((1usize, usize::MAX));
LL | | let _val = &*slice;
| | ^^^^^^^ invalid slice: total size is bigger than largest supported object
| | ^^^^^^^ invalid metadata in wide pointer: slice is bigger than largest supported object
LL | |
LL | |
LL | | } };
Expand Down
6 changes: 5 additions & 1 deletion src/test/ui/consts/const-eval/ub-wide-ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,16 @@ const STR_VALID: &str = unsafe { mem::transmute((&42u8, 1usize)) };
// bad str
const STR_TOO_LONG: &str = unsafe { mem::transmute((&42u8, 999usize)) };
//~^ ERROR it is undefined behavior to use this value
const NESTED_STR_MUCH_TOO_LONG: (&str,) = (unsafe { mem::transmute((&42, usize::MAX)) },);
//~^ ERROR it is undefined behavior to use this value
// bad str
const STR_LENGTH_PTR: &str = unsafe { mem::transmute((&42u8, &3)) };
//~^ ERROR it is undefined behavior to use this value
// bad str in user-defined unsized type
const MY_STR_LENGTH_PTR: &MyStr = unsafe { mem::transmute((&42u8, &3)) };
//~^ ERROR it is undefined behavior to use this value
const MY_STR_MUCH_TOO_LONG: &MyStr = unsafe { mem::transmute((&42u8, usize::MAX)) };
//~^ ERROR it is undefined behavior to use this value

// invalid UTF-8
const STR_NO_UTF8: &str = unsafe { mem::transmute::<&[u8], _>(&[0xFF]) };
Expand Down Expand Up @@ -83,7 +87,7 @@ const MYSLICE_SUFFIX_BAD: &MySliceBool = &MySlice(true, [unsafe { mem::transmute
// # raw slice
const RAW_SLICE_VALID: *const [u8] = unsafe { mem::transmute((&42u8, 1usize)) }; // ok
const RAW_SLICE_TOO_LONG: *const [u8] = unsafe { mem::transmute((&42u8, 999usize)) }; // ok because raw
const RAW_SLICE_MUCH_TOO_LONG: *const [u8] = unsafe { mem::transmute((&42u8, usize::max_value())) }; // ok because raw
const RAW_SLICE_MUCH_TOO_LONG: *const [u8] = unsafe { mem::transmute((&42u8, usize::MAX)) }; // ok because raw
const RAW_SLICE_LENGTH_UNINIT: *const [u8] = unsafe {
//~^ ERROR it is undefined behavior to use this value
let uninit_len = MaybeUninit::<usize> { uninit: () };
Expand Down
Loading

0 comments on commit 4971d03

Please sign in to comment.