-
Notifications
You must be signed in to change notification settings - Fork 4.7k
blob: clamp pre-epoch/overflow mtime in toJSTime to avoid @intCast panic #29459
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { closeSync, openSync, utimesSync } from "fs"; | ||
| import { isWindows, tempDir } from "harness"; | ||
| import { join } from "path"; | ||
|
|
||
| // jsc.toJSTime previously used unchecked @intCast to u64 on sec/nsec from | ||
| // fstat mtime. A file with mtime before the Unix epoch (negative seconds) | ||
| // would trip integerOutOfBounds in the ReadFile thread-pool task when | ||
| // resolveSizeAndLastModified stored last_modified. With the fix, the | ||
| // timestamp is clamped to 0 instead of crashing. | ||
| describe.skipIf(isWindows)("Bun.file with pre-epoch mtime", () => { | ||
| test("text() on a path-backed file with negative mtime does not crash", async () => { | ||
| using dir = tempDir("bun-file-neg-mtime", { "neg.txt": "hello" }); | ||
| const path = join(String(dir), "neg.txt"); | ||
| utimesSync(path, new Date(-12345678), new Date(-12345678)); | ||
|
|
||
| const f = Bun.file(path); | ||
| expect(await f.text()).toBe("hello"); | ||
| expect(f.lastModified).toBe(0); | ||
| }); | ||
|
|
||
| test("lastModified getter on a file with negative mtime does not crash", async () => { | ||
| using dir = tempDir("bun-file-neg-mtime", { "neg.txt": "x" }); | ||
| const path = join(String(dir), "neg.txt"); | ||
| utimesSync(path, new Date(-5000), new Date(-5000)); | ||
|
|
||
| expect(Bun.file(path).lastModified).toBe(0); | ||
| }); | ||
|
|
||
| test("text() on an fd-backed file with negative mtime does not crash", async () => { | ||
| using dir = tempDir("bun-file-neg-mtime", { "neg.txt": "from fd" }); | ||
| const path = join(String(dir), "neg.txt"); | ||
| utimesSync(path, new Date(-1000), new Date(-1000)); | ||
|
|
||
| const fd = openSync(path, "r"); | ||
| try { | ||
| const f = Bun.file(fd); | ||
| expect(await f.text()).toBe("from fd"); | ||
| expect(f.lastModified).toBe(0); | ||
| } finally { | ||
| closeSync(fd); | ||
| } | ||
| }); | ||
|
|
||
| test("normal mtime is still reported correctly", async () => { | ||
| using dir = tempDir("bun-file-neg-mtime", { "pos.txt": "ok" }); | ||
| const path = join(String(dir), "pos.txt"); | ||
| const now = Date.now(); | ||
| utimesSync(path, new Date(now), new Date(now)); | ||
|
|
||
| const f = Bun.file(path); | ||
| expect(await f.text()).toBe("ok"); | ||
| expect(Math.abs(f.lastModified - now)).toBeLessThan(2000); | ||
| }); | ||
| }); |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The upper bound of the
toJSTimeclamp (std.math.maxInt(JSTimeType)) is the same value as theinit_timestampsentinel used to mean "last_modified not yet loaded". For any file whose mtime converts to >= 4,503,599,627,370,495 ms (roughly year 144,680 CE, achievable viautimesSync(path, new Date(Number.MAX_SAFE_INTEGER), ...)),toJSTimereturns exactlyinit_timestamp, which is then stored instore.data.file.last_modified. ThegetLastModifiedgetter re-issuesresolveFileStatevery time it sees the sentinel, so the caching mechanism never fires -- every.lastModifiedaccess issues a freshstat(2)syscall. Fix by clamping tostd.math.maxInt(JSTimeType) - 1, or using a separatebool loadedflag.Extended reasoning...
Bug: toJSTime clamp collides with init_timestamp sentinel
What the bug is and how it manifests
init_timestamp = std.math.maxInt(JSTimeType) = maxInt(u52) = 4,503,599,627,370,495is used as a sentinel inStore.File.last_modified(Store.zig:258) to mean "mtime has not yet been loaded from the filesystem". The newtoJSTimeimplementation clamps to exactly this value as its upper bound:Any file whose mtime maps to >= 4,503,599,627,370,495 ms will have
toJSTimereturninit_timestamp. That clamped value is then written back intostore.data.file.last_modifiedbyresolveFileStat(Blob.zig:3303, 3317).The specific code path
getLastModified(Blob.zig:3067) checks:Since
resolveFileStatstores the clampedinit_timestampback, the condition remains true on every subsequent call. The result is a stat(2) syscall on every single.lastModifiedaccess -- the caching mechanism is permanently bypassed for affected files.Why existing code doesn't prevent it
Before this PR, a far-future mtime would panic via
@intCastoverflow. The bug was thus unreachable -- the sentinel collision existed conceptually but could never be observed. This PR introduces reachability: the new clamp makes the crash-path survivable, but in doing so creates a value path where the mtime is indistinguishable from the "not loaded" sentinel.Impact
The impact is a performance regression rather than a correctness issue:
.lastModifiedalways returnsmaxInt(u52)(a reasonable "max future date" value), but the result is never cached. Every access re-issues astat(2)syscall instead of returning the in-memory value.Concrete reproducer (step-by-step)
Number.MAX_SAFE_INTEGER = 9,007,199,254,740,991 ms > maxInt(u52) = 4,503,599,627,370,495 ms, so:utimesSync(path, new Date(Number.MAX_SAFE_INTEGER), new Date(Number.MAX_SAFE_INTEGER))-- sets mtime to a value > maxInt(u52)const f = Bun.file(path)-- creates a Blob;last_modifiedstarts atinit_timestampf.lastModified--getLastModifiedseeslast_modified == init_timestamp, callsresolveFileStatresolveFileStatcallstoJSTime(sec, nsec)wheresec * 1000 > maxInt(u52); clamp returnsmaxInt(u52) = init_timestamplast_modifiedis set toinit_timestamp-- unchanged from the sentinelf.lastModifiedcall --last_modified == init_timestampagain;resolveFileStatfires again.lastModifiedaccess issues astat(2)syscallAddressing the "implausible" objection
While year 144,680 CE timestamps don't appear on real filesystems organically,
utimesSyncwith any Date > ~year 144,000 CE is reachable from JS. Test harnesses, archive tools, and synthetic filesystem generators routinely set unusual timestamps. More importantly, a code fix that introduces a latent sentinel collision is a maintenance hazard regardless of practical frequency.Fix
Change the clamp upper bound by one:
Or use a dedicated
bool loadedflag inStore.Fileto avoid using an in-band sentinel entirely.