From cae1918b2939824e4dbbba003099c0ccf21715e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Sat, 12 Jun 2021 13:13:38 +0200 Subject: [PATCH 1/2] Turn incorrect vtable size/alignment errors into hard const-UB errors They were "freeform const UB" error message, but could reach validation and trigger ICEs there. We now catch them during validation to avoid that. --- compiler/rustc_middle/src/mir/interpret/error.rs | 12 ++++++++++-- compiler/rustc_mir/src/interpret/traits.rs | 13 +++++-------- compiler/rustc_mir/src/interpret/validity.rs | 6 +++++- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/compiler/rustc_middle/src/mir/interpret/error.rs b/compiler/rustc_middle/src/mir/interpret/error.rs index 65d9c1dd90efb..aca39d438c103 100644 --- a/compiler/rustc_middle/src/mir/interpret/error.rs +++ b/compiler/rustc_middle/src/mir/interpret/error.rs @@ -227,7 +227,11 @@ pub enum UndefinedBehaviorInfo<'tcx> { /// Invalid metadata in a wide pointer (using `str` to avoid allocations). InvalidMeta(&'static str), /// Invalid drop function in vtable. - InvalidDropFn(FnSig<'tcx>), + InvalidVtableDropFn(FnSig<'tcx>), + /// Invalid size in a vtable: too large. + InvalidVtableSize, + /// Invalid alignment in a vtable: too large, or not a power of 2. + InvalidVtableAlignment(String), /// Reading a C string that does not end within its allocation. UnterminatedCString(Pointer), /// Dereferencing a dangling pointer after it got freed. @@ -287,11 +291,15 @@ impl fmt::Display for UndefinedBehaviorInfo<'_> { 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), - InvalidDropFn(sig) => write!( + InvalidVtableDropFn(sig) => write!( f, "invalid drop function signature: got {}, expected exactly one argument which must be a pointer type", sig ), + InvalidVtableSize => { + write!(f, "invalid vtable: size is bigger than largest supported object") + } + InvalidVtableAlignment(msg) => write!(f, "invalid vtable: alignment {}", msg), UnterminatedCString(p) => write!( f, "reading a null-terminated string starting at {} with no null found before end of allocation", diff --git a/compiler/rustc_mir/src/interpret/traits.rs b/compiler/rustc_mir/src/interpret/traits.rs index d0c04b5b414eb..9a59161f08f50 100644 --- a/compiler/rustc_mir/src/interpret/traits.rs +++ b/compiler/rustc_mir/src/interpret/traits.rs @@ -137,9 +137,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { // The drop function takes `*mut T` where `T` is the type being dropped, so get that. let args = fn_sig.inputs(); if args.len() != 1 { - throw_ub!(InvalidDropFn(fn_sig)); + throw_ub!(InvalidVtableDropFn(fn_sig)); } - let ty = args[0].builtin_deref(true).ok_or_else(|| err_ub!(InvalidDropFn(fn_sig)))?.ty; + let ty = + args[0].builtin_deref(true).ok_or_else(|| err_ub!(InvalidVtableDropFn(fn_sig)))?.ty; Ok((drop_instance, ty)) } @@ -158,14 +159,10 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { let size = u64::try_from(self.force_bits(size, pointer_size)?).unwrap(); let align = vtable.read_ptr_sized(pointer_size * 2)?.check_init()?; let align = u64::try_from(self.force_bits(align, pointer_size)?).unwrap(); - let align = Align::from_bytes(align) - .map_err(|e| err_ub_format!("invalid vtable: alignment {}", e))?; + let align = Align::from_bytes(align).map_err(|e| err_ub!(InvalidVtableAlignment(e)))?; if size >= self.tcx.data_layout.obj_size_bound() { - throw_ub_format!( - "invalid vtable: \ - size is bigger than largest supported object" - ); + throw_ub!(InvalidVtableSize); } Ok((Size::from_bytes(size), align)) } diff --git a/compiler/rustc_mir/src/interpret/validity.rs b/compiler/rustc_mir/src/interpret/validity.rs index fb165a991bceb..c9ebffe8d1cf4 100644 --- a/compiler/rustc_mir/src/interpret/validity.rs +++ b/compiler/rustc_mir/src/interpret/validity.rs @@ -349,12 +349,16 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, ' err_ub!(InvalidFunctionPointer(..)) | err_unsup!(ReadBytesAsPointer) => { "invalid drop function pointer in vtable (not pointing to a function)" }, - err_ub!(InvalidDropFn(..)) => + err_ub!(InvalidVtableDropFn(..)) => { "invalid drop function pointer in vtable (function has incompatible signature)" }, ); try_validation!( self.ecx.read_size_and_align_from_vtable(vtable), self.path, + err_ub!(InvalidVtableSize) => + { "invalid vtable: size is bigger than largest supported object" }, + err_ub!(InvalidVtableAlignment(msg)) => + { "invalid vtable: alignment {}", msg }, err_unsup!(ReadPointerAsBytes) => { "invalid size or align in vtable" }, ); // FIXME: More checks for the vtable. From e29f3e837fb6262040d42d4cde8e2f775dee0fe7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Rakic?= Date: Sat, 12 Jun 2021 13:15:23 +0200 Subject: [PATCH 2/2] Test invalid vtable size/alignment const UB errors --- .../ub-incorrect-vtable.32bit.stderr | 52 +++++++++++++++++++ .../ub-incorrect-vtable.64bit.stderr | 52 +++++++++++++++++++ .../consts/const-eval/ub-incorrect-vtable.rs | 25 +++++++++ .../const-eval/ub-incorrect-vtable.stderr | 27 ---------- 4 files changed, 129 insertions(+), 27 deletions(-) create mode 100644 src/test/ui/consts/const-eval/ub-incorrect-vtable.32bit.stderr create mode 100644 src/test/ui/consts/const-eval/ub-incorrect-vtable.64bit.stderr delete mode 100644 src/test/ui/consts/const-eval/ub-incorrect-vtable.stderr diff --git a/src/test/ui/consts/const-eval/ub-incorrect-vtable.32bit.stderr b/src/test/ui/consts/const-eval/ub-incorrect-vtable.32bit.stderr new file mode 100644 index 0000000000000..a0b449657da76 --- /dev/null +++ b/src/test/ui/consts/const-eval/ub-incorrect-vtable.32bit.stderr @@ -0,0 +1,52 @@ +error: any use of this value will cause an error + --> $DIR/ub-incorrect-vtable.rs:19:14 + | +LL | / const INVALID_VTABLE_ALIGNMENT: &dyn Trait = +LL | | unsafe { std::mem::transmute((&92u8, &[0usize, 1usize, 1000usize])) }; + | |______________^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^__- + | | + | invalid vtable: alignment `1000` is not a power of 2 + | + = note: `#[deny(const_err)]` on by default + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #71800 + +error: any use of this value will cause an error + --> $DIR/ub-incorrect-vtable.rs:25:14 + | +LL | / const INVALID_VTABLE_SIZE: &dyn Trait = +LL | | unsafe { std::mem::transmute((&92u8, &[1usize, usize::MAX, 1usize])) }; + | |______________^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^__- + | | + | invalid vtable: size is bigger than largest supported object + | + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #71800 + +error[E0080]: it is undefined behavior to use this value + --> $DIR/ub-incorrect-vtable.rs:36:1 + | +LL | / const INVALID_VTABLE_ALIGNMENT_UB: W<&dyn Trait> = +LL | | unsafe { std::mem::transmute((&92u8, &(drop_me as fn(*mut usize), 1usize, 1000usize))) }; + | |_____________________________________________________________________________________________^ type validation failed: encountered invalid vtable: alignment `1000` is not a power of 2 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 rustc repository if you believe it should not be considered undefined behavior. + = note: the raw bytes of the constant (size: 8, align: 4) { + ╾─allocN─╼ ╾─allocN─╼ │ ╾──╼╾──╼ + } + +error[E0080]: it is undefined behavior to use this value + --> $DIR/ub-incorrect-vtable.rs:41:1 + | +LL | / const INVALID_VTABLE_SIZE_UB: W<&dyn Trait> = +LL | | unsafe { std::mem::transmute((&92u8, &(drop_me as fn(*mut usize), usize::MAX, 1usize))) }; + | |______________________________________________________________________________________________^ type validation failed: encountered invalid vtable: size is bigger than largest supported object 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 rustc repository if you believe it should not be considered undefined behavior. + = note: the raw bytes of the constant (size: 8, align: 4) { + ╾─allocN─╼ ╾─allocN─╼ │ ╾──╼╾──╼ + } + +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/ub-incorrect-vtable.64bit.stderr b/src/test/ui/consts/const-eval/ub-incorrect-vtable.64bit.stderr new file mode 100644 index 0000000000000..70ae5e0a8c7e2 --- /dev/null +++ b/src/test/ui/consts/const-eval/ub-incorrect-vtable.64bit.stderr @@ -0,0 +1,52 @@ +error: any use of this value will cause an error + --> $DIR/ub-incorrect-vtable.rs:19:14 + | +LL | / const INVALID_VTABLE_ALIGNMENT: &dyn Trait = +LL | | unsafe { std::mem::transmute((&92u8, &[0usize, 1usize, 1000usize])) }; + | |______________^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^__- + | | + | invalid vtable: alignment `1000` is not a power of 2 + | + = note: `#[deny(const_err)]` on by default + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #71800 + +error: any use of this value will cause an error + --> $DIR/ub-incorrect-vtable.rs:25:14 + | +LL | / const INVALID_VTABLE_SIZE: &dyn Trait = +LL | | unsafe { std::mem::transmute((&92u8, &[1usize, usize::MAX, 1usize])) }; + | |______________^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^__- + | | + | invalid vtable: size is bigger than largest supported object + | + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #71800 + +error[E0080]: it is undefined behavior to use this value + --> $DIR/ub-incorrect-vtable.rs:36:1 + | +LL | / const INVALID_VTABLE_ALIGNMENT_UB: W<&dyn Trait> = +LL | | unsafe { std::mem::transmute((&92u8, &(drop_me as fn(*mut usize), 1usize, 1000usize))) }; + | |_____________________________________________________________________________________________^ type validation failed: encountered invalid vtable: alignment `1000` is not a power of 2 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 rustc repository if you believe it should not be considered undefined behavior. + = note: the raw bytes of the constant (size: 16, align: 8) { + ╾───────allocN───────╼ ╾───────allocN───────╼ │ ╾──────╼╾──────╼ + } + +error[E0080]: it is undefined behavior to use this value + --> $DIR/ub-incorrect-vtable.rs:41:1 + | +LL | / const INVALID_VTABLE_SIZE_UB: W<&dyn Trait> = +LL | | unsafe { std::mem::transmute((&92u8, &(drop_me as fn(*mut usize), usize::MAX, 1usize))) }; + | |______________________________________________________________________________________________^ type validation failed: encountered invalid vtable: size is bigger than largest supported object 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 rustc repository if you believe it should not be considered undefined behavior. + = note: the raw bytes of the constant (size: 16, align: 8) { + ╾───────allocN───────╼ ╾───────allocN───────╼ │ ╾──────╼╾──────╼ + } + +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/ub-incorrect-vtable.rs b/src/test/ui/consts/const-eval/ub-incorrect-vtable.rs index 0c0e3682de4d6..7c514e804e01a 100644 --- a/src/test/ui/consts/const-eval/ub-incorrect-vtable.rs +++ b/src/test/ui/consts/const-eval/ub-incorrect-vtable.rs @@ -3,6 +3,15 @@ // triggers an error // - a similar test that triggers a previously-untested const UB error: emitted close to the above // error, it checks the correctness of the size +// +// As is, this code will only hard error when the constants are used, and the errors are emitted via +// the `#[allow]`-able `const_err` lint. However, if the transparent wrapper technique to prevent +// reborrows is used -- from `ub-wide-ptr.rs` -- these two errors reach validation and would trigger +// ICEs as tracked by #86193. So we also use the transparent wrapper to verify proper validation +// errors are emitted instead of ICEs. + +// stderr-per-bitwidth +// normalize-stderr-test "alloc\d+" -> "allocN" trait Trait {} @@ -18,4 +27,20 @@ const INVALID_VTABLE_SIZE: &dyn Trait = //~| WARNING this was previously accepted by the compiler //~| invalid vtable: size is bigger than largest supported object +#[repr(transparent)] +struct W(T); + +// The drop fn is checked before size/align are, so get ourselves a "sufficiently valid" drop fn +fn drop_me(_: *mut usize) {} + +const INVALID_VTABLE_ALIGNMENT_UB: W<&dyn Trait> = + unsafe { std::mem::transmute((&92u8, &(drop_me as fn(*mut usize), 1usize, 1000usize))) }; +//~^^ ERROR it is undefined behavior to use this value +//~| invalid vtable: alignment `1000` is not a power of 2 + +const INVALID_VTABLE_SIZE_UB: W<&dyn Trait> = + unsafe { std::mem::transmute((&92u8, &(drop_me as fn(*mut usize), usize::MAX, 1usize))) }; +//~^^ ERROR it is undefined behavior to use this value +//~| invalid vtable: size is bigger than largest supported object + fn main() {} diff --git a/src/test/ui/consts/const-eval/ub-incorrect-vtable.stderr b/src/test/ui/consts/const-eval/ub-incorrect-vtable.stderr deleted file mode 100644 index c937d039d381e..0000000000000 --- a/src/test/ui/consts/const-eval/ub-incorrect-vtable.stderr +++ /dev/null @@ -1,27 +0,0 @@ -error: any use of this value will cause an error - --> $DIR/ub-incorrect-vtable.rs:10:14 - | -LL | / const INVALID_VTABLE_ALIGNMENT: &dyn Trait = -LL | | unsafe { std::mem::transmute((&92u8, &[0usize, 1usize, 1000usize])) }; - | |______________^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^__- - | | - | invalid vtable: alignment `1000` is not a power of 2 - | - = note: `#[deny(const_err)]` on by default - = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! - = note: for more information, see issue #71800 - -error: any use of this value will cause an error - --> $DIR/ub-incorrect-vtable.rs:16:14 - | -LL | / const INVALID_VTABLE_SIZE: &dyn Trait = -LL | | unsafe { std::mem::transmute((&92u8, &[1usize, usize::MAX, 1usize])) }; - | |______________^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^__- - | | - | invalid vtable: size is bigger than largest supported object - | - = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! - = note: for more information, see issue #71800 - -error: aborting due to 2 previous errors -