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

Implement checked_add_duration for SystemTime #55527

Merged
merged 3 commits into from
Nov 25, 2018

Conversation

sgeisler
Copy link
Contributor

@sgeisler sgeisler commented Oct 31, 2018

Original discussion on the rust user forum

Since SystemTime is opaque there is no way to check if the result of an addition will be in bounds. That makes the Add<Duration> 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 PR implements checked_add_duration(&self, &Duration) -> Option<SystemTime> 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.

I wasn't sure which stabilization attribute to add to the newly introduced function, so I just chose #[stable(feature = "time_checked_add", since = "1.32.0")] for now to make it compile. Please let me know how I should change it or if I violated any other conventions.

P.S.: I could only test on Linux so far, so I don't necessarily expect it to compile for all platforms.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 31, 2018
@sgeisler
Copy link
Contributor Author

I just noticed that I also need to make make dur2intervals checked for some platforms. I will amend this PR later.

@sgeisler sgeisler changed the title Implement checked_add_duration for SystemTime WIP: Implement checked_add_duration for SystemTime Oct 31, 2018
@sgeisler sgeisler changed the title WIP: Implement checked_add_duration for SystemTime Implement checked_add_duration for SystemTime Nov 2, 2018
@sgeisler
Copy link
Contributor Author

sgeisler commented Nov 2, 2018

I think the PR is ready for review/an automated all-platforms test run. Is there an easy way to just try to compile it on all platforms?

src/libstd/time.rs Outdated Show resolved Hide resolved
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:19881f0a:start=1541395676690045891,finish=1541395729826103929,duration=53136058038
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:49:48] .................................................................................................... 100/4989
[00:49:51] .................................................................................................... 200/4989
[00:49:53] ...........................................................................................ii....... 300/4989
[00:49:56] .........................................................................................iii........ 400/4989
[00:49:59] iiiiiiii.iii...........................iii...........................................i...........i.. 500/4989
[00:50:06] .................................................................................................... 700/4989
[00:50:13] ..................................................................i...........i..................... 800/4989
[00:50:15] .....................................................................................iiiii.......... 900/4989
[00:50:19] .................................................................................................... 1000/4989
---
[00:50:55] .................................................................................................... 2200/4989
[00:50:59] .................................................................................................... 2300/4989
[00:51:03] .................................................................................................... 2400/4989
[00:51:06] .................................................................................................... 2500/4989
[00:51:10] ......................................................................iiiiiiiii..................... 2600/4989
[00:51:17] .....................ii............................................................................. 2800/4989
[00:51:20] .................................................................................................... 2900/4989
[00:51:24] .................................................................................................... 3000/4989
[00:51:26] ................i................................................................................... 3100/4989
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:05:01] 
[01:05:01] running 115 tests
[01:05:04] i..ii...iii..iii.....i...i.........i..iii...........i.....i.....ii...i..i.ii..............i...ii..ii 100/115
[01:05:04] .i....iiii.....
[01:05:04] 
[01:05:04]  finished in 3.424
[01:05:04] travis_fold:end:test_codegen

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:05:18] 
[01:05:18] running 118 tests
[01:05:42] .iiiii...i.....i..i...i..i.i..i.i..i.....i..i....i..........iiii.........i.i....i...i.......ii.i.i.i 100/118
[01:05:46] ......iii.i.....ii
[01:05:46] 
[01:05:46]  finished in 27.824
[01:05:46] travis_fold:end:test_debuginfo

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

src/libstd/time.rs Outdated Show resolved Hide resolved
@sfackler
Copy link
Member

LGTM with the issue number changed!

@sgeisler
Copy link
Contributor Author

When will the full test suite (all platforms) be run? I expect some problems at first due to conditional compilation (which I can't test).

@sfackler
Copy link
Member

@bors r+

It'll run now! No worries if it bounces a couple of times due to the platform specific stuff - it happens all the time.

@bors
Copy link
Contributor

bors commented Nov 14, 2018

📌 Commit 8e4711d65612f61f57845738db90bccec59d9833 has been approved by sfackler

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 14, 2018
@kennytm
Copy link
Member

kennytm commented Nov 14, 2018

@bors r-

Failed in #55943 (comment) on wasm32.

@bors
Copy link
Contributor

bors commented Nov 16, 2018

⌛ Testing commit 8231831 with merge dee94cd6869ed5b55506b11479dafa5f1ad523da...

@bors
Copy link
Contributor

bors commented Nov 16, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 16, 2018
@rust-highfive
Copy link
Collaborator

The job dist-various-1 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[01:13:58]    Compiling panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
[01:13:58] [RUSTC-TIMING] panic_unwind test:false 0.281
[01:13:58] warning: dropping unsupported crate type `dylib` for target `x86_64-unknown-redox`
[01:13:58] 
[01:14:02] error[E0599]: no method named `map` found for type `sys::redox::time::Timespec` in the current scope
[01:14:02]    --> libstd/sys/redox/time.rs:193:36
[01:14:02] 21  | struct Timespec {
[01:14:02] 21  | struct Timespec {
[01:14:02]     | --------------- method `map` not found for this
[01:14:02] ...
[01:14:02] 193 |         self.t.add_duration(other).map(|t| SystemTime { t })
[01:14:02]     |
[01:14:02]     = note: the method `map` exists but the following trait bounds were not satisfied:
[01:14:02]     = note: the method `map` exists but the following trait bounds were not satisfied:
[01:14:02]             `&mut sys::redox::time::Timespec : core::iter::Iterator`
[01:14:02]     = note: the following trait defines an item `map`, perhaps you need to implement it:
[01:14:02]             candidate #1: `core::iter::Iterator`
[01:14:02] 
[01:14:02] error: aborting due to previous error
---
[01:14:02] 
[01:14:02] To learn more, run the command again with --verbose.
[01:14:02] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-redox" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[01:14:02] expected success, got: exit code: 101
[01:14:02] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1101:9
[01:14:02] travis_fold:end:stage2-std

[01:14:02] travis_time:end:stage2-std:start=1542343362191920766,finish=1542343413595164716,duration=51403243950

---
travis_time:end:02ad9620:start=1542343415600548897,finish=1542343415609620272,duration=9071375
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0af297e4
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0189cbf4
travis_time:start:0189cbf4
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0291edfb
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

Since SystemTime is opaque there is no way to check if the result
of an addition will be in bounds. That makes the Add<Duration>
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<SystemTime>
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.
@XAMPPRocky
Copy link
Member

Triage; @sfackler Hello, have you been able to get back to this PR?

@sfackler
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 25, 2018

📌 Commit f2106d0 has been approved by sfackler

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 25, 2018
@sfackler
Copy link
Member

Sorry, missed that you updated this - github doesn't notify on force pushes :(

@bors
Copy link
Contributor

bors commented Nov 25, 2018

⌛ Testing commit f2106d0 with merge 6acbb5b...

bors added a commit that referenced this pull request Nov 25, 2018
Implement checked_add_duration for SystemTime

[Original discussion on the rust user forum](https://users.rust-lang.org/t/std-systemtime-misses-a-checked-add-function/21785)

Since `SystemTime` is opaque there is no way to check if the result of an addition will be in bounds. That makes the `Add<Duration>` 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 PR implements `checked_add_duration(&self, &Duration) -> Option<SystemTime>` 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.

I wasn't sure which stabilization attribute to add to the newly introduced function, so I just chose `#[stable(feature = "time_checked_add", since = "1.32.0")]` for now to make it compile. Please let me know how I should change it or if I violated any other conventions.

P.S.: I could only test on Linux so far, so I don't necessarily expect it to compile for all platforms.
@bors
Copy link
Contributor

bors commented Nov 25, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: sfackler
Pushing 6acbb5b to master...

@bors bors merged commit f2106d0 into rust-lang:master Nov 25, 2018
@sgeisler sgeisler deleted the time-checked-add branch December 14, 2018 22:29
@jethrogb
Copy link
Contributor

This PR acknowledges that SystemTime may encompass a finite range. There are a couple of time-related unit tests in std that don't. In particular, time::tests::since_epoch and time::tests::system_time_math fail if the platform represents a SystemTime as unix epoch + u64. WASM, CloudABI, SGX do this right now. Should these tests be changed so they work on those platforms?

@sfackler
Copy link
Member

Yeah, seems best to alter the tests to avoid pre-epoch times if those aren't universally supported.

@jethrogb
Copy link
Contributor

#59147

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants