Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ICEs on invalid vtable size/alignment const UB errors #86245

Merged
merged 2 commits into from
Jun 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions compiler/rustc_middle/src/mir/interpret/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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",
Expand Down
13 changes: 5 additions & 8 deletions compiler/rustc_mir/src/interpret/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}

Expand All @@ -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))
}
Expand Down
6 changes: 5 additions & 1 deletion compiler/rustc_mir/src/interpret/validity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
52 changes: 52 additions & 0 deletions src/test/ui/consts/const-eval/ub-incorrect-vtable.32bit.stderr
Original file line number Diff line number Diff line change
@@ -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 <https://github.com/rust-lang/rust/issues/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 <https://github.com/rust-lang/rust/issues/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`.
52 changes: 52 additions & 0 deletions src/test/ui/consts/const-eval/ub-incorrect-vtable.64bit.stderr
Original file line number Diff line number Diff line change
@@ -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 <https://github.com/rust-lang/rust/issues/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 <https://github.com/rust-lang/rust/issues/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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oli-obk reading this, I feel like we should reorder these error messages a bit, and make them more like

type validation failed at .0: encountered invalid vtable: size is bigger than largest supported object

What do you think? (@lqd certainly not for this PR though)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you and oli want this, I can certainly do that in this PR

Copy link
Member

@RalfJung RalfJung Jun 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will affect many tests, so it should be a separate PR. Reviewing is also much easier if we keep PRs small. But sure, if you want to help with this that'd be great. :)

Copy link
Member Author

@lqd lqd Jun 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I'll do that in another PR.

Would something like this work for you ? Changing ValidationFailure's content from a String message to a path: Option<String>, mesg: String so that

macro_rules! throw_validation_failure {
($where:expr, { $( $what_fmt:expr ),+ } $( expected { $( $expected_fmt:expr ),+ } )?) => {{
let msg = rustc_middle::ty::print::with_no_trimmed_paths(|| {
let mut msg = String::new();
msg.push_str("encountered ");
write!(&mut msg, $($what_fmt),+).unwrap();
let where_ = &$where;
if !where_.is_empty() {
msg.push_str(" at ");
write_path(&mut msg, where_);
}
$(
msg.push_str(", but expected ");
write!(&mut msg, $($expected_fmt),+).unwrap();
)?
msg
});
throw_ub!(ValidationFailure(msg))
}};
}
can handle the empty "where", and the formatting you want can be implemented at
ValidationFailure(ref err) => write!(f, "type validation failed: {}", err),


(I don't know when I'll get to this though: I'm currently having trouble executing some 32bits tests of the master branch, which fail with syntax errors when executed in the test runner but not by themselves, making blessing tests rather ... difficult. Thankfully it did not happen on the test in this PR 😓 )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss the implementation details elsewhere (possibly on Zulip), if/when @oli-obk agrees that we even want to do this. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the proposed ordering is better than the current one

|
= 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`.
25 changes: 25 additions & 0 deletions src/test/ui/consts/const-eval/ub-incorrect-vtable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}

Expand All @@ -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>(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() {}
27 changes: 0 additions & 27 deletions src/test/ui/consts/const-eval/ub-incorrect-vtable.stderr

This file was deleted.