Skip to content

Commit 4971d03

Browse files
RalfJungRalfJung
committed
fix some cases of unexpected exceptions leaving validation
1 parent 938f852 commit 4971d03

File tree

13 files changed

+122
-63
lines changed

13 files changed

+122
-63
lines changed

‎src/librustc/mir/interpret/error.rs

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ fn print_backtrace(backtrace: &mut Backtrace) {
245245
eprintln!("\n\nAn error occurred in miri:\n{:?}", backtrace);
246246
}
247247

248-
impl From<ErrorHandled> for InterpErrorInfo<'tcx> {
248+
impl From<ErrorHandled> for InterpErrorInfo<'_> {
249249
fn from(err: ErrorHandled) -> Self {
250250
match err {
251251
ErrorHandled::Reported => err_inval!(ReferencedConstant),
@@ -291,7 +291,7 @@ pub enum InvalidProgramInfo<'tcx> {
291291
Layout(layout::LayoutError<'tcx>),
292292
}
293293

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

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

326-
impl fmt::Debug for UndefinedBehaviorInfo {
328+
impl fmt::Debug for UndefinedBehaviorInfo<'_> {
327329
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
328330
use UndefinedBehaviorInfo::*;
329331
match self {
@@ -338,6 +340,7 @@ impl fmt::Debug for UndefinedBehaviorInfo {
338340
DivisionByZero => write!(f, "dividing by zero"),
339341
RemainderByZero => write!(f, "calculating the remainder with a divisor of zero"),
340342
PointerArithOverflow => write!(f, "overflowing in-bounds pointer arithmetic"),
343+
InvalidMeta(msg) => write!(f, "invalid metadata in wide pointer: {}", msg),
341344
}
342345
}
343346
}
@@ -577,7 +580,7 @@ impl fmt::Debug for ResourceExhaustionInfo {
577580

578581
pub enum InterpError<'tcx> {
579582
/// The program caused undefined behavior.
580-
UndefinedBehavior(UndefinedBehaviorInfo),
583+
UndefinedBehavior(UndefinedBehaviorInfo<'tcx>),
581584
/// The program did something the interpreter does not support (some of these *might* be UB
582585
/// but the interpreter is not sure).
583586
Unsupported(UnsupportedOpInfo<'tcx>),

‎src/librustc_mir/const_eval/eval_queries.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,12 @@ fn validate_and_turn_into_const<'tcx>(
186186
if cid.promoted.is_none() {
187187
let mut ref_tracking = RefTracking::new(mplace);
188188
while let Some((mplace, path)) = ref_tracking.todo.pop() {
189-
ecx.validate_operand(mplace.into(), path, Some(&mut ref_tracking))?;
189+
ecx.const_validate_operand(
190+
mplace.into(),
191+
path,
192+
&mut ref_tracking,
193+
/*may_ref_to_static*/ is_static,
194+
)?;
190195
}
191196
}
192197
// Now that we validated, turn this into a proper constant.

‎src/librustc_mir/interpret/eval_context.rs

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -457,10 +457,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
457457

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

477474
// Make sure the slice is not too big.
478475
let size = elem.size.checked_mul(len, &*self.tcx).ok_or_else(|| {
479-
err_ub_format!(
480-
"invalid slice: \
481-
total size is bigger than largest supported object"
482-
)
476+
err_ub!(InvalidMeta("slice is bigger than largest supported object"))
483477
})?;
484478
Ok(Some((size, elem.align.abi)))
485479
}
@@ -685,7 +679,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
685679
// invariant -- that is, unless a function somehow has a ptr to
686680
// its return place... but the way MIR is currently generated, the
687681
// return place is always a local and then this cannot happen.
688-
self.validate_operand(self.place_to_op(return_place)?, vec![], None)?;
682+
self.validate_operand(self.place_to_op(return_place)?)?;
689683
}
690684
} else {
691685
// Uh, that shouldn't happen... the function did not intend to return

‎src/librustc_mir/interpret/place.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -689,7 +689,7 @@ where
689689

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

695695
Ok(())
@@ -706,7 +706,7 @@ where
706706

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

712712
Ok(())
@@ -843,7 +843,7 @@ where
843843

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

849849
Ok(())
@@ -951,7 +951,7 @@ where
951951

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

957957
Ok(())

‎src/librustc_mir/interpret/validity.rs

Lines changed: 50 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,7 @@ struct ValidityVisitor<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> {
171171
path: Vec<PathElem>,
172172
ref_tracking_for_consts:
173173
Option<&'rt mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>>,
174+
may_ref_to_static: bool,
174175
ecx: &'rt InterpCx<'mir, 'tcx, M>,
175176
}
176177

@@ -324,9 +325,17 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M
324325
self.check_wide_ptr_meta(place.meta, place.layout)?;
325326
}
326327
// Make sure this is dereferenceable and all.
327-
let (size, align) = self
328-
.ecx
329-
.size_and_align_of(place.meta, place.layout)?
328+
let size_and_align = match self.ecx.size_and_align_of(place.meta, place.layout) {
329+
Ok(res) => res,
330+
Err(err) => match err.kind {
331+
err_ub!(InvalidMeta(msg)) => throw_validation_failure!(
332+
format_args!("invalid {} metadata: {}", kind, msg),
333+
self.path
334+
),
335+
_ => bug!("Unexpected error during ptr size_and_align_of: {}", err),
336+
},
337+
};
338+
let (size, align) = size_and_align
330339
// for the purpose of validity, consider foreign types to have
331340
// alignment and size determined by the layout (size will be 0,
332341
// alignment should take attributes into account).
@@ -387,6 +396,12 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, 'tcx, M
387396
if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
388397
return Ok(());
389398
}
399+
if !self.may_ref_to_static && self.ecx.tcx.is_static(did) {
400+
throw_validation_failure!(
401+
format_args!("a {} pointing to a static variable", kind),
402+
self.path
403+
);
404+
}
390405
}
391406
}
392407
// Proceed recursively even for ZST, no reason to skip them!
@@ -781,26 +796,20 @@ impl<'rt, 'mir, 'tcx, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
781796
}
782797

783798
impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
784-
/// This function checks the data at `op`. `op` is assumed to cover valid memory if it
785-
/// is an indirect operand.
786-
/// It will error if the bits at the destination do not match the ones described by the layout.
787-
///
788-
/// `ref_tracking_for_consts` can be `None` to avoid recursive checking below references.
789-
/// This also toggles between "run-time" (no recursion) and "compile-time" (with recursion)
790-
/// validation (e.g., pointer values are fine in integers at runtime) and various other const
791-
/// specific validation checks.
792-
pub fn validate_operand(
799+
fn validate_operand_internal(
793800
&self,
794801
op: OpTy<'tcx, M::PointerTag>,
795802
path: Vec<PathElem>,
796803
ref_tracking_for_consts: Option<
797804
&mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>,
798805
>,
806+
may_ref_to_static: bool,
799807
) -> InterpResult<'tcx> {
800-
trace!("validate_operand: {:?}, {:?}", *op, op.layout.ty);
808+
trace!("validate_operand_internal: {:?}, {:?}", *op, op.layout.ty);
801809

802810
// Construct a visitor
803-
let mut visitor = ValidityVisitor { path, ref_tracking_for_consts, ecx: self };
811+
let mut visitor =
812+
ValidityVisitor { path, ref_tracking_for_consts, may_ref_to_static, ecx: self };
804813

805814
// Try to cast to ptr *once* instead of all the time.
806815
let op = self.force_op_ptr(op).unwrap_or(op);
@@ -815,4 +824,31 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
815824
Err(err) => Err(err),
816825
}
817826
}
827+
828+
/// This function checks the data at `op` to be const-valid.
829+
/// `op` is assumed to cover valid memory if it is an indirect operand.
830+
/// It will error if the bits at the destination do not match the ones described by the layout.
831+
///
832+
/// `ref_tracking` is used to record references that we encounter so that they
833+
/// can be checked recursively by an outside driving loop.
834+
///
835+
/// `may_ref_to_static` controls whether references are allowed to point to statics.
836+
#[inline(always)]
837+
pub fn const_validate_operand(
838+
&self,
839+
op: OpTy<'tcx, M::PointerTag>,
840+
path: Vec<PathElem>,
841+
ref_tracking: &mut RefTracking<MPlaceTy<'tcx, M::PointerTag>, Vec<PathElem>>,
842+
may_ref_to_static: bool,
843+
) -> InterpResult<'tcx> {
844+
self.validate_operand_internal(op, path, Some(ref_tracking), may_ref_to_static)
845+
}
846+
847+
/// This function checks the data at `op` to be runtime-valid.
848+
/// `op` is assumed to cover valid memory if it is an indirect operand.
849+
/// It will error if the bits at the destination do not match the ones described by the layout.
850+
#[inline(always)]
851+
pub fn validate_operand(&self, op: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx> {
852+
self.validate_operand_internal(op, vec![], None, false)
853+
}
818854
}

‎src/librustc_mir/transform/const_prop.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -637,11 +637,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
637637
source_info: SourceInfo,
638638
) {
639639
trace!("attepting to replace {:?} with {:?}", rval, value);
640-
if let Err(e) = self.ecx.validate_operand(
640+
if let Err(e) = self.ecx.const_validate_operand(
641641
value,
642642
vec![],
643643
// FIXME: is ref tracking too expensive?
644-
Some(&mut interpret::RefTracking::empty()),
644+
&mut interpret::RefTracking::empty(),
645+
/*may_ref_to_static*/ true,
645646
) {
646647
trace!("validation error, attempt failed: {:?}", e);
647648
return;

‎src/test/ui/consts/const-eval/dangling.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use std::{mem, usize};
66
const TEST: () = { unsafe { //~ NOTE
77
let slice: *const [u8] = mem::transmute((1usize, usize::MAX));
88
let _val = &*slice; //~ ERROR: any use of this value will cause an error
9-
//~^ NOTE: total size is bigger than largest supported object
9+
//~^ NOTE: slice is bigger than largest supported object
1010
//~^^ on by default
1111
} };
1212

‎src/test/ui/consts/const-eval/dangling.stderr

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ error: any use of this value will cause an error
44
LL | / const TEST: () = { unsafe {
55
LL | | let slice: *const [u8] = mem::transmute((1usize, usize::MAX));
66
LL | | let _val = &*slice;
7-
| | ^^^^^^^ invalid slice: total size is bigger than largest supported object
7+
| | ^^^^^^^ invalid metadata in wide pointer: slice is bigger than largest supported object
88
LL | |
99
LL | |
1010
LL | | } };

‎src/test/ui/consts/const-eval/ub-wide-ptr.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,16 @@ const STR_VALID: &str = unsafe { mem::transmute((&42u8, 1usize)) };
3131
// bad str
3232
const STR_TOO_LONG: &str = unsafe { mem::transmute((&42u8, 999usize)) };
3333
//~^ ERROR it is undefined behavior to use this value
34+
const NESTED_STR_MUCH_TOO_LONG: (&str,) = (unsafe { mem::transmute((&42, usize::MAX)) },);
35+
//~^ ERROR it is undefined behavior to use this value
3436
// bad str
3537
const STR_LENGTH_PTR: &str = unsafe { mem::transmute((&42u8, &3)) };
3638
//~^ ERROR it is undefined behavior to use this value
3739
// bad str in user-defined unsized type
3840
const MY_STR_LENGTH_PTR: &MyStr = unsafe { mem::transmute((&42u8, &3)) };
3941
//~^ ERROR it is undefined behavior to use this value
42+
const MY_STR_MUCH_TOO_LONG: &MyStr = unsafe { mem::transmute((&42u8, usize::MAX)) };
43+
//~^ ERROR it is undefined behavior to use this value
4044

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

0 commit comments

Comments
 (0)