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

Receiver::recv_timeout() returns instantly when duration overflows #44216

Closed
vandenoever opened this issue Aug 31, 2017 · 6 comments
Closed

Receiver::recv_timeout() returns instantly when duration overflows #44216

vandenoever opened this issue Aug 31, 2017 · 6 comments
Labels
C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@vandenoever
Copy link

When Receiver::recv_timeout receives certain large values, it returns immediately. This is probably due to an overflowing value. Instead, when there is overflow, it should fall back to Receiver::recv.

use std::sync::mpsc::Receiver;
use std::time::Duration;

let max = Duration::from_secs(u64::max_value());
receiver.recv_timeout(max);

Instant should probably not overflow like this:

let now = Instant::now();
let end_of_time = Instant::now() + Duration::from_secs(u64::max_value());
println!("{:?}", now);
println!("{:?}", end_of_time);
println!("did we pass end of time? {}", now > end_of_time);
@kennytm
Copy link
Member

kennytm commented Aug 31, 2017

The Instance addition should panic on overflow (try u64::max_value()/2 instead of u64::max_value()). It seems there is some bug causing the panic not trigger.

@vandenoever
Copy link
Author

u64::max_value()/2 panics. u64::max_value()/4 is fine.

@kennytm
Copy link
Member

kennytm commented Aug 31, 2017

Oops.

fn add_duration(&self, other: &Duration) -> Timespec {
let secs = (self.t.tv_sec as i64).checked_add(other.as_secs() as i64);
let mut secs = secs.expect("overflow when adding duration to time");

If only we used checked_cast (try_into)...

@vandenoever
Copy link
Author

Why is it casting to i64?

@kennytm
Copy link
Member

kennytm commented Aug 31, 2017

@vandenoever An Instant can represent time before the epoch, thus the number of seconds is a signed value. Arithmetic in Rust can only be performed between values of the same type, so the duration is cast to i64.

@vandenoever
Copy link
Author

@kennytm I see. And the other cast self.t.tv_sec as i64 is not usually a cast because tv_sec is time_t which is (usually?) i64.

@shepmaster shepmaster added C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Sep 1, 2017
bors added a commit that referenced this issue Sep 3, 2017
…hould-panic, r=alexcrichton

Properly detect overflow in Instance ± Duration.

Fix #44216.

The computation `Instant::now() + Duration::from_secs(u64::max_value())` now panics. The call `receiver.recv_timeout(Duration::from_secs(u64::max_value()))`, which involves such time addition, will also panic.

The reason #44216 arises is because of an unchecked cast from `u64` to `i64`, making the duration equivalent to -1 second.

Note that the current implementation is over-conservative, since e.g. (-2⁶²) + (2⁶³) is perfectly fine for an `i64`, yet this is rejected because (2⁶³) overflows the `i64`.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Sep 6, 2017
…-duration-should-panic, r=alexcrichton

Properly detect overflow in Instance ± Duration.

Fix rust-lang#44216.

The computation `Instant::now() + Duration::from_secs(u64::max_value())` now panics. The call `receiver.recv_timeout(Duration::from_secs(u64::max_value()))`, which involves such time addition, will also panic.

The reason rust-lang#44216 arises is because of an unchecked cast from `u64` to `i64`, making the duration equivalent to -1 second.

Note that the current implementation is over-conservative, since e.g. (-2⁶²) + (2⁶³) is perfectly fine for an `i64`, yet this is rejected because (2⁶³) overflows the `i64`.
bors added a commit that referenced this issue Sep 10, 2017
…hould-panic, r=alexcrichton

Properly detect overflow in Instance ± Duration.

Fix #44216.
Fix #42622

The computation `Instant::now() + Duration::from_secs(u64::max_value())` now panics. The call `receiver.recv_timeout(Duration::from_secs(u64::max_value()))`, which involves such time addition, will also panic.

The reason #44216 arises is because of an unchecked cast from `u64` to `i64`, making the duration equivalent to -1 second.

Note that the current implementation is over-conservative, since e.g. (-2⁶²) + (2⁶³) is perfectly fine for an `i64`, yet this is rejected because (2⁶³) overflows the `i64`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants