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

Properly detect overflow in Instance ± Duration. #44220

Conversation

kennytm
Copy link
Member

@kennytm kennytm commented Aug 31, 2017

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.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

@bors: r+

Looks great, thanks!

@bors
Copy link
Contributor

bors commented Aug 31, 2017

📌 Commit abc11c3 has been approved by alexcrichton

@shepmaster shepmaster added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 1, 2017
@bors
Copy link
Contributor

bors commented Sep 3, 2017

⌛ Testing commit abc11c3 with merge f2b7ddd...

bors added a commit that referenced this pull request 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`.
@bors
Copy link
Contributor

bors commented Sep 3, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member Author

kennytm commented Sep 3, 2017

armhf-gnu failed, legit.

[01:13:00] failures:
[01:13:00] 
[01:13:00] ---- time::tests::system_time_math stdout ----
[01:13:00] 	thread 'time::tests::system_time_math' panicked at 'overflow when subtracting duration from time', /checkout/src/libcore/option.rs:839:4
[01:13:00] 
[01:13:00] 
[01:13:00] failures:
[01:13:00]     time::tests::system_time_math
[01:13:00] 
[01:13:00] test result: FAILED. 801 passed; 1 failed; 1 ignored; 0 measured; 0 filtered out
[01:13:00] 
[01:13:00] �[m�[m�[31m�[1merror:�[m test failed, to rerun pass '--lib'

@kennytm
Copy link
Member Author

kennytm commented Sep 3, 2017

@alexcrichton Failure should be fixed in ef8c204:

diff --git a/src/libstd/time/mod.rs b/src/libstd/time/mod.rs
index 5b893505b3..bd96f2133e 100644
--- a/src/libstd/time/mod.rs
+++ b/src/libstd/time/mod.rs
@@ -498,7 +498,7 @@ mod tests {
                 let dur = dur.duration();
                 assert!(a > b);
                 assert_almost_eq!(b + dur, a);
-                assert_almost_eq!(b - dur, a);
+                assert_almost_eq!(a - dur, b);
             }

If I understand correctly, the original test is wrong.

@alexcrichton
Copy link
Member

Hm no I don't believe that's the purpose of the test (that's a different equality). I know though that the arm setup has really weird clocks and it's not quite right in qemu, so it's fine to ignore the test there.

@kennytm
Copy link
Member Author

kennytm commented Sep 3, 2017

@alexcrichton if the original test is correct that means we assert a ≈ b + dur && a ≈ b - dur? That's only possible if dur ≈ 0, which won't be true especially when the SystemClock::now is "weird".

Do you mean we should revert ef8c204 and just #[cfg] out the test outside of x86?

@alexcrichton
Copy link
Member

Ah sorry no I think I misread and I believe you're right in that this is the intended test! Let's see if this works...

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 5, 2017

📌 Commit ef8c204 has been approved by alexcrichton

@Mark-Simulacrum
Copy link
Member

@bors rollup

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request 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`.
@Mark-Simulacrum
Copy link
Member

I suspect that this is the cause of the following failure in the rollup (on armhf-gnu):

@bors r-

[01:11:59] 	thread 'time::tests::system_time_math' panicked at 'overflow when subtracting duration from time', /checkout/src/libcore/option.rs:839:4
[01:11:59] 
[01:11:59] 
[01:11:59] failures:
[01:11:59]     time::tests::system_time_math
[01:11:59] 
[01:11:59] test result: FAILED. 807 passed; 1 failed; 1 ignored; 0 measured; 0 filtered out

@aidanhs aidanhs added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 7, 2017
@kennytm kennytm changed the title Properly detect overflow in Instance ± Duration. [WIP] Properly detect overflow in Instance ± Duration. Sep 7, 2017
@kennytm kennytm force-pushed the fix-44216-instance-plus-max-duration-should-panic branch from ef8c204 to 1ba090c Compare September 7, 2017 05:09
@kennytm
Copy link
Member Author

kennytm commented Sep 7, 2017

Looks like the actual problem is that time_t on armhf-gnu is 32-bit, so 80 years is too long.

@kennytm kennytm force-pushed the fix-44216-instance-plus-max-duration-should-panic branch 2 times, most recently from ab93811 to f4e6bcc Compare September 7, 2017 07:06
@kennytm kennytm force-pushed the fix-44216-instance-plus-max-duration-should-panic branch from f4e6bcc to 83d14bd Compare September 7, 2017 09:14
@kennytm kennytm changed the title [WIP] Properly detect overflow in Instance ± Duration. Properly detect overflow in Instance ± Duration. Sep 7, 2017
@kennytm
Copy link
Member Author

kennytm commented Sep 7, 2017

@alexcrichton Actual failure cause is found. As stated before, time_t on armhf-gnu is 32-bit, thus won't support a difference more than ~68 years (231 seconds). The two tests below will always fail:

        let eighty_years = second * 60 * 60 * 24 * 365 * 80;
        assert_almost_eq!(a - eighty_years + eighty_years, a);
        assert_almost_eq!(a - (eighty_years * 10) + (eighty_years * 10), a);

Previously the test passed because the computation is done at i64 and unchecked-cast to time_t as the final step. The a - (800 years) test should overflow.

Currently I disabled these tests when time_t is 32-bit. But would it make more sense we store the system time as i64 unconditionally instead of time_t?


@bors rollup-

@alexcrichton
Copy link
Member

@bors: r+

Thanks for the investigation!

We may want to investigate a platform-independent representation of Duration and Instant in the future, yeah, but that's fine to happen in a separate PR.

@bors
Copy link
Contributor

bors commented Sep 7, 2017

📌 Commit 83d14bd has been approved by alexcrichton

@kennytm
Copy link
Member Author

kennytm commented Sep 7, 2017

@alexcrichton Filed issue #44394 for platform-indepndent representation of SystemTime. (Duration is already platform-independent. Instant have some tricky interaction on Windows which may be difficult to port.)

@alexcrichton
Copy link
Member

Thanks!

@bors
Copy link
Contributor

bors commented Sep 9, 2017

⌛ Testing commit 83d14bd with merge 9999dc617244b1af1ee4bb5680406d61e82f161c...

@bors
Copy link
Contributor

bors commented Sep 9, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member Author

kennytm commented Sep 9, 2017

Legit. On macOS an Instant is represented by 64-bit nanoseconds, so u64::MAX seconds will overflow and converting to an Instant. The panic message is "overflow converting duration to nanoseconds", which is different from Windows' "overflow when converting duration to intervals". Fixed by just checking for the word "overflow" in the error pattern.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 9, 2017

📌 Commit c5e9ef6 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Sep 10, 2017

⌛ Testing commit c5e9ef6593de104004512102bf0dfb4372c04ae4 with merge 49097d101b6187947e489134dca7dc2c8f93c1f9...

@bors
Copy link
Contributor

bors commented Sep 10, 2017

💔 Test failed - status-travis

@kennytm kennytm force-pushed the fix-44216-instance-plus-max-duration-should-panic branch from c5e9ef6 to 4962f9d Compare September 10, 2017 04:36
@kennytm
Copy link
Member Author

kennytm commented Sep 10, 2017

@alexcrichton Fixed build failure. Sorry the import statement was wrong 😓.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Sep 10, 2017

📌 Commit 4962f9d has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Sep 10, 2017

⌛ Testing commit 4962f9d with merge ca94c75...

bors added a commit that referenced this pull request 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`.
@bors
Copy link
Contributor

bors commented Sep 10, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing ca94c75 to master...

@bors bors merged commit 4962f9d into rust-lang:master Sep 10, 2017
@kennytm kennytm deleted the fix-44216-instance-plus-max-duration-should-panic branch September 11, 2017 06:19
@alexcrichton alexcrichton added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants