blob: clamp pre-epoch/overflow mtime in toJSTime to avoid @intCast panic#29459
blob: clamp pre-epoch/overflow mtime in toJSTime to avoid @intCast panic#29459robobun wants to merge 1 commit into
Conversation
|
Updated 2:11 PM PT - Apr 18th, 2026
❌ @robobun, your commit f267b16 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 29459That installs a local version of the PR into your bun-29459 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughUpdated the Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Duplicate of #29431, which covers this plus the |
| pub const init_timestamp = std.math.maxInt(JSTimeType); | ||
| pub const JSTimeType = u52; | ||
| pub fn toJSTime(sec: isize, nsec: isize) JSTimeType { | ||
| const millisec = @as(u64, @intCast(@divTrunc(nsec, std.time.ns_per_ms))); | ||
| return @as(JSTimeType, @truncate(@as(u64, @intCast(sec * std.time.ms_per_s)) + millisec)); | ||
| const millisec = @as(i128, sec) * std.time.ms_per_s + @divTrunc(nsec, std.time.ns_per_ms); | ||
| return @intCast(std.math.clamp(millisec, 0, std.math.maxInt(JSTimeType))); |
There was a problem hiding this comment.
🟡 The upper bound of the toJSTime clamp (std.math.maxInt(JSTimeType)) is the same value as the init_timestamp sentinel 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 via utimesSync(path, new Date(Number.MAX_SAFE_INTEGER), ...)), toJSTime returns exactly init_timestamp, which is then stored in store.data.file.last_modified. The getLastModified getter re-issues resolveFileStat every time it sees the sentinel, so the caching mechanism never fires -- every .lastModified access issues a fresh stat(2) syscall. Fix by clamping to std.math.maxInt(JSTimeType) - 1, or using a separate bool loaded flag.
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,495 is used as a sentinel in Store.File.last_modified (Store.zig:258) to mean "mtime has not yet been loaded from the filesystem". The new toJSTime implementation clamps to exactly this value as its upper bound:
return @intCast(std.math.clamp(millisec, 0, std.math.maxInt(JSTimeType)));Any file whose mtime maps to >= 4,503,599,627,370,495 ms will have toJSTime return init_timestamp. That clamped value is then written back into store.data.file.last_modified by resolveFileStat (Blob.zig:3303, 3317).
The specific code path
getLastModified (Blob.zig:3067) checks:
if (store.data.file.last_modified == jsc.init_timestamp and !this.isS3()) {
resolveFileStat(store);
}Since resolveFileStat stores the clamped init_timestamp back, the condition remains true on every subsequent call. The result is a stat(2) syscall on every single .lastModified access -- 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 @intCast overflow. 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: .lastModified always returns maxInt(u52) (a reasonable "max future date" value), but the result is never cached. Every access re-issues a stat(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 sentinel- Next
f.lastModifiedcall --last_modified == init_timestampagain;resolveFileStatfires again - Steps 4-6 repeat indefinitely; every
.lastModifiedaccess issues astat(2)syscall
Addressing the "implausible" objection
While year 144,680 CE timestamps don't appear on real filesystems organically, utimesSync with 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:
return @intCast(std.math.clamp(millisec, 0, std.math.maxInt(JSTimeType) - 1));Or use a dedicated bool loaded flag in Store.File to avoid using an in-band sentinel entirely.
What does this PR do?
Fixes a Zig safety-check panic in
jsc.toJSTimewhenfstatreports an mtime before the Unix epoch (negativesec) or far enough in the future to overflowisize * 1000. All four call sites feedstat.mtime()straight into this helper, so any such file crashesBun.file(path).text()/.lastModifiedon a thread-pool worker.Fuzzer fingerprint:
b576ba5fca6d3710Root cause
@intCast(@divTrunc(nsec, ...))tou64panics ifnsec < 0.sec * std.time.ms_per_spanics onisizeoverflow.@intCast(sec * 1000)tou64panics ifsec < 0.Reached from
ReadFile.resolveSizeAndLastModified(line 324) →runAsyncWithFD, and from theBlob.lastModifiedgetter. The fuzzer trace showedrunAsyncWithFDas the top frame with the two inner callees inlined in that build; the fingerprint differs from #29355 (which fixed the adjacentstat.size@intCast) but sits in the same call chain.Reproduces deterministically with:
Fix
Compute the millisecond timestamp in
i128(soisize * 1000and thensecaddend cannot overflow) and clamp to[0, maxInt(JSTimeType)]before the final cast.JSTimeTypeisu52so negative times collapse to0; this matches the existing storage type rather than changing the field to signed.How did you verify your code works?
test/js/bun/util/bun-file-negative-mtime.test.tscovers path-backed read, fd-backed read, thelastModifiedgetter, and a positive-mtime sanity check. Panics on the unfixed binary, passes with the fix.bun bd test test/js/bun/util/bun-file-fd-read.test.ts test/js/bun/util/bun-file.test.ts test/js/bun/util/bun-file-read.test.ts test/js/bun/util/bun-stdin-slice.test.tsall pass.objdumpof the rebuilttoJSTimeshows the remaining safety checks are oni128arithmetic / post-clamp@intCast, all provably unreachable forisizeinputs.