Auto merge of #795 - RalfJung:intptrcast, r=RalfJung

tweak inttoptr allocation behavior

- Make `align_addr` not offset by `align` for no reason.
- Add some random slack between allocations to give them the chance to not be aligned.

Cc @christianpoveda

Fixes rust-lang/miri#791

Author: bors

src/eval.rs

@@ -10,7 +10,7 @@ use rustc::mir;
 use crate::{
     InterpResult, InterpError, InterpretCx, StackPopCleanup, struct_error,
     Scalar, Tag, Pointer,
-    MiriMemoryKind, Evaluator, TlsEvalContextExt,
+    MemoryExtra, MiriMemoryKind, Evaluator, TlsEvalContextExt,
 };
 
 /// Configuration needed to spawn a Miri instance.
@@ -36,7 +36,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
     );
 
     // FIXME: InterpretCx::new should take an initial MemoryExtra
-    ecx.memory_mut().extra.rng = config.seed.map(StdRng::seed_from_u64);
+    ecx.memory_mut().extra = MemoryExtra::with_rng(config.seed.map(StdRng::seed_from_u64));
 
     let main_instance = ty::Instance::mono(ecx.tcx.tcx, main_id);
     let main_mir = ecx.load_mir(main_instance.def)?;

src/fn_call.rs

@@ -982,6 +982,7 @@ fn gen_random<'mir, 'tcx>(
 
     let data = match &mut this.memory_mut().extra.rng {
         Some(rng) => {
+            let mut rng = rng.borrow_mut();
             let mut data = vec![0; len];
             rng.fill_bytes(&mut data);
             data

src/intptrcast.rs

@@ -1,5 +1,7 @@
 use std::cell::{Cell, RefCell};
 
+use rand::Rng;
+
 use rustc::mir::interpret::{AllocId, Pointer, InterpResult};
 use rustc_mir::interpret::Memory;
 use rustc_target::abi::Size;
@@ -73,14 +75,24 @@ impl<'mir, 'tcx> GlobalState {
         let mut global_state = memory.extra.intptrcast.borrow_mut();
 
         let alloc = memory.get(ptr.alloc_id)?;
+        let align = alloc.align.bytes();
 
         let base_addr = match alloc.extra.intptrcast.base_addr.get() { 
             Some(base_addr) => base_addr,
             None => {
                 // This allocation does not have a base address yet, pick one.
-                let base_addr = Self::align_addr(global_state.next_base_addr, alloc.align.bytes());
-                global_state.next_base_addr = base_addr + alloc.bytes.len() as u64;
+                // Leave some space to the previous allocation, to give it some chance to be less aligned.
+                let slack = {
+                    let mut rng = memory.extra.rng.as_ref().unwrap().borrow_mut();
+                    // This means that `(global_state.next_base_addr + slack) % 16` is uniformly distributed.
+                    rng.gen_range(0, 16)
+                };
+                // From next_base_addr + slack, round up to adjust for alignment.
+                let base_addr = Self::align_addr(global_state.next_base_addr + slack, align);
                 alloc.extra.intptrcast.base_addr.set(Some(base_addr));
+
+                // Remember next base address.
+                global_state.next_base_addr = base_addr + alloc.bytes.len() as u64;
                 // Given that `next_base_addr` increases in each allocation, pushing the
                 // corresponding tuple keeps `int_to_ptr_map` sorted
                 global_state.int_to_ptr_map.push((base_addr, ptr.alloc_id)); 
@@ -89,13 +101,27 @@ impl<'mir, 'tcx> GlobalState {
             }
         };
 
-        debug_assert_eq!(base_addr % alloc.align.bytes(), 0); // sanity check
+        debug_assert_eq!(base_addr % align, 0); // sanity check
         Ok(base_addr + ptr.offset.bytes())
     }
 
     /// Shifts `addr` to make it aligned with `align` by rounding `addr` to the smallest multiple
-    /// of `align` that is strictly larger to `addr`
+    /// of `align` that is larger or equal to `addr`
     fn align_addr(addr: u64, align: u64) -> u64 {
-        addr + align - addr % align
+        match addr % align {
+            0 => addr,
+            rem => addr + align - rem
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_align_addr() {
+        assert_eq!(GlobalState::align_addr(37, 4), 40);
+        assert_eq!(GlobalState::align_addr(44, 4), 44);
     }
 }

src/machine.rs

@@ -1,6 +1,7 @@
 use std::rc::Rc;
 use std::borrow::Cow;
 use std::collections::HashMap;
+use std::cell::RefCell;
 
 use rand::rngs::StdRng;
 
@@ -46,15 +47,15 @@ pub struct MemoryExtra {
     pub intptrcast: intptrcast::MemoryExtra,
     /// The random number generator to use if Miri is running in non-deterministic mode and to
     /// enable intptrcast
-    pub(crate) rng: Option<StdRng>
+    pub(crate) rng: Option<RefCell<StdRng>>
 }
 
 impl MemoryExtra {
     pub fn with_rng(rng: Option<StdRng>) -> Self {
         MemoryExtra {
             stacked_borrows: Default::default(),
             intptrcast: Default::default(),
-            rng,
+            rng: rng.map(RefCell::new),
         }
     }
 }

tests/compile-fail/intptrcast_alignment_check.rs

@@ -0,0 +1,18 @@
+// Validation makes this fail in the wrong place
+// compile-flags: -Zmiri-disable-validation -Zmiri-seed=0000000000000000
+
+// Even with intptrcast and without validation, we want to be *sure* to catch bugs
+// that arise from pointers being insufficiently aligned. The only way to achieve
+// that is not not let programs exploit integer information for alignment, so here
+// we test that this is indeed the case.
+fn main() {
+    let x = &mut [0u8; 3];
+    let base_addr = x as *mut _ as usize;
+    let u16_ref = unsafe { if base_addr % 2 == 0 {
+        &mut *(base_addr as *mut u16)
+    } else {
+        &mut *((base_addr+1) as *mut u16)
+    } };
+    *u16_ref = 2; //~ ERROR tried to access memory with alignment 1, but alignment 2 is required
+    println!("{:?}", x);
+}