Fix overflow error in thread::sleep #34363
Conversation
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
|
Rather than a test we should just iteratate and call sleep multiple times with duration of up-to the allowed amount. |
| @@ -125,8 +126,10 @@ impl Thread { | |||
| } | |||
|
|
|||
| pub fn sleep(dur: Duration) { | |||
| let secs = dur.as_secs(); | |||
| assert!(secs <= (u64::MAX >> 1), "secs number is too big"); | |||
There was a problem hiding this comment.
The error happens because “The rqtp argument specified a nanosecond value less than zero or greater than or equal to 1000 million.” u64::MAX >> 1 is not 1000 million and that’s seconds too.
There was a problem hiding this comment.
I think it still would be more clear if you did something like:
let secs = dur.as_secs() as libc::time_t;
assert!(secs.is_positive(), "...");
Your current approach won’t catch the cases where time_t is i32 (some versions of windows, mips linux etc)
|
Also needs a regression test. You should either mock-out the nanosleep function or invoke a new thread/process and catch a signal of some sort sent to it by the parent. |
GuillaumeGomez
changed the title
| fn get_secs(secs: &mut u64) -> libc::time_t { | ||
| if *secs >= libc::time_t::max_value() as u64 { | ||
| *secs -= libc::time_t::max_value() as u64 - 1; | ||
| libc::time_t::max_value() - 1 |
There was a problem hiding this comment.
Why - 1 here and above this line?
There was a problem hiding this comment.
It fails if >= 1000000. So I put the max minus one.
|
r=me with |
| @@ -125,16 +125,32 @@ impl Thread { | |||
| } | |||
|
|
|||
| pub fn sleep(dur: Duration) { | |||
| fn get_secs(secs: &mut u64) -> libc::time_t { | |||
| if *secs > libc::time_t::max_value() as u64 { | |||
There was a problem hiding this comment.
Perhaps this could be:
let amt = cmp::min(libc::time_t::max_value() as u64, *secs);
*secs -= amt;
amtThere was a problem hiding this comment.
This seems like a right thing to do.
| assert_eq!(os::errno(), libc::EINTR); | ||
| } | ||
| secs += ts.tv_sec as u64; | ||
| nsecs = ts.tv_nsec; |
There was a problem hiding this comment.
Interestingly, documentation for nanosleep does not specify what ts contains after a successful call, therefore, in order to be on the safe side, you want to only do this in case the nanosleep call fails and set these fields to 0 otherwise.
|
@bors r=nagisa f053216 |
|
⌛ Testing commit f053216 with merge ac9ba4e... |
|
@bors: r- Ah there's still a loop that's needed, I'll comment to it. |
|
@bors: r+ oh nvmd I see, just had to think for a second... |
|
📌 Commit f053216 has been approved by |
|
💔 Test failed - auto-linux-64-x-android-t |
|
Ah, |
|
What about |
|
Or... spawn a thread which sleeps for a smaller amount of time and then exits? |
|
@alexcrichton: I take your solution! :) |
|
Updated. |
|
|
||
| #![feature(libc)] | ||
|
|
||
| extern crate libc; |
There was a problem hiding this comment.
I think this and feature and // ignore above can all be removed
|
Updated. |
|
@bors: r+ ae544c5b156a56b1a170ea669487fde0c417545a |
I put back the feature. |
|
Actually |
Fix overflow error in thread::sleep Fixes rust-lang#34330 I added a test to have a more clear error inside the function. Since `time_t` is `i64` and we expect `u64`, maybe we should changed the awaited type?
Fix overflow error in thread::sleep Fixes rust-lang#34330 I added a test to have a more clear error inside the function. Since `time_t` is `i64` and we expect `u64`, maybe we should changed the awaited type?
Fixes #34330
I added a test to have a more clear error inside the function. Since
time_tisi64and we expectu64, maybe we should changed the awaited type?