From 54d95ed25aa45f94b2a3d0a0e3a3323852878ecd Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 5 Jul 2020 12:14:12 +0200 Subject: [PATCH 1/4] catch InvalidUninitBytes during validation --- src/librustc_mir/interpret/validity.rs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 3bb9ba3712058..058c9ffc37ecf 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -366,7 +366,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' let place = try_validation!( self.ecx.ref_to_mplace(value), self.path, - err_ub!(InvalidUninitBytes { .. }) => { "uninitialized {}", kind }, + err_ub!(InvalidUninitBytes(None)) => { "uninitialized {}", kind }, ); if place.layout.is_unsized() { self.check_wide_ptr_meta(place.meta, place.layout)?; @@ -477,7 +477,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' try_validation!( value.to_bool(), self.path, - err_ub!(InvalidBool(..)) => { "{}", value } expected { "a boolean" }, + err_ub!(InvalidBool(..)) | err_ub!(InvalidUninitBytes(None)) => + { "{}", value } expected { "a boolean" }, ); Ok(true) } @@ -486,7 +487,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' try_validation!( value.to_char(), self.path, - err_ub!(InvalidChar(..)) => { "{}", value } expected { "a valid unicode scalar value (in `0..=0x10FFFF` but not in `0xD800..=0xDFFF`)" }, + err_ub!(InvalidChar(..)) | err_ub!(InvalidUninitBytes(None)) => + { "{}", value } expected { "a valid unicode scalar value (in `0..=0x10FFFF` but not in `0xD800..=0xDFFF`)" }, ); Ok(true) } @@ -515,7 +517,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' let place = try_validation!( self.ecx.ref_to_mplace(self.ecx.read_immediate(value)?), self.path, - err_ub!(InvalidUninitBytes { .. } ) => { "uninitialized raw pointer" }, + err_ub!(InvalidUninitBytes(None)) => { "uninitialized raw pointer" }, ); if place.layout.is_unsized() { self.check_wide_ptr_meta(place.meta, place.layout)?; @@ -537,6 +539,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' self.path, err_ub!(DanglingIntPointer(..)) | err_ub!(InvalidFunctionPointer(..)) | + err_ub!(InvalidUninitBytes(None)) | err_unsup!(ReadBytesAsPointer) => { "{}", value } expected { "a function pointer" }, ); @@ -593,7 +596,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' let value = try_validation!( value.not_undef(), self.path, - err_ub!(InvalidUninitBytes { .. }) => { "{}", value } + err_ub!(InvalidUninitBytes(None)) => { "{}", value } expected { "something {}", wrapping_range_format(valid_range, max_hi) }, ); let bits = match value.to_bits_or_ptr(op.layout.size, self.ecx) { @@ -699,6 +702,9 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> self.path, err_ub!(InvalidTag(val)) => { "{}", val } expected { "a valid enum tag" }, + // `InvalidUninitBytes` can be caused by `read_discriminant` in Miri if all initialized tags are valid. + err_ub!(InvalidUninitBytes(None)) => + { "uninitialized bytes" } expected { "a valid enum tag" }, err_unsup!(ReadPointerAsBytes) => { "a pointer" } expected { "plain (non-pointer) bytes" }, ); From c3fc4f0420b642a0e0041bb13492df2d4a68ae80 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 5 Jul 2020 13:40:27 +0200 Subject: [PATCH 2/4] catch errors more locally around read_discriminant --- src/librustc_mir/interpret/validity.rs | 50 +++++++++++-------- src/librustc_mir/interpret/visitor.rs | 11 +++- .../ui/consts/const-eval/double_check2.stderr | 2 +- src/test/ui/consts/const-eval/ub-enum.stderr | 4 +- 4 files changed, 43 insertions(+), 24 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 058c9ffc37ecf..2e28ed0f91765 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -276,19 +276,21 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' } } - fn visit_elem( + fn with_elem( &mut self, - new_op: OpTy<'tcx, M::PointerTag>, elem: PathElem, - ) -> InterpResult<'tcx> { + f: impl FnOnce(&mut Self) -> InterpResult<'tcx, R>, + ) -> InterpResult<'tcx, R> { // Remember the old state let path_len = self.path.len(); - // Perform operation + // Record new element self.path.push(elem); - self.visit_value(new_op)?; + // Perform operation + let r = f(self)?; // Undo changes self.path.truncate(path_len); - Ok(()) + // Done + Ok(r) } fn check_wide_ptr_meta( @@ -649,6 +651,21 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> &self.ecx } + fn read_discriminant(&mut self, op: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx, VariantIdx> { + self.with_elem(PathElem::EnumTag, move |this| { + Ok(try_validation!( + this.ecx.read_discriminant(op), + this.path, + err_ub!(InvalidTag(val)) => + { "{}", val } expected { "a valid enum tag" }, + err_ub!(InvalidUninitBytes(None)) => + { "uninitialized bytes" } expected { "a valid enum tag" }, + err_unsup!(ReadPointerAsBytes) => + { "a pointer" } expected { "a valid enum tag" }, + ).1) + }) + } + #[inline] fn visit_field( &mut self, @@ -657,7 +674,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> new_op: OpTy<'tcx, M::PointerTag>, ) -> InterpResult<'tcx> { let elem = self.aggregate_field_path_elem(old_op.layout, field); - self.visit_elem(new_op, elem) + self.with_elem(elem, move |this| this.visit_value(new_op)) } #[inline] @@ -673,7 +690,7 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> ty::Generator(..) => PathElem::GeneratorState(variant_id), _ => bug!("Unexpected type with variant: {:?}", old_op.layout.ty), }; - self.visit_elem(new_op, name) + self.with_elem(name, move |this| this.visit_value(new_op)) } #[inline(always)] @@ -696,18 +713,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> // Sanity check: `builtin_deref` does not know any pointers that are not primitive. assert!(op.layout.ty.builtin_deref(true).is_none()); - // Recursively walk the type. Translate some possible errors to something nicer. - try_validation!( - self.walk_value(op), - self.path, - err_ub!(InvalidTag(val)) => - { "{}", val } expected { "a valid enum tag" }, - // `InvalidUninitBytes` can be caused by `read_discriminant` in Miri if all initialized tags are valid. - err_ub!(InvalidUninitBytes(None)) => - { "uninitialized bytes" } expected { "a valid enum tag" }, - err_unsup!(ReadPointerAsBytes) => - { "a pointer" } expected { "plain (non-pointer) bytes" }, - ); + // Recursively walk the value at its type. + self.walk_value(op)?; // *After* all of this, check the ABI. We need to check the ABI to handle // types like `NonNull` where the `Scalar` info is more restrictive than what @@ -822,6 +829,9 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> throw_validation_failure!(self.path, { "uninitialized bytes" }) } + err_unsup!(ReadPointerAsBytes) => + throw_validation_failure!(self.path, { "a pointer" } expected { "plain (non-pointer) bytes" }), + // Propagate upwards (that will also check for unexpected errors). _ => return Err(err), } diff --git a/src/librustc_mir/interpret/visitor.rs b/src/librustc_mir/interpret/visitor.rs index 903aa377a3d7d..6c53df40a7c9a 100644 --- a/src/librustc_mir/interpret/visitor.rs +++ b/src/librustc_mir/interpret/visitor.rs @@ -125,6 +125,15 @@ macro_rules! make_value_visitor { fn ecx(&$($mutability)? self) -> &$($mutability)? InterpCx<'mir, 'tcx, M>; + /// `read_discriminant` can be hooked for better error messages. + #[inline(always)] + fn read_discriminant( + &mut self, + op: OpTy<'tcx, M::PointerTag>, + ) -> InterpResult<'tcx, VariantIdx> { + Ok(self.ecx().read_discriminant(op)?.1) + } + // Recursive actions, ready to be overloaded. /// Visits the given value, dispatching as appropriate to more specialized visitors. #[inline(always)] @@ -245,7 +254,7 @@ macro_rules! make_value_visitor { // with *its* fields. Variants::Multiple { .. } => { let op = v.to_op(self.ecx())?; - let idx = self.ecx().read_discriminant(op)?.1; + let idx = self.read_discriminant(op)?; let inner = v.project_downcast(self.ecx(), idx)?; trace!("walk_value: variant layout: {:#?}", inner.layout()); // recurse with the inner type diff --git a/src/test/ui/consts/const-eval/double_check2.stderr b/src/test/ui/consts/const-eval/double_check2.stderr index 93dd9a53ec99f..513b71f0c6fdc 100644 --- a/src/test/ui/consts/const-eval/double_check2.stderr +++ b/src/test/ui/consts/const-eval/double_check2.stderr @@ -5,7 +5,7 @@ LL | / static FOO: (&Foo, &Bar) = unsafe {( LL | | Union { u8: &BAR }.foo, LL | | Union { u8: &BAR }.bar, LL | | )}; - | |___^ type validation failed: encountered 0x05 at .1., but expected a valid enum tag + | |___^ type validation failed: encountered 0x05 at .1.., but expected a valid enum tag | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. diff --git a/src/test/ui/consts/const-eval/ub-enum.stderr b/src/test/ui/consts/const-eval/ub-enum.stderr index 1f7593c6db9b6..217bfb628a018 100644 --- a/src/test/ui/consts/const-eval/ub-enum.stderr +++ b/src/test/ui/consts/const-eval/ub-enum.stderr @@ -2,7 +2,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-enum.rs:24:1 | LL | const BAD_ENUM: Enum = unsafe { mem::transmute(1usize) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0x00000001, but expected a valid enum tag + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0x00000001 at ., but expected a valid enum tag | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. @@ -26,7 +26,7 @@ error[E0080]: it is undefined behavior to use this value --> $DIR/ub-enum.rs:42:1 | LL | const BAD_ENUM2: Enum2 = unsafe { mem::transmute(0usize) }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0x00000000, but expected a valid enum tag + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0x00000000 at ., but expected a valid enum tag | = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior. From 751b594cc843a28c660a662db1a822a759af9603 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 5 Jul 2020 13:48:06 +0200 Subject: [PATCH 3/4] const validation: add test for uninit bool --- src/test/ui/consts/const-eval/union-ub.rs | 3 +++ src/test/ui/consts/const-eval/union-ub.stderr | 12 ++++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/test/ui/consts/const-eval/union-ub.rs b/src/test/ui/consts/const-eval/union-ub.rs index 848826e6ef7f2..512359f5b1c38 100644 --- a/src/test/ui/consts/const-eval/union-ub.rs +++ b/src/test/ui/consts/const-eval/union-ub.rs @@ -2,6 +2,7 @@ #[repr(C)] union DummyUnion { + unit: (), u8: u8, bool: bool, } @@ -30,6 +31,8 @@ union Bar { // the value is not valid for bools const BAD_BOOL: bool = unsafe { DummyUnion { u8: 42 }.bool}; //~^ ERROR it is undefined behavior to use this value +const UNINIT_BOOL: bool = unsafe { DummyUnion { unit: () }.bool}; +//~^ ERROR it is undefined behavior to use this value // The value is not valid for any union variant, but that's fine // unions are just a convenient way to transmute bits around diff --git a/src/test/ui/consts/const-eval/union-ub.stderr b/src/test/ui/consts/const-eval/union-ub.stderr index fd3e66765c61b..e8869d0d76c1c 100644 --- a/src/test/ui/consts/const-eval/union-ub.stderr +++ b/src/test/ui/consts/const-eval/union-ub.stderr @@ -1,11 +1,19 @@ error[E0080]: it is undefined behavior to use this value - --> $DIR/union-ub.rs:31:1 + --> $DIR/union-ub.rs:32:1 | LL | const BAD_BOOL: bool = unsafe { DummyUnion { u8: 42 }.bool}; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0x2a, 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 rustc repository if you believe it should not be considered undefined behavior. -error: aborting due to previous error +error[E0080]: it is undefined behavior to use this value + --> $DIR/union-ub.rs:34:1 + | +LL | const UNINIT_BOOL: bool = unsafe { DummyUnion { unit: () }.bool}; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered uninitialized bytes, 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 rustc 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`. From 319c7f77dedf671b01194adb257dbe662c73df32 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 5 Jul 2020 16:01:18 +0200 Subject: [PATCH 4/4] fmt --- src/librustc_mir/interpret/validity.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs index 2e28ed0f91765..ab836595a7acc 100644 --- a/src/librustc_mir/interpret/validity.rs +++ b/src/librustc_mir/interpret/validity.rs @@ -651,7 +651,10 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> &self.ecx } - fn read_discriminant(&mut self, op: OpTy<'tcx, M::PointerTag>) -> InterpResult<'tcx, VariantIdx> { + fn read_discriminant( + &mut self, + op: OpTy<'tcx, M::PointerTag>, + ) -> InterpResult<'tcx, VariantIdx> { self.with_elem(PathElem::EnumTag, move |this| { Ok(try_validation!( this.ecx.read_discriminant(op), @@ -662,7 +665,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> { "uninitialized bytes" } expected { "a valid enum tag" }, err_unsup!(ReadPointerAsBytes) => { "a pointer" } expected { "a valid enum tag" }, - ).1) + ) + .1) }) } @@ -829,8 +833,9 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M> throw_validation_failure!(self.path, { "uninitialized bytes" }) } - err_unsup!(ReadPointerAsBytes) => - throw_validation_failure!(self.path, { "a pointer" } expected { "plain (non-pointer) bytes" }), + err_unsup!(ReadPointerAsBytes) => { + throw_validation_failure!(self.path, { "a pointer" } expected { "plain (non-pointer) bytes" }) + } // Propagate upwards (that will also check for unexpected errors). _ => return Err(err),