Refactor timespec parsing, improve error handling

src/helpers.rs

@@ -1,6 +1,7 @@
 use std::convert::{TryFrom, TryInto};
 use std::mem;
 use std::num::NonZeroUsize;
+use std::time::Duration;
 
 use log::trace;
 
@@ -41,6 +42,9 @@ fn try_resolve_did<'mir, 'tcx>(tcx: TyCtxt<'tcx>, path: &[&str]) -> Option<DefId
         })
 }
 
+/// This error indicates that the value in a `timespec` C struct was invalid.
+pub struct TimespecError;
+
 pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
     /// Gets an instance for a path.
     fn resolve_path(&self, path: &[&str]) -> ty::Instance<'tcx> {
@@ -512,6 +516,39 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let value_place = op_place.offset(offset, MemPlaceMeta::None, layout, this)?;
         this.write_scalar(value, value_place.into())
     }
+
+    /// Parse a `timespec` struct and return it as a `std::time::Duration`. The outer `Result` is
+    /// for interpreter errors encountered while reading memory, and the inner `Result` indicates
+    /// whether the value in the `timespec` struct is invalid. Some libc functions will return
+    /// `EINVAL` if the struct's value is invalid.
+    fn read_timespec(
+        &mut self,
+        timespec_ptr_op: OpTy<'tcx, Tag>,
+    ) -> InterpResult<'tcx, Result<Duration, TimespecError>> {
+        let this = self.eval_context_mut();
+        let tp = this.deref_operand(timespec_ptr_op)?;
+        let seconds_place = this.mplace_field(tp, 0)?;
+        let seconds_scalar = this.read_scalar(seconds_place.into())?;
+        let seconds = seconds_scalar.to_machine_isize(this)?;
+        let nanoseconds_place = this.mplace_field(tp, 1)?;
+        let nanoseconds_scalar = this.read_scalar(nanoseconds_place.into())?;
+        let nanoseconds = nanoseconds_scalar.to_machine_isize(this)?;
+
+        let seconds: u64 = if let Ok(s) = seconds.try_into() {
+            s
+        } else {
+            return Ok(Err(TimespecError));
+        };
+        let nanoseconds: u32 = if let Ok(ns) = nanoseconds.try_into() {
+            if ns >= 1_000_000_000 {
+                return Ok(Err(TimespecError));
+            }
+            ns
+        } else {
+            return Ok(Err(TimespecError));
+        };
+        Ok(Ok(Duration::new(seconds, nanoseconds)))
+    }
 }
 
 /// Check that the number of args is what we expect.

src/shims/posix/sync.rs

@@ -1,11 +1,10 @@
-use std::convert::TryInto;
-use std::time::{Duration, SystemTime};
+use std::time::SystemTime;
 
 use crate::*;
+use helpers::TimespecError;
 use stacked_borrows::Tag;
 use thread::Time;
 
-
 // pthread_mutexattr_t is either 4 or 8 bytes, depending on the platform.
 
 // Our chosen memory layout for emulation (does not have to match the platform layout!):
@@ -698,25 +697,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let mutex_id = mutex_get_or_create_id(this, mutex_op)?;
         let active_thread = this.get_active_thread();
 
-        release_cond_mutex_and_block(this, active_thread, mutex_id)?;
-        this.condvar_wait(id, active_thread, mutex_id);
-
-        // We return success for now and override it in the timeout callback.
-        this.write_scalar(Scalar::from_i32(0), dest)?;
-
         // Extract the timeout.
         let clock_id = cond_get_clock_id(this, cond_op)?.to_i32()?;
-        let duration = {
-            let tp = this.deref_operand(abstime_op)?;
-            let seconds_place = this.mplace_field(tp, 0)?;
-            let seconds = this.read_scalar(seconds_place.into())?;
-            let nanoseconds_place = this.mplace_field(tp, 1)?;
-            let nanoseconds = this.read_scalar(nanoseconds_place.into())?;
-            let (seconds, nanoseconds) = (
-                seconds.to_machine_usize(this)?,
-                nanoseconds.to_machine_usize(this)?.try_into().unwrap(),
-            );
-            Duration::new(seconds, nanoseconds)
+        let duration = match this.read_timespec(abstime_op)? {
+            Ok(duration) => duration,
+            Err(TimespecError) => {
+                let einval = this.eval_libc("EINVAL")?;
+                this.write_scalar(einval, dest)?;
+                return Ok(());
+            }
         };
 
         let timeout_time = if clock_id == this.eval_libc_i32("CLOCK_REALTIME")? {
@@ -727,6 +716,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
             throw_unsup_format!("unsupported clock id: {}", clock_id);
         };
 
+        release_cond_mutex_and_block(this, active_thread, mutex_id)?;
+        this.condvar_wait(id, active_thread, mutex_id);
+
+        // We return success for now and override it in the timeout callback.
+        this.write_scalar(Scalar::from_i32(0), dest)?;
+
         // Register the timeout callback.
         this.register_timeout_callback(
             active_thread,
@@ -740,8 +735,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 ecx.condvar_remove_waiter(id, active_thread);
 
                 // Set the return value: we timed out.
-                let timeout = ecx.eval_libc_i32("ETIMEDOUT")?;
-                ecx.write_scalar(Scalar::from_i32(timeout), dest)?;
+                let etimedout = ecx.eval_libc("ETIMEDOUT")?;
+                ecx.write_scalar(etimedout, dest)?;
 
                 Ok(())
             }),

tests/run-pass/concurrency/libc_pthread_cond.rs

@@ -37,6 +37,26 @@ fn test_timed_wait_timeout(clock_id: i32) {
         );
         let elapsed_time = current_time.elapsed().as_millis();
         assert!(900 <= elapsed_time && elapsed_time <= 1300);
+
+        let invalid_timeout_1 = libc::timespec { tv_sec: now.tv_sec + 1, tv_nsec: 1_000_000_000 };
+        assert_eq!(
+            libc::pthread_cond_timedwait(
+                &mut cond as *mut _,
+                &mut mutex as *mut _,
+                &invalid_timeout_1
+            ),
+            libc::EINVAL
+        );
+        let invalid_timeout_2 = libc::timespec { tv_sec: now.tv_sec + 1, tv_nsec: -1 };
+        assert_eq!(
+            libc::pthread_cond_timedwait(
+                &mut cond as *mut _,
+                &mut mutex as *mut _,
+                &invalid_timeout_2
+            ),
+            libc::EINVAL
+        );
+
         assert_eq!(libc::pthread_mutex_unlock(&mut mutex as *mut _), 0);
         assert_eq!(libc::pthread_mutex_destroy(&mut mutex as *mut _), 0);
         assert_eq!(libc::pthread_cond_destroy(&mut cond as *mut _), 0);