Skip to content

Commit

Permalink
Panic if NaN is passed to Duration constructors
Browse files Browse the repository at this point in the history
  • Loading branch information
jhpratt committed Sep 25, 2022
1 parent 8cb9c44 commit 9516a7e
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/duration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,9 @@ impl Duration {
if seconds > i64::MAX as f64 || seconds < i64::MIN as f64 {
crate::expect_failed("overflow constructing `time::Duration`");
}
if seconds.is_nan() {
crate::expect_failed("passed NaN to `time::Duration::seconds_f64`");
}
Self::new_unchecked(seconds as _, ((seconds % 1.) * 1_000_000_000.) as _)

This comment has been minimized.

Copy link
@lopopolo

lopopolo Sep 25, 2022

FWIW, I believe casting NAN to an integer was UB in Rust before 1.45.0:

It looks like the 0.2 branch of time has this same unchecked as cast:

seconds: seconds as i64,

and its MSRV appears to be In the Rust 1.32 range:

rust: [1.32.0, 1.36.0, stable]

I'm not sure if you want to, but I think it might be prudent to file a RustSec advisory for time 0.2.x.

This comment has been minimized.

Copy link
@jhpratt

jhpratt Sep 25, 2022

Author Member

Feel free, but time 0.2 isn't used much nowadays. People have generally either stuck with 0.1 for whatever reason or upgraded to 0.3. As such I won't be filing an advisory. It's not clear whether it's undefined behavior or unspecified behavior, as those two were largely conflated for a significant period of time.

This comment has been minimized.

Copy link
@lopopolo

lopopolo Sep 25, 2022

Thanks for the additional context @jhpratt. I'll defer to your instincts as the maintainer and follow your lead. I also won't file an advisory.

}

Expand All @@ -337,6 +340,9 @@ impl Duration {
if seconds > i64::MAX as f32 || seconds < i64::MIN as f32 {
crate::expect_failed("overflow constructing `time::Duration`");
}
if seconds.is_nan() {
crate::expect_failed("passed NaN to `time::Duration::seconds_f32`");
}
Self::new_unchecked(seconds as _, ((seconds % 1.) * 1_000_000_000.) as _)
}

Expand Down
2 changes: 2 additions & 0 deletions tests/integration/duration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ fn seconds_f64() {

assert_panic!(Duration::seconds_f64(f64::MAX));
assert_panic!(Duration::seconds_f64(f64::MIN));
assert_panic!(Duration::seconds_f64(f64::NAN));
}

#[test]
Expand All @@ -206,6 +207,7 @@ fn seconds_f32() {

assert_panic!(Duration::seconds_f32(f32::MAX));
assert_panic!(Duration::seconds_f32(f32::MIN));
assert_panic!(Duration::seconds_f32(f32::NAN));
}

#[test]
Expand Down

0 comments on commit 9516a7e

Please sign in to comment.