Merge pull request #121 from oli-obk/master

fix `static mut` dealloc or freeze

Author: solson

src/error.rs

@@ -49,8 +49,8 @@ pub enum EvalError<'tcx> {
     AssumptionNotHeld,
     InlineAsm,
     TypeNotPrimitive(Ty<'tcx>),
-    ReallocatedFrozenMemory,
-    DeallocatedFrozenMemory,
+    ReallocatedStaticMemory,
+    DeallocatedStaticMemory,
     Layout(layout::LayoutError<'tcx>),
     Unreachable,
     ExpectedConcreteFunction(Function<'tcx>),
@@ -118,10 +118,10 @@ impl<'tcx> Error for EvalError<'tcx> {
                 "cannot evaluate inline assembly",
             EvalError::TypeNotPrimitive(_) =>
                 "expected primitive type, got nonprimitive",
-            EvalError::ReallocatedFrozenMemory =>
-                "tried to reallocate frozen memory",
-            EvalError::DeallocatedFrozenMemory =>
-                "tried to deallocate frozen memory",
+            EvalError::ReallocatedStaticMemory =>
+                "tried to reallocate static memory",
+            EvalError::DeallocatedStaticMemory =>
+                "tried to deallocate static memory",
             EvalError::Layout(_) =>
                 "rustc layout computation failed",
             EvalError::UnterminatedCString(_) =>

src/eval_context.rs

@@ -100,9 +100,11 @@ pub struct Frame<'tcx> {
 #[derive(Clone, Debug, Eq, PartialEq, Hash)]
 pub enum StackPopCleanup {
     /// The stackframe existed to compute the initial value of a static/constant, make sure it
-    /// isn't modifyable afterwards. The allocation of the result is frozen iff it's an
-    /// actual allocation. `PrimVal`s are unmodifyable anyway.
-    Freeze,
+    /// isn't modifyable afterwards in case of constants.
+    /// In case of `static mut`, mark the memory to ensure it's never marked as immutable through
+    /// references or deallocated
+    /// The bool decides whether the value is mutable (true) or not (false)
+    MarkStatic(bool),
     /// A regular stackframe added due to a function call will need to get forwarded to the next
     /// block
     Goto(mir::BasicBlock),
@@ -170,7 +172,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
         // FIXME: cache these allocs
         let ptr = self.memory.allocate(s.len() as u64, 1)?;
         self.memory.write_bytes(ptr, s.as_bytes())?;
-        self.memory.freeze(ptr.alloc_id)?;
+        self.memory.mark_static(ptr.alloc_id, false)?;
         Ok(Value::ByValPair(PrimVal::Ptr(ptr), PrimVal::from_u128(s.len() as u128)))
     }
 
@@ -194,7 +196,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
             ByteStr(ref bs) => {
                 let ptr = self.memory.allocate(bs.len() as u64, 1)?;
                 self.memory.write_bytes(ptr, bs)?;
-                self.memory.freeze(ptr.alloc_id)?;
+                self.memory.mark_static(ptr.alloc_id, false)?;
                 PrimVal::Ptr(ptr)
             }
 
@@ -310,27 +312,27 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
         ::log_settings::settings().indentation -= 1;
         let frame = self.stack.pop().expect("tried to pop a stack frame, but there were none");
         match frame.return_to_block {
-            StackPopCleanup::Freeze => if let Lvalue::Global(id) = frame.return_lvalue {
+            StackPopCleanup::MarkStatic(mutable) => if let Lvalue::Global(id) = frame.return_lvalue {
                 let global_value = self.globals.get_mut(&id)
-                    .expect("global should have been cached (freeze)");
+                    .expect("global should have been cached (static)");
                 match global_value.value {
-                    Value::ByRef(ptr) => self.memory.freeze(ptr.alloc_id)?,
+                    Value::ByRef(ptr) => self.memory.mark_static(ptr.alloc_id, mutable)?,
                     Value::ByVal(val) => if let PrimVal::Ptr(ptr) = val {
-                        self.memory.freeze(ptr.alloc_id)?;
+                        self.memory.mark_static(ptr.alloc_id, mutable)?;
                     },
                     Value::ByValPair(val1, val2) => {
                         if let PrimVal::Ptr(ptr) = val1 {
-                            self.memory.freeze(ptr.alloc_id)?;
+                            self.memory.mark_static(ptr.alloc_id, mutable)?;
                         }
                         if let PrimVal::Ptr(ptr) = val2 {
-                            self.memory.freeze(ptr.alloc_id)?;
+                            self.memory.mark_static(ptr.alloc_id, mutable)?;
                         }
                     },
                 }
                 assert!(global_value.mutable);
-                global_value.mutable = false;
+                global_value.mutable = mutable;
             } else {
-                bug!("StackPopCleanup::Freeze on: {:?}", frame.return_lvalue);
+                bug!("StackPopCleanup::MarkStatic on: {:?}", frame.return_lvalue);
             },
             StackPopCleanup::Goto(target) => self.goto_block(target),
             StackPopCleanup::None => {},
@@ -341,11 +343,9 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
                 trace!("deallocating local");
                 self.memory.dump_alloc(ptr.alloc_id);
                 match self.memory.deallocate(ptr) {
-                    // Any frozen memory means that it belongs to a constant or something referenced
-                    // by a constant. We could alternatively check whether the alloc_id is frozen
-                    // before calling deallocate, but this is much simpler and is probably the
-                    // rare case.
-                    Ok(()) | Err(EvalError::DeallocatedFrozenMemory) => {},
+                    // We could alternatively check whether the alloc_id is static before calling
+                    // deallocate, but this is much simpler and is probably the rare case.
+                    Ok(()) | Err(EvalError::DeallocatedStaticMemory) => {},
                     other => return other,
                 }
             }
@@ -868,9 +868,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
                     _ => {
                         let ptr = self.alloc_ptr_with_substs(global_val.ty, cid.substs)?;
                         self.write_value_to_ptr(global_val.value, ptr, global_val.ty)?;
-                        if !global_val.mutable {
-                            self.memory.freeze(ptr.alloc_id)?;
-                        }
+                        self.memory.mark_static(ptr.alloc_id, global_val.mutable)?;
                         let lval = self.globals.get_mut(&cid).expect("already checked");
                         *lval = Global {
                             value: Value::ByRef(ptr),

src/memory.rs

@@ -38,9 +38,19 @@ pub struct Allocation {
     /// The alignment of the allocation to detect unaligned reads.
     pub align: u64,
     /// Whether the allocation may be modified.
-    /// Use the `freeze` method of `Memory` to ensure that an error occurs, if the memory of this
-    /// allocation is modified in the future.
-    pub immutable: bool,
+    /// Use the `mark_static` method of `Memory` to ensure that an error occurs, if the memory of this
+    /// allocation is modified or deallocated in the future.
+    pub static_kind: StaticKind,
+}
+
+#[derive(Debug, PartialEq, Copy, Clone)]
+pub enum StaticKind {
+    /// may be deallocated without breaking miri's invariants
+    NotStatic,
+    /// may be modified, but never deallocated
+    Mutable,
+    /// may neither be modified nor deallocated
+    Immutable,
 }
 
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -272,7 +282,7 @@ impl<'a, 'tcx> Memory<'a, 'tcx> {
             relocations: BTreeMap::new(),
             undef_mask: UndefMask::new(size),
             align,
-            immutable: false,
+            static_kind: StaticKind::NotStatic,
         };
         let id = self.next_id;
         self.next_id.0 += 1;
@@ -290,8 +300,8 @@ impl<'a, 'tcx> Memory<'a, 'tcx> {
         if ptr.points_to_zst() {
             return self.allocate(new_size, align);
         }
-        if self.get(ptr.alloc_id).map(|alloc| alloc.immutable).ok() == Some(true) {
-            return Err(EvalError::ReallocatedFrozenMemory);
+        if self.get(ptr.alloc_id).ok().map_or(false, |alloc| alloc.static_kind != StaticKind::NotStatic) {
+            return Err(EvalError::ReallocatedStaticMemory);
         }
 
         let size = self.get(ptr.alloc_id)?.bytes.len() as u64;
@@ -325,8 +335,8 @@ impl<'a, 'tcx> Memory<'a, 'tcx> {
             // TODO(solson): Report error about non-__rust_allocate'd pointer.
             return Err(EvalError::Unimplemented(format!("bad pointer offset: {}", ptr.offset)));
         }
-        if self.get(ptr.alloc_id).map(|alloc| alloc.immutable).ok() == Some(true) {
-            return Err(EvalError::DeallocatedFrozenMemory);
+        if self.get(ptr.alloc_id).ok().map_or(false, |alloc| alloc.static_kind != StaticKind::NotStatic) {
+            return Err(EvalError::DeallocatedStaticMemory);
         }
 
         if let Some(alloc) = self.alloc_map.remove(&ptr.alloc_id) {
@@ -430,8 +440,11 @@ impl<'a, 'tcx> Memory<'a, 'tcx> {
 
     pub fn get_mut(&mut self, id: AllocId) -> EvalResult<'tcx, &mut Allocation> {
         match self.alloc_map.get_mut(&id) {
-            Some(ref alloc) if alloc.immutable => Err(EvalError::ModifiedConstantMemory),
-            Some(alloc) => Ok(alloc),
+            Some(alloc) => match alloc.static_kind {
+                StaticKind::Mutable |
+                StaticKind::NotStatic => Ok(alloc),
+                StaticKind::Immutable => Err(EvalError::ModifiedConstantMemory),
+            },
             None => match self.functions.get(&id) {
                 Some(_) => Err(EvalError::DerefFunctionPointer),
                 None if id == NEVER_ALLOC_ID || id == ZST_ALLOC_ID => Err(EvalError::InvalidMemoryAccess),
@@ -514,7 +527,11 @@ impl<'a, 'tcx> Memory<'a, 'tcx> {
                 }
             }
 
-            let immutable = if alloc.immutable { " (immutable)" } else { "" };
+            let immutable = match alloc.static_kind {
+                StaticKind::Mutable => " (static mut)",
+                StaticKind::Immutable => " (immutable)",
+                StaticKind::NotStatic => "",
+            };
             trace!("{}({} bytes){}", msg, alloc.bytes.len(), immutable);
 
             if !relocations.is_empty() {
@@ -607,23 +624,28 @@ impl<'a, 'tcx> Memory<'a, 'tcx> {
 
 /// Reading and writing
 impl<'a, 'tcx> Memory<'a, 'tcx> {
-    pub fn freeze(&mut self, alloc_id: AllocId) -> EvalResult<'tcx> {
-        // do not use `self.get_mut(alloc_id)` here, because we might have already frozen a
+    /// mark an allocation as static, either mutable or not
+    pub fn mark_static(&mut self, alloc_id: AllocId, mutable: bool) -> EvalResult<'tcx> {
+        // do not use `self.get_mut(alloc_id)` here, because we might have already marked a
         // sub-element or have circular pointers (e.g. `Rc`-cycles)
         let relocations = match self.alloc_map.get_mut(&alloc_id) {
-            Some(ref mut alloc) if !alloc.immutable => {
-                alloc.immutable = true;
+            Some(&mut Allocation { ref mut relocations, static_kind: ref mut kind @ StaticKind::NotStatic, .. }) => {
+                *kind = if mutable {
+                    StaticKind::Mutable
+                } else {
+                    StaticKind::Immutable
+                };
                 // take out the relocations vector to free the borrow on self, so we can call
-                // freeze recursively
-                mem::replace(&mut alloc.relocations, Default::default())
+                // mark recursively
+                mem::replace(relocations, Default::default())
             },
             None if alloc_id == NEVER_ALLOC_ID || alloc_id == ZST_ALLOC_ID => return Ok(()),
             None if !self.functions.contains_key(&alloc_id) => return Err(EvalError::DanglingPointerDeref),
             _ => return Ok(()),
         };
         // recurse into inner allocations
         for &alloc in relocations.values() {
-            self.freeze(alloc)?;
+            self.mark_static(alloc, mutable)?;
         }
         // put back the relocations
         self.alloc_map.get_mut(&alloc_id).expect("checked above").relocations = relocations;

src/step.rs

@@ -156,11 +156,7 @@ impl<'a, 'b, 'tcx> ConstantExtractor<'a, 'b, 'tcx> {
         self.try(|this| {
             let mir = this.ecx.load_mir(def_id)?;
             this.ecx.globals.insert(cid, Global::uninitialized(mir.return_ty));
-            let cleanup = if immutable && !mir.return_ty.type_contents(this.ecx.tcx).interior_unsafe() {
-                StackPopCleanup::Freeze
-            } else {
-                StackPopCleanup::None
-            };
+            let cleanup = StackPopCleanup::MarkStatic(!immutable || mir.return_ty.type_contents(this.ecx.tcx).interior_unsafe());
             let name = ty::tls::with(|tcx| tcx.item_path_str(def_id));
             trace!("pushing stack frame for global: {}", name);
             this.ecx.push_stack_frame(def_id, span, mir, substs, Lvalue::Global(cid), cleanup, Vec::new())
@@ -213,7 +209,7 @@ impl<'a, 'b, 'tcx> Visitor<'tcx> for ConstantExtractor<'a, 'b, 'tcx> {
                                               mir,
                                               this.substs,
                                               Lvalue::Global(cid),
-                                              StackPopCleanup::Freeze,
+                                              StackPopCleanup::MarkStatic(false),
                                               Vec::new())
                 });
             }

src/vtable.rs

@@ -112,7 +112,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
             }
         }
 
-        self.memory.freeze(vtable.alloc_id)?;
+        self.memory.mark_static(vtable.alloc_id, false)?;
 
         Ok(vtable)
     }

tests/compile-fail/modifying_constants.rs

@@ -1,5 +1,5 @@
 fn main() {
-    let x = &1; // the `&1` is promoted to a constant, but it used to be that only the pointer is frozen, not the pointee
+    let x = &1; // the `&1` is promoted to a constant, but it used to be that only the pointer is marked static, not the pointee
     let y = unsafe { &mut *(x as *const i32 as *mut i32) };
     *y = 42; //~ ERROR tried to modify constant memory
     assert_eq!(*x, 42);

tests/run-pass/const-vec-of-fns.rs

@@ -0,0 +1,29 @@
+// Copyright 2013 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// pretty-expanded FIXME #23616
+
+/*!
+ * Try to double-check that static fns have the right size (with or
+ * without dummy env ptr, as appropriate) by iterating a size-2 array.
+ * If the static size differs from the runtime size, the second element
+ * should be read as a null or otherwise wrong pointer and crash.
+ */
+
+fn f() { }
+static mut CLOSURES: &'static mut [fn()] = &mut [f as fn(), f as fn()];
+
+pub fn main() {
+    unsafe {
+        for closure in &mut *CLOSURES {
+            (*closure)()
+        }
+    }
+}

tests/run-pass/recursive_static.rs

@@ -0,0 +1,11 @@
+#![feature(static_recursion)]
+
+struct S(&'static S);
+static S1: S = S(&S2);
+static S2: S = S(&S1);
+
+fn main() {
+    let p: *const S = S2.0;
+    let q: *const S = &S1;
+    assert_eq!(p, q);
+}

tests/run-pass/static_mut.rs

@@ -0,0 +1,17 @@
+#![allow(dead_code)]
+
+static mut FOO: i32 = 42;
+static BAR: Foo = Foo(unsafe { &FOO as *const _} );
+
+struct Foo(*const i32);
+
+unsafe impl Sync for Foo {}
+
+fn main() {
+    unsafe {
+        assert_eq!(*BAR.0, 42);
+        FOO = 5;
+        assert_eq!(FOO, 5);
+        assert_eq!(*BAR.0, 5);
+    }
+}