From 33e928c9ca456f36ac662657333d6ca046be17bd Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 7 Sep 2020 10:54:39 -0500 Subject: [PATCH] Review comments --- src/helpers.rs | 23 +++++++++---------- src/shims/posix/sync.rs | 5 ++-- src/shims/time.rs | 8 +++---- .../run-pass/concurrency/libc_pthread_cond.rs | 12 ++++++++++ tests/run-pass/time.rs | 1 + 5 files changed, 30 insertions(+), 19 deletions(-) diff --git a/src/helpers.rs b/src/helpers.rs index f8bf9598c14..f5094b169f9 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -42,9 +42,6 @@ fn try_resolve_did<'mir, 'tcx>(tcx: TyCtxt<'tcx>, path: &[&str]) -> Option: crate::MiriEvalContextExt<'mir, 'tcx> { /// Gets an instance for a path. fn resolve_path(&self, path: &[&str]) -> ty::Instance<'tcx> { @@ -517,14 +514,13 @@ fn write_scalar_at_offset( 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. + /// Parse a `timespec` struct and return it as a `std::time::Duration`. It returns `None` + /// if the value in the `timespec` struct is invalid. Some libc functions will return + /// `EINVAL` in this case. fn read_timespec( &mut self, timespec_ptr_op: OpTy<'tcx, Tag>, - ) -> InterpResult<'tcx, Result> { + ) -> InterpResult<'tcx, Option> { let this = self.eval_context_mut(); let tp = this.deref_operand(timespec_ptr_op)?; let seconds_place = this.mplace_field(tp, 0)?; @@ -537,17 +533,20 @@ fn read_timespec( let seconds: u64 = if let Ok(s) = seconds.try_into() { s } else { - return Ok(Err(TimespecError)); + // tv_sec must be non-negative. + return Ok(None); }; let nanoseconds: u32 = if let Ok(ns) = nanoseconds.try_into() { if ns >= 1_000_000_000 { - return Ok(Err(TimespecError)); + // tv_nsec must not be greater than 999,999,999. + return Ok(None); } ns } else { - return Ok(Err(TimespecError)); + // tv_nsec must be non-negative. + return Ok(None); }; - Ok(Ok(Duration::new(seconds, nanoseconds))) + Ok(Some(Duration::new(seconds, nanoseconds))) } } diff --git a/src/shims/posix/sync.rs b/src/shims/posix/sync.rs index 94704bff32a..6918fb7fd7e 100644 --- a/src/shims/posix/sync.rs +++ b/src/shims/posix/sync.rs @@ -1,7 +1,6 @@ use std::time::SystemTime; use crate::*; -use helpers::TimespecError; use stacked_borrows::Tag; use thread::Time; @@ -700,8 +699,8 @@ fn pthread_cond_timedwait( // Extract the timeout. let clock_id = cond_get_clock_id(this, cond_op)?.to_i32()?; let duration = match this.read_timespec(abstime_op)? { - Ok(duration) => duration, - Err(TimespecError) => { + Some(duration) => duration, + None => { let einval = this.eval_libc("EINVAL")?; this.write_scalar(einval, dest)?; return Ok(()); diff --git a/src/shims/time.rs b/src/shims/time.rs index 32c1ce888ed..9d6d6ed38da 100644 --- a/src/shims/time.rs +++ b/src/shims/time.rs @@ -3,7 +3,7 @@ use crate::stacked_borrows::Tag; use crate::*; -use helpers::{immty_from_int_checked, immty_from_uint_checked, TimespecError}; +use helpers::{immty_from_int_checked, immty_from_uint_checked}; use thread::Time; /// Returns the time elapsed between the provided time and the unix epoch as a `Duration`. @@ -191,14 +191,14 @@ fn nanosleep( this.check_no_isolation("nanosleep")?; let duration = match this.read_timespec(req_op)? { - Ok(duration) => duration, - Err(TimespecError) => { + Some(duration) => duration, + None => { let einval = this.eval_libc("EINVAL")?; this.set_last_error(einval)?; return Ok(-1); } }; - let timeout_time = Time::RealTime(SystemTime::now().checked_add(duration).unwrap()); + let timeout_time = Time::Monotonic(Instant::now().checked_add(duration).unwrap()); let active_thread = this.get_active_thread(); this.block_thread(active_thread); diff --git a/tests/run-pass/concurrency/libc_pthread_cond.rs b/tests/run-pass/concurrency/libc_pthread_cond.rs index e1ca63f9ca4..d4e52bb3a97 100644 --- a/tests/run-pass/concurrency/libc_pthread_cond.rs +++ b/tests/run-pass/concurrency/libc_pthread_cond.rs @@ -38,6 +38,8 @@ fn test_timed_wait_timeout(clock_id: i32) { let elapsed_time = current_time.elapsed().as_millis(); assert!(900 <= elapsed_time && elapsed_time <= 1300); + // Test that invalid nanosecond values (above 10^9 or negative) are rejected with the + // correct error code. let invalid_timeout_1 = libc::timespec { tv_sec: now.tv_sec + 1, tv_nsec: 1_000_000_000 }; assert_eq!( libc::pthread_cond_timedwait( @@ -56,6 +58,16 @@ fn test_timed_wait_timeout(clock_id: i32) { ), libc::EINVAL ); + // Test that invalid second values (negative) are rejected with the correct error code. + let invalid_timeout_3 = libc::timespec { tv_sec: -1, tv_nsec: 0 }; + assert_eq!( + libc::pthread_cond_timedwait( + &mut cond as *mut _, + &mut mutex as *mut _, + &invalid_timeout_3 + ), + libc::EINVAL + ); assert_eq!(libc::pthread_mutex_unlock(&mut mutex as *mut _), 0); assert_eq!(libc::pthread_mutex_destroy(&mut mutex as *mut _), 0); diff --git a/tests/run-pass/time.rs b/tests/run-pass/time.rs index ae287234d17..e76c8573c51 100644 --- a/tests/run-pass/time.rs +++ b/tests/run-pass/time.rs @@ -8,6 +8,7 @@ fn duration_sanity(diff: Duration) { assert!(diff.as_millis() < 500); } +// Thus far, only `libc::nanosleep`, is implemented, not `c::Sleep`. #[cfg(unix)] fn test_sleep() { let before = Instant::now(); -- 2.44.0