From 22364f86aeba440066db931132ecee8292b709fd Mon Sep 17 00:00:00 2001 From: Neven Villani Date: Tue, 9 Jul 2024 14:24:05 +0200 Subject: [PATCH 1/6] 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 00000000000..1f52537f9a2 --- /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 2/6] 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 86074384084..4d9595b352f 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 @@ fn from_ref_ty( ) -> 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 @@ fn from_ref_ty( _ => 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 7aa9c3e862b..9c19ae76a2d 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 7d000ba55e6..ce9a5b7f158 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 1f52537f9a2..5e7cb5e34cc 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 inner(y: &mut Cell, b: IdxBarrier) -> *mut u8 { 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 00000000000..9be6c157c0c --- /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 0d0d52c717f..d149a4065f9 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 3/6] 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 4d9595b352f..123d4b407fb 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 @@ fn from_ref_ty( // 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 5e7cb5e34cc..611f64dce5e 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 @@ // 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 inner(x: &mut u8, b: IdxBarrier, 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 00000000000..5ac9c912ba9 --- /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 9be6c157c0c..97c71fdedef 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 4/6] 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 611f64dce5e..6ae79be6cc7 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 inner(x: &mut u8, b: IdxBarrier) { 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 5ac9c912ba9..0e4517e9010 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 97c71fdedef..cbeef90243b 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 5/6] 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 6ae79be6cc7..73f227fee2f 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 6/6] 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 9c19ae76a2d..8e23257b6c0 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 @@ fn child_write(state: PermissionPriv, protected: bool) -> Option /// 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,