Rollup merge of #122249 - RalfJung:machine-read-hook, r=oli-obk

interpret: do not call machine read hooks during validation

Fixes rust-lang/miri#3347

r? ``@oli-obk``

Author: workingjubilee

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

@@ -380,16 +380,12 @@ pub fn eval_in_interpreter<'mir, 'tcx>(
         }
         Ok(mplace) => {
             // Since evaluation had no errors, validate the resulting constant.
-
-            // Temporarily allow access to the static_root_alloc_id for the purpose of validation.
-            let static_root_alloc_id = ecx.machine.static_root_alloc_id.take();
-            let validation = const_validate_mplace(&ecx, &mplace, cid);
-            ecx.machine.static_root_alloc_id = static_root_alloc_id;
+            let res = const_validate_mplace(&ecx, &mplace, cid);
 
             let alloc_id = mplace.ptr().provenance.unwrap().alloc_id();
 
             // Validation failed, report an error.
-            if let Err(error) = validation {
+            if let Err(error) = res {
                 Err(const_report_error(&ecx, error, alloc_id))
             } else {
                 // Convert to raw constant

compiler/rustc_const_eval/src/interpret/machine.rs

@@ -391,6 +391,8 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
 
     /// Hook for performing extra checks on a memory read access.
     ///
+    /// This will *not* be called during validation!
+    ///
     /// Takes read-only access to the allocation so we can keep all the memory read
     /// operations take `&self`. Use a `RefCell` in `AllocExtra` if you
     /// need to mutate.
@@ -410,6 +412,8 @@ pub trait Machine<'mir, 'tcx: 'mir>: Sized {
     /// Hook for performing extra checks on any memory read access,
     /// that involves an allocation, even ZST reads.
     ///
+    /// This will *not* be called during validation!
+    ///
     /// Used to prevent statics from self-initializing by reading from their own memory
     /// as it is being initialized.
     fn before_alloc_read(

compiler/rustc_const_eval/src/interpret/memory.rs

@@ -8,6 +8,7 @@
 
 use std::assert_matches::assert_matches;
 use std::borrow::Cow;
+use std::cell::Cell;
 use std::collections::VecDeque;
 use std::fmt;
 use std::ptr;
@@ -111,6 +112,11 @@ pub struct Memory<'mir, 'tcx, M: Machine<'mir, 'tcx>> {
     /// that do not exist any more.
     // FIXME: this should not be public, but interning currently needs access to it
     pub(super) dead_alloc_map: FxIndexMap<AllocId, (Size, Align)>,
+
+    /// This stores whether we are currently doing reads purely for the purpose of validation.
+    /// Those reads do not trigger the machine's hooks for memory reads.
+    /// Needless to say, this must only be set with great care!
+    validation_in_progress: Cell<bool>,
 }
 
 /// A reference to some allocation that was already bounds-checked for the given region
@@ -137,6 +143,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'mir, 'tcx, M> {
             alloc_map: M::MemoryMap::default(),
             extra_fn_ptr_map: FxIndexMap::default(),
             dead_alloc_map: FxIndexMap::default(),
+            validation_in_progress: Cell::new(false),
         }
     }
 
@@ -624,18 +631,28 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             size,
             CheckInAllocMsg::MemoryAccessTest,
             |alloc_id, offset, prov| {
-                // We want to call the hook on *all* accesses that involve an AllocId,
-                // including zero-sized accesses. That means we have to do it here
-                // rather than below in the `Some` branch.
-                M::before_alloc_read(self, alloc_id)?;
+                if !self.memory.validation_in_progress.get() {
+                    // We want to call the hook on *all* accesses that involve an AllocId,
+                    // including zero-sized accesses. That means we have to do it here
+                    // rather than below in the `Some` branch.
+                    M::before_alloc_read(self, alloc_id)?;
+                }
                 let alloc = self.get_alloc_raw(alloc_id)?;
                 Ok((alloc.size(), alloc.align, (alloc_id, offset, prov, alloc)))
             },
         )?;
 
         if let Some((alloc_id, offset, prov, alloc)) = ptr_and_alloc {
             let range = alloc_range(offset, size);
-            M::before_memory_read(self.tcx, &self.machine, &alloc.extra, (alloc_id, prov), range)?;
+            if !self.memory.validation_in_progress.get() {
+                M::before_memory_read(
+                    self.tcx,
+                    &self.machine,
+                    &alloc.extra,
+                    (alloc_id, prov),
+                    range,
+                )?;
+            }
             Ok(Some(AllocRef { alloc, range, tcx: *self.tcx, alloc_id }))
         } else {
             Ok(None)
@@ -909,6 +926,21 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
             }
         })
     }
+
+    /// Runs the close in "validation" mode, which means the machine's memory read hooks will be
+    /// suppressed. Needless to say, this must only be set with great care! Cannot be nested.
+    pub(super) fn run_for_validation<R>(&self, f: impl FnOnce() -> R) -> R {
+        assert!(
+            self.memory.validation_in_progress.replace(true) == false,
+            "`validation_in_progress` was already set"
+        );
+        let res = f();
+        assert!(
+            self.memory.validation_in_progress.replace(false) == true,
+            "`validation_in_progress` was unset by someone else"
+        );
+        res
+    }
 }
 
 #[doc(hidden)]
@@ -1154,6 +1186,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         };
         let src_alloc = self.get_alloc_raw(src_alloc_id)?;
         let src_range = alloc_range(src_offset, size);
+        assert!(!self.memory.validation_in_progress.get(), "we can't be copying during validation");
         M::before_memory_read(
             tcx,
             &self.machine,

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -971,7 +971,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
         let mut visitor = ValidityVisitor { path, ref_tracking, ctfe_mode, ecx: self };
 
         // Run it.
-        match visitor.visit_value(op) {
+        match self.run_for_validation(|| visitor.visit_value(op)) {
             Ok(()) => Ok(()),
             // Pass through validation failures and "invalid program" issues.
             Err(err)

src/tools/miri/tests/pass/alloc-access-tracking.rs

@@ -0,0 +1,25 @@
+#![feature(start)]
+#![no_std]
+//@compile-flags: -Zmiri-track-alloc-id=17 -Zmiri-track-alloc-accesses -Cpanic=abort
+//@only-target-linux: alloc IDs differ between OSes for some reason
+
+extern "Rust" {
+    fn miri_alloc(size: usize, align: usize) -> *mut u8;
+    fn miri_dealloc(ptr: *mut u8, size: usize, align: usize);
+}
+
+#[start]
+fn start(_: isize, _: *const *const u8) -> isize {
+    unsafe {
+        let ptr = miri_alloc(123, 1);
+        *ptr = 42; // Crucially, only a write is printed here, no read!
+        assert_eq!(*ptr, 42);
+        miri_dealloc(ptr, 123, 1);
+    }
+    0
+}
+
+#[panic_handler]
+fn panic_handler(_: &core::panic::PanicInfo) -> ! {
+    loop {}
+}

src/tools/miri/tests/pass/alloc-access-tracking.stderr

@@ -0,0 +1,37 @@
+note: tracking was triggered
+  --> $DIR/alloc-access-tracking.rs:LL:CC
+   |
+LL |         let ptr = miri_alloc(123, 1);
+   |                   ^^^^^^^^^^^^^^^^^^ created Miri bare-metal heap allocation of 123 bytes (alignment ALIGN bytes) with id 17
+   |
+   = note: BACKTRACE:
+   = note: inside `start` at $DIR/alloc-access-tracking.rs:LL:CC
+
+note: tracking was triggered
+  --> $DIR/alloc-access-tracking.rs:LL:CC
+   |
+LL |         *ptr = 42; // Crucially, only a write is printed here, no read!
+   |         ^^^^^^^^^ write access to allocation with id 17
+   |
+   = note: BACKTRACE:
+   = note: inside `start` at $DIR/alloc-access-tracking.rs:LL:CC
+
+note: tracking was triggered
+  --> $DIR/alloc-access-tracking.rs:LL:CC
+   |
+LL |         assert_eq!(*ptr, 42);
+   |         ^^^^^^^^^^^^^^^^^^^^ read access to allocation with id 17
+   |
+   = note: BACKTRACE:
+   = note: inside `start` at RUSTLIB/core/src/macros/mod.rs:LL:CC
+   = note: this note originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+note: tracking was triggered
+  --> $DIR/alloc-access-tracking.rs:LL:CC
+   |
+LL |         miri_dealloc(ptr, 123, 1);
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^ freed allocation with id 17
+   |
+   = note: BACKTRACE:
+   = note: inside `start` at $DIR/alloc-access-tracking.rs:LL:CC
+