Skip to content

Commit

Permalink
Rollup merge of #122249 - RalfJung:machine-read-hook, r=oli-obk
Browse files Browse the repository at this point in the history
interpret: do not call machine read hooks during validation

Fixes rust-lang/miri#3347

r? ``@oli-obk``
  • Loading branch information
workingjubilee authored Mar 11, 2024
2 parents afa0581 + bf47df8 commit f6ca425
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 12 deletions.
8 changes: 2 additions & 6 deletions compiler/rustc_const_eval/src/const_eval/eval_queries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_const_eval/src/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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(
Expand Down
43 changes: 38 additions & 5 deletions compiler/rustc_const_eval/src/interpret/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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),
}
}

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/validity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
25 changes: 25 additions & 0 deletions src/tools/miri/tests/pass/alloc-access-tracking.rs
Original file line number Diff line number Diff line change
@@ -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 {}
}
37 changes: 37 additions & 0 deletions src/tools/miri/tests/pass/alloc-access-tracking.stderr
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit f6ca425

Please sign in to comment.