Merge pull request rust-lang#53 from oli-obk/dont_touch_my_precious_constants

solson · web-flow · commit 4051b0e36a67 · 2016-09-11T03:17:58.000-06:00
Detect modifications of immutable memory
diff --git a/src/error.rs b/src/error.rs
@@ -42,6 +42,7 @@ pub enum EvalError<'tcx> {
     },
     CalledClosureAsFunction,
     VtableForArgumentlessMethod,
+    ModifiedConstantMemory,
 }
 
 pub type EvalResult<'tcx, T> = Result<T, EvalError<'tcx>>;
@@ -94,6 +95,8 @@ impl<'tcx> Error for EvalError<'tcx> {
                 "tried to call a closure through a function pointer",
             EvalError::VtableForArgumentlessMethod =>
                 "tried to call a vtable function without arguments",
+            EvalError::ModifiedConstantMemory =>
+                "tried to modify constant memory",
         }
     }
 
diff --git a/src/interpreter/mod.rs b/src/interpreter/mod.rs
@@ -15,7 +15,7 @@ use std::iter;
 use syntax::codemap::{self, DUMMY_SP};
 
 use error::{EvalError, EvalResult};
-use memory::{Memory, Pointer};
+use memory::{Memory, Pointer, AllocId};
 use primval::{self, PrimVal};
 
 use std::collections::HashMap;
@@ -74,7 +74,7 @@ pub struct Frame<'a, 'tcx: 'a> {
     pub return_ptr: Option<Pointer>,
 
     /// The block to return to when returning from the current stack frame
-    pub return_to_block: Option<mir::BasicBlock>,
+    pub return_to_block: StackPopCleanup,
 
     /// The list of locals for the current function, stored in order as
     /// `[arguments..., variables..., temporaries...]`. The variables begin at `self.var_offset`
@@ -139,6 +139,18 @@ enum ConstantKind {
     Global,
 }
 
+#[derive(Clone, Debug, Eq, PartialEq, Hash)]
+pub enum StackPopCleanup {
+    /// The stackframe existed to compute the initial value of a static/constant, make sure the
+    /// static isn't modifyable afterwards
+    Freeze(AllocId),
+    /// A regular stackframe added due to a function call will need to get forwarded to the next
+    /// block
+    Goto(mir::BasicBlock),
+    /// The main function and diverging functions have nowhere to return to
+    None,
+}
+
 impl<'a, 'tcx> EvalContext<'a, 'tcx> {
     pub fn new(tcx: TyCtxt<'a, 'tcx, 'tcx>, mir_map: &'a MirMap<'tcx>, memory_size: usize, stack_limit: usize) -> Self {
         EvalContext {
@@ -313,7 +325,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
         mir: CachedMir<'a, 'tcx>,
         substs: &'tcx Substs<'tcx>,
         return_ptr: Option<Pointer>,
-        return_to_block: Option<mir::BasicBlock>,
+        return_to_block: StackPopCleanup,
     ) -> EvalResult<'tcx, ()> {
         let arg_tys = mir.arg_decls.iter().map(|a| a.ty);
         let var_tys = mir.var_decls.iter().map(|v| v.ty);
@@ -350,13 +362,16 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
         }
     }
 
-    fn pop_stack_frame(&mut self) {
+    fn pop_stack_frame(&mut self) -> EvalResult<'tcx, ()> {
         ::log_settings::settings().indentation -= 1;
         let frame = self.stack.pop().expect("tried to pop a stack frame, but there were none");
-        if let Some(target) = frame.return_to_block {
-            self.goto_block(target);
+        match frame.return_to_block {
+            StackPopCleanup::Freeze(alloc_id) => self.memory.freeze(alloc_id)?,
+            StackPopCleanup::Goto(target) => self.goto_block(target),
+            StackPopCleanup::None => {},
         }
         // TODO(solson): Deallocate local variables.
+        Ok(())
     }
 
     /// Applies the binary operation `op` to the two operands and writes a tuple of the result
@@ -1036,7 +1051,7 @@ pub fn eval_main<'a, 'tcx: 'a>(
     let return_ptr = ecx.alloc_ret_ptr(mir.return_ty, substs)
         .expect("should at least be able to allocate space for the main function's return value");
 
-    ecx.push_stack_frame(def_id, mir.span, CachedMir::Ref(mir), substs, Some(return_ptr), None)
+    ecx.push_stack_frame(def_id, mir.span, CachedMir::Ref(mir), substs, Some(return_ptr), StackPopCleanup::None)
         .expect("could not allocate first stack frame");
 
     if mir.arg_decls.len() == 2 {
diff --git a/src/interpreter/step.rs b/src/interpreter/step.rs
@@ -7,11 +7,13 @@ use super::{
     ConstantId,
     EvalContext,
     ConstantKind,
+    StackPopCleanup,
 };
 use error::EvalResult;
 use rustc::mir::repr as mir;
 use rustc::ty::{subst, self};
 use rustc::hir::def_id::DefId;
+use rustc::hir;
 use rustc::mir::visit::{Visitor, LvalueContext};
 use syntax::codemap::Span;
 use std::rc::Rc;
@@ -110,7 +112,7 @@ struct ConstantExtractor<'a, 'b: 'a, 'tcx: 'b> {
 }
 
 impl<'a, 'b, 'tcx> ConstantExtractor<'a, 'b, 'tcx> {
-    fn global_item(&mut self, def_id: DefId, substs: &'tcx subst::Substs<'tcx>, span: Span) {
+    fn global_item(&mut self, def_id: DefId, substs: &'tcx subst::Substs<'tcx>, span: Span, immutable: bool) {
         let cid = ConstantId {
             def_id: def_id,
             substs: substs,
@@ -123,7 +125,12 @@ impl<'a, 'b, 'tcx> ConstantExtractor<'a, 'b, 'tcx> {
         self.try(|this| {
             let ptr = this.ecx.alloc_ret_ptr(mir.return_ty, substs)?;
             this.ecx.statics.insert(cid.clone(), ptr);
-            this.ecx.push_stack_frame(def_id, span, mir, substs, Some(ptr), None)
+            let cleanup = if immutable && !mir.return_ty.type_contents(this.ecx.tcx).interior_unsafe() {
+                StackPopCleanup::Freeze(ptr.alloc_id)
+            } else {
+                StackPopCleanup::None
+            };
+            this.ecx.push_stack_frame(def_id, span, mir, substs, Some(ptr), cleanup)
         });
     }
     fn try<F: FnOnce(&mut Self) -> EvalResult<'tcx, ()>>(&mut self, f: F) {
@@ -150,7 +157,7 @@ impl<'a, 'b, 'tcx> Visitor<'tcx> for ConstantExtractor<'a, 'b, 'tcx> {
                     // because the type is the actual function, not the signature of the function.
                     // Thus we can simply create a zero sized allocation in `evaluate_operand`
                 } else {
-                    self.global_item(def_id, substs, constant.span);
+                    self.global_item(def_id, substs, constant.span, true);
                 }
             },
             mir::Literal::Promoted { index } => {
@@ -168,7 +175,12 @@ impl<'a, 'b, 'tcx> Visitor<'tcx> for ConstantExtractor<'a, 'b, 'tcx> {
                     let return_ptr = this.ecx.alloc_ret_ptr(return_ty, cid.substs)?;
                     let mir = CachedMir::Owned(Rc::new(mir));
                     this.ecx.statics.insert(cid.clone(), return_ptr);
-                    this.ecx.push_stack_frame(this.def_id, constant.span, mir, this.substs, Some(return_ptr), None)
+                    this.ecx.push_stack_frame(this.def_id,
+                                              constant.span,
+                                              mir,
+                                              this.substs,
+                                              Some(return_ptr),
+                                              StackPopCleanup::Freeze(return_ptr.alloc_id))
                 });
             }
         }
@@ -179,7 +191,17 @@ impl<'a, 'b, 'tcx> Visitor<'tcx> for ConstantExtractor<'a, 'b, 'tcx> {
         if let mir::Lvalue::Static(def_id) = *lvalue {
             let substs = subst::Substs::empty(self.ecx.tcx);
             let span = self.span;
-            self.global_item(def_id, substs, span);
+            if let hir::map::Node::NodeItem(&hir::Item { ref node, .. }) = self.ecx.tcx.map.get_if_local(def_id).expect("static not found") {
+                if let hir::ItemStatic(_, m, _) = *node {
+                    self.global_item(def_id, substs, span, m == hir::MutImmutable);
+                    return;
+                } else {
+                    bug!("static def id doesn't point to static");
+                }
+            } else {
+                bug!("static def id doesn't point to item");
+            }
+            self.global_item(def_id, substs, span, false);
         }
     }
 }
diff --git a/src/interpreter/terminator.rs b/src/interpreter/terminator.rs
@@ -10,7 +10,7 @@ use std::iter;
 use syntax::{ast, attr};
 use syntax::codemap::{DUMMY_SP, Span};
 
-use super::{EvalContext, IntegerExt};
+use super::{EvalContext, IntegerExt, StackPopCleanup};
 use error::{EvalError, EvalResult};
 use memory::Pointer;
 
@@ -27,7 +27,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
     ) -> EvalResult<'tcx, ()> {
         use rustc::mir::repr::TerminatorKind::*;
         match terminator.kind {
-            Return => self.pop_stack_frame(),
+            Return => self.pop_stack_frame()?,
 
             Goto { target } => self.goto_block(target),
 
@@ -210,8 +210,8 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
 
                 let mir = self.load_mir(resolved_def_id);
                 let (return_ptr, return_to_block) = match destination {
-                    Some((ptr, block)) => (Some(ptr), Some(block)),
-                    None => (None, None),
+                    Some((ptr, block)) => (Some(ptr), StackPopCleanup::Goto(block)),
+                    None => (None, StackPopCleanup::None),
                 };
                 self.push_stack_frame(def_id, span, mir, resolved_substs, return_ptr, return_to_block)?;
 
diff --git a/src/interpreter/vtable.rs b/src/interpreter/vtable.rs
@@ -96,6 +96,8 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
             }
         }
 
+        self.memory.freeze(vtable.alloc_id)?;
+
         Ok(vtable)
     }
 
diff --git a/src/memory.rs b/src/memory.rs
@@ -26,10 +26,20 @@ impl fmt::Display for AllocId {
 
 #[derive(Debug)]
 pub struct Allocation {
+    /// The actual bytes of the allocation.
+    /// Note that the bytes of a pointer represent the offset of the pointer
     pub bytes: Vec<u8>,
+    /// Maps from byte addresses to allocations.
+    /// Only the first byte of a pointer is inserted into the map.
     pub relocations: BTreeMap<usize, AllocId>,
+    /// Denotes undefined memory. Reading from undefined memory is forbidden in miri
     pub undef_mask: UndefMask,
+    /// The alignment of the allocation to detect unaligned reads.
     pub align: usize,
+    /// 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,
 }
 
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
@@ -111,6 +121,7 @@ impl<'a, 'tcx> Memory<'a, 'tcx> {
             relocations: BTreeMap::new(),
             undef_mask: UndefMask::new(0),
             align: 1,
+            immutable: false, // must be mutable, because sometimes we "move out" of a ZST
         };
         mem.alloc_map.insert(ZST_ALLOC_ID, alloc);
         // check that additional zst allocs work
@@ -179,6 +190,7 @@ impl<'a, 'tcx> Memory<'a, 'tcx> {
             relocations: BTreeMap::new(),
             undef_mask: UndefMask::new(size),
             align: align,
+            immutable: false,
         };
         let id = self.next_id;
         self.next_id.0 += 1;
@@ -287,6 +299,7 @@ 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),
             None => match self.functions.get(&id) {
                 Some(_) => Err(EvalError::DerefFunctionPointer),
@@ -430,10 +443,16 @@ 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, ()> {
+        self.get_mut(alloc_id)?.immutable = true;
+        Ok(())
+    }
+
     pub fn copy(&mut self, src: Pointer, dest: Pointer, size: usize, align: usize) -> EvalResult<'tcx, ()> {
         self.check_relocation_edges(src, size)?;
 
-        let src_bytes = self.get_bytes_unchecked_mut(src, size)?.as_mut_ptr();
+        let src_bytes = self.get_bytes_unchecked(src, size)?.as_ptr();
         let dest_bytes = self.get_bytes_mut(dest, size, align)?.as_mut_ptr();
 
         // SAFE: The above indexing would have panicked if there weren't at least `size` bytes
diff --git a/tests/compile-fail/static_memory_modification.rs b/tests/compile-fail/static_memory_modification.rs
@@ -0,0 +1,9 @@
+static X: usize = 5;
+
+#[allow(mutable_transmutes)]
+fn main() {
+    unsafe {
+        *std::mem::transmute::<&usize, &mut usize>(&X) = 6; //~ ERROR: tried to modify constant memory
+        assert_eq!(X, 6);
+    }
+}
diff --git a/tests/run-pass/static_memory_modification.rs b/tests/run-pass/static_memory_modification.rs

Original file line number	Diff line number	Diff line change
`@@ -42,6 +42,7 @@ pub enum EvalError<'tcx> {`
`42`	`42`	`},`
`43`	`43`	`CalledClosureAsFunction,`
`44`	`44`	`VtableForArgumentlessMethod,`
	`45`	`+ ModifiedConstantMemory,`
`45`	`46`	`}`
`46`	`47`
`47`	`48`	`pub type EvalResult<'tcx, T> = Result<T, EvalError<'tcx>>;`
`@@ -94,6 +95,8 @@ impl<'tcx> Error for EvalError<'tcx> {`
`94`	`95`	`"tried to call a closure through a function pointer",`
`95`	`96`	`EvalError::VtableForArgumentlessMethod =>`
`96`	`97`	`"tried to call a vtable function without arguments",`
	`98`	`+ EvalError::ModifiedConstantMemory =>`
	`99`	`+ "tried to modify constant memory",`
`97`	`100`	`}`
`98`	`101`	`}`
`99`	`102`
Original file line number	Diff line number	Diff line change
`@@ -96,6 +96,8 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {`
`96`	`96`	`}`
`97`	`97`	`}`
`98`	`98`
	`99`	`+ self.memory.freeze(vtable.alloc_id)?;`
	`100`	`+`
`99`	`101`	`Ok(vtable)`
`100`	`102`	`}`
`101`	`103`