Repository navigation
enum padding incorrectly has Scalar::Initialized layout #96158
Description
Activity
Here's a stand-alone reproducer for Miri:
use std::mem::MaybeUninit; fn main() { unsafe { let mut x = [MaybeUninit::<i32>::zeroed(); 3]; // Put in a ptr into the last 8 bytes. x.as_mut_ptr().offset(1).cast::<&i32>().write_unaligned(&0); // Read first 8 bytes as an `Option<i32>` let c = x.as_ptr().cast::<Option<i32>>().read(); println!("{:?}", c); } }
This can probably be turned into a CTFE testcase as well.
FWIW one reason this went unnoticed is that we don't actually check
ScalarPairstuff during validation:
rust/compiler/rustc_const_eval/src/interpret/validity.rs
Lines 822 to 827 in 38004b7
Maybe we should have a sort of debug mode where we do that check anyway? (For normal Miri operation this sounds like a waste though.)
Ah, that testcase will actually still fail since it is still copying parts of a pointer.
A testcase that demonstrates the bug would either require us to validate ScalarPair, or it'd have to be something horrible where parts of that padding are uninit, and we'd test if the init'ed parts are preserved. (Which is not actually guaranteed, padding can reset to uninit on a typed copy like this.)
Oh fun. So this is specifically about the enum layout and not the layout of a downcasted variant, right?
Yes. I have not looked at the layout of the downcasted variants.
Variants seem to be even wilder (according to my debugging while working on #96185): the variant of a
ScalarPairenum might haveAggregatelayout! That's a disaster since Miri assumes that allScalarPairvalues haveScalarPairlayout. That seemed like a reasonable sanity check, but apparently it is not true...Except codegen also assumes this, but somehow there it works out. Maybe it handles downcasts differently.
#94527 extended our
Scalarlayout with the notion of whether a scalar has to be initialized or not. Miri enforces this, and also uses it to know whether it can use a more efficient representation for scalars that have to be initialized.However, in some situations we currently compute a
Scalar::Initializedlayout when it should beScalar::Union(aka "may be uninitialized" -- maybe we should rename it toScalar::MaybeUninit). Specifically,Option<u32>has:Note that this claims that the second field must be always initialized. This is not true though, since for a
Nonethat second field is padding and hence does not have to be initialized.This causes rust-lang/miri#2068.
@oli-obk any idea how hard this is to fix?
(To be clear, this bug only causes problems in Miri, not during regular codegen -- at least for now, since we don't [yet] tell LLVM about this "must be initialized" thing.)