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

lqd · 2021-06-12T11:56:37Z

The invalid vtable size/alignment errors from InterpCx::read_size_and_align_from_vtable were "freeform const UB errors", causing ICEs when reaching validation. This PR turns them into const UB hard errors to catch them during validation and avoid that.

Fixes #86193

r? @RalfJung

(It seemed cleaner to have 2 variants but they can be merged into one variant with a message payload if you prefer that ?)

rust-highfive · 2021-06-12T11:56:39Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung · 2021-06-13T08:56:29Z

compiler/rustc_middle/src/mir/interpret/error.rs

+    /// Invalid size in a vtable: too large.
+    InvalidSize,
+    /// Invalid alignment in a vtable: too large, or not a power of 2.
+    InvalidAlignment(String),


I feel like InvalidVtable{Size,Alignment,DropFn} would be clearer names for these variants. What do you think?

sure, I'll do that ! I initially did name those specifically for vtables but then InvalidDropFn "convinced me" not to do so ^^

RalfJung · 2021-06-13T08:57:59Z

src/test/ui/consts/const-eval/ub-incorrect-vtable.rs

+// ICEs as tracked by #86193. So we also use the transparent wrapper to verify actual const UB hard
+// errors are emitted now instead of ICEs.


Suggested change

// ICEs as tracked by #86193. So we also use the transparent wrapper to verify actual const UB hard

// errors are emitted now instead of ICEs.

// ICEs as tracked by #86193. So we also use the transparent wrapper to verify proper validation

// errors are emitted instead of ICEs.

RalfJung · 2021-06-13T08:59:33Z

src/test/ui/consts/const-eval/ub-incorrect-vtable.64bit.stderr

+   |
+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


@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)

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

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. :)

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

rust/compiler/rustc_mir/src/interpret/validity.rs

Lines 27 to 47 in 3e827cc

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

rust/compiler/rustc_middle/src/mir/interpret/error.rs

Line 326 in 2779fc1

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 😓 )

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

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

RalfJung · 2021-06-13T10:54:37Z

This looks great, thanks a lot. :-)
@bors r+

bors · 2021-06-13T10:54:39Z

📌 Commit 9c7133c95bdfaf3b14798f61d40edeb1564b2ed3 has been approved by RalfJung

RalfJung · 2021-06-13T11:03:02Z

@bors r-

They were "freeform const UB" error message, but could reach validation and trigger ICEs there. We now catch them during validation to avoid that.

lqd · 2021-06-13T12:08:53Z

CI is green after actually calling rustfmt...

@bors r=RalfJung

bors · 2021-06-13T12:08:54Z

📌 Commit e29f3e8 has been approved by RalfJung

bors · 2021-06-13T12:09:03Z

⌛ Testing commit e29f3e8 with merge fb3ea63...

bors · 2021-06-13T14:49:44Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing fb3ea63 to master...

Improve CTFE UB validation error messages As mentioned in rust-lang#86245 (comment) this PR slightly improves the formatting of validation errors, to move the path to the error prefix. From: `type validation failed: encountered invalid vtable: size is bigger than largest supported object at .0` To: `type validation failed at .0: encountered invalid vtable: size is bigger than largest supported object`.

rust-highfive assigned RalfJung Jun 12, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 12, 2021

This comment has been minimized.

Sign in to view

lqd force-pushed the const-ub-align branch 2 times, most recently from c29f599 to b38e376 Compare June 12, 2021 13:50

RalfJung reviewed Jun 13, 2021

View reviewed changes

lqd force-pushed the const-ub-align branch from b38e376 to 9c7133c Compare June 13, 2021 10:47

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 13, 2021

This comment has been minimized.

Sign in to view

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 13, 2021

lqd added 2 commits June 13, 2021 13:11

Turn incorrect vtable size/alignment errors into hard const-UB errors

cae1918

They were "freeform const UB" error message, but could reach validation and trigger ICEs there. We now catch them during validation to avoid that.

Test invalid vtable size/alignment const UB errors

e29f3e8

lqd force-pushed the const-ub-align branch from 9c7133c to e29f3e8 Compare June 13, 2021 11:11

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 13, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 13, 2021

bors merged commit fb3ea63 into rust-lang:master Jun 13, 2021

rustbot added this to the 1.55.0 milestone Jun 13, 2021

lqd deleted the const-ub-align branch June 13, 2021 15:35

lqd mentioned this pull request Jun 13, 2021

Improve CTFE UB validation error messages #86275

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lqd commented Jun 12, 2021

rust-highfive commented Jun 12, 2021

This comment has been minimized.

RalfJung Jun 13, 2021

lqd Jun 13, 2021

RalfJung Jun 13, 2021

RalfJung Jun 13, 2021

lqd Jun 13, 2021

RalfJung Jun 13, 2021 •

edited

Loading

lqd Jun 13, 2021 •

edited

Loading

RalfJung Jun 13, 2021

oli-obk Jun 13, 2021

RalfJung commented Jun 13, 2021

bors commented Jun 13, 2021

This comment has been minimized.

RalfJung commented Jun 13, 2021

lqd commented Jun 13, 2021

bors commented Jun 13, 2021

bors commented Jun 13, 2021

bors commented Jun 13, 2021

		// ICEs as tracked by #86193. So we also use the transparent wrapper to verify actual const UB hard
		// errors are emitted now instead of ICEs.

	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))
	}};
	}

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

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

Conversation

lqd commented Jun 12, 2021

rust-highfive commented Jun 12, 2021

This comment has been minimized.

RalfJung Jun 13, 2021

Choose a reason for hiding this comment

lqd Jun 13, 2021

Choose a reason for hiding this comment

RalfJung Jun 13, 2021

Choose a reason for hiding this comment

RalfJung Jun 13, 2021

Choose a reason for hiding this comment

lqd Jun 13, 2021

Choose a reason for hiding this comment

RalfJung Jun 13, 2021 • edited Loading

Choose a reason for hiding this comment

lqd Jun 13, 2021 • edited Loading

Choose a reason for hiding this comment

RalfJung Jun 13, 2021

Choose a reason for hiding this comment

oli-obk Jun 13, 2021

Choose a reason for hiding this comment

RalfJung commented Jun 13, 2021

bors commented Jun 13, 2021

This comment has been minimized.

RalfJung commented Jun 13, 2021

lqd commented Jun 13, 2021

bors commented Jun 13, 2021

bors commented Jun 13, 2021

bors commented Jun 13, 2021

RalfJung Jun 13, 2021 •

edited

Loading

lqd Jun 13, 2021 •

edited

Loading