From e574c77aa2292ac2a776754a60b1320058bdb071 Mon Sep 17 00:00:00 2001
From: Ralf Jung <post@ralfj.de>
Date: Sat, 19 Oct 2019 12:28:39 +0200
Subject: [PATCH] audit our bounds checks

---
 src/eval.rs                |  2 +-
 src/helpers.rs             |  1 +
 src/shims/foreign_items.rs | 19 ++++++-------------
 src/shims/intrinsics.rs    |  2 ++
 4 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/src/eval.rs b/src/eval.rs
index bb0bad0ea03..21a7dd35212 100644
--- a/src/eval.rs
+++ b/src/eval.rs
@@ -162,7 +162,7 @@ pub fn create_ecx<'mir, 'tcx: 'mir>(
             MiriMemoryKind::Env.into(),
         );
         ecx.machine.cmd_line = Some(cmd_ptr);
-        // Store the UTF-16 string.
+        // Store the UTF-16 string. We just allocated so we know the bounds are fine.
         let char_size = Size::from_bytes(2);
         let cmd_alloc = ecx.memory.get_mut(cmd_ptr.alloc_id)?;
         let mut cur_ptr = cmd_ptr;
diff --git a/src/helpers.rs b/src/helpers.rs
index c09d5c823e1..0c22a0f2e06 100644
--- a/src/helpers.rs
+++ b/src/helpers.rs
@@ -94,6 +94,7 @@ fn gen_random(
         }
         let this = self.eval_context_mut();
 
+        // Don't forget the bounds check.
         let ptr = this.memory.check_ptr_access(
             ptr,
             Size::from_bytes(len as u64),
diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs
index cfbb02f6081..cfd3743089e 100644
--- a/src/shims/foreign_items.rs
+++ b/src/shims/foreign_items.rs
@@ -50,7 +50,7 @@ fn malloc(&mut self, size: u64, zero_init: bool, kind: MiriMemoryKind) -> Scalar
                 .memory
                 .allocate(Size::from_bytes(size), align, kind.into());
             if zero_init {
-                // We just allocated this, the access cannot fail
+                // We just allocated this, the access is definitely in-bounds.
                 this.memory
                     .get_mut(ptr.alloc_id)
                     .unwrap()
@@ -227,7 +227,7 @@ fn emulate_foreign_item(
                     Align::from_bytes(align).unwrap(),
                     MiriMemoryKind::Rust.into(),
                 );
-                // We just allocated this, the access cannot fail
+                // We just allocated this, the access is definitely in-bounds.
                 this.memory
                     .get_mut(ptr.alloc_id)
                     .unwrap()
@@ -643,7 +643,7 @@ fn emulate_foreign_item(
 
             // Hook pthread calls that go to the thread-local storage memory subsystem.
             "pthread_key_create" => {
-                let key_ptr = this.read_scalar(args[0])?.not_undef()?;
+                let key_place = this.deref_operand(args[0])?;
 
                 // Extract the function type out of the signature (that seems easier than constructing it ourselves).
                 let dtor = match this.test_null(this.read_scalar(args[1])?.not_undef()?)? {
@@ -668,16 +668,7 @@ fn emulate_foreign_item(
                     throw_unsup!(OutOfTls);
                 }
 
-                let key_ptr = this
-                    .memory
-                    .check_ptr_access(key_ptr, key_layout.size, key_layout.align.abi)?
-                    .expect("cannot be a ZST");
-                this.memory.get_mut(key_ptr.alloc_id)?.write_scalar(
-                    tcx,
-                    key_ptr,
-                    Scalar::from_uint(key, key_layout.size).into(),
-                    key_layout.size,
-                )?;
+                this.write_scalar(Scalar::from_uint(key, key_layout.size), key_place.into())?;
 
                 // Return success (`0`).
                 this.write_null(dest)?;
@@ -856,6 +847,7 @@ fn emulate_foreign_item(
                 let system_info_ptr = this
                     .check_mplace_access(system_info, None)?
                     .expect("cannot be a ZST");
+                // We rely on `deref_operand` doing bounds checks for us.
                 // Initialize with `0`.
                 this.memory
                     .get_mut(system_info_ptr.alloc_id)?
@@ -992,6 +984,7 @@ fn eval_path_scalar(
     fn set_last_error(&mut self, scalar: Scalar<Tag>) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
         let errno_ptr = this.machine.last_error.unwrap();
+        // We allocated this during machine initialziation so the bounds are fine.
         this.memory.get_mut(errno_ptr.alloc_id)?.write_scalar(
             &*this.tcx,
             errno_ptr,
diff --git a/src/shims/intrinsics.rs b/src/shims/intrinsics.rs
index 4666557e200..cd7db097366 100644
--- a/src/shims/intrinsics.rs
+++ b/src/shims/intrinsics.rs
@@ -359,6 +359,7 @@ fn call_intrinsic(
                             assert!(mplace.meta.is_none());
                             // not a zst, must be valid pointer
                             let ptr = mplace.ptr.to_ptr()?;
+                            // we know the return place is in-bounds
                             this.memory.get_mut(ptr.alloc_id)?.write_repeat(tcx, ptr, 0, dest.layout.size)?;
                         }
                     }
@@ -548,6 +549,7 @@ fn call_intrinsic(
                             let mplace = this.force_allocation(dest)?;
                             assert!(mplace.meta.is_none());
                             let ptr = mplace.ptr.to_ptr()?;
+                            // We know the return place is in-bounds
                             this.memory
                                 .get_mut(ptr.alloc_id)?
                                 .mark_definedness(ptr, dest.layout.size, false);
-- 
2.44.0