fix(windows): avoid panic converting far-future FILETIME values - #62
Merged
Conversation
UNIX_EPOCH + Duration panics when the result is unrepresentable. A Windows SystemTime is FILETIME-backed and ends near year 30828, so a garbage process creation time overflows it and aborts the scan. The suite never caught this: `cargo test` only runs in the Linux `quality` job, so the `#[cfg(windows)]` and `#[cfg(macos)]` test modules compile but never execute. Run the suite on the natively-runnable platform targets too. Reported by @Guflly in #60.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while reviewing #60 — @Guflly noted in passing that "the full Windows suite still hits the existing far-future FILETIME panic on
main." They are right, and the reason it survived this long is the more interesting half.The panic
AddforSystemTimepanics when the result is unrepresentable.filetime_to_u64(u32::MAX, u32::MAX)isu64::MAX, which works out to roughly year 58,000. A WindowsSystemTimeis itselfFILETIME-backed and ends near year 30828, so it overflows and takes the whole scan down.Linux never sees it:
SystemTimethere is a timespec with i64 seconds, which swallows the value happily. That platform split is exactly why a test namedfiletime_to_system_time_far_future_no_panichas been sitting there green.Fixed with
checked_add. The function already returnsOptionand callers already handleNone, so nothing downstream changes.Why no test caught it
cargo testruns only in the Linuxqualityjob. The platform matrix jobs runcargo build --release --target ...and a smoke test — no tests. So every#[cfg(windows)]and#[cfg(macos)]test in the repo has been compiled and never executed, on any runner.This PR runs the suite on the platform targets that are natively runnable:
x86_64-unknown-linux-gnuqualityx86_64-apple-darwinaarch64-apple-darwinx86_64-pc-windows-msvcThe old assertion was
assert!(result.is_some())— asserting the very thing that overflows, which is what made this a panic rather than a failure. It now assertsNone, the correct Windows answer.Verification
cargo fmt,clippy -D warnings, 169 tests, and both cross-target checks pass locally. But a#[cfg(windows)]fix cannot be genuinely verified from Linux — the CI change is the verification. If the Windows job goes green here, it is the first time these tests have ever run.