enum padding incorrectly has Scalar::Initialized layout #96158

Activity

1. RalfJung commented on Apr 17, 2022

   

   RalfJung

   on Apr 17, 2022

   Last edited by RalfJung

   MemberAuthor

   More actions

   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.
2. RalfJung commented on Apr 17, 2022

   

   RalfJung

   on Apr 17, 2022

   MemberAuthor

   More actions

   FWIW one reason this went unnoticed is that we don't actually check ScalarPair stuff during validation:
   

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

   Lines 822 to 827 in 38004b7

   	Abi::ScalarPair { .. } | Abi::Vector { .. } => {
   	// These have fields that we already visited above, so we already checked
   	// all their scalar-level restrictions.
   	// There is also no equivalent to `rustc_layout_scalar_valid_range_start`
   	// that would make skipping them here an issue.
   	}

   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.)
3. RalfJung commented on Apr 18, 2022

   

   RalfJung

   on Apr 18, 2022

   MemberAuthor

   More actions

   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.)
4. oli-obk commented on Apr 18, 2022

   

   oli-obk

   on Apr 18, 2022

   Contributor

   More actions

   Oh fun. So this is specifically about the enum layout and not the layout of a downcasted variant, right?
5. RalfJung commented on Apr 18, 2022

   

   RalfJung

   on Apr 18, 2022

   MemberAuthor

   More actions

   Yes. I have not looked at the layout of the downcasted variants.
6. RalfJung commented on Apr 18, 2022

   

   RalfJung

   on Apr 18, 2022

   Last edited by RalfJung

   MemberAuthor

   More actions

   Variants seem to be even wilder (according to my debugging while working on #96185): the variant of a ScalarPair enum might have Aggregate layout! That's a disaster since Miri assumes that all ScalarPair values have ScalarPair layout. 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.
7. bors

   closed this as completedin a8272f2on Apr 22, 2022
8. matthiaskrgr

   added a commit that references this issue on May 5, 2022

   Rollup merge of rust-lang#96714 - RalfJung:scalar-pair-debug, r=oli-obk

   ...

   6804819