From 22364f86aeba440066db931132ecee8292b709fd Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Tue, 9 Jul 2024 14:24:05 +0200 Subject: [PATCH 01/21] This pattern using lazy protected Reserved IM prevents spurious writes --- .../tree_borrows/reservedim_spurious_write.rs | 109 ++++++++++++++++++ 1 file changed, 109 insertions(+) create mode 100644 src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs diff --git a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs new file mode 100644 index 0000000000000..1f52537f9a295 --- /dev/null +++ b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs @@ -0,0 +1,109 @@ +// Illustrating a problematic interaction between Reserved, interior mutability, +// and protectors, that makes spurious writes fail in the previous model of Tree Borrows. +// As for all similar tests, we disable preemption so that the error message is deterministic. +//@compile-flags: -Zmiri-tree-borrows -Zmiri-preemption-rate=0 + +use std::cell::Cell; +use std::sync::{Arc, Barrier}; +use std::thread; + +// Here is the problematic interleaving: +// - thread 1: retag and activate `x` (protected) +// - thread 2: create but do not retag (lazy) `y` as Reserved with interior mutability +// - thread 1: spurious write through `x` would go here +// - thread 2: function exit (noop due to lazyness) +// - thread 1: function exit (no permanent effect on `y` because it is now Reserved IM unprotected) +// - thread 2: write through `y` +// In the source code nothing happens to `y` + +// `Send`able raw pointer wrapper. +#[derive(Copy, Clone)] +struct SendPtr(*mut u8); +unsafe impl Send for SendPtr {} + +type IdxBarrier = (usize, Arc); + +// Barriers to enforce the interleaving. +// This macro expects `synchronized!(thread, msg)` where `thread` is a `IdxBarrier`, +// and `msg` is the message to be displayed when the thread reaches this point in the execution. +macro_rules! synchronized { + ($thread:expr, $msg:expr) => {{ + let (thread_id, barrier) = &$thread; + eprintln!("Thread {} executing: {}", thread_id, $msg); + barrier.wait(); + }}; +} + +fn main() { + eprintln!("Without spurious write"); + example(false); + + eprintln!("\nIf this text is visible then the model forbids spurious writes.\n"); + + eprintln!("With spurious write"); + example(true); + + eprintln!("\nIf this text is visible then the model fails to detect a noalias violation.\n"); +} + +fn example(spurious: bool) { + // For this example it is important that we have at least two bytes + // because lazyness is involved. + let mut data = [0u8; 2]; + let ptr = SendPtr(std::ptr::addr_of_mut!(data[0])); + let barrier = Arc::new(Barrier::new(2)); + let bx = Arc::clone(&barrier); + let by = Arc::clone(&barrier); + + // Retag and activate `x`, wait until the other thread creates a lazy permission. + // Then do a spurious write. Finally exit the function after the other thread. + let thread_1 = thread::spawn(move || { + let b = (1, bx); + synchronized!(b, "start"); + let ptr = ptr; + synchronized!(b, "retag x (&mut, protect)"); + fn inner(x: &mut u8, b: IdxBarrier, spurious: bool) { + *x = 42; // activate immediately + synchronized!(b, "[lazy] retag y (&mut, protect, IM)"); + // A spurious write should be valid here because `x` is + // `Active` and protected. + if spurious { + synchronized!(b, "spurious write x (executed)"); + *x = 64; + } else { + synchronized!(b, "spurious write x (skipped)"); + } + synchronized!(b, "ret y"); + synchronized!(b, "ret x"); + } + inner(unsafe { &mut *ptr.0 }, b.clone(), spurious); + synchronized!(b, "write y"); + synchronized!(b, "end"); + }); + + // Create a lazy Reserved with interior mutability. + // Wait for the other thread's spurious write then observe the side effects + // of that write. + let thread_2 = thread::spawn(move || { + let b = (2, by); + synchronized!(b, "start"); + let ptr = ptr; + synchronized!(b, "retag x (&mut, protect)"); + synchronized!(b, "[lazy] retag y (&mut, protect, IM)"); + fn inner(y: &mut Cell, b: IdxBarrier) -> *mut u8 { + synchronized!(b, "spurious write x"); + synchronized!(b, "ret y"); + y as *mut Cell as *mut u8 + } + // Currently `ptr` points to `data[0]`. We retag it for `data[1]` + // then use it for `data[0]` where its initialization has been deferred. + let y = inner(unsafe { &mut *(ptr.0 as *mut Cell).wrapping_add(1) }, b.clone()); + synchronized!(b, "ret x"); + synchronized!(b, "write y"); + unsafe { *y.wrapping_sub(1) = 13 } + synchronized!(b, "end"); + }); + + thread_1.join().unwrap(); + thread_2.join().unwrap(); +} From 2de6e7f3a680a8000f83f2b62252d18a1bb06966 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Tue, 9 Jul 2024 14:36:46 +0200 Subject: [PATCH 02/21] Implement fix for reservedim_spurious_write: ignore IM on protected --- .../src/borrow_tracker/tree_borrows/mod.rs | 10 ++++- .../src/borrow_tracker/tree_borrows/perms.rs | 5 +++ .../reserved/cell-protected-write.stderr | 6 +-- .../tree_borrows/reservedim_spurious_write.rs | 2 +- .../reservedim_spurious_write.stderr | 41 +++++++++++++++++++ .../tests/pass/tree_borrows/reserved.stderr | 2 +- 6 files changed, 59 insertions(+), 7 deletions(-) create mode 100644 src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.stderr diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs index 86074384084d5..4d9595b352f28 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs @@ -141,8 +141,14 @@ impl<'tcx> NewPermission { ) -> Option { let ty_is_freeze = pointee.is_freeze(*cx.tcx, cx.param_env()); let ty_is_unpin = pointee.is_unpin(*cx.tcx, cx.param_env()); + let is_protected = kind == RetagKind::FnEntry; + // As demonstrated by `tests/fail/tree_borrows/reservedim_spurious_write.rs`, + // interior mutability and protectors interact poorly. + // To eliminate the case of Protected Reserved IM we override interior mutability + // in the case of a protected reference. let initial_state = match mutability { - Mutability::Mut if ty_is_unpin => Permission::new_reserved(ty_is_freeze), + Mutability::Mut if ty_is_unpin => + Permission::new_reserved(ty_is_freeze || is_protected), Mutability::Not if ty_is_freeze => Permission::new_frozen(), // Raw pointers never enter this function so they are not handled. // However raw pointers are not the only pointers that take the parent @@ -151,7 +157,7 @@ impl<'tcx> NewPermission { _ => return None, }; - let protector = (kind == RetagKind::FnEntry).then_some(ProtectorKind::StrongProtector); + let protector = is_protected.then_some(ProtectorKind::StrongProtector); Some(Self { zero_size: false, initial_state, protector }) } diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs index 7aa9c3e862bcc..9c19ae76a2d01 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs @@ -22,6 +22,11 @@ enum PermissionPriv { /// - foreign-read then child-write is UB due to `conflicted`, /// - child-write then foreign-read is UB since child-write will activate and then /// foreign-read disables a protected `Active`, which is UB. + /// + /// Note: since the discovery of `tests/fail/tree_borrows/reservedim_spurious_write.rs`, + /// `ty_is_freeze` does not strictly mean that the type has no interior mutability, + /// it could be an interior mutable type that lost its interior mutability privileges + /// when retagged with a protector. Reserved { ty_is_freeze: bool, conflicted: bool }, /// represents: a unique pointer; /// allows: child reads, child writes; diff --git a/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr b/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr index 7d000ba55e60b..ce9a5b7f15865 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/reserved/cell-protected-write.stderr @@ -5,7 +5,7 @@ Warning: this tree is indicative only. Some tags may have been hidden. | RsM | └─┬── | RsM | ├─┬── | RsM | │ └─┬── -| RsM | │ └──── Strongly protected +| Rs | │ └──── Strongly protected | RsM | └──── ────────────────────────────────────────────────── error: Undefined Behavior: write access through (y, callee:y, caller:y) at ALLOC[0x0] is forbidden @@ -16,14 +16,14 @@ LL | *y = 1; | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag (y, callee:y, caller:y) is foreign to the protected tag (callee:x) (i.e., it is not a child) - = help: this foreign write access would cause the protected tag (callee:x) (currently Reserved (interior mutable)) to become Disabled + = help: this foreign write access would cause the protected tag (callee:x) (currently Reserved) to become Disabled = help: protected tags must never be Disabled help: the accessed tag was created here --> $DIR/cell-protected-write.rs:LL:CC | LL | let y = (&mut *n).get(); | ^^^^^^^^^ -help: the protected tag was created here, in the initial state Reserved (interior mutable) +help: the protected tag was created here, in the initial state Reserved --> $DIR/cell-protected-write.rs:LL:CC | LL | unsafe fn write_second(x: &mut UnsafeCell, y: *mut u8) { diff --git a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs index 1f52537f9a295..5e7cb5e34cc2b 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs +++ b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs @@ -100,7 +100,7 @@ fn example(spurious: bool) { let y = inner(unsafe { &mut *(ptr.0 as *mut Cell).wrapping_add(1) }, b.clone()); synchronized!(b, "ret x"); synchronized!(b, "write y"); - unsafe { *y.wrapping_sub(1) = 13 } + unsafe { *y.wrapping_sub(1) = 13 } //~ERROR: /write access through .* is forbidden/ synchronized!(b, "end"); }); diff --git a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.stderr b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.stderr new file mode 100644 index 0000000000000..9be6c157c0cbb --- /dev/null +++ b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.stderr @@ -0,0 +1,41 @@ +Without spurious write +Thread 1 executing: start +Thread 2 executing: start +Thread 2 executing: retag x (&mut, protect) +Thread 1 executing: retag x (&mut, protect) +Thread 1 executing: [lazy] retag y (&mut, protect, IM) +Thread 2 executing: [lazy] retag y (&mut, protect, IM) +Thread 2 executing: spurious write x +Thread 1 executing: spurious write x (skipped) +Thread 1 executing: ret y +Thread 2 executing: ret y +Thread 2 executing: ret x +Thread 1 executing: ret x +Thread 1 executing: write y +Thread 2 executing: write y +error: Undefined Behavior: write access through at ALLOC[0x0] is forbidden + --> $DIR/reservedim_spurious_write.rs:LL:CC + | +LL | unsafe { *y.wrapping_sub(1) = 13 } + | ^^^^^^^^^^^^^^^^^^^^^^^ write access through at ALLOC[0x0] is forbidden + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental + = help: the accessed tag has state Disabled which forbids this child write access +help: the accessed tag was created here, in the initial state Reserved + --> $DIR/reservedim_spurious_write.rs:LL:CC + | +LL | fn inner(y: &mut Cell, b: IdxBarrier) -> *mut u8 { + | ^ +help: the accessed tag later transitioned to Disabled due to a protector release (acting as a foreign write access) on every location previously accessed by this tag + --> $DIR/reservedim_spurious_write.rs:LL:CC + | +LL | } + | ^ + = help: this transition corresponds to a loss of read and write permissions + = note: BACKTRACE (of the first span) on thread `unnamed-ID`: + = note: inside closure at $DIR/reservedim_spurious_write.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 + diff --git a/src/tools/miri/tests/pass/tree_borrows/reserved.stderr b/src/tools/miri/tests/pass/tree_borrows/reserved.stderr index 0d0d52c717fed..d149a4065f92b 100644 --- a/src/tools/miri/tests/pass/tree_borrows/reserved.stderr +++ b/src/tools/miri/tests/pass/tree_borrows/reserved.stderr @@ -6,7 +6,7 @@ Warning: this tree is indicative only. Some tags may have been hidden. | RsM | └─┬── | RsM | ├─┬── | RsM | │ └─┬── -| RsCM| │ └──── +| RsC | │ └──── | RsM | └──── ────────────────────────────────────────────────── [interior mut] Foreign Read: Re* -> Re* From 22996ad073870c5ac7753ba1acbaba784adc534d Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Tue, 9 Jul 2024 19:42:12 +0200 Subject: [PATCH 03/21] Apply suggestions - split test into two revisions - clarify comments --- .../src/borrow_tracker/tree_borrows/mod.rs | 3 +- .../tree_borrows/reservedim_spurious_write.rs | 26 +++++------- .../reservedim_spurious_write.with.stderr | 40 +++++++++++++++++++ ... reservedim_spurious_write.without.stderr} | 1 - 4 files changed, 52 insertions(+), 18 deletions(-) create mode 100644 src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.with.stderr rename src/tools/miri/tests/fail/tree_borrows/{reservedim_spurious_write.stderr => reservedim_spurious_write.without.stderr} (98%) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs index 4d9595b352f28..123d4b407fb4c 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/mod.rs @@ -145,7 +145,8 @@ impl<'tcx> NewPermission { // As demonstrated by `tests/fail/tree_borrows/reservedim_spurious_write.rs`, // interior mutability and protectors interact poorly. // To eliminate the case of Protected Reserved IM we override interior mutability - // in the case of a protected reference. + // in the case of a protected reference: protected references are always considered + // "freeze". let initial_state = match mutability { Mutability::Mut if ty_is_unpin => Permission::new_reserved(ty_is_freeze || is_protected), diff --git a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs index 5e7cb5e34cc2b..611f64dce5e50 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs +++ b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs @@ -2,6 +2,12 @@ // and protectors, that makes spurious writes fail in the previous model of Tree Borrows. // As for all similar tests, we disable preemption so that the error message is deterministic. //@compile-flags: -Zmiri-tree-borrows -Zmiri-preemption-rate=0 +// +// One revision without spurious read (default source code) and one with spurious read. +// Both are expected to be UB. Both revisions are expected to have the *same* error +// because we are aligning the behavior of `without` to that of `with` so that the +// spurious write is effectively a noop in the long term. +//@revisions: without with use std::cell::Cell; use std::sync::{Arc, Barrier}; @@ -9,7 +15,7 @@ use std::thread; // Here is the problematic interleaving: // - thread 1: retag and activate `x` (protected) -// - thread 2: create but do not retag (lazy) `y` as Reserved with interior mutability +// - thread 2: retag but do not initialize (lazy) `y` as Reserved with interior mutability // - thread 1: spurious write through `x` would go here // - thread 2: function exit (noop due to lazyness) // - thread 1: function exit (no permanent effect on `y` because it is now Reserved IM unprotected) @@ -35,18 +41,6 @@ macro_rules! synchronized { } fn main() { - eprintln!("Without spurious write"); - example(false); - - eprintln!("\nIf this text is visible then the model forbids spurious writes.\n"); - - eprintln!("With spurious write"); - example(true); - - eprintln!("\nIf this text is visible then the model fails to detect a noalias violation.\n"); -} - -fn example(spurious: bool) { // For this example it is important that we have at least two bytes // because lazyness is involved. let mut data = [0u8; 2]; @@ -62,12 +56,12 @@ fn example(spurious: bool) { synchronized!(b, "start"); let ptr = ptr; synchronized!(b, "retag x (&mut, protect)"); - fn inner(x: &mut u8, b: IdxBarrier, spurious: bool) { + fn inner(x: &mut u8, b: IdxBarrier) { *x = 42; // activate immediately synchronized!(b, "[lazy] retag y (&mut, protect, IM)"); // A spurious write should be valid here because `x` is // `Active` and protected. - if spurious { + if cfg!(with) { synchronized!(b, "spurious write x (executed)"); *x = 64; } else { @@ -76,7 +70,7 @@ fn example(spurious: bool) { synchronized!(b, "ret y"); synchronized!(b, "ret x"); } - inner(unsafe { &mut *ptr.0 }, b.clone(), spurious); + inner(unsafe { &mut *ptr.0 }, b.clone()); synchronized!(b, "write y"); synchronized!(b, "end"); }); diff --git a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.with.stderr b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.with.stderr new file mode 100644 index 0000000000000..5ac9c912ba9ee --- /dev/null +++ b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.with.stderr @@ -0,0 +1,40 @@ +Thread 1 executing: start +Thread 2 executing: start +Thread 2 executing: retag x (&mut, protect) +Thread 1 executing: retag x (&mut, protect) +Thread 1 executing: [lazy] retag y (&mut, protect, IM) +Thread 2 executing: [lazy] retag y (&mut, protect, IM) +Thread 2 executing: spurious write x +Thread 1 executing: spurious write x (executed) +Thread 1 executing: ret y +Thread 2 executing: ret y +Thread 2 executing: ret x +Thread 1 executing: ret x +Thread 1 executing: write y +Thread 2 executing: write y +error: Undefined Behavior: write access through at ALLOC[0x0] is forbidden + --> $DIR/reservedim_spurious_write.rs:LL:CC + | +LL | unsafe { *y.wrapping_sub(1) = 13 } + | ^^^^^^^^^^^^^^^^^^^^^^^ write access through at ALLOC[0x0] is forbidden + | + = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental + = help: the accessed tag has state Disabled which forbids this child write access +help: the accessed tag was created here, in the initial state Reserved + --> $DIR/reservedim_spurious_write.rs:LL:CC + | +LL | fn inner(y: &mut Cell, b: IdxBarrier) -> *mut u8 { + | ^ +help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x1] + --> $DIR/reservedim_spurious_write.rs:LL:CC + | +LL | *x = 64; + | ^^^^^^^ + = help: this transition corresponds to a loss of read and write permissions + = note: BACKTRACE (of the first span) on thread `unnamed-ID`: + = note: inside closure at $DIR/reservedim_spurious_write.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 + diff --git a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.stderr b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.without.stderr similarity index 98% rename from src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.stderr rename to src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.without.stderr index 9be6c157c0cbb..97c71fdedefb3 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.without.stderr @@ -1,4 +1,3 @@ -Without spurious write Thread 1 executing: start Thread 2 executing: start Thread 2 executing: retag x (&mut, protect) From 68aed4a5cebf846d88716489e4ea66074d669c5b Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Wed, 10 Jul 2024 14:32:02 +0200 Subject: [PATCH 04/21] Second byte is not involved in the example; use a Cell<()> instead --- .../tree_borrows/reservedim_spurious_write.rs | 23 +++++++++++-------- .../reservedim_spurious_write.with.stderr | 6 ++--- .../reservedim_spurious_write.without.stderr | 6 ++--- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs index 611f64dce5e50..6ae79be6cc7e8 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs +++ b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs @@ -41,10 +41,10 @@ macro_rules! synchronized { } fn main() { - // For this example it is important that we have at least two bytes - // because lazyness is involved. - let mut data = [0u8; 2]; - let ptr = SendPtr(std::ptr::addr_of_mut!(data[0])); + // The conflict occurs one one single location but the example involves + // lazily initialized permissions. + let mut data = 0u8; + let ptr = SendPtr(std::ptr::addr_of_mut!(data)); let barrier = Arc::new(Barrier::new(2)); let bx = Arc::clone(&barrier); let by = Arc::clone(&barrier); @@ -84,17 +84,20 @@ fn main() { let ptr = ptr; synchronized!(b, "retag x (&mut, protect)"); synchronized!(b, "[lazy] retag y (&mut, protect, IM)"); - fn inner(y: &mut Cell, b: IdxBarrier) -> *mut u8 { + fn inner(y: &mut Cell<()>, b: IdxBarrier) -> *mut u8 { synchronized!(b, "spurious write x"); synchronized!(b, "ret y"); - y as *mut Cell as *mut u8 + // `y` is not retagged for any bytes, so the pointer we return + // has its permission lazily initialized. + y as *mut Cell<()> as *mut u8 } - // Currently `ptr` points to `data[0]`. We retag it for `data[1]` - // then use it for `data[0]` where its initialization has been deferred. - let y = inner(unsafe { &mut *(ptr.0 as *mut Cell).wrapping_add(1) }, b.clone()); + // Currently `ptr` points to `data`. + // We do a zero-sized retag so that its permission is lazy. + let y_zst = unsafe { &mut *(ptr.0 as *mut Cell<()>) }; + let y = inner(y_zst, b.clone()); synchronized!(b, "ret x"); synchronized!(b, "write y"); - unsafe { *y.wrapping_sub(1) = 13 } //~ERROR: /write access through .* is forbidden/ + unsafe { *y = 13 } //~ERROR: /write access through .* is forbidden/ synchronized!(b, "end"); }); diff --git a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.with.stderr b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.with.stderr index 5ac9c912ba9ee..0e4517e90105c 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.with.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.with.stderr @@ -15,15 +15,15 @@ Thread 2 executing: write y error: Undefined Behavior: write access through at ALLOC[0x0] is forbidden --> $DIR/reservedim_spurious_write.rs:LL:CC | -LL | unsafe { *y.wrapping_sub(1) = 13 } - | ^^^^^^^^^^^^^^^^^^^^^^^ write access through at ALLOC[0x0] is forbidden +LL | unsafe { *y = 13 } + | ^^^^^^^ write access through at ALLOC[0x0] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag has state Disabled which forbids this child write access help: the accessed tag was created here, in the initial state Reserved --> $DIR/reservedim_spurious_write.rs:LL:CC | -LL | fn inner(y: &mut Cell, b: IdxBarrier) -> *mut u8 { +LL | fn inner(y: &mut Cell<()>, b: IdxBarrier) -> *mut u8 { | ^ help: the accessed tag later transitioned to Disabled due to a foreign write access at offsets [0x0..0x1] --> $DIR/reservedim_spurious_write.rs:LL:CC diff --git a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.without.stderr b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.without.stderr index 97c71fdedefb3..cbeef90243bfb 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.without.stderr +++ b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.without.stderr @@ -15,15 +15,15 @@ Thread 2 executing: write y error: Undefined Behavior: write access through at ALLOC[0x0] is forbidden --> $DIR/reservedim_spurious_write.rs:LL:CC | -LL | unsafe { *y.wrapping_sub(1) = 13 } - | ^^^^^^^^^^^^^^^^^^^^^^^ write access through at ALLOC[0x0] is forbidden +LL | unsafe { *y = 13 } + | ^^^^^^^ write access through at ALLOC[0x0] is forbidden | = help: this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental = help: the accessed tag has state Disabled which forbids this child write access help: the accessed tag was created here, in the initial state Reserved --> $DIR/reservedim_spurious_write.rs:LL:CC | -LL | fn inner(y: &mut Cell, b: IdxBarrier) -> *mut u8 { +LL | fn inner(y: &mut Cell<()>, b: IdxBarrier) -> *mut u8 { | ^ help: the accessed tag later transitioned to Disabled due to a protector release (acting as a foreign write access) on every location previously accessed by this tag --> $DIR/reservedim_spurious_write.rs:LL:CC From 78f6386b623c64160fa3475605c7ebfb065f62bf Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Fri, 12 Jul 2024 10:51:32 +0200 Subject: [PATCH 05/21] Clarify comment in tests/fail/tree_borrows/reservedim_spurious_write.rs Co-authored-by: Ralf Jung --- .../tests/fail/tree_borrows/reservedim_spurious_write.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs index 6ae79be6cc7e8..73f227fee2fcb 100644 --- a/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs +++ b/src/tools/miri/tests/fail/tree_borrows/reservedim_spurious_write.rs @@ -41,8 +41,9 @@ macro_rules! synchronized { } fn main() { - // The conflict occurs one one single location but the example involves - // lazily initialized permissions. + // The conflict occurs on one single location but the example involves + // lazily initialized permissions. We will use `&mut Cell<()>` references + // to `data` to achieve this. let mut data = 0u8; let ptr = SendPtr(std::ptr::addr_of_mut!(data)); let barrier = Arc::new(Barrier::new(2)); From fd81880c9108c4b98839d55fe29884a625f05f02 Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Fri, 12 Jul 2024 15:58:59 +0200 Subject: [PATCH 06/21] Leave a trace of the current suboptimal status of foreign_write --- src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs index 9c19ae76a2d01..8e23257b6c006 100644 --- a/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs +++ b/src/tools/miri/src/borrow_tracker/tree_borrows/perms.rs @@ -146,6 +146,12 @@ mod transition { /// non-protected interior mutable `Reserved` which stay the same. fn foreign_write(state: PermissionPriv, protected: bool) -> Option { Some(match state { + // FIXME: since the fix related to reservedim_spurious_write, it is now possible + // to express these transitions of the state machine without an explicit dependency + // on `protected`: because `ty_is_freeze: false` implies `!protected` then + // the line handling `Reserved { .. } if protected` could be deleted. + // This will however require optimizations to the exhaustive tests because + // fewer initial conditions are valid. Reserved { .. } if protected => Disabled, res @ Reserved { ty_is_freeze: false, .. } => res, _ => Disabled, From e1e5b8a2b69da18b11147aa8d18e731ee32d9819 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Tue, 16 Jul 2024 05:14:32 +0000 Subject: [PATCH 07/21] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index e90d3732ca577..2c29af8b9357f 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -99b7134389e9766462601a2fc4013840b9d31745 +5c8488605624d67b272953bc21d41db60dbd5654 From e5544dc087acc760d987781f48680ce505c1855f Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Sat, 20 Jul 2024 05:04:25 +0000 Subject: [PATCH 08/21] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 2c29af8b9357f..b09d4f11f68ef 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -5c8488605624d67b272953bc21d41db60dbd5654 +9057c3ffec44926d5e149dc13ff3ce1613b69cce From 69b9eab4aba6d13c348e09190785fa9c015baae9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=90=D1=80=D1=82=D1=91=D0=BC=20=D0=9F=D0=B0=D0=B2=D0=BB?= =?UTF-8?q?=D0=BE=D0=B2=20=5BArtyom=20Pavlov=5D?= Date: Mon, 22 Jul 2024 16:51:20 +0300 Subject: [PATCH 09/21] Add `O_NOFOLLOW` flag support --- src/tools/miri/src/shims/unix/fs.rs | 29 +++++++++++++++++-- ...libc-fs-readlink.rs => libc-fs-symlink.rs} | 20 +++++++++++++ src/tools/miri/tests/pass-dep/libc/libc-fs.rs | 9 ++++++ 3 files changed, 55 insertions(+), 3 deletions(-) rename src/tools/miri/tests/pass-dep/libc/{libc-fs-readlink.rs => libc-fs-symlink.rs} (76%) diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index e34aa5c09dfe1..d1218114e0d7a 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -266,7 +266,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let this = self.eval_context_mut(); - let path = this.read_pointer(&args[0])?; + let path_raw = this.read_pointer(&args[0])?; + let path = this.read_path_from_c_str(path_raw)?; let flag = this.read_scalar(&args[1])?.to_i32()?; let mut options = OpenOptions::new(); @@ -366,14 +367,36 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { return Ok(-1); } } + + let o_nofollow = this.eval_libc_i32("O_NOFOLLOW"); + if flag & o_nofollow == o_nofollow { + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.custom_flags(libc::O_NOFOLLOW); + } + // Strictly speaking, this emulation is not equivalent to the O_NOFOLLOW flag behavior: + // the path could change between us checking it here and the later call to `open`. + // But it's good enough for Miri purposes. + #[cfg(not(unix))] + { + // O_NOFOLLOW only fails when the trailing component is a symlink; + // the entire rest of the path can still contain symlinks. + if path.is_symlink() { + let eloop = this.eval_libc("ELOOP"); + this.set_last_error(eloop)?; + return Ok(-1); + } + } + mirror |= o_nofollow; + } + // If `flag` is not equal to `mirror`, there is an unsupported option enabled in `flag`, // then we throw an error. if flag != mirror { throw_unsup_format!("unsupported flags {:#x}", flag & !mirror); } - let path = this.read_path_from_c_str(path)?; - // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { this.reject_in_isolation("`open`", reject_with)?; diff --git a/src/tools/miri/tests/pass-dep/libc/libc-fs-readlink.rs b/src/tools/miri/tests/pass-dep/libc/libc-fs-symlink.rs similarity index 76% rename from src/tools/miri/tests/pass-dep/libc/libc-fs-readlink.rs rename to src/tools/miri/tests/pass-dep/libc/libc-fs-symlink.rs index d72edd7d9e3f2..96ced05cd1e8e 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-fs-readlink.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-fs-symlink.rs @@ -11,6 +11,11 @@ use std::os::unix::ffi::OsStrExt; mod utils; fn main() { + test_readlink(); + test_nofollow_symlink(); +} + +fn test_readlink() { let bytes = b"Hello, World!\n"; let path = utils::prepare_with_content("miri_test_fs_link_target.txt", bytes); let expected_path = path.as_os_str().as_bytes(); @@ -49,3 +54,18 @@ fn main() { assert_eq!(res, -1); assert_eq!(Error::last_os_error().kind(), ErrorKind::NotFound); } + +fn test_nofollow_symlink() { + let bytes = b"Hello, World!\n"; + let path = utils::prepare_with_content("test_nofollow_symlink_target.txt", bytes); + + let symlink_path = utils::prepare("test_nofollow_symlink.txt"); + std::os::unix::fs::symlink(&path, &symlink_path).unwrap(); + + let symlink_cpath = CString::new(symlink_path.as_os_str().as_bytes()).unwrap(); + + let ret = unsafe { libc::open(symlink_cpath.as_ptr(), libc::O_NOFOLLOW | libc::O_CLOEXEC) }; + assert_eq!(ret, -1); + let err = io::Error::last_os_error().raw_os_error().unwrap(); + assert_eq!(err, libc::ELOOP); +} diff --git a/src/tools/miri/tests/pass-dep/libc/libc-fs.rs b/src/tools/miri/tests/pass-dep/libc/libc-fs.rs index eddea92353e3d..5b2bbfbb27d08 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-fs.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-fs.rs @@ -37,6 +37,7 @@ fn main() { test_sync_file_range(); test_isatty(); test_read_and_uninit(); + test_nofollow_not_symlink(); } fn test_file_open_unix_allow_two_args() { @@ -423,3 +424,11 @@ fn test_read_and_uninit() { remove_file(&path).unwrap(); } } + +fn test_nofollow_not_symlink() { + let bytes = b"Hello, World!\n"; + let path = utils::prepare_with_content("test_nofollow_not_symlink.txt", bytes); + let cpath = CString::new(path.as_os_str().as_bytes()).unwrap(); + let ret = unsafe { libc::open(cpath.as_ptr(), libc::O_NOFOLLOW | libc::O_CLOEXEC) }; + assert!(ret >= 0); +} From 06a14f1990d9baf984a4162c32c54f707d317114 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=90=D1=80=D1=82=D1=91=D0=BC=20=D0=9F=D0=B0=D0=B2=D0=BB?= =?UTF-8?q?=D0=BE=D0=B2=20=5BArtyom=20Pavlov=5D?= Date: Mon, 22 Jul 2024 17:01:11 +0300 Subject: [PATCH 10/21] Fix test --- src/tools/miri/tests/pass-dep/libc/libc-fs-symlink.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/tests/pass-dep/libc/libc-fs-symlink.rs b/src/tools/miri/tests/pass-dep/libc/libc-fs-symlink.rs index 96ced05cd1e8e..619c6db3a29de 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-fs-symlink.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-fs-symlink.rs @@ -66,6 +66,6 @@ fn test_nofollow_symlink() { let ret = unsafe { libc::open(symlink_cpath.as_ptr(), libc::O_NOFOLLOW | libc::O_CLOEXEC) }; assert_eq!(ret, -1); - let err = io::Error::last_os_error().raw_os_error().unwrap(); + let err = Error::last_os_error().raw_os_error().unwrap(); assert_eq!(err, libc::ELOOP); } From 56d672e8f7605af25dcce6483a3dda9a3ae29e54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=D0=90=D1=80=D1=82=D1=91=D0=BC=20=D0=9F=D0=B0=D0=B2=D0=BB?= =?UTF-8?q?=D0=BE=D0=B2=20=5BArtyom=20Pavlov=5D?= Date: Mon, 22 Jul 2024 18:51:36 +0300 Subject: [PATCH 11/21] Add `pread` and `pwrite` shims --- src/tools/miri/src/shims/unix/fd.rs | 93 +++++++++++++++---- .../miri/src/shims/unix/foreign_items.rs | 44 ++++++++- src/tools/miri/src/shims/unix/fs.rs | 48 ++++++++++ src/tools/miri/tests/pass/shims/fs.rs | 41 ++++++++ 4 files changed, 205 insertions(+), 21 deletions(-) diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 8fb046b5e64cf..de6d27f5290db 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -36,6 +36,30 @@ pub trait FileDescription: std::fmt::Debug + Any { throw_unsup_format!("cannot write to {}", self.name()); } + /// Reads as much as possible into the given buffer from a given offset, + /// and returns the number of bytes read. + fn pread<'tcx>( + &mut self, + _communicate_allowed: bool, + _bytes: &mut [u8], + _offset: u64, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, io::Result> { + throw_unsup_format!("cannot pread from {}", self.name()); + } + + /// Writes as much as possible from the given buffer starting at a given offset, + /// and returns the number of bytes written. + fn pwrite<'tcx>( + &mut self, + _communicate_allowed: bool, + _bytes: &[u8], + _offset: u64, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, io::Result> { + throw_unsup_format!("cannot pwrite to {}", self.name()); + } + /// Seeks to the given offset (which can be relative to the beginning, end, or current position). /// Returns the new position from the start of the stream. fn seek<'tcx>( @@ -380,7 +404,18 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { Ok((-1).into()) } - fn read(&mut self, fd: i32, buf: Pointer, count: u64) -> InterpResult<'tcx, i64> { + /// Read data from `fd` into buffer specified by `buf` and `count`. + /// + /// If `offset` is `None`, reads data from current cursor position associated with `fd` + /// and updates cursor position on completion. Otherwise, reads from the specified offset + /// and keeps the cursor unchanged. + fn read( + &mut self, + fd: i32, + buf: Pointer, + count: u64, + offset: Option, + ) -> InterpResult<'tcx, i64> { let this = self.eval_context_mut(); // Isolation check is done via `FileDescriptor` trait. @@ -398,25 +433,31 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let communicate = this.machine.communicate(); // We temporarily dup the FD to be able to retain mutable access to `this`. - let Some(file_descriptor) = this.machine.fds.dup(fd) else { + let Some(fd) = this.machine.fds.dup(fd) else { trace!("read: FD not found"); return this.fd_not_found(); }; - trace!("read: FD mapped to {:?}", file_descriptor); + trace!("read: FD mapped to {fd:?}"); // We want to read at most `count` bytes. We are sure that `count` is not negative // because it was a target's `usize`. Also we are sure that its smaller than // `usize::MAX` because it is bounded by the host's `isize`. let mut bytes = vec![0; usize::try_from(count).unwrap()]; - // `File::read` never returns a value larger than `count`, - // so this cannot fail. - let result = file_descriptor - .borrow_mut() - .read(communicate, &mut bytes, this)? - .map(|c| i64::try_from(c).unwrap()); - drop(file_descriptor); - - match result { + let result = match offset { + None => fd.borrow_mut().read(communicate, &mut bytes, this), + Some(offset) => { + let Ok(offset) = u64::try_from(offset) else { + let einval = this.eval_libc("EINVAL"); + this.set_last_error(einval)?; + return Ok(-1); + }; + fd.borrow_mut().pread(communicate, &mut bytes, offset, this) + } + }; + drop(fd); + + // `File::read` never returns a value larger than `count`, so this cannot fail. + match result?.map(|c| i64::try_from(c).unwrap()) { Ok(read_bytes) => { // If reading to `bytes` did not fail, we write those bytes to the buffer. // Crucially, if fewer than `bytes.len()` bytes were read, only write @@ -434,7 +475,13 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { } } - fn write(&mut self, fd: i32, buf: Pointer, count: u64) -> InterpResult<'tcx, i64> { + fn write( + &mut self, + fd: i32, + buf: Pointer, + count: u64, + offset: Option, + ) -> InterpResult<'tcx, i64> { let this = self.eval_context_mut(); // Isolation check is done via `FileDescriptor` trait. @@ -451,16 +498,24 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let bytes = this.read_bytes_ptr_strip_provenance(buf, Size::from_bytes(count))?.to_owned(); // We temporarily dup the FD to be able to retain mutable access to `this`. - let Some(file_descriptor) = this.machine.fds.dup(fd) else { + let Some(fd) = this.machine.fds.dup(fd) else { return this.fd_not_found(); }; - let result = file_descriptor - .borrow_mut() - .write(communicate, &bytes, this)? - .map(|c| i64::try_from(c).unwrap()); - drop(file_descriptor); + let result = match offset { + None => fd.borrow_mut().write(communicate, &bytes, this), + Some(offset) => { + let Ok(offset) = u64::try_from(offset) else { + let einval = this.eval_libc("EINVAL"); + this.set_last_error(einval)?; + return Ok(-1); + }; + fd.borrow_mut().pwrite(communicate, &bytes, offset, this) + } + }; + drop(fd); + let result = result?.map(|c| i64::try_from(c).unwrap()); this.try_unwrap_io_result(result) } } diff --git a/src/tools/miri/src/shims/unix/foreign_items.rs b/src/tools/miri/src/shims/unix/foreign_items.rs index 3a18d62203333..966e590fcc41e 100644 --- a/src/tools/miri/src/shims/unix/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/foreign_items.rs @@ -92,7 +92,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let fd = this.read_scalar(fd)?.to_i32()?; let buf = this.read_pointer(buf)?; let count = this.read_target_usize(count)?; - let result = this.read(fd, buf, count)?; + let result = this.read(fd, buf, count, None)?; this.write_scalar(Scalar::from_target_isize(result, this), dest)?; } "write" => { @@ -101,7 +101,47 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let buf = this.read_pointer(buf)?; let count = this.read_target_usize(n)?; trace!("Called write({:?}, {:?}, {:?})", fd, buf, count); - let result = this.write(fd, buf, count)?; + let result = this.write(fd, buf, count, None)?; + // Now, `result` is the value we return back to the program. + this.write_scalar(Scalar::from_target_isize(result, this), dest)?; + } + "pread" => { + let [fd, buf, count, offset] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; + let fd = this.read_scalar(fd)?.to_i32()?; + let buf = this.read_pointer(buf)?; + let count = this.read_target_usize(count)?; + let offset = this.read_scalar(offset)?.to_int(this.libc_ty_layout("off_t").size)?; + let result = this.read(fd, buf, count, Some(offset))?; + this.write_scalar(Scalar::from_target_isize(result, this), dest)?; + } + "pwrite" => { + let [fd, buf, n, offset] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; + let fd = this.read_scalar(fd)?.to_i32()?; + let buf = this.read_pointer(buf)?; + let count = this.read_target_usize(n)?; + let offset = this.read_scalar(offset)?.to_int(this.libc_ty_layout("off_t").size)?; + trace!("Called pwrite({:?}, {:?}, {:?}, {:?})", fd, buf, count, offset); + let result = this.write(fd, buf, count, Some(offset))?; + // Now, `result` is the value we return back to the program. + this.write_scalar(Scalar::from_target_isize(result, this), dest)?; + } + "pread64" => { + let [fd, buf, count, offset] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; + let fd = this.read_scalar(fd)?.to_i32()?; + let buf = this.read_pointer(buf)?; + let count = this.read_target_usize(count)?; + let offset = this.read_scalar(offset)?.to_int(this.libc_ty_layout("off64_t").size)?; + let result = this.read(fd, buf, count, Some(offset))?; + this.write_scalar(Scalar::from_target_isize(result, this), dest)?; + } + "pwrite64" => { + let [fd, buf, n, offset] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; + let fd = this.read_scalar(fd)?.to_i32()?; + let buf = this.read_pointer(buf)?; + let count = this.read_target_usize(n)?; + let offset = this.read_scalar(offset)?.to_int(this.libc_ty_layout("off64_t").size)?; + trace!("Called pwrite64({:?}, {:?}, {:?}, {:?})", fd, buf, count, offset); + let result = this.write(fd, buf, count, Some(offset))?; // Now, `result` is the value we return back to the program. this.write_scalar(Scalar::from_target_isize(result, this), dest)?; } diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index e34aa5c09dfe1..80268ced48de7 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -49,6 +49,54 @@ impl FileDescription for FileHandle { Ok(self.file.write(bytes)) } + fn pread<'tcx>( + &mut self, + communicate_allowed: bool, + bytes: &mut [u8], + offset: u64, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, io::Result> { + assert!(communicate_allowed, "isolation should have prevented even opening a file"); + // Emulates pread using seek + read + seek to restore cursor position. + // Correctness of this emulation relies on sequential nature of Miri execution. + // The closure is used to emulate `try` block, since we "bubble" `io::Error` using `?`. + let mut f = || { + let cursor_pos = self.file.stream_position()?; + self.file.seek(SeekFrom::Start(offset))?; + let res = self.file.read(bytes); + // Attempt to restore cursor position even if the read has failed + self.file + .seek(SeekFrom::Start(cursor_pos)) + .expect("failed to restore file position, this shouldn't be possible"); + res + }; + Ok(f()) + } + + fn pwrite<'tcx>( + &mut self, + communicate_allowed: bool, + bytes: &[u8], + offset: u64, + _ecx: &mut MiriInterpCx<'tcx>, + ) -> InterpResult<'tcx, io::Result> { + assert!(communicate_allowed, "isolation should have prevented even opening a file"); + // Emulates pwrite using seek + write + seek to restore cursor position. + // Correctness of this emulation relies on sequential nature of Miri execution. + // The closure is used to emulate `try` block, since we "bubble" `io::Error` using `?`. + let mut f = || { + let cursor_pos = self.file.stream_position()?; + self.file.seek(SeekFrom::Start(offset))?; + let res = self.file.write(bytes); + // Attempt to restore cursor position even if the write has failed + self.file + .seek(SeekFrom::Start(cursor_pos)) + .expect("failed to restore file position, this shouldn't be possible"); + res + }; + Ok(f()) + } + fn seek<'tcx>( &mut self, communicate_allowed: bool, diff --git a/src/tools/miri/tests/pass/shims/fs.rs b/src/tools/miri/tests/pass/shims/fs.rs index 16d3e8cab30ab..70e375b098128 100644 --- a/src/tools/miri/tests/pass/shims/fs.rs +++ b/src/tools/miri/tests/pass/shims/fs.rs @@ -30,6 +30,8 @@ fn main() { test_directory(); test_canonicalize(); test_from_raw_os_error(); + #[cfg(unix)] + test_pread_pwrite(); } fn test_path_conversion() { @@ -303,3 +305,42 @@ fn test_from_raw_os_error() { // Make sure we can also format this. let _ = format!("{error:?}"); } + +#[cfg(unix)] +fn test_pread_pwrite() { + use std::os::unix::fs::FileExt; + + let bytes = b"hello world"; + let path = utils::prepare_with_content("miri_test_fs_pread_pwrite.txt", bytes); + let mut f = OpenOptions::new().read(true).write(true).open(path).unwrap(); + + let mut buf1 = [0u8; 3]; + f.seek(SeekFrom::Start(5)).unwrap(); + + // Check that we get expected result after seek + f.read_exact(&mut buf1).unwrap(); + assert_eq!(&buf1, b" wo"); + f.seek(SeekFrom::Start(5)).unwrap(); + + // Check pread + f.read_exact_at(&mut buf1, 2).unwrap(); + assert_eq!(&buf1, b"llo"); + f.read_exact_at(&mut buf1, 6).unwrap(); + assert_eq!(&buf1, b"wor"); + + // Ensure that cursor position is not changed + f.read_exact(&mut buf1).unwrap(); + assert_eq!(&buf1, b" wo"); + f.seek(SeekFrom::Start(5)).unwrap(); + + // Check pwrite + f.write_all_at(b" mo", 6).unwrap(); + + let mut buf2 = [0u8; 11]; + f.read_exact_at(&mut buf2, 0).unwrap(); + assert_eq!(&buf2, b"hello mold"); + + // Ensure that cursor position is not changed + f.read_exact(&mut buf1).unwrap(); + assert_eq!(&buf1, b" m"); +} From c646256f0639002cd9e5872f711f09ec77630a74 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Wed, 24 Jul 2024 05:00:48 +0000 Subject: [PATCH 12/21] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index b09d4f11f68ef..60497630ae0d0 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -9057c3ffec44926d5e149dc13ff3ce1613b69cce +42103d69b73fb4e9d03d5cf66ec12985bb526f6e From a0088d7a813a1f63e5af4d6edc5d2c50a0b3702e Mon Sep 17 00:00:00 2001 From: Konstantinos Andrikopoulos Date: Sat, 20 Jul 2024 11:28:48 +0200 Subject: [PATCH 13/21] Allow getpid in isolation mode, add gettid support In order to support gettid when isolation is enabled and when it is disabled, and satisfy its requirement that: In a single-threaded process, the thread ID is equal to the process ID (PID, as returned by getpid(2)). we define the thread ID to be getpid() + . Since the internal thread id of the main thread is zero, this will satisfy that requirement. However, getpid for now was only supported when isolation was disabled. To support getpid in isolation mode, we return a hardcoded value (1000) and return that instead of the real PID. --- src/tools/miri/src/shims/env.rs | 5 +++++ src/tools/miri/src/shims/unix/env.rs | 17 +++++++++++--- .../src/shims/unix/linux/foreign_items.rs | 5 +++++ src/tools/miri/src/shims/windows/env.rs | 3 +-- src/tools/miri/tests/pass-dep/libc/gettid.rs | 22 +++++++++++++++++++ src/tools/miri/tests/pass/getpid.rs | 15 +++++++++++-- 6 files changed, 60 insertions(+), 7 deletions(-) create mode 100644 src/tools/miri/tests/pass-dep/libc/gettid.rs diff --git a/src/tools/miri/src/shims/env.rs b/src/tools/miri/src/shims/env.rs index 7ad395cccb799..6586ea8e48cff 100644 --- a/src/tools/miri/src/shims/env.rs +++ b/src/tools/miri/src/shims/env.rs @@ -108,4 +108,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { EnvVars::Windows(vars) => vars.get(name), } } + + fn get_pid(&self) -> u32 { + let this = self.eval_context_ref(); + if this.machine.communicate() { std::process::id() } else { 1000 } + } } diff --git a/src/tools/miri/src/shims/unix/env.rs b/src/tools/miri/src/shims/unix/env.rs index 405431f4327db..3b8ad65195b8a 100644 --- a/src/tools/miri/src/shims/unix/env.rs +++ b/src/tools/miri/src/shims/unix/env.rs @@ -274,12 +274,23 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let this = self.eval_context_mut(); this.assert_target_os_is_unix("getpid"); - this.check_no_isolation("`getpid`")?; - // The reason we need to do this wacky of a conversion is because // `libc::getpid` returns an i32, however, `std::process::id()` return an u32. // So we un-do the conversion that stdlib does and turn it back into an i32. #[allow(clippy::cast_possible_wrap)] - Ok(std::process::id() as i32) + Ok(this.get_pid() as i32) + } + + fn linux_gettid(&mut self) -> InterpResult<'tcx, i32> { + let this = self.eval_context_ref(); + this.assert_target_os("linux", "gettid"); + + let index = this.machine.threads.active_thread().to_u32(); + + // Compute a TID for this thread, ensuring that the main thread has PID == TID. + let tid = this.get_pid().strict_add(index); + + #[allow(clippy::cast_possible_wrap)] + Ok(tid as i32) } } diff --git a/src/tools/miri/src/shims/unix/linux/foreign_items.rs b/src/tools/miri/src/shims/unix/linux/foreign_items.rs index 95bee38cd7835..20c6a23479421 100644 --- a/src/tools/miri/src/shims/unix/linux/foreign_items.rs +++ b/src/tools/miri/src/shims/unix/linux/foreign_items.rs @@ -94,6 +94,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { )?; this.write_scalar(res, dest)?; } + "gettid" => { + let [] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; + let result = this.linux_gettid()?; + this.write_scalar(Scalar::from_i32(result), dest)?; + } // Dynamically invoked syscalls "syscall" => { diff --git a/src/tools/miri/src/shims/windows/env.rs b/src/tools/miri/src/shims/windows/env.rs index ed3eb69798637..77ae06bd5c2d8 100644 --- a/src/tools/miri/src/shims/windows/env.rs +++ b/src/tools/miri/src/shims/windows/env.rs @@ -200,9 +200,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { fn GetCurrentProcessId(&mut self) -> InterpResult<'tcx, u32> { let this = self.eval_context_mut(); this.assert_target_os("windows", "GetCurrentProcessId"); - this.check_no_isolation("`GetCurrentProcessId`")?; - Ok(std::process::id()) + Ok(this.get_pid()) } #[allow(non_snake_case)] diff --git a/src/tools/miri/tests/pass-dep/libc/gettid.rs b/src/tools/miri/tests/pass-dep/libc/gettid.rs new file mode 100644 index 0000000000000..87405b02ac35d --- /dev/null +++ b/src/tools/miri/tests/pass-dep/libc/gettid.rs @@ -0,0 +1,22 @@ +//@only-target-linux +//@revisions: with_isolation without_isolation +//@[without_isolation] compile-flags: -Zmiri-disable-isolation + +use libc::{getpid, gettid}; +use std::thread; + +fn main() { + thread::spawn(|| { + // Test that in isolation mode a deterministic value will be returned. + // The value 1001 is not important, we only care that whatever the value + // is, won't change from execution to execution. + #[cfg(with_isolation)] + assert_eq!(unsafe { gettid() }, 1001); + + assert_ne!(unsafe { gettid() }, unsafe { getpid() }); + }); + + // Test that the thread ID of the main thread is the same as the process + // ID. + assert_eq!(unsafe { gettid() }, unsafe { getpid() }); +} diff --git a/src/tools/miri/tests/pass/getpid.rs b/src/tools/miri/tests/pass/getpid.rs index 733545462ebc0..f350fafff4a26 100644 --- a/src/tools/miri/tests/pass/getpid.rs +++ b/src/tools/miri/tests/pass/getpid.rs @@ -1,9 +1,20 @@ -//@compile-flags: -Zmiri-disable-isolation +//@revisions: with_isolation without_isolation +//@[without_isolation] compile-flags: -Zmiri-disable-isolation fn getpid() -> u32 { std::process::id() } fn main() { - getpid(); + let pid = getpid(); + + std::thread::spawn(move || { + assert_eq!(getpid(), pid); + }); + + // Test that in isolation mode a deterministic value will be returned. + // The value 1000 is not important, we only care that whatever the value + // is, won't change from execution to execution. + #[cfg(with_isolation)] + assert_eq!(pid, 1000); } From c45f4646393b4ecab0c75b251f409bf12a1999da Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 22 Jun 2024 16:45:20 +0200 Subject: [PATCH 14/21] show warning when Stacked Borrows skips a reborrow due to 'extern type' --- .../src/borrow_tracker/stacked_borrows/mod.rs | 15 ++++++++- src/tools/miri/src/diagnostics.rs | 31 ++++++++++++++++--- .../fail/extern-type-field-offset.stderr | 13 +++++++- .../ptr_metadata_uninit_slice_len.stderr | 10 +++--- src/tools/miri/tests/pass/box.stack.stderr | 10 +++--- .../miri/tests/pass/extern_types.stack.stderr | 21 ++++++++++--- .../stacked-borrows/issue-miri-2389.stderr | 10 +++--- 7 files changed, 83 insertions(+), 27 deletions(-) diff --git a/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs b/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs index 603733f9dc000..1d75486a78189 100644 --- a/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs +++ b/src/tools/miri/src/borrow_tracker/stacked_borrows/mod.rs @@ -5,6 +5,7 @@ pub mod diagnostics; mod item; mod stack; +use std::cell::RefCell; use std::cmp; use std::fmt::Write; use std::mem; @@ -820,7 +821,19 @@ trait EvalContextPrivExt<'tcx, 'ecx>: crate::MiriInterpCxExt<'tcx> { // See https://github.com/rust-lang/unsafe-code-guidelines/issues/276. let size = match size { Some(size) => size, - None => return Ok(place.clone()), + None => { + // The first time this happens, show a warning. + thread_local! { static WARNING_SHOWN: RefCell = const { RefCell::new(false) }; } + WARNING_SHOWN.with_borrow_mut(|shown| { + if *shown { + return; + } + // Not yet shown. Show it! + *shown = true; + this.emit_diagnostic(NonHaltingDiagnostic::ExternTypeReborrow); + }); + return Ok(place.clone()); + } }; // Compute new borrow. diff --git a/src/tools/miri/src/diagnostics.rs b/src/tools/miri/src/diagnostics.rs index 647d7d44bb1bb..45a8a54bc298a 100644 --- a/src/tools/miri/src/diagnostics.rs +++ b/src/tools/miri/src/diagnostics.rs @@ -130,6 +130,7 @@ pub enum NonHaltingDiagnostic { WeakMemoryOutdatedLoad { ptr: Pointer, }, + ExternTypeReborrow, } /// Level of Miri specific diagnostics @@ -593,6 +594,8 @@ impl<'tcx> MiriMachine<'tcx> { RejectedIsolatedOp(_) => ("operation rejected by isolation".to_string(), DiagLevel::Warning), Int2Ptr { .. } => ("integer-to-pointer cast".to_string(), DiagLevel::Warning), + ExternTypeReborrow => + ("reborrow of reference to `extern type`".to_string(), DiagLevel::Warning), CreatedPointerTag(..) | PoppedPointerTag(..) | CreatedCallId(..) @@ -630,6 +633,8 @@ impl<'tcx> MiriMachine<'tcx> { Int2Ptr { .. } => format!("integer-to-pointer cast"), WeakMemoryOutdatedLoad { ptr } => format!("weak memory emulation: outdated value returned from load at {ptr}"), + ExternTypeReborrow => + format!("reborrow of a reference to `extern type` is not properly supported"), }; let notes = match &e { @@ -647,34 +652,50 @@ impl<'tcx> MiriMachine<'tcx> { ( None, format!( - "This program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program." + "this program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program" ), ), ( None, format!( - "See https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation." + "see https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation" ), ), ( None, format!( - "To ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead." + "to ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead" ), ), ( None, format!( - "You can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics." + "you can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics" ), ), ( None, format!( - "Alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning." + "alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning" ), ), ], + ExternTypeReborrow => { + vec![ + ( + None, + format!( + "`extern type` are not compatible with the Stacked Borrows aliasing model implemented by Miri; Miri may miss bugs in this code" + ), + ), + ( + None, + format!( + "try running with `MIRIFLAGS=-Zmiri-tree-borrows` to use the more permissive but also even more experimental Tree Borrows aliasing checks instead" + ), + ), + ] + } _ => vec![], }; diff --git a/src/tools/miri/tests/fail/extern-type-field-offset.stderr b/src/tools/miri/tests/fail/extern-type-field-offset.stderr index 3ed5732b4eb06..c07b63e0c0384 100644 --- a/src/tools/miri/tests/fail/extern-type-field-offset.stderr +++ b/src/tools/miri/tests/fail/extern-type-field-offset.stderr @@ -1,3 +1,14 @@ +warning: reborrow of reference to `extern type` + --> $DIR/extern-type-field-offset.rs:LL:CC + | +LL | let x: &Newtype = unsafe { &*(&buf as *const _ as *const Newtype) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ reborrow of a reference to `extern type` is not properly supported + | + = help: `extern type` are not compatible with the Stacked Borrows aliasing model implemented by Miri; Miri may miss bugs in this code + = help: try running with `MIRIFLAGS=-Zmiri-tree-borrows` to use the more permissive but also even more experimental Tree Borrows aliasing checks instead + = note: BACKTRACE: + = note: inside `main` at $DIR/extern-type-field-offset.rs:LL:CC + error: unsupported operation: `extern type` field does not have a known offset --> $DIR/extern-type-field-offset.rs:LL:CC | @@ -10,5 +21,5 @@ LL | let _field = &x.a; note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace -error: aborting due to 1 previous error +error: aborting due to 1 previous error; 1 warning emitted diff --git a/src/tools/miri/tests/fail/intrinsics/ptr_metadata_uninit_slice_len.stderr b/src/tools/miri/tests/fail/intrinsics/ptr_metadata_uninit_slice_len.stderr index 217bc82010df9..84023cf793713 100644 --- a/src/tools/miri/tests/fail/intrinsics/ptr_metadata_uninit_slice_len.stderr +++ b/src/tools/miri/tests/fail/intrinsics/ptr_metadata_uninit_slice_len.stderr @@ -4,11 +4,11 @@ warning: integer-to-pointer cast LL | (*p.as_mut_ptr().cast::<[*const i32; 2]>())[0] = 4 as *const i32; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ integer-to-pointer cast | - = help: This program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program. - = help: See https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation. - = help: To ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead. - = help: You can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics. - = help: Alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning. + = help: this program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program + = help: see https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation + = help: to ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead + = help: you can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics + = help: alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning = note: BACKTRACE: = note: inside `main` at $DIR/ptr_metadata_uninit_slice_len.rs:LL:CC diff --git a/src/tools/miri/tests/pass/box.stack.stderr b/src/tools/miri/tests/pass/box.stack.stderr index 341f84c8992df..f2d01b518fc80 100644 --- a/src/tools/miri/tests/pass/box.stack.stderr +++ b/src/tools/miri/tests/pass/box.stack.stderr @@ -4,11 +4,11 @@ warning: integer-to-pointer cast LL | let r2 = ((r as usize) + 0) as *mut i32; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ integer-to-pointer cast | - = help: This program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program. - = help: See https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation. - = help: To ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead. - = help: You can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics. - = help: Alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning. + = help: this program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program + = help: see https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation + = help: to ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead + = help: you can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics + = help: alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning = note: BACKTRACE: = note: inside `into_raw` at $DIR/box.rs:LL:CC note: inside `main` diff --git a/src/tools/miri/tests/pass/extern_types.stack.stderr b/src/tools/miri/tests/pass/extern_types.stack.stderr index 03a9167abbc7c..9b6f632eb5a22 100644 --- a/src/tools/miri/tests/pass/extern_types.stack.stderr +++ b/src/tools/miri/tests/pass/extern_types.stack.stderr @@ -4,11 +4,22 @@ warning: integer-to-pointer cast LL | let x: &Foo = unsafe { &*(16 as *const Foo) }; | ^^^^^^^^^^^^^^^^^^ integer-to-pointer cast | - = help: This program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program. - = help: See https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation. - = help: To ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead. - = help: You can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics. - = help: Alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning. + = help: this program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program + = help: see https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation + = help: to ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead + = help: you can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics + = help: alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning + = note: BACKTRACE: + = note: inside `main` at $DIR/extern_types.rs:LL:CC + +warning: reborrow of reference to `extern type` + --> $DIR/extern_types.rs:LL:CC + | +LL | let x: &Foo = unsafe { &*(16 as *const Foo) }; + | ^^^^^^^^^^^^^^^^^^^^ reborrow of a reference to `extern type` is not properly supported + | + = help: `extern type` are not compatible with the Stacked Borrows aliasing model implemented by Miri; Miri may miss bugs in this code + = help: try running with `MIRIFLAGS=-Zmiri-tree-borrows` to use the more permissive but also even more experimental Tree Borrows aliasing checks instead = note: BACKTRACE: = note: inside `main` at $DIR/extern_types.rs:LL:CC diff --git a/src/tools/miri/tests/pass/stacked-borrows/issue-miri-2389.stderr b/src/tools/miri/tests/pass/stacked-borrows/issue-miri-2389.stderr index b0e1adf27d183..216bb6c76bc9a 100644 --- a/src/tools/miri/tests/pass/stacked-borrows/issue-miri-2389.stderr +++ b/src/tools/miri/tests/pass/stacked-borrows/issue-miri-2389.stderr @@ -4,11 +4,11 @@ warning: integer-to-pointer cast LL | let wildcard = &root0 as *const Cell as usize as *const Cell; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ integer-to-pointer cast | - = help: This program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program. - = help: See https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation. - = help: To ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead. - = help: You can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics. - = help: Alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning. + = help: this program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program + = help: see https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation + = help: to ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead + = help: you can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics + = help: alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning = note: BACKTRACE: = note: inside `main` at $DIR/issue-miri-2389.rs:LL:CC From 6da04f95a7233089cf714d24a160a5dbaa8d84b9 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Thu, 25 Jul 2024 05:10:15 +0000 Subject: [PATCH 15/21] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 60497630ae0d0..9868188eeee70 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -42103d69b73fb4e9d03d5cf66ec12985bb526f6e +e7d66eac5e8e8f60370c98d186aee9fa0ebd7845 From b5490357bac87ac70ca6f4729bd7fd127529eff5 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Fri, 26 Jul 2024 05:00:25 +0000 Subject: [PATCH 16/21] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index 9868188eeee70..e1a6084b22eda 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -e7d66eac5e8e8f60370c98d186aee9fa0ebd7845 +72d73cec61aa8f85901358cd5d386d5dd066fe52 From 7bc7e6789ac0cd4bab726f3d18c8e1201674f1fc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 26 Jul 2024 08:53:38 +0200 Subject: [PATCH 17/21] better diagnostics for Tree Borrows + int2ptr casts --- src/tools/miri/src/bin/miri.rs | 8 ++ src/tools/miri/src/borrow_tracker/mod.rs | 4 + src/tools/miri/src/diagnostics.rs | 23 +++-- src/tools/miri/tests/pass/adjacent-allocs.rs | 2 - src/tools/miri/tests/pass/box.rs | 3 +- src/tools/miri/tests/pass/box.stack.stderr | 33 ------- .../pass/{box.stack.stdout => box.stdout} | 0 src/tools/miri/tests/pass/box.tree.stdout | 3 - src/tools/miri/tests/pass/extern_types.rs | 8 +- .../miri/tests/pass/extern_types.stack.stderr | 18 +--- src/tools/miri/tests/pass/intptrcast.rs | 2 - src/tools/miri/tests/pass/pointers.rs | 2 - src/tools/miri/tests/pass/ptr_int_casts.rs | 3 +- .../miri/tests/pass/ptr_int_casts.tree.stderr | 89 +++++++++++++++++++ .../miri/tests/pass/ptr_int_from_exposed.rs | 3 +- .../pass/ptr_int_from_exposed.tree.stderr | 19 ++++ 16 files changed, 150 insertions(+), 70 deletions(-) delete mode 100644 src/tools/miri/tests/pass/box.stack.stderr rename src/tools/miri/tests/pass/{box.stack.stdout => box.stdout} (100%) delete mode 100644 src/tools/miri/tests/pass/box.tree.stdout create mode 100644 src/tools/miri/tests/pass/ptr_int_casts.tree.stderr create mode 100644 src/tools/miri/tests/pass/ptr_int_from_exposed.tree.stderr diff --git a/src/tools/miri/src/bin/miri.rs b/src/tools/miri/src/bin/miri.rs index 9f3fa075f38f6..25b154a8206c9 100644 --- a/src/tools/miri/src/bin/miri.rs +++ b/src/tools/miri/src/bin/miri.rs @@ -620,6 +620,14 @@ fn main() { "-Zmiri-unique-is-unique only has an effect when -Zmiri-tree-borrows is also used" ); } + // Tree Borrows + permissive provenance does not work. + if miri_config.provenance_mode == ProvenanceMode::Permissive + && matches!(miri_config.borrow_tracker, Some(BorrowTrackerMethod::TreeBorrows)) + { + show_error!( + "Tree Borrows does not support integer-to-pointer casts, and is hence not compatible with permissive provenance" + ); + } debug!("rustc arguments: {:?}", rustc_args); debug!("crate arguments: {:?}", miri_config.args); diff --git a/src/tools/miri/src/borrow_tracker/mod.rs b/src/tools/miri/src/borrow_tracker/mod.rs index c9e7e300593bc..d537a7fbc1798 100644 --- a/src/tools/miri/src/borrow_tracker/mod.rs +++ b/src/tools/miri/src/borrow_tracker/mod.rs @@ -232,6 +232,10 @@ impl GlobalStateInner { pub fn remove_unreachable_allocs(&mut self, allocs: &LiveAllocs<'_, '_>) { self.root_ptr_tags.retain(|id, _| allocs.is_live(*id)); } + + pub fn borrow_tracker_method(&self) -> BorrowTrackerMethod { + self.borrow_tracker_method + } } /// Which borrow tracking method to use diff --git a/src/tools/miri/src/diagnostics.rs b/src/tools/miri/src/diagnostics.rs index 45a8a54bc298a..6d10a21293b65 100644 --- a/src/tools/miri/src/diagnostics.rs +++ b/src/tools/miri/src/diagnostics.rs @@ -647,8 +647,8 @@ impl<'tcx> MiriMachine<'tcx> { }; let helps = match &e { - Int2Ptr { details: true } => - vec![ + Int2Ptr { details: true } => { + let mut v = vec![ ( None, format!( @@ -673,13 +673,26 @@ impl<'tcx> MiriMachine<'tcx> { "you can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics" ), ), - ( + ]; + if self.borrow_tracker.as_ref().is_some_and(|b| { + matches!(b.borrow().borrow_tracker_method(), BorrowTrackerMethod::TreeBorrows) + }) { + v.push(( + None, + format!( + "Tree Borrows does not support integer-to-pointer casts, so the program is likely to go wrong when this pointer gets used" + ), + )); + } else { + v.push(( None, format!( "alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning" ), - ), - ], + )); + } + v + } ExternTypeReborrow => { vec![ ( diff --git a/src/tools/miri/tests/pass/adjacent-allocs.rs b/src/tools/miri/tests/pass/adjacent-allocs.rs index 8be4bdac7e1bb..711c54fd68ad5 100644 --- a/src/tools/miri/tests/pass/adjacent-allocs.rs +++ b/src/tools/miri/tests/pass/adjacent-allocs.rs @@ -1,5 +1,3 @@ -//@revisions: stack tree -//@[tree]compile-flags: -Zmiri-tree-borrows //@compile-flags: -Zmiri-permissive-provenance fn ensure_allocs_can_be_adjacent() { diff --git a/src/tools/miri/tests/pass/box.rs b/src/tools/miri/tests/pass/box.rs index 174bf8be30b22..693209c04568f 100644 --- a/src/tools/miri/tests/pass/box.rs +++ b/src/tools/miri/tests/pass/box.rs @@ -1,5 +1,4 @@ -//@revisions: stack tree -//@[tree]compile-flags: -Zmiri-tree-borrows -Zmiri-permissive-provenance +//@compile-flags: -Zmiri-permissive-provenance #![feature(ptr_internals)] fn main() { diff --git a/src/tools/miri/tests/pass/box.stack.stderr b/src/tools/miri/tests/pass/box.stack.stderr deleted file mode 100644 index f2d01b518fc80..0000000000000 --- a/src/tools/miri/tests/pass/box.stack.stderr +++ /dev/null @@ -1,33 +0,0 @@ -warning: integer-to-pointer cast - --> $DIR/box.rs:LL:CC - | -LL | let r2 = ((r as usize) + 0) as *mut i32; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ integer-to-pointer cast - | - = help: this program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program - = help: see https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation - = help: to ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead - = help: you can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics - = help: alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning - = note: BACKTRACE: - = note: inside `into_raw` at $DIR/box.rs:LL:CC -note: inside `main` - --> $DIR/box.rs:LL:CC - | -LL | into_raw(); - | ^^^^^^^^^^ - -warning: integer-to-pointer cast - --> $DIR/box.rs:LL:CC - | -LL | let r = ((u.as_ptr() as usize) + 0) as *mut i32; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ integer-to-pointer cast - | - = note: BACKTRACE: - = note: inside `into_unique` at $DIR/box.rs:LL:CC -note: inside `main` - --> $DIR/box.rs:LL:CC - | -LL | into_unique(); - | ^^^^^^^^^^^^^ - diff --git a/src/tools/miri/tests/pass/box.stack.stdout b/src/tools/miri/tests/pass/box.stdout similarity index 100% rename from src/tools/miri/tests/pass/box.stack.stdout rename to src/tools/miri/tests/pass/box.stdout diff --git a/src/tools/miri/tests/pass/box.tree.stdout b/src/tools/miri/tests/pass/box.tree.stdout deleted file mode 100644 index 230ef368da644..0000000000000 --- a/src/tools/miri/tests/pass/box.tree.stdout +++ /dev/null @@ -1,3 +0,0 @@ -pair_foo = PairFoo { fst: Foo(42), snd: Foo(1337) } -foo #0 = Foo(42) -foo #1 = Foo(1337) diff --git a/src/tools/miri/tests/pass/extern_types.rs b/src/tools/miri/tests/pass/extern_types.rs index 7ac93577f0cd5..eade5c6ac0874 100644 --- a/src/tools/miri/tests/pass/extern_types.rs +++ b/src/tools/miri/tests/pass/extern_types.rs @@ -1,12 +1,14 @@ //@revisions: stack tree -//@[tree]compile-flags: -Zmiri-tree-borrows -Zmiri-permissive-provenance -#![feature(extern_types)] +//@[tree]compile-flags: -Zmiri-tree-borrows +#![feature(extern_types, strict_provenance)] + +use std::ptr; extern "C" { type Foo; } fn main() { - let x: &Foo = unsafe { &*(16 as *const Foo) }; + let x: &Foo = unsafe { &*(ptr::without_provenance::<()>(16) as *const Foo) }; let _y: &Foo = &*x; } diff --git a/src/tools/miri/tests/pass/extern_types.stack.stderr b/src/tools/miri/tests/pass/extern_types.stack.stderr index 9b6f632eb5a22..2c9fc0192af83 100644 --- a/src/tools/miri/tests/pass/extern_types.stack.stderr +++ b/src/tools/miri/tests/pass/extern_types.stack.stderr @@ -1,22 +1,8 @@ -warning: integer-to-pointer cast - --> $DIR/extern_types.rs:LL:CC - | -LL | let x: &Foo = unsafe { &*(16 as *const Foo) }; - | ^^^^^^^^^^^^^^^^^^ integer-to-pointer cast - | - = help: this program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program - = help: see https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation - = help: to ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead - = help: you can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics - = help: alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning - = note: BACKTRACE: - = note: inside `main` at $DIR/extern_types.rs:LL:CC - warning: reborrow of reference to `extern type` --> $DIR/extern_types.rs:LL:CC | -LL | let x: &Foo = unsafe { &*(16 as *const Foo) }; - | ^^^^^^^^^^^^^^^^^^^^ reborrow of a reference to `extern type` is not properly supported +LL | let x: &Foo = unsafe { &*(ptr::without_provenance::<()>(16) as *const Foo) }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ reborrow of a reference to `extern type` is not properly supported | = help: `extern type` are not compatible with the Stacked Borrows aliasing model implemented by Miri; Miri may miss bugs in this code = help: try running with `MIRIFLAGS=-Zmiri-tree-borrows` to use the more permissive but also even more experimental Tree Borrows aliasing checks instead diff --git a/src/tools/miri/tests/pass/intptrcast.rs b/src/tools/miri/tests/pass/intptrcast.rs index fb1a1dfae5d14..a304f2751bdb6 100644 --- a/src/tools/miri/tests/pass/intptrcast.rs +++ b/src/tools/miri/tests/pass/intptrcast.rs @@ -1,5 +1,3 @@ -//@revisions: stack tree -//@[tree]compile-flags: -Zmiri-tree-borrows //@compile-flags: -Zmiri-permissive-provenance use std::mem; diff --git a/src/tools/miri/tests/pass/pointers.rs b/src/tools/miri/tests/pass/pointers.rs index c7b720dafa20b..280a815abc430 100644 --- a/src/tools/miri/tests/pass/pointers.rs +++ b/src/tools/miri/tests/pass/pointers.rs @@ -1,5 +1,3 @@ -//@revisions: stack tree -//@[tree]compile-flags: -Zmiri-tree-borrows //@compile-flags: -Zmiri-permissive-provenance #![feature(ptr_metadata, const_raw_ptr_comparison)] #![allow(ambiguous_wide_pointer_comparisons)] diff --git a/src/tools/miri/tests/pass/ptr_int_casts.rs b/src/tools/miri/tests/pass/ptr_int_casts.rs index a2fcd098107a6..684e8f6ec7bdd 100644 --- a/src/tools/miri/tests/pass/ptr_int_casts.rs +++ b/src/tools/miri/tests/pass/ptr_int_casts.rs @@ -1,6 +1,7 @@ //@revisions: stack tree +// Tree Borrows doesn't support int2ptr casts, but let's make sure we don't immediately crash either. //@[tree]compile-flags: -Zmiri-tree-borrows -//@compile-flags: -Zmiri-permissive-provenance +//@[stack]compile-flags: -Zmiri-permissive-provenance use std::mem; use std::ptr; diff --git a/src/tools/miri/tests/pass/ptr_int_casts.tree.stderr b/src/tools/miri/tests/pass/ptr_int_casts.tree.stderr new file mode 100644 index 0000000000000..a34474ee0d627 --- /dev/null +++ b/src/tools/miri/tests/pass/ptr_int_casts.tree.stderr @@ -0,0 +1,89 @@ +warning: integer-to-pointer cast + --> $DIR/ptr_int_casts.rs:LL:CC + | +LL | assert_eq!(1 as *const i32 as usize, 1); + | ^^^^^^^^^^^^^^^ integer-to-pointer cast + | + = help: this program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program + = help: see https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation + = help: to ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead + = help: you can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics + = help: Tree Borrows does not support integer-to-pointer casts, so the program is likely to go wrong when this pointer gets used + = note: BACKTRACE: + = note: inside `ptr_int_casts` at $DIR/ptr_int_casts.rs:LL:CC +note: inside `main` + --> $DIR/ptr_int_casts.rs:LL:CC + | +LL | ptr_int_casts(); + | ^^^^^^^^^^^^^^^ + +warning: integer-to-pointer cast + --> $DIR/ptr_int_casts.rs:LL:CC + | +LL | assert_eq!((1 as *const i32).wrapping_offset(4) as usize, 1 + 4 * 4); + | ^^^^^^^^^^^^^^^^^ integer-to-pointer cast + | + = note: BACKTRACE: + = note: inside `ptr_int_casts` at $DIR/ptr_int_casts.rs:LL:CC +note: inside `main` + --> $DIR/ptr_int_casts.rs:LL:CC + | +LL | ptr_int_casts(); + | ^^^^^^^^^^^^^^^ + +warning: integer-to-pointer cast + --> $DIR/ptr_int_casts.rs:LL:CC + | +LL | *val = (1 as *const u8).wrapping_offset(-4); + | ^^^^^^^^^^^^^^^^ integer-to-pointer cast + | + = note: BACKTRACE: + = note: inside `ptr_int_casts` at $DIR/ptr_int_casts.rs:LL:CC +note: inside `main` + --> $DIR/ptr_int_casts.rs:LL:CC + | +LL | ptr_int_casts(); + | ^^^^^^^^^^^^^^^ + +warning: integer-to-pointer cast + --> $DIR/ptr_int_casts.rs:LL:CC + | +LL | let y = y as *const _; + | ^^^^^^^^^^^^^ integer-to-pointer cast + | + = note: BACKTRACE: + = note: inside `ptr_int_casts` at $DIR/ptr_int_casts.rs:LL:CC +note: inside `main` + --> $DIR/ptr_int_casts.rs:LL:CC + | +LL | ptr_int_casts(); + | ^^^^^^^^^^^^^^^ + +warning: integer-to-pointer cast + --> $DIR/ptr_int_casts.rs:LL:CC + | +LL | let x: fn() -> i32 = unsafe { mem::transmute(y as *mut u8) }; + | ^^^^^^^^^^^^ integer-to-pointer cast + | + = note: BACKTRACE: + = note: inside `ptr_int_casts` at $DIR/ptr_int_casts.rs:LL:CC +note: inside `main` + --> $DIR/ptr_int_casts.rs:LL:CC + | +LL | ptr_int_casts(); + | ^^^^^^^^^^^^^^^ + +warning: integer-to-pointer cast + --> $DIR/ptr_int_casts.rs:LL:CC + | +LL | assert_eq!((-1i32) as usize as *const i32 as usize, (-1i32) as usize); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ integer-to-pointer cast + | + = note: BACKTRACE: + = note: inside `ptr_int_casts` at $DIR/ptr_int_casts.rs:LL:CC +note: inside `main` + --> $DIR/ptr_int_casts.rs:LL:CC + | +LL | ptr_int_casts(); + | ^^^^^^^^^^^^^^^ + diff --git a/src/tools/miri/tests/pass/ptr_int_from_exposed.rs b/src/tools/miri/tests/pass/ptr_int_from_exposed.rs index 5690d7865bbc3..f6ebf0632a179 100644 --- a/src/tools/miri/tests/pass/ptr_int_from_exposed.rs +++ b/src/tools/miri/tests/pass/ptr_int_from_exposed.rs @@ -1,6 +1,7 @@ //@revisions: stack tree +// Tree Borrows doesn't support int2ptr casts, but let's make sure we don't immediately crash either. //@[tree]compile-flags: -Zmiri-tree-borrows -//@compile-flags: -Zmiri-permissive-provenance +//@[stack]compile-flags: -Zmiri-permissive-provenance #![feature(strict_provenance, exposed_provenance)] use std::ptr; diff --git a/src/tools/miri/tests/pass/ptr_int_from_exposed.tree.stderr b/src/tools/miri/tests/pass/ptr_int_from_exposed.tree.stderr new file mode 100644 index 0000000000000..614b0d26a6304 --- /dev/null +++ b/src/tools/miri/tests/pass/ptr_int_from_exposed.tree.stderr @@ -0,0 +1,19 @@ +warning: integer-to-pointer cast + --> $DIR/ptr_int_from_exposed.rs:LL:CC + | +LL | let ptr = ptr::with_exposed_provenance::(x_usize).wrapping_offset(-128); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ integer-to-pointer cast + | + = help: this program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program + = help: see https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation + = help: to ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead + = help: you can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics + = help: Tree Borrows does not support integer-to-pointer casts, so the program is likely to go wrong when this pointer gets used + = note: BACKTRACE: + = note: inside `ptr_roundtrip_out_of_bounds` at $DIR/ptr_int_from_exposed.rs:LL:CC +note: inside `main` + --> $DIR/ptr_int_from_exposed.rs:LL:CC + | +LL | ptr_roundtrip_out_of_bounds(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + From 5e1f8e2d07d2a32a1f234cf80efcb703aa87a989 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 26 Jul 2024 09:06:58 +0200 Subject: [PATCH 18/21] diagnostics: add a macro to make things a bit easier to read --- src/tools/miri/src/diagnostics.rs | 135 +++++++++++++----------------- 1 file changed, 59 insertions(+), 76 deletions(-) diff --git a/src/tools/miri/src/diagnostics.rs b/src/tools/miri/src/diagnostics.rs index 6d10a21293b65..1bed55743d4af 100644 --- a/src/tools/miri/src/diagnostics.rs +++ b/src/tools/miri/src/diagnostics.rs @@ -140,6 +140,15 @@ pub enum DiagLevel { Note, } +/// Generate a note/help text without a span. +macro_rules! note { + ($($tt:tt)*) => { (None, format!($($tt)*)) }; +} +/// Generate a note/help text with a span. +macro_rules! note_span { + ($span:expr, $($tt:tt)*) => { (Some($span), format!($($tt)*)) }; +} + /// Attempts to prune a stacktrace to omit the Rust runtime, and returns a bool indicating if any /// frames were pruned. If the stacktrace does not have any local frames, we conclude that it must /// be pointing to a problem in the Rust runtime itself, and do not prune it at all. @@ -228,38 +237,38 @@ pub fn report_error<'tcx>( let helps = match info { UnsupportedInIsolation(_) => vec![ - (None, format!("set `MIRIFLAGS=-Zmiri-disable-isolation` to disable isolation;")), - (None, format!("or set `MIRIFLAGS=-Zmiri-isolation-error=warn` to make Miri return an error code from isolated operations (if supported for that operation) and continue with a warning")), + note!("set `MIRIFLAGS=-Zmiri-disable-isolation` to disable isolation;"), + note!("or set `MIRIFLAGS=-Zmiri-isolation-error=warn` to make Miri return an error code from isolated operations (if supported for that operation) and continue with a warning"), ], UnsupportedForeignItem(_) => { vec![ - (None, format!("if this is a basic API commonly used on this target, please report an issue with Miri")), - (None, format!("however, note that Miri does not aim to support every FFI function out there; for instance, we will not support APIs for things such as GUIs, scripting languages, or databases")), + note!("if this is a basic API commonly used on this target, please report an issue with Miri"), + note!("however, note that Miri does not aim to support every FFI function out there; for instance, we will not support APIs for things such as GUIs, scripting languages, or databases"), ] } StackedBorrowsUb { help, history, .. } => { msg.extend(help.clone()); let mut helps = vec![ - (None, format!("this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental")), - (None, format!("see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information")), + note!("this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental"), + note!("see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information"), ]; if let Some(TagHistory {created, invalidated, protected}) = history.clone() { helps.push((Some(created.1), created.0)); if let Some((msg, span)) = invalidated { - helps.push((Some(span), msg)); + helps.push(note_span!(span, "{msg}")); } if let Some((protector_msg, protector_span)) = protected { - helps.push((Some(protector_span), protector_msg)); + helps.push(note_span!(protector_span, "{protector_msg}")); } } helps }, TreeBorrowsUb { title: _, details, history } => { let mut helps = vec![ - (None, format!("this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental")) + note!("this indicates a potential bug in the program: it performed an invalid operation, but the Tree Borrows rules it violated are still experimental") ]; for m in details { - helps.push((None, m.clone())); + helps.push(note!("{m}")); } for event in history.events.clone() { helps.push(event); @@ -268,26 +277,26 @@ pub fn report_error<'tcx>( } MultipleSymbolDefinitions { first, first_crate, second, second_crate, .. } => vec![ - (Some(*first), format!("it's first defined here, in crate `{first_crate}`")), - (Some(*second), format!("then it's defined here again, in crate `{second_crate}`")), + note_span!(*first, "it's first defined here, in crate `{first_crate}`"), + note_span!(*second, "then it's defined here again, in crate `{second_crate}`"), ], SymbolShimClashing { link_name, span } => - vec![(Some(*span), format!("the `{link_name}` symbol is defined here"))], + vec![note_span!(*span, "the `{link_name}` symbol is defined here")], Int2PtrWithStrictProvenance => - vec![(None, format!("use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead"))], + vec![note!("use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead")], DataRace { op1, extra, retag_explain, .. } => { - let mut helps = vec![(Some(op1.span), format!("and (1) occurred earlier here"))]; + let mut helps = vec![note_span!(op1.span, "and (1) occurred earlier here")]; if let Some(extra) = extra { - helps.push((None, format!("{extra}"))); - helps.push((None, format!("see https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#memory-model-for-atomic-accesses for more information about the Rust memory model"))); + helps.push(note!("{extra}")); + helps.push(note!("see https://doc.rust-lang.org/nightly/std/sync/atomic/index.html#memory-model-for-atomic-accesses for more information about the Rust memory model")); } if *retag_explain { - helps.push((None, "retags occur on all (re)borrows and as well as when references are copied or moved".to_owned())); - helps.push((None, "retags permit optimizations that insert speculative reads or writes".to_owned())); - helps.push((None, "therefore from the perspective of data races, a retag has the same implications as a read or write".to_owned())); + helps.push(note!("retags occur on all (re)borrows and as well as when references are copied or moved")); + helps.push(note!("retags permit optimizations that insert speculative reads or writes")); + helps.push(note!("therefore from the perspective of data races, a retag has the same implications as a read or write")); } - helps.push((None, format!("this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior"))); - helps.push((None, format!("see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information"))); + helps.push(note!("this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior")); + helps.push(note!("see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information")); helps } , @@ -332,32 +341,32 @@ pub fn report_error<'tcx>( let helps = match e.kind() { Unsupported(_) => vec![ - (None, format!("this is likely not a bug in the program; it indicates that the program performed an operation that Miri does not support")), + note!("this is likely not a bug in the program; it indicates that the program performed an operation that Miri does not support"), ], UndefinedBehavior(AlignmentCheckFailed { .. }) if ecx.machine.check_alignment == AlignmentCheck::Symbolic => vec![ - (None, format!("this usually indicates that your program performed an invalid operation and caused Undefined Behavior")), - (None, format!("but due to `-Zmiri-symbolic-alignment-check`, alignment errors can also be false positives")), + note!("this usually indicates that your program performed an invalid operation and caused Undefined Behavior"), + note!("but due to `-Zmiri-symbolic-alignment-check`, alignment errors can also be false positives"), ], UndefinedBehavior(info) => { let mut helps = vec![ - (None, format!("this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior")), - (None, format!("see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information")), + note!("this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior"), + note!("see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information"), ]; match info { PointerUseAfterFree(alloc_id, _) | PointerOutOfBounds { alloc_id, .. } => { if let Some(span) = ecx.machine.allocated_span(*alloc_id) { - helps.push((Some(span), format!("{:?} was allocated here:", alloc_id))); + helps.push(note_span!(span, "{:?} was allocated here:", alloc_id)); } if let Some(span) = ecx.machine.deallocated_span(*alloc_id) { - helps.push((Some(span), format!("{:?} was deallocated here:", alloc_id))); + helps.push(note_span!(span, "{:?} was deallocated here:", alloc_id)); } } AbiMismatchArgument { .. } | AbiMismatchReturn { .. } => { - helps.push((None, format!("this means these two types are not *guaranteed* to be ABI-compatible across all targets"))); - helps.push((None, format!("if you think this code should be accepted anyway, please report an issue with Miri"))); + helps.push(note!("this means these two types are not *guaranteed* to be ABI-compatible across all targets")); + helps.push(note!("if you think this code should be accepted anyway, please report an issue with Miri")); } _ => {}, } @@ -639,9 +648,7 @@ impl<'tcx> MiriMachine<'tcx> { let notes = match &e { ProgressReport { block_count } => { - // It is important that each progress report is slightly different, since - // identical diagnostics are being deduplicated. - vec![(None, format!("so far, {block_count} basic blocks have been executed"))] + vec![note!("so far, {block_count} basic blocks have been executed")] } _ => vec![], }; @@ -649,63 +656,39 @@ impl<'tcx> MiriMachine<'tcx> { let helps = match &e { Int2Ptr { details: true } => { let mut v = vec![ - ( - None, - format!( - "this program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program" - ), + note!( + "this program is using integer-to-pointer casts or (equivalently) `ptr::with_exposed_provenance`, which means that Miri might miss pointer bugs in this program" ), - ( - None, - format!( - "see https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation" - ), + note!( + "see https://doc.rust-lang.org/nightly/std/ptr/fn.with_exposed_provenance.html for more details on that operation" ), - ( - None, - format!( - "to ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead" - ), + note!( + "to ensure that Miri does not miss bugs in your program, use Strict Provenance APIs (https://doc.rust-lang.org/nightly/std/ptr/index.html#strict-provenance, https://crates.io/crates/sptr) instead" ), - ( - None, - format!( - "you can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics" - ), + note!( + "you can then set `MIRIFLAGS=-Zmiri-strict-provenance` to ensure you are not relying on `with_exposed_provenance` semantics" ), ]; if self.borrow_tracker.as_ref().is_some_and(|b| { matches!(b.borrow().borrow_tracker_method(), BorrowTrackerMethod::TreeBorrows) }) { - v.push(( - None, - format!( - "Tree Borrows does not support integer-to-pointer casts, so the program is likely to go wrong when this pointer gets used" - ), - )); + v.push( + note!("Tree Borrows does not support integer-to-pointer casts, so the program is likely to go wrong when this pointer gets used") + ); } else { - v.push(( - None, - format!( - "alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning" - ), - )); + v.push( + note!("alternatively, `MIRIFLAGS=-Zmiri-permissive-provenance` disables this warning") + ); } v } ExternTypeReborrow => { vec![ - ( - None, - format!( - "`extern type` are not compatible with the Stacked Borrows aliasing model implemented by Miri; Miri may miss bugs in this code" - ), + note!( + "`extern type` are not compatible with the Stacked Borrows aliasing model implemented by Miri; Miri may miss bugs in this code" ), - ( - None, - format!( - "try running with `MIRIFLAGS=-Zmiri-tree-borrows` to use the more permissive but also even more experimental Tree Borrows aliasing checks instead" - ), + note!( + "try running with `MIRIFLAGS=-Zmiri-tree-borrows` to use the more permissive but also even more experimental Tree Borrows aliasing checks instead" ), ] } From a52b1d6e29475037a677ceab7d5be5f682c90515 Mon Sep 17 00:00:00 2001 From: tiif Date: Fri, 26 Jul 2024 21:13:58 +0800 Subject: [PATCH 19/21] Let insert_fd takes in FileDescription instead of FileDescriptor --- src/tools/miri/src/machine.rs | 2 +- src/tools/miri/src/shims/unix/fd.rs | 27 ++++++++++--------- src/tools/miri/src/shims/unix/fs.rs | 13 +++------ src/tools/miri/src/shims/unix/linux/epoll.rs | 4 +-- .../miri/src/shims/unix/linux/eventfd.rs | 6 ++--- src/tools/miri/src/shims/unix/socket.rs | 6 ++--- 6 files changed, 24 insertions(+), 34 deletions(-) diff --git a/src/tools/miri/src/machine.rs b/src/tools/miri/src/machine.rs index adb8459356165..e492793a65135 100644 --- a/src/tools/miri/src/machine.rs +++ b/src/tools/miri/src/machine.rs @@ -660,7 +660,7 @@ impl<'tcx> MiriMachine<'tcx> { tls: TlsData::default(), isolated_op: config.isolated_op, validate: config.validate, - fds: shims::FdTable::new(config.mute_stdout_stderr), + fds: shims::FdTable::init(config.mute_stdout_stderr), dirs: Default::default(), layouts, threads, diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index de6d27f5290db..0fffecd99d5cb 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -192,10 +192,6 @@ impl FileDescription for NullOutput { pub struct FileDescriptor(Rc>>); impl FileDescriptor { - pub fn new(fd: T) -> Self { - FileDescriptor(Rc::new(RefCell::new(Box::new(fd)))) - } - pub fn borrow(&self) -> Ref<'_, dyn FileDescription> { Ref::map(self.0.borrow(), |fd| fd.as_ref()) } @@ -227,20 +223,25 @@ impl VisitProvenance for FdTable { } impl FdTable { - pub(crate) fn new(mute_stdout_stderr: bool) -> FdTable { - let mut fds: BTreeMap<_, FileDescriptor> = BTreeMap::new(); - fds.insert(0i32, FileDescriptor::new(io::stdin())); + fn new() -> Self { + FdTable { fds: BTreeMap::new() } + } + pub(crate) fn init(mute_stdout_stderr: bool) -> FdTable { + let mut fds = FdTable::new(); + fds.insert_fd(io::stdin()); if mute_stdout_stderr { - fds.insert(1i32, FileDescriptor::new(NullOutput)); - fds.insert(2i32, FileDescriptor::new(NullOutput)); + assert_eq!(fds.insert_fd(NullOutput), 1); + assert_eq!(fds.insert_fd(NullOutput), 2); } else { - fds.insert(1i32, FileDescriptor::new(io::stdout())); - fds.insert(2i32, FileDescriptor::new(io::stderr())); + assert_eq!(fds.insert_fd(io::stdout()), 1); + assert_eq!(fds.insert_fd(io::stderr()), 2); } - FdTable { fds } + fds } - pub fn insert_fd(&mut self, file_handle: FileDescriptor) -> i32 { + /// Insert a file descriptor to the FdTable. + pub fn insert_fd(&mut self, fd: T) -> i32 { + let file_handle = FileDescriptor(Rc::new(RefCell::new(Box::new(fd)))); self.insert_fd_with_min_fd(file_handle, 0) } diff --git a/src/tools/miri/src/shims/unix/fs.rs b/src/tools/miri/src/shims/unix/fs.rs index 9ea7f9c37ea16..6923b39733f0b 100644 --- a/src/tools/miri/src/shims/unix/fs.rs +++ b/src/tools/miri/src/shims/unix/fs.rs @@ -16,8 +16,6 @@ use crate::shims::unix::*; use crate::*; use shims::time::system_time_to_duration; -use self::fd::FileDescriptor; - #[derive(Debug)] struct FileHandle { file: File, @@ -452,10 +450,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { return Ok(-1); } - let fd = options.open(path).map(|file| { - let fh = &mut this.machine.fds; - fh.insert_fd(FileDescriptor::new(FileHandle { file, writable })) - }); + let fd = options + .open(path) + .map(|file| this.machine.fds.insert_fd(FileHandle { file, writable })); this.try_unwrap_io_result(fd) } @@ -1547,9 +1544,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { match file { Ok(f) => { - let fh = &mut this.machine.fds; - let fd = - fh.insert_fd(FileDescriptor::new(FileHandle { file: f, writable: true })); + let fd = this.machine.fds.insert_fd(FileHandle { file: f, writable: true }); return Ok(fd); } Err(e) => diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index a5661460e95cf..ad35d67876ccb 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -5,8 +5,6 @@ use rustc_data_structures::fx::FxHashMap; use crate::shims::unix::*; use crate::*; -use self::shims::unix::fd::FileDescriptor; - /// An `Epoll` file descriptor connects file handles and epoll events #[derive(Clone, Debug, Default)] struct Epoll { @@ -66,7 +64,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ); } - let fd = this.machine.fds.insert_fd(FileDescriptor::new(Epoll::default())); + let fd = this.machine.fds.insert_fd(Epoll::default()); Ok(Scalar::from_i32(fd)) } diff --git a/src/tools/miri/src/shims/unix/linux/eventfd.rs b/src/tools/miri/src/shims/unix/linux/eventfd.rs index cae5abca3bd08..0fc28c72bb6fb 100644 --- a/src/tools/miri/src/shims/unix/linux/eventfd.rs +++ b/src/tools/miri/src/shims/unix/linux/eventfd.rs @@ -8,8 +8,6 @@ use rustc_target::abi::Endian; use crate::shims::unix::*; use crate::{concurrency::VClock, *}; -use self::shims::unix::fd::FileDescriptor; - // We'll only do reads and writes in chunks of size u64. const U64_ARRAY_SIZE: usize = mem::size_of::(); @@ -180,11 +178,11 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { throw_unsup_format!("eventfd: encountered unknown unsupported flags {:#x}", flags); } - let fd = this.machine.fds.insert_fd(FileDescriptor::new(Event { + let fd = this.machine.fds.insert_fd(Event { counter: val.into(), is_nonblock, clock: VClock::default(), - })); + }); Ok(Scalar::from_i32(fd)) } } diff --git a/src/tools/miri/src/shims/unix/socket.rs b/src/tools/miri/src/shims/unix/socket.rs index 6d3d63b4efa02..cc0f932e03829 100644 --- a/src/tools/miri/src/shims/unix/socket.rs +++ b/src/tools/miri/src/shims/unix/socket.rs @@ -7,8 +7,6 @@ use std::rc::{Rc, Weak}; use crate::shims::unix::*; use crate::{concurrency::VClock, *}; -use self::fd::FileDescriptor; - /// The maximum capacity of the socketpair buffer in bytes. /// This number is arbitrary as the value can always /// be configured in the real system. @@ -221,9 +219,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { }; let fds = &mut this.machine.fds; - let sv0 = fds.insert_fd(FileDescriptor::new(socketpair_0)); + let sv0 = fds.insert_fd(socketpair_0); + let sv1 = fds.insert_fd(socketpair_1); let sv0 = Scalar::from_int(sv0, sv.layout.size); - let sv1 = fds.insert_fd(FileDescriptor::new(socketpair_1)); let sv1 = Scalar::from_int(sv1, sv.layout.size); this.write_scalar(sv0, &sv)?; From 80a32f86187f897cc075a314cd689c6bd45c4671 Mon Sep 17 00:00:00 2001 From: The Miri Cronjob Bot Date: Sat, 27 Jul 2024 05:14:59 +0000 Subject: [PATCH 20/21] Preparing for merge from rustc --- src/tools/miri/rust-version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/rust-version b/src/tools/miri/rust-version index e1a6084b22eda..58ec4e10856df 100644 --- a/src/tools/miri/rust-version +++ b/src/tools/miri/rust-version @@ -1 +1 @@ -72d73cec61aa8f85901358cd5d386d5dd066fe52 +a526d7ce45fd2284e0e7c7556ccba2425b9d25e5 From 822286f0758bea84b798e494b5b6103d288a3221 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 27 Jul 2024 08:31:07 +0200 Subject: [PATCH 21/21] fix clippy --- src/tools/miri/src/shims/x86/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/miri/src/shims/x86/mod.rs b/src/tools/miri/src/shims/x86/mod.rs index 0bbf2a8e13e9a..1bd32fce8bd9e 100644 --- a/src/tools/miri/src/shims/x86/mod.rs +++ b/src/tools/miri/src/shims/x86/mod.rs @@ -1178,7 +1178,7 @@ fn pclmulqdq<'tcx>( // if the i-th bit in right is set if (right & (1 << i)) != 0 { // xor result with `left` shifted to the left by i positions - result ^= (left as u128) << i; + result ^= u128::from(left) << i; } }