Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use TryFrom for TimeSpec/Duration conversions which may underflow/overflow #2522

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 47 additions & 12 deletions src/sys/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,15 +209,41 @@ impl From<timespec> for TimeSpec {
}
}

impl From<Duration> for TimeSpec {
fn from(duration: Duration) -> Self {
Self::from_duration(duration)
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub struct TryFromDurationError;

impl TryFrom<Duration> for TimeSpec {
type Error = TryFromDurationError;

fn try_from(duration: Duration) -> Result<Self, Self::Error> {
Self::try_from_duration(duration)
}
}

impl From<TimeSpec> for Duration {
fn from(timespec: TimeSpec) -> Self {
Duration::new(timespec.0.tv_sec as u64, timespec.0.tv_nsec as u32)
#[derive(Clone, Copy, Debug, Eq, PartialEq)]
pub struct TryFromTimeSpecError;

impl TryFrom<TimeSpec> for Duration {
type Error = TryFromTimeSpecError;

fn try_from(timespec: TimeSpec) -> Result<Self, Self::Error> {
let secs = timespec
.0
.tv_sec
.try_into()
.map_err(|_| TryFromTimeSpecError)?;
let nanos = timespec
.0
.tv_nsec
.try_into()
.map_err(|_| TryFromTimeSpecError)?;

// Error out here rather than letting Duration::new panic.
if nanos > Duration::MAX.subsec_nanos() {
return Err(TryFromTimeSpecError);
}

Ok(Duration::new(secs, nanos))
}
}

Expand Down Expand Up @@ -365,13 +391,22 @@ impl TimeSpec {
self.0.tv_nsec
}

#[cfg_attr(target_env = "musl", allow(deprecated))]
// https://github.com/rust-lang/libc/issues/1848
pub const fn from_duration(duration: Duration) -> Self {
#[allow(clippy::unnecessary_fallible_conversions)]
pub fn try_from_duration(
duration: Duration,
) -> Result<Self, TryFromDurationError> {
let mut ts = zero_init_timespec();
ts.tv_sec = duration.as_secs() as time_t;
ts.tv_nsec = duration.subsec_nanos() as timespec_tv_nsec_t;
TimeSpec(ts)
ts.tv_sec = duration
.as_secs()
.try_into()
.map_err(|_| TryFromDurationError)?;
// There are targets with tv_nsec being i32. Use the fallible conversion for all targets as
// we are returning a Result due to the previous conversion anyway.
ts.tv_nsec = duration
.subsec_nanos()
.try_into()
.map_err(|_| TryFromDurationError)?;
Ok(TimeSpec(ts))
}

pub const fn from_timespec(timespec: timespec) -> Self {
Expand Down
2 changes: 1 addition & 1 deletion src/sys/timer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
//! });
//!
//! let mut timer = Timer::new(clockid, sigevent).unwrap();
//! let expiration = Expiration::Interval(Duration::from_millis(250).into());
//! let expiration = Expiration::Interval(Duration::from_millis(250).try_into().unwrap());
//! let flags = TimerSetTimeFlags::empty();
//! timer.set(expiration, flags).expect("could not set timer");
//!
Expand Down
15 changes: 9 additions & 6 deletions test/sys/test_socket.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ pub fn test_timestamping() {
} else {
sys_time - ts
};
assert!(std::time::Duration::from(diff).as_secs() < 60);
assert!(std::time::Duration::try_from(diff).unwrap().as_secs() < 60);
}

#[cfg(target_os = "freebsd")]
Expand Down Expand Up @@ -131,7 +131,7 @@ pub fn test_timestamping_realtime() {
} else {
sys_time - ts
};
assert!(std::time::Duration::from(diff).as_secs() < 60);
assert!(std::time::Duration::try_from(diff).unwrap().as_secs() < 60);
}

#[cfg(target_os = "freebsd")]
Expand Down Expand Up @@ -189,7 +189,7 @@ pub fn test_timestamping_monotonic() {
::nix::time::clock_gettime(::nix::time::ClockId::CLOCK_MONOTONIC)
.unwrap();
let diff = sys_time - ts; // Monotonic clock sys_time must be greater
assert!(std::time::Duration::from(diff).as_secs() < 60);
assert!(std::time::Duration::try_from(diff).unwrap().as_secs() < 60);
}

#[test]
Expand Down Expand Up @@ -2985,7 +2985,7 @@ pub fn test_txtime() {
let iov1 = [std::io::IoSlice::new(&sbuf)];

let now = clock_gettime(ClockId::CLOCK_MONOTONIC).unwrap();
let delay = std::time::Duration::from_secs(1).into();
let delay = std::time::Duration::from_secs(1).try_into().unwrap();
let txtime = (now + delay).num_nanoseconds() as u64;

let cmsg = ControlMessage::TxTime(&txtime);
Expand Down Expand Up @@ -3099,7 +3099,8 @@ fn test_recvmm2() -> nix::Result<()> {

let mut data = MultiHeaders::<()>::preallocate(recv_iovs.len(), Some(cmsg));

let t = TimeSpec::from_duration(std::time::Duration::from_secs(10));
let t = TimeSpec::try_from_duration(std::time::Duration::from_secs(10))
.unwrap();

let recv = recvmmsg(
rsock.as_raw_fd(),
Expand All @@ -3125,7 +3126,9 @@ fn test_recvmm2() -> nix::Result<()> {
} else {
sys_time - ts
};
assert!(std::time::Duration::from(diff).as_secs() < 60);
assert!(
std::time::Duration::try_from(diff).unwrap().as_secs() < 60
);
#[cfg(not(any(qemu, target_arch = "aarch64")))]
{
saw_time = true;
Expand Down
6 changes: 3 additions & 3 deletions test/sys/test_time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@ pub fn test_timespec() {
}

#[test]
pub fn test_timespec_from() {
pub fn test_timespec_try_from() {
let duration = Duration::new(123, 123_456_789);
let timespec = TimeSpec::nanoseconds(123_123_456_789);

assert_eq!(TimeSpec::from(duration), timespec);
assert_eq!(Duration::from(timespec), duration);
assert_eq!(TimeSpec::try_from(duration).unwrap(), timespec);
assert_eq!(Duration::try_from(timespec).unwrap(), duration);
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion test/sys/test_timer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ fn alarm_fires() {
});
let mut timer =
Timer::new(clockid, sigevent).expect("failed to create timer");
let expiration = Expiration::Interval(TIMER_PERIOD.into());
let expiration = Expiration::Interval(TIMER_PERIOD.try_into().unwrap());
let flags = TimerSetTimeFlags::empty();
timer.set(expiration, flags).expect("could not set timer");

Expand Down