Auto merge of #2661 - DrMeepster:deref_operand_as, r=oli-obk

Dereference pointers in shims as correct types

Currently, shims will dereference pointers as the type written by the user. This can cause false positives, incorrect behavior such as #2136, and even ICEs if a field is not present.

This PR fixes this by having shims dereference pointers with types from `std` or `libc` that we can rely on the layout and field names of instead of with whatever the user passed in.

Fixes #1123

Author: bors

Diff for: src/concurrency/init_once.rs

@@ -2,6 +2,7 @@ use std::collections::VecDeque;
 use std::num::NonZeroU32;
 
 use rustc_index::Idx;
+use rustc_middle::ty::layout::TyAndLayout;
 
 use super::sync::EvalContextExtPriv as _;
 use super::thread::MachineCallback;
@@ -94,10 +95,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
     fn init_once_get_or_create_id(
         &mut self,
         lock_op: &OpTy<'tcx, Provenance>,
+        lock_layout: TyAndLayout<'tcx>,
         offset: u64,
     ) -> InterpResult<'tcx, InitOnceId> {
         let this = self.eval_context_mut();
-        this.init_once_get_or_create(|ecx, next_id| ecx.get_or_create_id(next_id, lock_op, offset))
+        this.init_once_get_or_create(|ecx, next_id| {
+            ecx.get_or_create_id(next_id, lock_op, lock_layout, offset)
+        })
     }
 
     /// Provides the closure with the next InitOnceId. Creates that InitOnce if the closure returns None,

Diff for: src/concurrency/sync.rs

@@ -6,6 +6,7 @@ use log::trace;
 
 use rustc_data_structures::fx::FxHashMap;
 use rustc_index::{Idx, IndexVec};
+use rustc_middle::ty::layout::TyAndLayout;
 
 use super::init_once::InitOnce;
 use super::vector_clock::VClock;
@@ -200,11 +201,12 @@ pub(super) trait EvalContextExtPriv<'mir, 'tcx: 'mir>:
         &mut self,
         next_id: Id,
         lock_op: &OpTy<'tcx, Provenance>,
+        lock_layout: TyAndLayout<'tcx>,
         offset: u64,
     ) -> InterpResult<'tcx, Option<Id>> {
         let this = self.eval_context_mut();
         let value_place =
-            this.deref_operand_and_offset(lock_op, offset, this.machine.layouts.u32)?;
+            this.deref_operand_and_offset(lock_op, offset, lock_layout, this.machine.layouts.u32)?;
 
         // Since we are lazy, this update has to be atomic.
         let (old, success) = this
@@ -278,28 +280,37 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
     fn mutex_get_or_create_id(
         &mut self,
         lock_op: &OpTy<'tcx, Provenance>,
+        lock_layout: TyAndLayout<'tcx>,
         offset: u64,
     ) -> InterpResult<'tcx, MutexId> {
         let this = self.eval_context_mut();
-        this.mutex_get_or_create(|ecx, next_id| ecx.get_or_create_id(next_id, lock_op, offset))
+        this.mutex_get_or_create(|ecx, next_id| {
+            ecx.get_or_create_id(next_id, lock_op, lock_layout, offset)
+        })
     }
 
     fn rwlock_get_or_create_id(
         &mut self,
         lock_op: &OpTy<'tcx, Provenance>,
+        lock_layout: TyAndLayout<'tcx>,
         offset: u64,
     ) -> InterpResult<'tcx, RwLockId> {
         let this = self.eval_context_mut();
-        this.rwlock_get_or_create(|ecx, next_id| ecx.get_or_create_id(next_id, lock_op, offset))
+        this.rwlock_get_or_create(|ecx, next_id| {
+            ecx.get_or_create_id(next_id, lock_op, lock_layout, offset)
+        })
     }
 
     fn condvar_get_or_create_id(
         &mut self,
         lock_op: &OpTy<'tcx, Provenance>,
+        lock_layout: TyAndLayout<'tcx>,
         offset: u64,
     ) -> InterpResult<'tcx, CondvarId> {
         let this = self.eval_context_mut();
-        this.condvar_get_or_create(|ecx, next_id| ecx.get_or_create_id(next_id, lock_op, offset))
+        this.condvar_get_or_create(|ecx, next_id| {
+            ecx.get_or_create_id(next_id, lock_op, lock_layout, offset)
+        })
     }
 
     #[inline]

Diff for: src/helpers.rs

@@ -730,31 +730,63 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         }
     }
 
+    /// Dereference a pointer operand to a place using `layout` instead of the pointer's declared type
+    fn deref_operand_as(
+        &self,
+        op: &OpTy<'tcx, Provenance>,
+        layout: TyAndLayout<'tcx>,
+    ) -> InterpResult<'tcx, MPlaceTy<'tcx, Provenance>> {
+        let this = self.eval_context_ref();
+        let ptr = this.read_pointer(op)?;
+
+        let mplace = MPlaceTy::from_aligned_ptr(ptr, layout);
+
+        this.check_mplace(mplace)?;
+
+        Ok(mplace)
+    }
+
+    fn deref_pointer_as(
+        &self,
+        val: &ImmTy<'tcx, Provenance>,
+        layout: TyAndLayout<'tcx>,
+    ) -> InterpResult<'tcx, MPlaceTy<'tcx, Provenance>> {
+        let this = self.eval_context_ref();
+        let mut mplace = this.ref_to_mplace(val)?;
+
+        mplace.layout = layout;
+        mplace.align = layout.align.abi;
+
+        Ok(mplace)
+    }
+
     /// Calculates the MPlaceTy given the offset and layout of an access on an operand
     fn deref_operand_and_offset(
         &self,
         op: &OpTy<'tcx, Provenance>,
         offset: u64,
-        layout: TyAndLayout<'tcx>,
+        base_layout: TyAndLayout<'tcx>,
+        value_layout: TyAndLayout<'tcx>,
     ) -> InterpResult<'tcx, MPlaceTy<'tcx, Provenance>> {
         let this = self.eval_context_ref();
-        let op_place = this.deref_operand(op)?; // FIXME: we still deref with the original type!
+        let op_place = this.deref_operand_as(op, base_layout)?;
         let offset = Size::from_bytes(offset);
 
         // Ensure that the access is within bounds.
-        assert!(op_place.layout.size >= offset + layout.size);
-        let value_place = op_place.offset(offset, layout, this)?;
+        assert!(base_layout.size >= offset + value_layout.size);
+        let value_place = op_place.offset(offset, value_layout, this)?;
         Ok(value_place)
     }
 
     fn read_scalar_at_offset(
         &self,
         op: &OpTy<'tcx, Provenance>,
         offset: u64,
-        layout: TyAndLayout<'tcx>,
+        base_layout: TyAndLayout<'tcx>,
+        value_layout: TyAndLayout<'tcx>,
     ) -> InterpResult<'tcx, Scalar<Provenance>> {
         let this = self.eval_context_ref();
-        let value_place = this.deref_operand_and_offset(op, offset, layout)?;
+        let value_place = this.deref_operand_and_offset(op, offset, base_layout, value_layout)?;
         this.read_scalar(&value_place.into())
     }
 
@@ -763,10 +795,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         op: &OpTy<'tcx, Provenance>,
         offset: u64,
         value: impl Into<Scalar<Provenance>>,
-        layout: TyAndLayout<'tcx>,
+        base_layout: TyAndLayout<'tcx>,
+        value_layout: TyAndLayout<'tcx>,
     ) -> InterpResult<'tcx, ()> {
         let this = self.eval_context_mut();
-        let value_place = this.deref_operand_and_offset(op, offset, layout)?;
+        let value_place = this.deref_operand_and_offset(op, offset, base_layout, value_layout)?;
         this.write_scalar(value, &value_place.into())
     }
 

Diff for: src/shims/foreign_items.rs

@@ -409,14 +409,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         //     &mut self,
         //     arg1: &OpTy<'tcx, Provenance>,
         //     arg2: &OpTy<'tcx, Provenance>,
-        //     arg3: &OpTy<'tcx, Provenance>)
+        //     arg3: &OpTy<'tcx, Provenance>,
+        //     arg4: &OpTy<'tcx, Provenance>)
         // -> InterpResult<'tcx, Scalar<Provenance>> {
         //     let this = self.eval_context_mut();
         //
         //     // First thing: load all the arguments. Details depend on the shim.
         //     let arg1 = this.read_scalar(arg1)?.to_u32()?;
         //     let arg2 = this.read_pointer(arg2)?; // when you need to work with the pointer directly
-        //     let arg3 = this.deref_operand(arg3)?; // when you want to load/store through the pointer at its declared type
+        //     let arg3 = this.deref_operand(arg3)?; // when you want to load/store through the pointer
+        //     let arg4 = this.deref_operand_as(arg4, this.libc_ty_layout("some_libc_struct")?)
         //
         //     // ...
         //

Diff for: src/shims/time.rs

@@ -25,6 +25,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         this.assert_target_os_is_unix("clock_gettime");
 
         let clk_id = this.read_scalar(clk_id_op)?.to_i32()?;
+        let tp = this.deref_operand_as(tp_op, this.libc_ty_layout("timespec"))?;
 
         let absolute_clocks;
         let mut relative_clocks;
@@ -76,7 +77,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let tv_sec = duration.as_secs();
         let tv_nsec = duration.subsec_nanos();
 
-        this.write_int_fields(&[tv_sec.into(), tv_nsec.into()], &this.deref_operand(tp_op)?)?;
+        this.write_int_fields(&[tv_sec.into(), tv_nsec.into()], &tp)?;
 
         Ok(Scalar::from_i32(0))
     }
@@ -91,6 +92,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         this.assert_target_os_is_unix("gettimeofday");
         this.check_no_isolation("`gettimeofday`")?;
 
+        let tv = this.deref_operand_as(tv_op, this.libc_ty_layout("timeval"))?;
+
         // Using tz is obsolete and should always be null
         let tz = this.read_pointer(tz_op)?;
         if !this.ptr_is_null(tz)? {
@@ -103,7 +106,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let tv_sec = duration.as_secs();
         let tv_usec = duration.subsec_micros();
 
-        this.write_int_fields(&[tv_sec.into(), tv_usec.into()], &this.deref_operand(tv_op)?)?;
+        this.write_int_fields(&[tv_sec.into(), tv_usec.into()], &tv)?;
 
         Ok(0)
     }
@@ -118,6 +121,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         this.assert_target_os("windows", "GetSystemTimeAsFileTime");
         this.check_no_isolation("`GetSystemTimeAsFileTime`")?;
 
+        let filetime = this.deref_operand_as(LPFILETIME_op, this.windows_ty_layout("FILETIME"))?;
+
         let NANOS_PER_SEC = this.eval_windows_u64("time", "NANOS_PER_SEC");
         let INTERVALS_PER_SEC = this.eval_windows_u64("time", "INTERVALS_PER_SEC");
         let INTERVALS_TO_UNIX_EPOCH = this.eval_windows_u64("time", "INTERVALS_TO_UNIX_EPOCH");
@@ -131,10 +136,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
         let dwLowDateTime = u32::try_from(duration_ticks & 0x00000000FFFFFFFF).unwrap();
         let dwHighDateTime = u32::try_from((duration_ticks & 0xFFFFFFFF00000000) >> 32).unwrap();
-        this.write_int_fields(
-            &[dwLowDateTime.into(), dwHighDateTime.into()],
-            &this.deref_operand(LPFILETIME_op)?,
-        )?;
+        this.write_int_fields(&[dwLowDateTime.into(), dwHighDateTime.into()], &filetime)?;
 
         Ok(())
     }
@@ -177,7 +179,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         // and thus 10^9 counts per second.
         this.write_scalar(
             Scalar::from_i64(1_000_000_000),
-            &this.deref_operand(lpFrequency_op)?.into(),
+            &this.deref_operand_as(lpFrequency_op, this.machine.layouts.u64)?.into(),
         )?;
         Ok(Scalar::from_i32(-1)) // Return non-zero on success
     }
@@ -204,7 +206,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
         this.assert_target_os("macos", "mach_timebase_info");
 
-        let info = this.deref_operand(info_op)?;
+        let info = this.deref_operand_as(info_op, this.libc_ty_layout("mach_timebase_info"))?;
 
         // Since our emulated ticks in `mach_absolute_time` *are* nanoseconds,
         // no scaling needs to happen.
@@ -223,7 +225,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
 
         this.assert_target_os_is_unix("nanosleep");
 
-        let duration = match this.read_timespec(&this.deref_operand(req_op)?)? {
+        let req = this.deref_operand_as(req_op, this.libc_ty_layout("timespec"))?;
+
+        let duration = match this.read_timespec(&req)? {
             Some(duration) => duration,
             None => {
                 let einval = this.eval_libc("EINVAL");

Diff for: src/shims/unix/foreign_items.rs

@@ -259,7 +259,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             // Thread-local storage
             "pthread_key_create" => {
                 let [key, dtor] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
-                let key_place = this.deref_operand(key)?;
+                let key_place = this.deref_operand_as(key, this.libc_ty_layout("pthread_key_t"))?;
                 let dtor = this.read_pointer(dtor)?;
 
                 // Extract the function type out of the signature (that seems easier than constructing it ourselves).
@@ -520,7 +520,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 // Hence we can mostly ignore the input `attr_place`.
                 let [attr_place, addr_place, size_place] =
                     this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
-                let _attr_place = this.deref_operand(attr_place)?;
+                let _attr_place = this.deref_operand_as(attr_place, this.libc_ty_layout("pthread_attr_t"))?;
                 let addr_place = this.deref_operand(addr_place)?;
                 let size_place = this.deref_operand(size_place)?;
 
@@ -563,7 +563,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 this.check_no_isolation("`getpwuid_r`")?;
 
                 let uid = this.read_scalar(uid)?.to_u32()?;
-                let pwd = this.deref_operand(pwd)?;
+                let pwd = this.deref_operand_as(pwd, this.libc_ty_layout("passwd"))?;
                 let buf = this.read_pointer(buf)?;
                 let buflen = this.read_target_usize(buflen)?;
                 let result = this.deref_operand(result)?;

Diff for: src/shims/unix/fs.rs

@@ -344,7 +344,8 @@ trait EvalContextExtPrivate<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx
         let (created_sec, created_nsec) = metadata.created.unwrap_or((0, 0));
         let (modified_sec, modified_nsec) = metadata.modified.unwrap_or((0, 0));
 
-        let buf = this.deref_operand(buf_op)?;
+        let buf = this.deref_operand_as(buf_op, this.libc_ty_layout("stat"))?;
+
         this.write_int_fields_named(
             &[
                 ("st_dev", 0),
@@ -1013,15 +1014,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
             return Ok(-1);
         }
 
-        // Under normal circumstances, we would use `deref_operand(statxbuf_op)` to produce a
-        // proper `MemPlace` and then write the results of this function to it. However, the
-        // `syscall` function is untyped. This means that all the `statx` parameters are provided
-        // as `isize`s instead of having the proper types. Thus, we have to recover the layout of
-        // `statxbuf_op` by using the `libc::statx` struct type.
-        let statxbuf = {
-            let statx_layout = this.libc_ty_layout("statx");
-            MPlaceTy::from_aligned_ptr(statxbuf_ptr, statx_layout)
-        };
+        let statxbuf = this.deref_operand_as(statxbuf_op, this.libc_ty_layout("statx"))?;
 
         let path = this.read_path_from_c_str(pathname_ptr)?.into_owned();
         // See <https://github.com/rust-lang/rust/pull/79196> for a discussion of argument sizes.
@@ -1427,7 +1420,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                 //     pub d_name: [c_char; 1024],
                 // }
 
-                let entry_place = this.deref_operand(entry_op)?;
+                let entry_place = this.deref_operand_as(entry_op, this.libc_ty_layout("dirent"))?;
                 let name_place = this.mplace_field(&entry_place, 5)?;
 
                 let file_name = dir_entry.file_name(); // not a Path as there are no separators!
@@ -1443,8 +1436,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     );
                 }
 
-                let entry_place = this.deref_operand(entry_op)?;
-
                 // If the host is a Unix system, fill in the inode number with its real value.
                 // If not, use 0 as a fallback value.
                 #[cfg(unix)]

Diff for: src/shims/unix/linux/fd.rs

@@ -71,7 +71,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
         let epoll_ctl_del = this.eval_libc_i32("EPOLL_CTL_DEL");
 
         if op == epoll_ctl_add || op == epoll_ctl_mod {
-            let event = this.deref_operand(event)?;
+            let event = this.deref_operand_as(event, this.libc_ty_layout("epoll_event"))?;
 
             let events = this.mplace_field(&event, 0)?;
             let events = this.read_scalar(&events.into())?.to_u32()?;

Diff for: src/shims/unix/linux/foreign_items.rs

@@ -191,7 +191,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
                     this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
                 this.read_scalar(pid)?.to_i32()?;
                 this.read_target_usize(cpusetsize)?;
-                this.deref_operand(mask)?;
+                this.deref_operand_as(mask, this.libc_ty_layout("cpu_set_t"))?;
                 // FIXME: we just return an error; `num_cpus` then falls back to `sysconf`.
                 let einval = this.eval_libc("EINVAL");
                 this.set_last_error(einval)?;

Diff for: src/shims/unix/linux/sync.rs

@@ -85,8 +85,10 @@ pub fn futex<'tcx>(
                 return Ok(());
             }
 
-            // `deref_operand` but not actually dereferencing the ptr yet (it might be NULL!).
-            let timeout = this.ref_to_mplace(&this.read_immediate(&args[3])?)?;
+            let timeout = this.deref_pointer_as(
+                &this.read_immediate(&args[3])?,
+                this.libc_ty_layout("timespec"),
+            )?;
             let timeout_time = if this.ptr_is_null(timeout.ptr)? {
                 None
             } else {