gh-uuid-rs/uuid
Watch
1
0
Fork
You've already forked uuid
 0
Code Issues 10 Pull requests 2 Releases Activity

Don't panic when the nanoseconds carry overflows the seconds #911

Open
youdie006 wants to merge 1 commit from youdie006/systemtime-nanos-carry into main
pull from: youdie006/systemtime-nanos-carry
merge into: gh-uuid-rs:main
Conversation 1 Commits 1 Files changed 1 +27 -2
youdie006 commented 2026-09-14 19:17:46 -05:00 (Migrated from github.com)

Follow-up to #908 and #909.

#909 guarded the outer add, but Duration::new(seconds, subsec_nanos) is evaluated first and panics on its own when the nanoseconds carry past u64::MAX seconds. Timestamp::from_unix_time is a pub const fn taking a raw u32, so any subsec_nanos >= 1_000_000_000 reaches it, the same entry point #908 used:

// ok, this is what #909 fixed
SystemTime::from(Timestamp::from_unix_time(u64::MAX, 999_999_999, 0, 0));
// still panics: overflow in Duration::new
SystemTime::from(Timestamp::from_unix_time(u64::MAX, 1_000_000_000, 0, 0));

The doc at src/timestamp.rs:225-228 says an undefined SystemTime is returned rather than a failure, so this builds the Duration from its parts with the same checked_* -> unwrap_or(UNIX_EPOCH) shape #909 used, one level inward.

Verification

Base 9f92712.

row src/timestamp.rs md5 from_system_time_max from_system_time_nanos_carry lib suite
base impl + the new test f23cc9e478 ok FAILED 128 passed, 1 failed
this PR 27d00ed610 ok ok 129 passed, 0 failed
over-correct: reject every carry 1da1e439e2 ok FAILED 128 passed, 1 failed
drop the outer guard from #909, keep the new one 7502b0bb61 FAILED ok 128 passed, 1 failed

The first row is the point: the test #909 added still passes on base while the new one fails. The last two rows are killed by different tests, so neither guard is redundant, and the over-correction is caught by the (10, 2_500_000_000) -> 12.5s assertion, so a clamp-everything fix would not pass either.

cargo test --all-features 0 failures; cargo +1.85.0 build --manifest-path tests/smoke-test/Cargo.toml clean; cargo +beta clippy --all-features reports the same 4 warnings before and after, none at the changed lines.

Not run, and not claimed: the miri, wasm, wasi, embedded and fuzz jobs. The pinned nightly and those toolchains are not available on my machine. The change is safe arithmetic inside #[cfg(feature = "std")], so the no-std jobs do not compile it at all.

One thing I left alone: src/timestamp.rs:228 points callers at TryFrom to catch conversion failures, but there is no fallible TryFrom<Timestamp> for SystemTime. That seemed like separate scope.

Disclosure: written with the help of Claude (an AI assistant). Every number above is from runs on my machine.

Follow-up to #908 and #909. #909 guarded the outer add, but `Duration::new(seconds, subsec_nanos)` is evaluated first and panics on its own when the nanoseconds carry past `u64::MAX` seconds. `Timestamp::from_unix_time` is a `pub const fn` taking a raw `u32`, so any `subsec_nanos >= 1_000_000_000` reaches it, the same entry point #908 used: ```rust // ok, this is what #909 fixed SystemTime::from(Timestamp::from_unix_time(u64::MAX, 999_999_999, 0, 0)); // still panics: overflow in Duration::new SystemTime::from(Timestamp::from_unix_time(u64::MAX, 1_000_000_000, 0, 0)); ``` The doc at `src/timestamp.rs:225-228` says an undefined `SystemTime` is returned rather than a failure, so this builds the `Duration` from its parts with the same `checked_* -> unwrap_or(UNIX_EPOCH)` shape #909 used, one level inward. <details> <summary>Verification</summary> Base `9f92712`. | row | `src/timestamp.rs` md5 | `from_system_time_max` | `from_system_time_nanos_carry` | lib suite | |---|---|---|---|---| | base impl + the new test | `f23cc9e478` | ok | **FAILED** | 128 passed, 1 failed | | this PR | `27d00ed610` | ok | ok | 129 passed, 0 failed | | over-correct: reject every carry | `1da1e439e2` | ok | **FAILED** | 128 passed, 1 failed | | drop the outer guard from #909, keep the new one | `7502b0bb61` | **FAILED** | ok | 128 passed, 1 failed | The first row is the point: the test #909 added still passes on base while the new one fails. The last two rows are killed by different tests, so neither guard is redundant, and the over-correction is caught by the `(10, 2_500_000_000) -> 12.5s` assertion, so a clamp-everything fix would not pass either. `cargo test --all-features` 0 failures; `cargo +1.85.0 build --manifest-path tests/smoke-test/Cargo.toml` clean; `cargo +beta clippy --all-features` reports the same 4 warnings before and after, none at the changed lines. Not run, and not claimed: the `miri`, wasm, wasi, embedded and fuzz jobs. The pinned nightly and those toolchains are not available on my machine. The change is safe arithmetic inside `#[cfg(feature = "std")]`, so the no-std jobs do not compile it at all. </details> One thing I left alone: `src/timestamp.rs:228` points callers at `TryFrom` to catch conversion failures, but there is no fallible `TryFrom<Timestamp> for SystemTime`. That seemed like separate scope. --- Disclosure: written with the help of Claude (an AI assistant). Every number above is from runs on my machine.
KodrAus commented 2026-09-15 13:58:06 -05:00 (Migrated from github.com)

The bug here I think is that we accept a timestamp with nanos that overflow a second at all. I think it would be reasonable to panic here, just like Duration::new does.

The bug here I think is that we accept a timestamp with nanos that overflow a second at all. I think it would be reasonable to panic here, just like `Duration::new` does.
This pull request is broken due to missing fork information.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin 'youdie006/systemtime-nanos-carry:youdie006/systemtime-nanos-carry'
git switch 'youdie006/systemtime-nanos-carry'

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch 'main'
git merge --no-ff 'youdie006/systemtime-nanos-carry'
git switch 'youdie006/systemtime-nanos-carry'
git rebase 'main'
git switch 'main'
git merge --ff-only 'youdie006/systemtime-nanos-carry'
git switch 'youdie006/systemtime-nanos-carry'
git rebase 'main'
git switch 'main'
git merge --no-ff 'youdie006/systemtime-nanos-carry'
git switch 'main'
git merge --squash 'youdie006/systemtime-nanos-carry'
git switch 'main'
git merge --ff-only 'youdie006/systemtime-nanos-carry'
git switch 'main'
git merge 'youdie006/systemtime-nanos-carry'
git push origin 'main'
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
breaking
This causes a breaking change and will be merged to breaking branch.

  
bug

   
dependencies
Pull requests that update a dependency file

  
good first issue

   
help wanted

   
unstable
Tracking issue for an unstable optional dependency

No labels
breaking
bug
dependencies
good first issue
help wanted
unstable
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-uuid-rs/uuid!911
No description provided.