interpret: reset padding during validation

Author: RalfJung

compiler/rustc_const_eval/src/const_eval/machine.rs

@@ -1,16 +1,16 @@
-use std::borrow::Borrow;
+use std::borrow::{Borrow, Cow};
 use std::fmt;
 use std::hash::Hash;
 use std::ops::ControlFlow;
 
 use rustc_ast::Mutability;
-use rustc_data_structures::fx::{FxIndexMap, IndexEntry};
+use rustc_data_structures::fx::{FxHashMap, FxIndexMap, IndexEntry};
 use rustc_hir::def_id::{DefId, LocalDefId};
 use rustc_hir::{self as hir, LangItem, CRATE_HIR_ID};
 use rustc_middle::mir::AssertMessage;
 use rustc_middle::query::TyCtxtAt;
 use rustc_middle::ty::layout::{FnAbiOf, TyAndLayout};
-use rustc_middle::ty::{self, TyCtxt};
+use rustc_middle::ty::{self, Ty, TyCtxt};
 use rustc_middle::{bug, mir};
 use rustc_span::symbol::{sym, Symbol};
 use rustc_span::Span;
@@ -24,8 +24,8 @@ use crate::fluent_generated as fluent;
 use crate::interpret::{
     self, compile_time_machine, err_ub, throw_exhaust, throw_inval, throw_ub_custom, throw_unsup,
     throw_unsup_format, AllocId, AllocRange, ConstAllocation, CtfeProvenance, FnArg, Frame,
-    GlobalAlloc, ImmTy, InterpCx, InterpResult, MPlaceTy, OpTy, Pointer, PointerArithmetic, Scalar,
-    StackPopCleanup,
+    GlobalAlloc, ImmTy, InterpCx, InterpResult, MPlaceTy, OpTy, Pointer, PointerArithmetic,
+    RangeSet, Scalar, StackPopCleanup,
 };
 
 /// When hitting this many interpreted terminators we emit a deny by default lint
@@ -65,6 +65,9 @@ pub struct CompileTimeMachine<'tcx> {
     /// storing the result in the given `AllocId`.
     /// Used to prevent reads from a static's base allocation, as that may allow for self-initialization loops.
     pub(crate) static_root_ids: Option<(AllocId, LocalDefId)>,
+
+    /// A cache of "data range" computations for unions (i.e., the offsets of non-padding bytes).
+    union_data_ranges: FxHashMap<Ty<'tcx>, RangeSet>,
 }
 
 #[derive(Copy, Clone)]
@@ -99,6 +102,7 @@ impl<'tcx> CompileTimeMachine<'tcx> {
             can_access_mut_global,
             check_alignment,
             static_root_ids: None,
+            union_data_ranges: FxHashMap::default(),
         }
     }
 }
@@ -766,6 +770,19 @@ impl<'tcx> interpret::Machine<'tcx> for CompileTimeMachine<'tcx> {
         }
         Ok(())
     }
+
+    fn cached_union_data_range<'e>(
+        ecx: &'e mut InterpCx<'tcx, Self>,
+        ty: Ty<'tcx>,
+        compute_range: impl FnOnce() -> RangeSet,
+    ) -> Cow<'e, RangeSet> {
+        if ecx.tcx.sess.opts.unstable_opts.extra_const_ub_checks {
+            Cow::Borrowed(ecx.machine.union_data_ranges.entry(ty).or_insert_with(compute_range))
+        } else {
+            // Don't bother caching, we're only doing one validation at the end anyway.
+            Cow::Owned(compute_range())
+        }
+    }
 }
 
 // Please do not add any code below the above `Machine` trait impl. I (oli-obk) plan more cleanups

compiler/rustc_const_eval/src/interpret/machine.rs

@@ -10,6 +10,7 @@ use rustc_apfloat::{Float, FloatConvert};
 use rustc_ast::{InlineAsmOptions, InlineAsmTemplatePiece};
 use rustc_middle::query::TyCtxtAt;
 use rustc_middle::ty::layout::TyAndLayout;
+use rustc_middle::ty::Ty;
 use rustc_middle::{mir, ty};
 use rustc_span::def_id::DefId;
 use rustc_span::Span;
@@ -19,7 +20,7 @@ use rustc_target::spec::abi::Abi as CallAbi;
 use super::{
     throw_unsup, throw_unsup_format, AllocBytes, AllocId, AllocKind, AllocRange, Allocation,
     ConstAllocation, CtfeProvenance, FnArg, Frame, ImmTy, InterpCx, InterpResult, MPlaceTy,
-    MemoryKind, Misalignment, OpTy, PlaceTy, Pointer, Provenance, CTFE_ALLOC_SALT,
+    MemoryKind, Misalignment, OpTy, PlaceTy, Pointer, Provenance, RangeSet, CTFE_ALLOC_SALT,
 };
 
 /// Data returned by [`Machine::after_stack_pop`], and consumed by
@@ -588,6 +589,15 @@ pub trait Machine<'tcx>: Sized {
         ecx: &InterpCx<'tcx, Self>,
         instance: Option<ty::Instance<'tcx>>,
     ) -> usize;
+
+    fn cached_union_data_range<'e>(
+        _ecx: &'e mut InterpCx<'tcx, Self>,
+        _ty: Ty<'tcx>,
+        compute_range: impl FnOnce() -> RangeSet,
+    ) -> Cow<'e, RangeSet> {
+        // Default to no caching.
+        Cow::Owned(compute_range())
+    }
 }
 
 /// A lot of the flexibility above is just needed for `Miri`, but all "compile-time" machines

compiler/rustc_const_eval/src/interpret/memory.rs

@@ -1136,8 +1136,17 @@ impl<'tcx, 'a, Prov: Provenance, Extra, Bytes: AllocBytes>
         self.write_scalar(alloc_range(offset, self.tcx.data_layout().pointer_size), val)
     }
 
+    /// Mark the given sub-range (relative to this allocation reference) as uninitialized.
+    pub fn write_uninit(&mut self, range: AllocRange) -> InterpResult<'tcx> {
+        let range = self.range.subrange(range);
+        Ok(self
+            .alloc
+            .write_uninit(&self.tcx, range)
+            .map_err(|e| e.to_interp_error(self.alloc_id))?)
+    }
+
     /// Mark the entire referenced range as uninitialized
-    pub fn write_uninit(&mut self) -> InterpResult<'tcx> {
+    pub fn write_uninit_full(&mut self) -> InterpResult<'tcx> {
         Ok(self
             .alloc
             .write_uninit(&self.tcx, self.range)

compiler/rustc_const_eval/src/interpret/mod.rs

@@ -39,5 +39,5 @@ use self::place::{MemPlace, Place};
 pub use self::projection::{OffsetMode, Projectable};
 pub use self::stack::{Frame, FrameInfo, LocalState, StackPopCleanup, StackPopInfo};
 pub(crate) use self::util::create_static_alloc;
-pub use self::validity::{CtfeValidationMode, RefTracking};
+pub use self::validity::{CtfeValidationMode, RangeSet, RefTracking};
 pub use self::visitor::ValueVisitor;

compiler/rustc_const_eval/src/interpret/place.rs

@@ -602,10 +602,11 @@ where
 
         if M::enforce_validity(self, dest.layout()) {
             // Data got changed, better make sure it matches the type!
+            // Also needed to reset padding.
             self.validate_operand(
                 &dest.to_place(),
                 M::enforce_validity_recursively(self, dest.layout()),
-                /*reset_provenance*/ true,
+                /*reset_provenance_and_padding*/ true,
             )?;
         }
 
@@ -701,9 +702,11 @@ where
                 // fields do not match the `ScalarPair` components.
 
                 alloc.write_scalar(alloc_range(Size::ZERO, a_val.size()), a_val)?;
-                alloc.write_scalar(alloc_range(b_offset, b_val.size()), b_val)
+                alloc.write_scalar(alloc_range(b_offset, b_val.size()), b_val)?;
+                // We don't have to reset padding here, `write_immediate` will anyway do a validation run.
+                Ok(())
             }
-            Immediate::Uninit => alloc.write_uninit(),
+            Immediate::Uninit => alloc.write_uninit_full(),
         }
     }
 
@@ -720,7 +723,7 @@ where
                     // Zero-sized access
                     return Ok(());
                 };
-                alloc.write_uninit()?;
+                alloc.write_uninit_full()?;
             }
         }
         Ok(())
@@ -812,17 +815,17 @@ where
             // Given that there were two typed copies, we have to ensure this is valid at both types,
             // and we have to ensure this loses provenance and padding according to both types.
             // But if the types are identical, we only do one pass.
-            if src.layout().ty != dest.layout().ty {
+            if allow_transmute && src.layout().ty != dest.layout().ty {
                 self.validate_operand(
                     &dest.transmute(src.layout(), self)?,
                     M::enforce_validity_recursively(self, src.layout()),
-                    /*reset_provenance*/ true,
+                    /*reset_provenance_and_padding*/ true,
                 )?;
             }
             self.validate_operand(
                 &dest,
                 M::enforce_validity_recursively(self, dest.layout()),
-                /*reset_provenance*/ true,
+                /*reset_provenance_and_padding*/ true,
             )?;
         }
 

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -69,7 +69,7 @@ fn might_permit_raw_init_strict<'tcx>(
         .validate_operand(
             &allocated.into(),
             /*recursive*/ false,
-            /*reset_provenance*/ false,
+            /*reset_provenance_and_padding*/ false,
         )
         .is_ok())
 }

compiler/rustc_const_eval/src/util/check_validity_requirement.rs

@@ -1,6 +1,7 @@
 //! Global machine state as well as implementation of the interpreter engine
 //! `Machine` trait.
 
+use std::borrow::Cow;
 use std::cell::RefCell;
 use std::collections::hash_map::Entry;
 use std::fmt;
@@ -571,6 +572,9 @@ pub struct MiriMachine<'tcx> {
     /// Invariant: the promised alignment will never be less than the native alignment of the
     /// allocation.
     pub(crate) symbolic_alignment: RefCell<FxHashMap<AllocId, (Size, Align)>>,
+
+    /// A cache of "data range" computations for unions (i.e., the offsets of non-padding bytes).
+    union_data_ranges: FxHashMap<Ty<'tcx>, RangeSet>,
 }
 
 impl<'tcx> MiriMachine<'tcx> {
@@ -713,6 +717,7 @@ impl<'tcx> MiriMachine<'tcx> {
             allocation_spans: RefCell::new(FxHashMap::default()),
             const_cache: RefCell::new(FxHashMap::default()),
             symbolic_alignment: RefCell::new(FxHashMap::default()),
+            union_data_ranges: FxHashMap::default(),
         }
     }
 
@@ -825,6 +830,7 @@ impl VisitProvenance for MiriMachine<'_> {
             allocation_spans: _,
             const_cache: _,
             symbolic_alignment: _,
+            union_data_ranges: _,
         } = self;
 
         threads.visit_provenance(visit);
@@ -1602,4 +1608,12 @@ impl<'tcx> Machine<'tcx> for MiriMachine<'tcx> {
             ecx.machine.rng.borrow_mut().gen::<usize>() % ADDRS_PER_ANON_GLOBAL
         }
     }
+
+    fn cached_union_data_range<'e>(
+        ecx: &'e mut InterpCx<'tcx, Self>,
+        ty: Ty<'tcx>,
+        compute_range: impl FnOnce() -> RangeSet,
+    ) -> Cow<'e, RangeSet> {
+        Cow::Borrowed(ecx.machine.union_data_ranges.entry(ty).or_insert_with(compute_range))
+    }
 }

src/tools/miri/src/machine.rs

@@ -0,0 +1,19 @@
+use std::mem;
+
+#[allow(unused)]
+enum E {
+    None,
+    Some(&'static (), &'static ()),
+}
+
+fn main() { unsafe {
+    let p: E = mem::transmute((0usize, 0usize)); // The copy when `E` is returned from `transmute` should destroy padding.
+    // This is a `None`, so everything but the discriminant is padding.
+    assert!(matches!(p, E::None));
+
+    // Turns out the discriminant is (currently) stored
+    // in the 2nd pointer, so the first half is padding.
+    let c = &p as *const _ as *const u8;
+    let _val = *c.add(0); // Get the padding byte.
+    //~^ERROR: uninitialized
+} }

src/tools/miri/tests/fail/uninit/padding-enum.rs

@@ -0,0 +1,15 @@
+error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
+  --> $DIR/padding-enum.rs:LL:CC
+   |
+LL |     let _val = *c.add(0); // Get the padding byte.
+   |                ^^^^^^^^^ using uninitialized data, but this operation requires initialized memory
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside `main` at $DIR/padding-enum.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

src/tools/miri/tests/fail/uninit/padding-enum.stderr

@@ -0,0 +1,25 @@
+#![feature(core_intrinsics)]
+
+use std::mem::{self, MaybeUninit};
+
+fn main() {
+    // This constructs a `(usize, bool)` pair: 9 bytes initialized, the rest not.
+    // Ensure that these 9 bytes are indeed initialized, and the rest is indeed not.
+    // This should be the case even if we write into previously initialized storage.
+    let mut x: MaybeUninit<Box<[u8]>> = MaybeUninit::zeroed();
+    let z = std::intrinsics::add_with_overflow(0usize, 0usize);
+    unsafe { x.as_mut_ptr().cast::<(usize, bool)>().write(z) };
+    // Now read this bytewise. There should be (`ptr_size + 1`) def bytes followed by
+    // (`ptr_size - 1`) undef bytes (the padding after the bool) in there.
+    let z: *const u8 = &x as *const _ as *const _;
+    let first_undef = mem::size_of::<usize>() as isize + 1;
+    for i in 0..first_undef {
+        let byte = unsafe { *z.offset(i) };
+        assert_eq!(byte, 0);
+    }
+    let v = unsafe { *z.offset(first_undef) };
+    //~^ ERROR: uninitialized
+    if v == 0 {
+        println!("it is zero");
+    }
+}

src/tools/miri/tests/fail/uninit/padding-pair.rs

@@ -0,0 +1,15 @@
+error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
+  --> $DIR/padding-pair.rs:LL:CC
+   |
+LL |     let v = unsafe { *z.offset(first_undef) };
+   |                      ^^^^^^^^^^^^^^^^^^^^^^ using uninitialized data, but this operation requires initialized memory
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside `main` at $DIR/padding-pair.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

src/tools/miri/tests/fail/uninit/padding-pair.stderr

@@ -0,0 +1,32 @@
+#[repr(C)]
+#[derive(Debug, Copy, Clone)]
+struct Foo {
+    val16: u16,
+    // Padding bytes go here!
+    val32: u32,
+}
+
+#[repr(C)]
+#[derive(Debug, Copy, Clone)]
+struct Bar {
+    bytes: [u8; 8],
+}
+
+#[repr(C)]
+union FooBar {
+    foo: Foo,
+    bar: Bar,
+}
+
+pub fn main() {
+    // Initialize as u8 to ensure padding bytes are zeroed.
+    let mut foobar = FooBar { bar: Bar { bytes: [0u8; 8] } };
+    // Reading either field is ok.
+    let _val = unsafe { (foobar.foo, foobar.bar) };
+    // Does this assignment copy the uninitialized padding bytes
+    // over the initialized padding bytes? miri doesn't seem to think so.
+    foobar.foo = Foo { val16: 1, val32: 2 };
+    // This resets the padding to uninit.
+    let _val = unsafe { (foobar.foo, foobar.bar) };
+    //~^ ERROR: uninitialized
+}

src/tools/miri/tests/fail/uninit/padding-struct-assign.rs

@@ -0,0 +1,15 @@
+error: Undefined Behavior: constructing invalid value at .bytes[2]: encountered uninitialized memory, but expected an integer
+  --> $DIR/padding-struct-assign.rs:LL:CC
+   |
+LL |     let _val = unsafe { (foobar.foo, foobar.bar) };
+   |                                      ^^^^^^^^^^ constructing invalid value at .bytes[2]: encountered uninitialized memory, but expected an integer
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside `main` at $DIR/padding-struct-assign.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

src/tools/miri/tests/fail/uninit/padding-struct-assign.stderr

@@ -0,0 +1,11 @@
+use std::mem;
+
+#[repr(C)]
+struct Pair(u8, u16);
+
+fn main() { unsafe {
+    let p: Pair = mem::transmute(0u32); // The copy when `Pair` is returned from `transmute` should destroy padding.
+    let c = &p as *const _ as *const u8;
+    let _val = *c.add(1); // Get the padding byte.
+    //~^ERROR: uninitialized
+} }

src/tools/miri/tests/fail/uninit/padding-struct.rs

@@ -0,0 +1,15 @@
+error: Undefined Behavior: using uninitialized data, but this operation requires initialized memory
+  --> $DIR/padding-struct.rs:LL:CC
+   |
+LL |     let _val = *c.add(1); // Get the padding byte.
+   |                ^^^^^^^^^ using uninitialized data, but this operation requires initialized memory
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside `main` at $DIR/padding-struct.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

src/tools/miri/tests/fail/uninit/padding-struct.stderr

@@ -0,0 +1,14 @@
+use std::mem;
+
+#[allow(unused)]
+#[repr(C)]
+union U {
+    field: (u8, u16),
+}
+
+fn main() { unsafe {
+    let p: U = mem::transmute(0u32); // The copy when `U` is returned from `transmute` should destroy padding.
+    let c = &p as *const _ as *const [u8; 4];
+    let _val = *c; // Read the entire thing, definitely contains the padding byte.
+    //~^ERROR: uninitialized
+} }

src/tools/miri/tests/fail/uninit/padding-union.rs

@@ -0,0 +1,15 @@
+error: Undefined Behavior: constructing invalid value at [1]: encountered uninitialized memory, but expected an integer
+  --> $DIR/padding-union.rs:LL:CC
+   |
+LL |     let _val = *c; // Read the entire thing, definitely contains the padding byte.
+   |                ^^ constructing invalid value at [1]: encountered uninitialized memory, but expected an integer
+   |
+   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
+   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
+   = note: BACKTRACE:
+   = note: inside `main` at $DIR/padding-union.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+

src/tools/miri/tests/fail/uninit/padding-union.stderr

@@ -1,16 +1,17 @@
 #![feature(core_intrinsics)]
 
-use std::mem;
+use std::mem::{self, MaybeUninit};
 
 fn main() {
-    let x: Option<Box<[u8]>> = unsafe {
+    // This constructs a `(usize, bool)` pair: 9 bytes initialized, the rest not.
+    // Ensure that these 9 bytes are indeed initialized, and the rest is indeed not.
+    let x: MaybeUninit<Box<[u8]>> = unsafe {
         let z = std::intrinsics::add_with_overflow(0usize, 0usize);
-        std::mem::transmute::<(usize, bool), Option<Box<[u8]>>>(z)
+        std::mem::transmute::<(usize, bool), MaybeUninit<Box<[u8]>>>(z)
     };
-    let y = &x;
     // Now read this bytewise. There should be (`ptr_size + 1`) def bytes followed by
     // (`ptr_size - 1`) undef bytes (the padding after the bool) in there.
-    let z: *const u8 = y as *const _ as *const _;
+    let z: *const u8 = &x as *const _ as *const _;
     let first_undef = mem::size_of::<usize>() as isize + 1;
     for i in 0..first_undef {
         let byte = unsafe { *z.offset(i) };