Auto merge of #2287 - RalfJung:field-retagging, r=RalfJung

stacked borrows: add option for recursive field retagging

Author: bors

README.md

@@ -369,6 +369,9 @@ to Miri failing to detect cases of undefined behavior in a program.
   application instead of raising an error within the context of Miri (and halting
   execution). Note that code might not expect these operations to ever panic, so
   this flag can lead to strange (mis)behavior.
+* `-Zmiri-retag-fields` changes Stacked Borrows retagging to recurse into fields.
+  This means that references in fields of structs/enums/tuples/arrays/... are retagged,
+  and in particular, they are protected when passed as function arguments.
 * `-Zmiri-track-alloc-id=<id1>,<id2>,...` shows a backtrace when the given allocations are
   being allocated or freed.  This helps in debugging memory leaks and
   use after free bugs. Specifying this argument multiple times does not overwrite the previous

rust-version

@@ -1 +1 @@
-493c960a3e6cdd2e2fbe8b6ea130fadea05f1ab0
+ddcbba036aee08f0709f98a92a342a278eae5c05

src/bin/miri.rs

@@ -383,6 +383,8 @@ fn main() {
             miri_config.provenance_mode = ProvenanceMode::Permissive;
         } else if arg == "-Zmiri-mute-stdout-stderr" {
             miri_config.mute_stdout_stderr = true;
+        } else if arg == "-Zmiri-retag-fields" {
+            miri_config.retag_fields = true;
         } else if arg == "-Zmiri-track-raw-pointers" {
             eprintln!(
                 "WARNING: `-Zmiri-track-raw-pointers` has no effect; it is enabled by default"

src/eval.rs

@@ -124,6 +124,8 @@ pub struct MiriConfig {
     pub preemption_rate: f64,
     /// Report the current instruction being executed every N basic blocks.
     pub report_progress: Option<u32>,
+    /// Whether Stacked Borrows retagging should recurse into fields of datatypes.
+    pub retag_fields: bool,
 }
 
 impl Default for MiriConfig {
@@ -154,6 +156,7 @@ impl Default for MiriConfig {
             mute_stdout_stderr: false,
             preemption_rate: 0.01, // 1%
             report_progress: None,
+            retag_fields: false,
         }
     }
 }

src/machine.rs

@@ -354,6 +354,7 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
             Some(RefCell::new(stacked_borrows::GlobalStateInner::new(
                 config.tracked_pointer_tags.clone(),
                 config.tracked_call_ids.clone(),
+                config.retag_fields,
             )))
         } else {
             None

src/stacked_borrows.rs

@@ -156,6 +156,8 @@ pub struct GlobalStateInner {
     tracked_pointer_tags: HashSet<SbTag>,
     /// The call ids to trace
     tracked_call_ids: HashSet<CallId>,
+    /// Whether to recurse into datatypes when searching for pointers to retag.
+    retag_fields: bool,
 }
 
 /// We need interior mutable access to the global state.
@@ -204,14 +206,19 @@ impl fmt::Display for RefKind {
 
 /// Utilities for initialization and ID generation
 impl GlobalStateInner {
-    pub fn new(tracked_pointer_tags: HashSet<SbTag>, tracked_call_ids: HashSet<CallId>) -> Self {
+    pub fn new(
+        tracked_pointer_tags: HashSet<SbTag>,
+        tracked_call_ids: HashSet<CallId>,
+        retag_fields: bool,
+    ) -> Self {
         GlobalStateInner {
             next_ptr_tag: SbTag(NonZeroU64::new(1).unwrap()),
             base_ptr_tags: FxHashMap::default(),
             next_call_id: NonZeroU64::new(1).unwrap(),
             active_calls: FxHashSet::default(),
             tracked_pointer_tags,
             tracked_call_ids,
+            retag_fields,
         }
     }
 
@@ -1035,17 +1042,69 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             }
         }
 
-        // We only reborrow "bare" references/boxes.
-        // Not traversing into fields helps with <https://github.com/rust-lang/unsafe-code-guidelines/issues/125>,
-        // but might also cost us optimization and analyses. We will have to experiment more with this.
+        // We need a visitor to visit all references. However, that requires
+        // a `MPlaceTy` (or `OpTy), so we have a fast path for reference types that
+        // avoids allocating.
+
         if let Some((mutbl, protector)) = qualify(place.layout.ty, kind) {
             // Fast path.
             let val = this.read_immediate(&this.place_to_op(place)?)?;
             let val = this.retag_reference(&val, mutbl, protector)?;
             this.write_immediate(*val, place)?;
+            return Ok(());
         }
 
-        Ok(())
+        // If we don't want to recurse, we are already done.
+        if !this.machine.stacked_borrows.as_mut().unwrap().get_mut().retag_fields {
+            return Ok(());
+        }
+
+        // Skip some types that have no further structure we might care about.
+        if matches!(
+            place.layout.ty.kind(),
+            ty::RawPtr(..)
+                | ty::Ref(..)
+                | ty::Int(..)
+                | ty::Uint(..)
+                | ty::Float(..)
+                | ty::Bool
+                | ty::Char
+        ) {
+            return Ok(());
+        }
+        // Now go visit this thing.
+        let place = this.force_allocation(place)?;
+
+        let mut visitor = RetagVisitor { ecx: this, kind };
+        return visitor.visit_value(&place);
+
+        // The actual visitor.
+        struct RetagVisitor<'ecx, 'mir, 'tcx> {
+            ecx: &'ecx mut MiriEvalContext<'mir, 'tcx>,
+            kind: RetagKind,
+        }
+        impl<'ecx, 'mir, 'tcx> MutValueVisitor<'mir, 'tcx, Evaluator<'mir, 'tcx>>
+            for RetagVisitor<'ecx, 'mir, 'tcx>
+        {
+            type V = MPlaceTy<'tcx, Tag>;
+
+            #[inline(always)]
+            fn ecx(&mut self) -> &mut MiriEvalContext<'mir, 'tcx> {
+                &mut self.ecx
+            }
+
+            fn visit_value(&mut self, place: &MPlaceTy<'tcx, Tag>) -> InterpResult<'tcx> {
+                if let Some((mutbl, protector)) = qualify(place.layout.ty, self.kind) {
+                    let val = self.ecx.read_immediate(&place.into())?;
+                    let val = self.ecx.retag_reference(&val, mutbl, protector)?;
+                    self.ecx.write_immediate(*val, &(*place).into())?;
+                } else {
+                    // Maybe we need to go deeper.
+                    self.walk_value(place)?;
+                }
+                Ok(())
+            }
+        }
     }
 
     /// After a stack frame got pushed, retag the return place so that we are sure

tests/fail/stacked_borrows/newtype_retagging.rs

@@ -0,0 +1,19 @@
+// compile-flags: -Zmiri-retag-fields
+// error-pattern: incompatible item is protected
+struct Newtype<'a>(&'a mut i32);
+
+fn dealloc_while_running(_n: Newtype<'_>, dealloc: impl FnOnce()) {
+    dealloc();
+}
+
+// Make sure that we protect references inside structs.
+fn main() {
+    let ptr = Box::into_raw(Box::new(0i32));
+    #[rustfmt::skip] // I like my newlines
+    unsafe {
+        dealloc_while_running(
+            Newtype(&mut *ptr),
+            || drop(Box::from_raw(ptr)),
+        )
+    };
+}

tests/fail/stacked_borrows/newtype_retagging.stderr

@@ -0,0 +1,50 @@
+error: Undefined Behavior: not granting access to tag <TAG> because incompatible item is protected: [Unique for <TAG> (call ID)]
+  --> RUSTLIB/alloc/src/boxed.rs:LL:CC
+   |
+LL |         Box(unsafe { Unique::new_unchecked(raw) }, alloc)
+   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not granting access to tag <TAG> because incompatible item is protected: [Unique for <TAG> (call ID)]
+   |
+   = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
+   = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
+help: <TAG> was created by a retag at offsets [0x0..0x4]
+  --> $DIR/newtype_retagging.rs:LL:CC
+   |
+LL |     let ptr = Box::into_raw(Box::new(0i32));
+   |               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+help: <TAG> was protected due to <TAG> which was created here
+  --> $DIR/newtype_retagging.rs:LL:CC
+   |
+LL |             Newtype(&mut *ptr),
+   |             ^^^^^^^^^^^^^^^^^^
+help: this protector is live for this call
+  --> $DIR/newtype_retagging.rs:LL:CC
+   |
+LL | / fn dealloc_while_running(_n: Newtype<'_>, dealloc: impl FnOnce()) {
+LL | |     dealloc();
+LL | | }
+   | |_^
+   = note: inside `std::boxed::Box::<i32>::from_raw_in` at RUSTLIB/alloc/src/boxed.rs:LL:CC
+   = note: inside `std::boxed::Box::<i32>::from_raw` at RUSTLIB/alloc/src/boxed.rs:LL:CC
+note: inside closure at $DIR/newtype_retagging.rs:LL:CC
+  --> $DIR/newtype_retagging.rs:LL:CC
+   |
+LL |             || drop(Box::from_raw(ptr)),
+   |                     ^^^^^^^^^^^^^^^^^^
+note: inside `dealloc_while_running::<[closure@$DIR/newtype_retagging.rs:LL:CC]>` at $DIR/newtype_retagging.rs:LL:CC
+  --> $DIR/newtype_retagging.rs:LL:CC
+   |
+LL |     dealloc();
+   |     ^^^^^^^^^
+note: inside `main` at $DIR/newtype_retagging.rs:LL:CC
+  --> $DIR/newtype_retagging.rs:LL:CC
+   |
+LL | /         dealloc_while_running(
+LL | |             Newtype(&mut *ptr),
+LL | |             || drop(Box::from_raw(ptr)),
+LL | |         )
+   | |_________^
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to previous error
+

tests/pass/stacked-borrows/interior_mutability.rs

@@ -1,4 +1,5 @@
-use std::cell::{Cell, RefCell, UnsafeCell};
+// compile-flags: -Zmiri-retag-fields
+use std::cell::{Cell, Ref, RefCell, RefMut, UnsafeCell};
 use std::mem::{self, MaybeUninit};
 
 fn main() {
@@ -8,6 +9,10 @@ fn main() {
     unsafe_cell_2phase();
     unsafe_cell_deallocate();
     unsafe_cell_invalidate();
+    refcell_basic();
+    ref_protector();
+    ref_mut_protector();
+    rust_issue_68303();
 }
 
 fn aliasing_mut_and_shr() {
@@ -99,3 +104,72 @@ fn unsafe_cell_invalidate() {
     // So using raw1 invalidates raw2.
     f(unsafe { mem::transmute(raw2) }, raw1);
 }
+
+fn refcell_basic() {
+    let c = RefCell::new(42);
+    {
+        let s1 = c.borrow();
+        let _x: i32 = *s1;
+        let s2 = c.borrow();
+        let _x: i32 = *s1;
+        let _y: i32 = *s2;
+        let _x: i32 = *s1;
+        let _y: i32 = *s2;
+    }
+    {
+        let mut m = c.borrow_mut();
+        let _z: i32 = *m;
+        {
+            let s: &i32 = &*m;
+            let _x = *s;
+        }
+        *m = 23;
+        let _z: i32 = *m;
+    }
+    {
+        let s1 = c.borrow();
+        let _x: i32 = *s1;
+        let s2 = c.borrow();
+        let _x: i32 = *s1;
+        let _y: i32 = *s2;
+        let _x: i32 = *s1;
+        let _y: i32 = *s2;
+    }
+}
+
+// Adding a Stacked Borrows protector for `Ref` would break this
+fn ref_protector() {
+    fn break_it(rc: &RefCell<i32>, r: Ref<'_, i32>) {
+        // `r` has a shared reference, it is passed in as argument and hence
+        // a protector is added that marks this memory as read-only for the entire
+        // duration of this function.
+        drop(r);
+        // *oops* here we can mutate that memory.
+        *rc.borrow_mut() = 2;
+    }
+
+    let rc = RefCell::new(0);
+    break_it(&rc, rc.borrow())
+}
+
+fn ref_mut_protector() {
+    fn break_it(rc: &RefCell<i32>, r: RefMut<'_, i32>) {
+        // `r` has a shared reference, it is passed in as argument and hence
+        // a protector is added that marks this memory as inaccessible for the entire
+        // duration of this function
+        drop(r);
+        // *oops* here we can mutate that memory.
+        *rc.borrow_mut() = 2;
+    }
+
+    let rc = RefCell::new(0);
+    break_it(&rc, rc.borrow_mut())
+}
+
+/// Make sure we do not have bad enum layout optimizations.
+fn rust_issue_68303() {
+    let optional = Some(RefCell::new(false));
+    let mut handle = optional.as_ref().unwrap().borrow_mut();
+    assert!(optional.is_some());
+    *handle = true;
+}