From 6d40b7232eaa00ab5c060582011f350725703a1e Mon Sep 17 00:00:00 2001 From: Sebastian Geisler Date: Tue, 30 Oct 2018 22:24:33 -0700 Subject: [PATCH 1/3] Implement checked_add_duration for SystemTime Since SystemTime is opaque there is no way to check if the result of an addition will be in bounds. That makes the Add trait completely unusable with untrusted data. This is a big problem because adding a Duration to UNIX_EPOCH is the standard way of constructing a SystemTime from a unix timestamp. This commit implements checked_add_duration(&self, &Duration) -> Option for std::time::SystemTime and as a prerequisite also for all platform specific time structs. This also led to the refactoring of many add_duration(&self, &Duration) -> SystemTime functions to avoid redundancy (they now unwrap the result of checked_add_duration). Some basic unit tests for the newly introduced function were added too. --- src/libstd/sys/cloudabi/time.rs | 19 +++++++++++++------ src/libstd/sys/redox/time.rs | 25 +++++++++++++++++++------ src/libstd/sys/unix/time.rs | 29 +++++++++++++++++++++++------ src/libstd/sys/wasm/time.rs | 4 ++++ src/libstd/sys/windows/time.rs | 16 ++++++++++++---- src/libstd/time.rs | 21 +++++++++++++++++++++ 6 files changed, 92 insertions(+), 22 deletions(-) diff --git a/src/libstd/sys/cloudabi/time.rs b/src/libstd/sys/cloudabi/time.rs index ee12731619aac..191324e26a40f 100644 --- a/src/libstd/sys/cloudabi/time.rs +++ b/src/libstd/sys/cloudabi/time.rs @@ -19,10 +19,14 @@ pub struct Instant { t: abi::timestamp, } -pub fn dur2intervals(dur: &Duration) -> abi::timestamp { +fn checked_dur2intervals(dur: &Duration) -> Option { dur.as_secs() .checked_mul(NSEC_PER_SEC) .and_then(|nanos| nanos.checked_add(dur.subsec_nanos() as abi::timestamp)) +} + +pub fn dur2intervals(dur: &Duration) -> abi::timestamp { + checked_dur2intervals(dur) .expect("overflow converting duration to nanoseconds") } @@ -92,11 +96,14 @@ impl SystemTime { } pub fn add_duration(&self, other: &Duration) -> SystemTime { - SystemTime { - t: self.t - .checked_add(dur2intervals(other)) - .expect("overflow when adding duration to instant"), - } + self.checked_add_duration(other) + .expect("overflow when adding duration to instant") + } + + pub fn checked_add_duration(&self, other: &Duration) -> Option { + checked_dur2intervals(other) + .and_then(|d| self.t.checked_add(d)) + .map(|t| SystemTime {t}) } pub fn sub_duration(&self, other: &Duration) -> SystemTime { diff --git a/src/libstd/sys/redox/time.rs b/src/libstd/sys/redox/time.rs index 5c491115c5516..5f8799489aa6a 100644 --- a/src/libstd/sys/redox/time.rs +++ b/src/libstd/sys/redox/time.rs @@ -42,27 +42,36 @@ impl Timespec { } fn add_duration(&self, other: &Duration) -> Timespec { - let mut secs = other + self.checked_add_duration(other).expect("overflow when adding duration to time") + } + + fn checked_add_duration(&self, other: &Duration) -> Option { + let mut secs = match other .as_secs() .try_into() // <- target type would be `i64` .ok() .and_then(|secs| self.t.tv_sec.checked_add(secs)) - .expect("overflow when adding duration to time"); + { + Some(ts) => ts, + None => return None, + }; // Nano calculations can't overflow because nanos are <1B which fit // in a u32. let mut nsec = other.subsec_nanos() + self.t.tv_nsec as u32; if nsec >= NSEC_PER_SEC as u32 { nsec -= NSEC_PER_SEC as u32; - secs = secs.checked_add(1).expect("overflow when adding \ - duration to time"); + secs = match secs.checked_add(1) { + Some(ts) => ts, + None => return None, + } } - Timespec { + Some(Timespec { t: syscall::TimeSpec { tv_sec: secs, tv_nsec: nsec as i32, }, - } + }) } fn sub_duration(&self, other: &Duration) -> Timespec { @@ -180,6 +189,10 @@ impl SystemTime { SystemTime { t: self.t.add_duration(other) } } + pub fn checked_add_duration(&self, other: &Duration) -> Option { + self.t.checked_add_duration(other).map(|t| SystemTime { t }) + } + pub fn sub_duration(&self, other: &Duration) -> SystemTime { SystemTime { t: self.t.sub_duration(other) } } diff --git a/src/libstd/sys/unix/time.rs b/src/libstd/sys/unix/time.rs index 0b1fb726357e1..50c3c00382eb5 100644 --- a/src/libstd/sys/unix/time.rs +++ b/src/libstd/sys/unix/time.rs @@ -43,27 +43,36 @@ impl Timespec { } fn add_duration(&self, other: &Duration) -> Timespec { - let mut secs = other + self.checked_add_duration(other).expect("overflow when adding duration to time") + } + + fn checked_add_duration(&self, other: &Duration) -> Option { + let mut secs = match other .as_secs() .try_into() // <- target type would be `libc::time_t` .ok() .and_then(|secs| self.t.tv_sec.checked_add(secs)) - .expect("overflow when adding duration to time"); + { + Some(ts) => ts, + None => return None, + }; // Nano calculations can't overflow because nanos are <1B which fit // in a u32. let mut nsec = other.subsec_nanos() + self.t.tv_nsec as u32; if nsec >= NSEC_PER_SEC as u32 { nsec -= NSEC_PER_SEC as u32; - secs = secs.checked_add(1).expect("overflow when adding \ - duration to time"); + secs = match secs.checked_add(1) { + Some(ts) => ts, + None => return None, + } } - Timespec { + Some(Timespec { t: libc::timespec { tv_sec: secs, tv_nsec: nsec as _, }, - } + }) } fn sub_duration(&self, other: &Duration) -> Timespec { @@ -201,6 +210,10 @@ mod inner { SystemTime { t: self.t.add_duration(other) } } + pub fn checked_add_duration(&self, other: &Duration) -> Option { + self.t.checked_add_duration(other).map(|t| SystemTime { t }) + } + pub fn sub_duration(&self, other: &Duration) -> SystemTime { SystemTime { t: self.t.sub_duration(other) } } @@ -325,6 +338,10 @@ mod inner { SystemTime { t: self.t.add_duration(other) } } + pub fn checked_add_duration(&self, other: &Duration) -> Option { + self.t.checked_add_duration(other).map(|t| SystemTime { t }) + } + pub fn sub_duration(&self, other: &Duration) -> SystemTime { SystemTime { t: self.t.sub_duration(other) } } diff --git a/src/libstd/sys/wasm/time.rs b/src/libstd/sys/wasm/time.rs index e52435e63398f..991e8176edf6d 100644 --- a/src/libstd/sys/wasm/time.rs +++ b/src/libstd/sys/wasm/time.rs @@ -51,6 +51,10 @@ impl SystemTime { SystemTime(self.0 + *other) } + pub fn checked_add_duration(&self, other: &Duration) -> Option { + self.0.checked_add(*other).map(|d| SystemTime(d)) + } + pub fn sub_duration(&self, other: &Duration) -> SystemTime { SystemTime(self.0 - *other) } diff --git a/src/libstd/sys/windows/time.rs b/src/libstd/sys/windows/time.rs index 07e64d386a1c2..8eebe4a85fe16 100644 --- a/src/libstd/sys/windows/time.rs +++ b/src/libstd/sys/windows/time.rs @@ -128,9 +128,13 @@ impl SystemTime { } pub fn add_duration(&self, other: &Duration) -> SystemTime { - let intervals = self.intervals().checked_add(dur2intervals(other)) - .expect("overflow when adding duration to time"); - SystemTime::from_intervals(intervals) + self.checked_add_duration(other).expect("overflow when adding duration to time") + } + + pub fn checked_add_duration(&self, other: &Duration) -> Option { + checked_dur2intervals(other) + .and_then(|d| self.intervals().checked_add(d)) + .map(|i| SystemTime::from_intervals(i)) } pub fn sub_duration(&self, other: &Duration) -> SystemTime { @@ -180,11 +184,15 @@ impl Hash for SystemTime { } } -fn dur2intervals(d: &Duration) -> i64 { +fn checked_dur2intervals(d: &Duration) -> Option { d.as_secs() .checked_mul(INTERVALS_PER_SEC) .and_then(|i| i.checked_add(d.subsec_nanos() as u64 / 100)) .and_then(|i| i.try_into().ok()) +} + +fn dur2intervals(d: &Duration) -> i64 { + checked_dur2intervals(d) .expect("overflow when converting duration to intervals") } diff --git a/src/libstd/time.rs b/src/libstd/time.rs index 90ab349159915..94875d8993d33 100644 --- a/src/libstd/time.rs +++ b/src/libstd/time.rs @@ -357,6 +357,14 @@ impl SystemTime { pub fn elapsed(&self) -> Result { SystemTime::now().duration_since(*self) } + + /// Returns `Some(t)` where `t` is the time `self + duration` if `t` can be represented as + /// `SystemTime` (which means it's inside the bounds of the underlying data structure), `None` + /// otherwise. + #[stable(feature = "time_checked_add", since = "1.32.0")] + pub fn checked_add_duration(&self, duration: &Duration) -> Option { + self.0.checked_add_duration(duration).map(|t| SystemTime(t)) + } } #[stable(feature = "time2", since = "1.8.0")] @@ -557,6 +565,19 @@ mod tests { let one_second_from_epoch2 = UNIX_EPOCH + Duration::new(0, 500_000_000) + Duration::new(0, 500_000_000); assert_eq!(one_second_from_epoch, one_second_from_epoch2); + + // checked_add_duration will not panic on overflow + let mut maybe_t = Some(SystemTime::UNIX_EPOCH); + let max_duration = Duration::from_secs(u64::max_value()); + // in case `SystemTime` can store `>= UNIX_EPOCH + max_duration`. + for _ in 0..2 { + maybe_t = maybe_t.and_then(|t| t.checked_add_duration(&max_duration)); + } + assert_eq!(maybe_t, None); + + // checked_add_duration calculates the right time and will work for another year + let year = Duration::from_secs(60 * 60 * 24 * 365); + assert_eq!(a + year, a.checked_add_duration(&year).unwrap()); } #[test] From 86ef38b3b7a24959ca11a29c79cf921ed5986bc9 Mon Sep 17 00:00:00 2001 From: Sebastian Geisler Date: Sun, 4 Nov 2018 21:23:20 -0800 Subject: [PATCH 2/3] Rename checked_add_duration to checked_add and make it take the duration by value --- src/libstd/time.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libstd/time.rs b/src/libstd/time.rs index 94875d8993d33..c905af442de1a 100644 --- a/src/libstd/time.rs +++ b/src/libstd/time.rs @@ -361,9 +361,9 @@ impl SystemTime { /// Returns `Some(t)` where `t` is the time `self + duration` if `t` can be represented as /// `SystemTime` (which means it's inside the bounds of the underlying data structure), `None` /// otherwise. - #[stable(feature = "time_checked_add", since = "1.32.0")] - pub fn checked_add_duration(&self, duration: &Duration) -> Option { - self.0.checked_add_duration(duration).map(|t| SystemTime(t)) + #[unstable(feature = "time_checked_add", issue = "55940")] + pub fn checked_add(&self, duration: Duration) -> Option { + self.0.checked_add_duration(&duration).map(|t| SystemTime(t)) } } @@ -571,13 +571,13 @@ mod tests { let max_duration = Duration::from_secs(u64::max_value()); // in case `SystemTime` can store `>= UNIX_EPOCH + max_duration`. for _ in 0..2 { - maybe_t = maybe_t.and_then(|t| t.checked_add_duration(&max_duration)); + maybe_t = maybe_t.and_then(|t| t.checked_add(max_duration)); } assert_eq!(maybe_t, None); // checked_add_duration calculates the right time and will work for another year let year = Duration::from_secs(60 * 60 * 24 * 365); - assert_eq!(a + year, a.checked_add_duration(&year).unwrap()); + assert_eq!(a + year, a.checked_add(year).unwrap()); } #[test] From f2106d0746cdbd04ddad44c35b4e13eeced2a546 Mon Sep 17 00:00:00 2001 From: Sebastian Geisler Date: Thu, 15 Nov 2018 22:56:07 -0800 Subject: [PATCH 3/3] use ? operator instead of match --- src/libstd/sys/redox/time.rs | 13 +++---------- src/libstd/sys/unix/time.rs | 13 +++---------- 2 files changed, 6 insertions(+), 20 deletions(-) diff --git a/src/libstd/sys/redox/time.rs b/src/libstd/sys/redox/time.rs index 5f8799489aa6a..514629282ac2b 100644 --- a/src/libstd/sys/redox/time.rs +++ b/src/libstd/sys/redox/time.rs @@ -46,25 +46,18 @@ impl Timespec { } fn checked_add_duration(&self, other: &Duration) -> Option { - let mut secs = match other + let mut secs = other .as_secs() .try_into() // <- target type would be `i64` .ok() - .and_then(|secs| self.t.tv_sec.checked_add(secs)) - { - Some(ts) => ts, - None => return None, - }; + .and_then(|secs| self.t.tv_sec.checked_add(secs))?; // Nano calculations can't overflow because nanos are <1B which fit // in a u32. let mut nsec = other.subsec_nanos() + self.t.tv_nsec as u32; if nsec >= NSEC_PER_SEC as u32 { nsec -= NSEC_PER_SEC as u32; - secs = match secs.checked_add(1) { - Some(ts) => ts, - None => return None, - } + secs = secs.checked_add(1)?; } Some(Timespec { t: syscall::TimeSpec { diff --git a/src/libstd/sys/unix/time.rs b/src/libstd/sys/unix/time.rs index 50c3c00382eb5..6c7ee3dd922df 100644 --- a/src/libstd/sys/unix/time.rs +++ b/src/libstd/sys/unix/time.rs @@ -47,25 +47,18 @@ impl Timespec { } fn checked_add_duration(&self, other: &Duration) -> Option { - let mut secs = match other + let mut secs = other .as_secs() .try_into() // <- target type would be `libc::time_t` .ok() - .and_then(|secs| self.t.tv_sec.checked_add(secs)) - { - Some(ts) => ts, - None => return None, - }; + .and_then(|secs| self.t.tv_sec.checked_add(secs))?; // Nano calculations can't overflow because nanos are <1B which fit // in a u32. let mut nsec = other.subsec_nanos() + self.t.tv_nsec as u32; if nsec >= NSEC_PER_SEC as u32 { nsec -= NSEC_PER_SEC as u32; - secs = match secs.checked_add(1) { - Some(ts) => ts, - None => return None, - } + secs = secs.checked_add(1)?; } Some(Timespec { t: libc::timespec {