Merge pull request #557 from RalfJung/fix-mutability-gap

fix mutability gap: do not allow shared mutation when creating frozen reference

Author: RalfJung

rust-version

@@ -1 +1 @@
-nightly-2018-11-30
+nightly-2018-12-03

src/stacked_borrows.rs

@@ -303,14 +303,28 @@ impl<'tcx> Stack {
     /// is met: We cannot push `Uniq` onto frozen stacks.
     /// `kind` indicates which kind of reference is being created.
     fn create(&mut self, bor: Borrow, kind: RefKind) {
-        if self.frozen_since.is_some() {
-            // A frozen location?  Possible if we create a barrier, then push again.
-            assert!(bor.is_shared(), "We should never try creating a unique borrow for a frozen stack");
-            trace!("create: Not doing anything on frozen location");
+        // When creating a frozen reference, freeze.  This ensures F1.
+        // We also do *not* push anything else to the stack, making sure that no nother kind
+        // of access (like writing through raw pointers) is permitted.
+        if kind == RefKind::Frozen {
+            let bor_t = match bor {
+                Borrow::Shr(Some(t)) => t,
+                _ => bug!("Creating illegal borrow {:?} for frozen ref", bor),
+            };
+            // It is possible that we already are frozen (e.g. if we just pushed a barrier,
+            // the redundancy check would not have kicked in).
+            match self.frozen_since {
+                Some(loc_t) => assert!(loc_t <= bor_t, "Trying to freeze location for longer than it was already frozen"),
+                None => {
+                    trace!("create: Freezing");
+                    self.frozen_since = Some(bor_t);
+                }
+            }
             return;
         }
-        // First, push.  We do this even if we will later freeze, because we
-        // will allow mutation of shared data at the expense of unfreezing.
+        assert!(self.frozen_since.is_none(), "Trying to create non-frozen reference to frozen location");
+
+        // Push new item to the stack.
         let itm = match bor {
             Borrow::Uniq(t) => BorStackItem::Uniq(t),
             Borrow::Shr(_) => BorStackItem::Shr,
@@ -325,15 +339,6 @@ impl<'tcx> Stack {
             trace!("create: Pushing {:?}", itm);
             self.borrows.push(itm);
         }
-        // Then, maybe freeze.  This is part 2 of ensuring F1.
-        if kind == RefKind::Frozen {
-            let bor_t = match bor {
-                Borrow::Shr(Some(t)) => t,
-                _ => bug!("Creating illegal borrow {:?} for frozen ref", bor),
-            };
-            trace!("create: Freezing");
-            self.frozen_since = Some(bor_t);
-        }
     }
 
     /// Add a barrier

tests/compile-fail-fullmir/stacked_borrows/illegal_write3.rs

@@ -3,6 +3,6 @@ fn main() {
     // Make sure raw ptr with raw tag cannot mutate frozen location without breaking the shared ref.
     let r#ref = ⌖ // freeze
     let ptr = r#ref as *const _ as *mut _; // raw ptr, with raw tag
-    unsafe { *ptr = 42; }
-    let _val = *r#ref; //~ ERROR is not frozen
+    unsafe { *ptr = 42; } //~ ERROR does not exist on the stack
+    let _val = *r#ref;
 }

tests/compile-fail-fullmir/stacked_borrows/pass_invalid_shr.rs

@@ -3,7 +3,7 @@ fn foo(_: &i32) {}
 
 fn main() {
     let x = &mut 42;
-    let xraw = &*x as *const _ as *mut _;
+    let xraw = x as *mut _;
     let xref = unsafe { &*xraw };
     unsafe { *xraw = 42 }; // unfreeze
     foo(xref); //~ ERROR is not frozen

tests/compile-fail-fullmir/stacked_borrows/shr_frozen_violation1.rs

@@ -0,0 +1,16 @@
+fn foo(x: &mut i32) -> i32 {
+  *x = 5;
+  unknown_code(&*x);
+  *x // must return 5
+}
+
+fn main() {
+    println!("{}", foo(&mut 0));
+}
+
+// If we replace the `*const` by `&`, my current dev version of miri
+// *does* find the problem, but not for a good reason: It finds it because
+// of barriers, and we shouldn't rely on unknown code using barriers.
+fn unknown_code(x: *const i32) {
+    unsafe { *(x as *mut i32) = 7; } //~ ERROR barrier
+}

tests/run-pass-fullmir/vecdeque.rs

@@ -1,3 +1,6 @@
+// FIXME: Validation disabled until https://github.com/rust-lang/rust/pull/56161 lands
+// compile-flags: -Zmiri-disable-validation
+
 use std::collections::VecDeque;
 
 fn main() {

tests/run-pass/refcell.rs

@@ -39,6 +39,13 @@ fn aliasing_mut_and_shr() {
         *aliasing += 4;
         let _shr = &*rc;
         *aliasing += 4;
+        // also turning this into a frozen ref now must work
+        let aliasing = &*aliasing;
+        let _val = *aliasing;
+        let _escape_to_raw = rc as *const _; // this must NOT unfreeze
+        let _val = *aliasing;
+        let _shr = &*rc; // this must NOT unfreeze
+        let _val = *aliasing;
     }
 
     let rc = RefCell::new(23);
@@ -48,7 +55,23 @@ fn aliasing_mut_and_shr() {
     assert_eq!(*rc.borrow(), 23+12);
 }
 
+fn aliasing_frz_and_shr() {
+    fn inner(rc: &RefCell<i32>, aliasing: &i32) {
+        let _val = *aliasing;
+        let _escape_to_raw = rc as *const _; // this must NOT unfreeze
+        let _val = *aliasing;
+        let _shr = &*rc; // this must NOT unfreeze
+        let _val = *aliasing;
+    }
+
+    let rc = RefCell::new(23);
+    let bshr = rc.borrow();
+    inner(&rc, &*bshr);
+    assert_eq!(*rc.borrow(), 23);
+}
+
 fn main() {
     lots_of_funny_borrows();
     aliasing_mut_and_shr();
+    aliasing_frz_and_shr();
 }

tests/run-pass/stacked-borrows.rs

@@ -4,10 +4,11 @@ fn main() {
     read_does_not_invalidate1();
     read_does_not_invalidate2();
     ref_raw_int_raw();
-    mut_shr_raw();
     mut_raw_then_mut_shr();
+    mut_shr_then_mut_raw();
     mut_raw_mut();
     partially_invalidate_mut();
+    drop_after_sharing();
 }
 
 // Deref a raw ptr to access a field of a large struct, where the field
@@ -53,18 +54,6 @@ fn ref_raw_int_raw() {
     assert_eq!(unsafe { *xraw }, 3);
 }
 
-// Creating a raw from a `&mut` through an `&` works, even if we
-// write through that raw.
-fn mut_shr_raw() {
-    let mut x = 2;
-    {
-        let xref = &mut x;
-        let xraw = &*xref as *const i32 as *mut i32;
-        unsafe { *xraw = 4; }
-    }
-    assert_eq!(x, 4);
-}
-
 // Escape a mut to raw, then share the same mut and use the share, then the raw.
 // That should work.
 fn mut_raw_then_mut_shr() {
@@ -77,6 +66,16 @@ fn mut_raw_then_mut_shr() {
     assert_eq!(x, 4);
 }
 
+// Create first a shared reference and then a raw pointer from a `&mut`
+// should permit mutation through that raw pointer.
+fn mut_shr_then_mut_raw() {
+    let xref = &mut 2;
+    let _xshr = &*xref;
+    let xraw = xref as *mut _;
+    unsafe { *xraw = 3; }
+    assert_eq!(*xref, 3);
+}
+
 // Ensure that if we derive from a mut a raw, and then from that a mut,
 // and then read through the original mut, that does not invalidate the raw.
 // This shows that the read-exception for `&mut` applies even if the `Shr` item
@@ -107,3 +106,9 @@ fn partially_invalidate_mut() {
     *shard += 1; // so we can still use `shard`.
     assert_eq!(*data, (1, 1));
 }
+
+// Make sure that we can handle the situation where a loaction is frozen when being dropped.
+fn drop_after_sharing() {
+    let x = String::from("hello!");
+    let _len = x.len();
+}