Auto merge of #851 - RalfJung:intrptrcast-by-default, r=oli-obk

enable Intrptrcast by default

As laid out in #785: we change Miri to always have an RNG, seeded per default with 0. Then we adjust everything to remove dead code and dead tests.

r? @oli-obk
Cc @christianpoveda

README.md

@@ -262,10 +262,10 @@ With this, you should now have a working development setup!  See
 
 Several `-Z` flags are relevant for Miri:
 
-* `-Zmiri-seed=<hex>` is a custom `-Z` flag added by Miri.  It enables the
-  interpreted program to seed an RNG with system entropy.  Miri will keep an RNG
-  on its own that is seeded with the given seed, and use that to generate the
-  "system entropy" that seeds the RNG(s) in the interpreted program.
+* `-Zmiri-seed=<hex>` is a custom `-Z` flag added by Miri.  It configures the
+  seed of the RNG that Miri uses to resolve non-determinism.  This RNG is used
+  to pick base addresses for allocations, and when the interpreted program
+  requests system entropy.  The default seed is 0.
   **NOTE**: This entropy is not good enough for cryptographic use!  Do not
   generate secret keys in Miri or perform other kinds of cryptographic
   operations that rely on proper random numbers.

src/eval.rs

@@ -34,7 +34,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
         tcx.at(syntax::source_map::DUMMY_SP),
         ty::ParamEnv::reveal_all(),
         Evaluator::new(),
-        MemoryExtra::new(config.seed.map(StdRng::seed_from_u64), config.validate),
+        MemoryExtra::new(StdRng::seed_from_u64(config.seed.unwrap_or(0)), config.validate),
     );
 
     let main_instance = ty::Instance::mono(ecx.tcx.tcx, main_id);

src/helpers.rs

@@ -88,25 +88,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             None => return Ok(()), // zero-sized access
         };
 
-        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
-            }
-            None => {
-                return err!(Unimplemented(
-                    "miri does not support gathering system entropy in deterministic mode!
-                    Use '-Zmiri-seed=<seed>' to enable random number generation.
-                    WARNING: Miri does *not* generate cryptographically secure entropy -
-                    do not use Miri to run any program that needs secure random number generation".to_owned(),
-                ));
-            }
-        };
+        let rng = this.memory_mut().extra.rng.get_mut();
+        let mut data = vec![0; len];
+        rng.fill_bytes(&mut data);
+
         let tcx = &{this.tcx.tcx};
-        this.memory_mut().get_mut(ptr.alloc_id)?
-            .write_bytes(tcx, ptr, &data)
+        this.memory_mut().get_mut(ptr.alloc_id)?.write_bytes(tcx, ptr, &data)
     }
 
     /// Visits the memory covered by `place`, sensitive to freezing: the 3rd parameter

src/intptrcast.rs

@@ -42,6 +42,10 @@ impl<'mir, 'tcx> GlobalState {
         int: u64,
         memory: &Memory<'mir, 'tcx, Evaluator<'tcx>>,
     ) -> InterpResult<'tcx, Pointer<Tag>> {
+        if int == 0 {
+            return err!(InvalidNullPointerUsage);
+        }
+
         let global_state = memory.extra.intptrcast.borrow();
 
         match global_state.int_to_ptr_map.binary_search_by_key(&int, |(addr, _)| *addr) {
@@ -86,12 +90,13 @@ impl<'mir, 'tcx> GlobalState {
                 // This allocation does not have a base address yet, pick one.
                 // 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();
+                    let mut rng = memory.extra.rng.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.bytes());
+                let base_addr = global_state.next_base_addr.checked_add(slack).unwrap();
+                let base_addr = Self::align_addr(base_addr, align.bytes());
                 entry.insert(base_addr);
                 trace!(
                     "Assigning base address {:#x} to allocation {:?} (slack: {}, align: {})",
@@ -100,7 +105,7 @@ impl<'mir, 'tcx> GlobalState {
 
                 // Remember next base address.  If this allocation is zero-sized, leave a gap
                 // of at least 1 to avoid two allocations having the same base address.
-                global_state.next_base_addr = base_addr + max(size.bytes(), 1);
+                global_state.next_base_addr = base_addr.checked_add(max(size.bytes(), 1)).unwrap();
                 // 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)); 
@@ -120,7 +125,7 @@ impl<'mir, 'tcx> GlobalState {
     fn align_addr(addr: u64, align: u64) -> u64 {
         match addr % align {
             0 => addr,
-            rem => addr + align - rem
+            rem => addr.checked_add(align).unwrap() - rem
         }
     }
 }

src/machine.rs

@@ -57,20 +57,19 @@ pub struct MemoryExtra {
     pub stacked_borrows: stacked_borrows::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<RefCell<StdRng>>,
+    /// The random number generator used for resolving non-determinism.
+    pub(crate) rng: RefCell<StdRng>,
 
     /// Whether to enforce the validity invariant.
     pub(crate) validate: bool,
 }
 
 impl MemoryExtra {
-    pub fn new(rng: Option<StdRng>, validate: bool) -> Self {
+    pub fn new(rng: StdRng, validate: bool) -> Self {
         MemoryExtra {
             stacked_borrows: Default::default(),
             intptrcast: Default::default(),
-            rng: rng.map(RefCell::new),
+            rng: RefCell::new(rng),
             validate,
         }
     }
@@ -353,28 +352,20 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'tcx> {
         Ok(ecx.memory().extra.stacked_borrows.borrow_mut().end_call(extra))
     }
 
+    #[inline(always)]
     fn int_to_ptr(
         memory: &Memory<'mir, 'tcx, Self>,
         int: u64,
     ) -> InterpResult<'tcx, Pointer<Self::PointerTag>> {
-        if int == 0 {
-            err!(InvalidNullPointerUsage)
-        } else if memory.extra.rng.is_none() {
-            err!(ReadBytesAsPointer)
-        } else {
-           intptrcast::GlobalState::int_to_ptr(int, memory)
-        }
+        intptrcast::GlobalState::int_to_ptr(int, memory)
     }
 
+    #[inline(always)]
     fn ptr_to_int(
         memory: &Memory<'mir, 'tcx, Self>,
         ptr: Pointer<Self::PointerTag>,
     ) -> InterpResult<'tcx, u64> {
-        if memory.extra.rng.is_none() {
-            err!(ReadPointerAsBytes)
-        } else {
-            intptrcast::GlobalState::ptr_to_int(ptr, memory)
-        }
+        intptrcast::GlobalState::ptr_to_int(ptr, memory)
     }
 }
 

src/operator.rs

@@ -56,8 +56,8 @@ impl<'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'mir, 'tcx> {
 
         trace!("ptr_op: {:?} {:?} {:?}", *left, bin_op, *right);
 
-        // If intptrcast is enabled, treat everything of integer *type* at integer *value*.
-        if self.memory().extra.rng.is_some() && left.layout.ty.is_integral() {
+        // Treat everything of integer *type* at integer *value*.
+        if left.layout.ty.is_integral() {
             // This is actually an integer operation, so dispatch back to the core engine.
             // TODO: Once intptrcast is the default, librustc_mir should never even call us
             // for integer types.
@@ -188,104 +188,11 @@ impl<'mir, 'tcx> EvalContextExt<'tcx> for super::MiriEvalContext<'mir, 'tcx> {
         right: Scalar<Tag>,
     ) -> InterpResult<'tcx, bool> {
         let size = self.pointer_size();
-        if self.memory().extra.rng.is_some() {
-            // Just compare the integers.
-            // TODO: Do we really want to *always* do that, even when comparing two live in-bounds pointers?
-            let left = self.force_bits(left, size)?;
-            let right = self.force_bits(right, size)?;
-            return Ok(left == right);
-        }
-        Ok(match (left, right) {
-            (Scalar::Raw { .. }, Scalar::Raw { .. }) =>
-                left.to_bits(size)? == right.to_bits(size)?,
-            (Scalar::Ptr(left), Scalar::Ptr(right)) => {
-                // Comparison illegal if one of them is out-of-bounds, *unless* they
-                // are in the same allocation.
-                if left.alloc_id == right.alloc_id {
-                    left.offset == right.offset
-                } else {
-                    // Make sure both pointers are in-bounds.
-                    // This accepts one-past-the end. Thus, there is still technically
-                    // some non-determinism that we do not fully rule out when two
-                    // allocations sit right next to each other. The C/C++ standards are
-                    // somewhat fuzzy about this case, so pragmatically speaking I think
-                    // for now this check is "good enough".
-                    // FIXME: Once we support intptrcast, we could try to fix these holes.
-                    // Dead allocations in miri cannot overlap with live allocations, but
-                    // on read hardware this can easily happen. Thus for comparisons we require
-                    // both pointers to be live.
-                    if self.pointer_inbounds(left).is_ok() && self.pointer_inbounds(right).is_ok() {
-                        // Two in-bounds (and hence live) pointers in different allocations are different.
-                        false
-                    } else {
-                        return err!(InvalidPointerMath);
-                    }
-                }
-            }
-            // Comparing ptr and integer.
-            (Scalar::Ptr(ptr), Scalar::Raw { data, size }) |
-            (Scalar::Raw { data, size }, Scalar::Ptr(ptr)) => {
-                assert_eq!(size as u64, self.pointer_size().bytes());
-                let bits = data as u64;
-
-                // Case I: Comparing real pointers with "small" integers.
-                // Really we should only do this for NULL, but pragmatically speaking on non-bare-metal systems,
-                // an allocation will never be at the very bottom of the address space.
-                // Such comparisons can arise when comparing empty slices, which sometimes are "fake"
-                // integer pointers (okay because the slice is empty) and sometimes point into a
-                // real allocation.
-                // The most common source of such integer pointers is `NonNull::dangling()`, which
-                // equals the type's alignment. i128 might have an alignment of 16 bytes, but few types have
-                // alignment 32 or higher, hence the limit of 32.
-                // FIXME: Once we support intptrcast, we could try to fix these holes.
-                if bits < 32 {
-                    // Test if the pointer can be different from NULL or not.
-                    // We assume that pointers that are not NULL are also not "small".
-                    if !self.memory().ptr_may_be_null(ptr) {
-                        return Ok(false);
-                    }
-                }
-
-                let (alloc_size, alloc_align) = self.memory()
-                    .get_size_and_align(ptr.alloc_id, AllocCheck::MaybeDead)
-                    .expect("alloc info with MaybeDead cannot fail");
-
-                // Case II: Alignment gives it away
-                if ptr.offset.bytes() % alloc_align.bytes() == 0 {
-                    // The offset maintains the allocation alignment, so we know `base+offset`
-                    // is aligned by `alloc_align`.
-                    // FIXME: We could be even more general, e.g., offset 2 into a 4-aligned
-                    // allocation cannot equal 3.
-                    if bits % alloc_align.bytes() != 0 {
-                        // The integer is *not* aligned. So they cannot be equal.
-                        return Ok(false);
-                    }
-                }
-                // Case III: The integer is too big, and the allocation goes on a bit
-                // without wrapping around the address space.
-                {
-                    // Compute the highest address at which this allocation could live.
-                    // Substract one more, because it must be possible to add the size
-                    // to the base address without overflowing; that is, the very last address
-                    // of the address space is never dereferencable (but it can be in-bounds, i.e.,
-                    // one-past-the-end).
-                    let max_base_addr =
-                        ((1u128 << self.pointer_size().bits())
-                         - u128::from(alloc_size.bytes())
-                         - 1
-                        ) as u64;
-                    if let Some(max_addr) = max_base_addr.checked_add(ptr.offset.bytes()) {
-                        if bits > max_addr {
-                            // The integer is too big, this cannot possibly be equal.
-                            return Ok(false)
-                        }
-                    }
-                }
-
-                // None of the supported cases.
-                return err!(InvalidPointerMath);
-            }
-        })
+        // Just compare the integers.
+        // TODO: Do we really want to *always* do that, even when comparing two live in-bounds pointers?
+        let left = self.force_bits(left, size)?;
+        let right = self.force_bits(right, size)?;
+        Ok(left == right)
     }
 
     fn ptr_int_arithmetic(

tests/compile-fail/cast_box_int_to_fn_ptr.rs

@@ -4,8 +4,8 @@
 fn main() {
     let b = Box::new(42);
     let g = unsafe {
-        std::mem::transmute::<&usize, &fn(i32)>(&b)
+        std::mem::transmute::<&Box<usize>, &fn(i32)>(&b)
     };
 
-    (*g)(42) //~ ERROR a memory access tried to interpret some bytes as a pointer
+    (*g)(42) //~ ERROR tried to treat a memory pointer as a function pointer
 }

tests/compile-fail/cast_int_to_fn_ptr.rs

@@ -6,5 +6,5 @@ fn main() {
         std::mem::transmute::<usize, fn(i32)>(42)
     };
 
-    g(42) //~ ERROR a memory access tried to interpret some bytes as a pointer
+    g(42) //~ ERROR dangling pointer was dereferenced
 }

tests/compile-fail/intptrcast_alignment_check.rs

@@ -1,5 +1,5 @@
 // Validation makes this fail in the wrong place
-// compile-flags: -Zmiri-disable-validation -Zmiri-seed=0000000000000000
+// compile-flags: -Zmiri-disable-validation
 
 // 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