Detect incorrect vtable alignment during const eval

[lqd]

This PR fixes #86132 by detecting invalid alignment values for trait objects in the interpreter, and emitting an error about this conversion failure, to avoid the ICE.

I've noticed that the error emitted at

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

Lines 163 to 166 in a50d721

	throw_ub_format!(
	"invalid vtable: \
	size is bigger than largest supported object"
	);

doesn't seem to be present in the const-ub tests, so I've tried adding a test that triggers both of these cases: one for the invalid size, and another for the invalid alignment that #86132 tracks (I have found different magic values triggering different Align::from_bytes errors than the "power of 2" one, if need be).

However, when doing that, I cannot for the life of me figure out the correct incantation to make these 2 errors trigger with the "it is undefined behavior to use this value" message rather than the "any use of this value will cause an error" lint.

I've tried Oli's suggestions of different values, tuples and arrays, using the transparent wrapper trick from the other tests and I was only able to trigger the regular const-ub errors about the size of the vtable, or that the drop pointer was invalid. Maybe these "type validation failed" errors happen before this part of the interpreter is reached and there just needs some magic incorrect values to bypass them, I don't know.

Since this fixes an ICE, and if the constants are indeed used, these 2 tests will turn into a hard error, I thought I'd open the PR anyways. And if @RalfJung you know of a way I could manage that (if you think that these tests are worth checking that the throw_ub_format! does indeed create const-ub errors as we expect) I'd be grateful.

For that reason, r? @RalfJung and cc @oli-obk.

[rust-highfive]

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

[RalfJung]

However, when doing that, I cannot for the life of me figure out the correct incantation to make these 2 errors trigger with the "it is undefined behavior to use this value" message rather than the "any use of this value will cause an error" lint.

This is the error I expected -- it is a UB message coming from regular execution, not part of validation. If you can trigger this bug in validation that will likely ICE as the read_size_and_align_from_vtable call currently does not catch these UB messages...

I think what happens is that already during interning (or even earlier), we try to read the vtable and hence run into the "lint" error path. This means we never even reach validation where we would show a hard error and a different message. You could try to set RUSTC_CTFE_BACKTRACE=1 to see where in the CTFE engine the error originates; the backtrace should tell you which pass of const-evaluation is triggering this.

[lqd]

I think what happens is that already during interning (or even earlier), we try to read the vtable and hence run into the "lint" error path. This means we never even reach validation where we would show a hard error and a different message. You could try to set RUSTC_CTFE_BACKTRACE=1 to see where in the CTFE engine the error originates; the backtrace should tell you which pass of const-evaluation is triggering this.

I'm not super versed in miri traces but there's indeed a difference that seems to match your intuition:

• the UB error message added by this PR seems triggered by what I believe I would call "regular const evaluation" ^^ (full trace here)
• while the hard errors -- which I believe are the ones with actual UndefinedBehaviorInfos -- happen during validation (full trace here)

[RalfJung]

while the hard errors -- which I believe are the ones with actual UndefinedBehaviorInfos -- happen during validation

Their distinguishing factor is that they are UndefinedBehaviorInfo::ValidationFailure, which are raised during a second phase, after actual const-evalaution, which handles errors differently.

the UB error message added by this PR seems triggered by what I believe I would call "regular const evaluation"

Yeah, looks like something is dereferencing this reference; I assume there is an implicit reborrow happening somewhere.

I think you should totally keep these testcases, but here's an idea for another one that might trigger a ValidationFailure:

#[repr(transparent)]
struct W<T>(T);

const INVALID_VTABLE_SIZE: W<&dyn Send> =
    unsafe { std::mem::transmute((&92u8, &[1usize, usize::MAX, 0usize])) };

The wrapping W prevents the compiler from introducing reborrows, which avoids erroring early during evalutaion, so instead now we error late during validation. We error due to a bad drop pointer though, as you said -- which is correct, 1usize is indeed a bad drop pointer. ;)

So let's fabricate a proper drop pointer. In fact, here's how to cause an ICE since this kind of error is not handled properly 😂

#[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_SIZE: W<&dyn Send> =
    unsafe { std::mem::transmute((&92u8, &(drop_me as fn(*mut usize), usize::MAX, 1usize))) };

[lqd]

I think you should totally keep these testcases, but here's an idea for another one that might trigger a ValidationFailure:

#[repr(transparent)]
struct W<T>(T);

const INVALID_VTABLE_SIZE: W<&dyn Send> =
    unsafe { std::mem::transmute((&92u8, &[1usize, usize::MAX, 0usize])) };

The wrapping W prevents the compiler from introducing reborrows, which avoids erroring early during evalutaion, so instead now we error late during validation. We error due to a bad drop pointer though, as you said -- which is correct, 1usize is indeed a bad drop pointer. ;)

yes absolutely, this is what I was alluding to (very obscurely) in the OP: I've seen this "transparent wrapper trick" to prevent reborrows, in the ub-wide-ptr.rs test. With the test cases above triggering the lint errors, I had tried using W and it does indeed create validation failures about the drop pointer -- and this is all I was able to get. I was unfortunately expecting/hoping for a validation failure with the lint mesgs... and that seems more easily doable with the ICE you've found 😂.

I didn't end up adding a test case like this one because invalid drop pointers seemed well tested.

[lqd]

At least validation is honest about why it ICEs :)

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

Lines 949 to 954 in eab201d

	// Avoid other errors as those do not show *where* in the value the issue lies.
	Err(err) => {
	err.print_backtrace();
	bug!("Unexpected error during validation: {}", err);
	}
	}

[RalfJung]

Right, so we should fix that ICE and I can give you some hints for how to do that, but independent of that I think we can first land this one here. Thanks for the patch!

@bors r+ rollup

[bors]

📌 Commit d449903 has been approved by RalfJung