[JSC] Date setters: range-check year + floor(month / 12) as MakeDay does, not each argument - #616
[JSC] Date setters: range-check year + floor(month / 12) as MakeDay does, not each argument#616robobun wants to merge 1 commit into
Conversation
…oes, not each argument Date.prototype.setFullYear, setUTCFullYear, setMonth and setUTCMonth build their result with MakeDate(MakeDay(y, m, dt), TimeWithinDay(t)). MakeDay range-checks ym = y + floor(m / 12). It never looks at y or m alone. So new Date(0).setUTCFullYear(275761, -12, 1) is 275760-01-01T00:00:00Z, the value Date.UTC(275761, -12, 1) gives, and so is new Date(Date.UTC(-1, 0, 1)).setUTCMonth(12 * 275761). fillStructuresUsingDateArgs() instead required |years| <= 275760 and |months / 12| <= 275760 separately (295562@main and 296644@main added those bounds so that toInt32() of a huge argument could not wrap into range) and stored both as int. Every call above returned NaN, while the Date constructor and Date.UTC in the same engine, which go through makeDay() with doubles, returned the in-range time value. V8 and SpiderMonkey agree with the constructor. Make every BrokenDownDate field a double, store the setter arguments in it as they are (finite or not in range), and compute the result with the same makeDate(makeDay(), makeTime()) -> localTimeToMS() -> timeClip() steps the constructor and Date.UTC use. makeDay() combines year and month in double arithmetic and only then requires the normalized year to be an int32, so nothing wraps, and a normalized year that is really out of range still ends up NaN through timeClip(). The day argument now goes into makeDay() as MakeDay's date instead of being pre-multiplied by msPerDay into the milliseconds, so a setter is exact to the millisecond (and identical to Date.UTC / the constructor) even when |days * msPerDay| exceeds 2^53. Before, new Date(8.64e15 - 999).setUTCDate(-199000000) was 1 ms off. setYear drops its copy of the per-argument bound too. That changes nothing observable, because a |year| > 275760 with an in-range month and day is always out of range. makeDay() additionally rejects a month remainder outside [0, 12). That can only happen when |month| >= 2^53 makes the year/month split inexact. It used to hand dateToDaysFrom1970() such a month, which folds it into the year once more in int arithmetic: a wrong date at best, and a signed overflow when the year is already INT32_MAX. The setters could not reach that before because of the bound above. Date.UTC always could. The Temporal callers pass a month in [0, 11]. * JSTests/stress/date-setters-year-month-range.js: Added. * Source/JavaScriptCore/runtime/DateConstructor.h: (JSC::makeDay): * Source/JavaScriptCore/runtime/DatePrototype.cpp: (JSC::BrokenDownDate::toTimeValue const): (JSC::fillStructuresUsingTimeArgs): (JSC::fillStructuresUsingDateArgs): (JSC::setNewValueFromTimeArgs): (JSC::setNewValueFromDateArgs): (JSC::JSC_DEFINE_HOST_FUNCTION):
|
Reproduction (any time zone for the UTC lines): // repro.js: TZ=UTC bun repro.js / node repro.js / jsc repro.js
const p = typeof print === "function" ? print : console.log;
p(new Date(0).setUTCFullYear(275761, -12, 1), Date.UTC(275761, -12, 1)); // bun 1.4.3: NaN 8639977881600000 patched, node: 8639977881600000 twice
p(new Date(0).setFullYear(300000, -300000, 1)); // bun 1.4.3: NaN patched, node: 8615994624000000 (TZ=UTC)
p(new Date(Date.UTC(-1, 0, 1)).setUTCMonth(12 * 275761)); // bun 1.4.3: NaN patched, node: 8639977881600000
p(new Date(NaN).setUTCFullYear(-271821, 3600000)); // bun 1.4.3: NaN patched, node: 827076182400000
p(new Date(8.64e15 - 999).setUTCDate(-199000000)); // bun 1.4.3: -8553601036801000 patched, node: -8553601036800999
p(new Date(0).setUTCFullYear(275760, -12, 1), new Date(0).setUTCFullYear(275761, 0, 1)); // controls, unchanged: 8639946345600000 NaNHow I verified, with a Release JSCOnly build of this branch (clang 21, x86_64) and a
Bun side: oven-sh/bun#42193 pins |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes spec-conformance numerics for all Date setters and tightens the shared makeDay() helper (also used by Date.UTC, the Date constructor, and Temporal), and Source/JavaScriptCore / JSTests are CODEOWNER-covered, a human look is still worthwhile.
What was reviewed:
BrokenDownDatefield widening todoubleand the newtoTimeValue()path — recomposition now goes throughmakeDay/makeTime/makeDate→localTimeToMS→timeClip, matching the constructor path inDateConstructor.cpp.- Per-argument
std::isfinitechecks infillStructuresUsingTimeArgs/DateArgs— each argument is still individually gated, soInfinity, -Infinitycombinations still yield NaN as the spec requires. - The added
monthInt32 < 0 || monthInt32 > 11guard inmakeDay()— for |month| < 2^53 the remainder is exact and in [0,11], so no regression for existing in-range callers; Temporal call sites pass a normalized month. - The new stress test throws on failure, does no logging, and covers int32-wrap, 2^53 day-count precision, and non-finite arguments.
Extended reasoning...
Overview
This PR reworks how Date.prototype.set{FullYear,Month,Date,Hours,Minutes,Seconds,Milliseconds,Year} (and UTC variants) rebuild the time value in Source/JavaScriptCore/runtime/DatePrototype.cpp. BrokenDownDate fields become double, the two fillStructuresUsing*Args helpers now just store arguments and check finiteness, and a new BrokenDownDate::toTimeValue() runs the spec's MakeDate(MakeDay(...), MakeTime(...)) → localTimeToMS → timeClip sequence. DateConstructor.h's makeDay() gains a guard rejecting a month remainder outside [0, 11], which only occurs when |month| ≥ 2^53. A new JSTests/stress/date-setters-year-month-range.js exercises boundary years, huge months/days, int32-wrap cases, and non-finite arguments.
Security risks
None identified. This is pure numeric/spec-conformance work in Date runtime host functions. The int32 truncations that were removed were previously guarded by ad-hoc range checks; the replacement path keeps everything in double until makeDay()'s existing yearInt32 != ym check, and timeClip() still bounds the final result. No memory, auth, or injection surface is touched.
Level of scrutiny
Moderate-to-high. The change is small in line count and the recomposition now literally reuses the same helpers the Date constructor and Date.UTC already use, which is a strong correctness argument. However, it is a behavioral change to a widely used built-in, the makeDay() tweak is shared with Date.UTC/new Date()/Temporal callers, and floating-point edge-case reasoning (2^53 splits, day×msPerDay precision) is subtle enough that a JSC reviewer should sign off. .github/CODEOWNERS routes both Source/JavaScriptCore and JSTests to WebKit/jsc-reviewers.
Other factors
The PR description reports no new test262 failures, differential fuzzing against V8 across several time zones, and that the new stress test fails unpatched and passes patched under all JIT tiers in ~10 ms — consistent with JSTests conventions. DateCache::gregorianDateTimeToMS is intentionally left in place for the external Bun binding. Nothing in the timeline indicates outstanding third-party objections.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughChangesDate setters now retain numeric date fields through shared Date setter range handling
Merge Risk: ⚪ Minimal · up to Date setters now normalize and clip values consistently with the Date constructor and Date.UTC, with expanded boundary and overflow coverage. The change is ready to merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed problem statement, fix explanation, affected behavior, and validation results. However, it does not follow the repository template because it omits the required Bugzilla bug link, Reviewed by NOBODY line, and explicit changed-file/function list.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
2 participants
Problem
NaNonce the year argument alone (or month / 12 alone) is outside ±275760:new Date(0).setUTCFullYear(275761, -12, 1)isNaN, butDate.UTC(275761, -12, 1)is8639977881600000. V8 returns the date.year + floor(month / 12).fillStructuresUsingDateArgs()(runtime/DatePrototype.cpp) boundsyearsandmonths / 12separately and stores them asint(upstream 295562@main added the bounds so thattoInt32()cannot wrap).Fix
BrokenDownDatefields becomedouble. The setters store their arguments unchanged, andtoTimeValue()runs themakeDay/makeTime/makeDatehelpers the Date constructor andDate.UTCuse, thenlocalTimeToMS()andtimeClip().makeDay()adds year and floor(month / 12) as doubles before it requires an int32, so nothing wraps, andtimeClip()still rejects an out-of-range sum. The day count goes intomakeDay(), not into the milliseconds, so a setter equalsDate.UTCexactly even when |days × msPerDay| > 2^53.makeDay()also rejects a month remainder outside [0, 12). Only |month| ≥ 2^53 produces one, and it overflowed anintindateToDaysFrom1970().JSTests/stress/date-setters-year-month-range.js(new) fails unpatched, passes patched in 8 time zones and under V8. test262 Date directories: 1802 pass, 0 new failures. Self-reviewed: 2 concerns raised, 2 addressed.Background
timeClip()maps anything beyond toNaN.ym = y + floor(m / 12),mn = m mod 12), then addsdate - 1days. A month of -12 or a negative day count pulls an out-of-range year back in.BrokenDownDateis a setter's scratch copy of the decomposedthisdate, with the argument fields overwritten.Notes
jscvs. before:new Date(0).setUTCFullYear(275761, -12, 1)8639977881600000 (was NaN);new Date(0).setFullYear(300000, -300000, 1)=new Date(300000, -300000, 1)(was NaN);new Date(Date.UTC(-1, 0, 1)).setUTCMonth(12 * 275761)8639977881600000 (was NaN);new Date(NaN).setUTCFullYear(-271821, 3600000)827076182400000 (was NaN); controls unchanged.setYearloses its copy of the per-argument bound too. No observable change: |year| > 275760 with an in-range month and day is always out of range.days * msPerDay. With athisthat has a milliseconds part and |days| > ~1.04e8 that sum rounds, e.g.new Date(8.64e15 - 999).setUTCDate(-199000000)was -8553601036801000 instead of -8553601036800999. Passingdaysas MakeDay'sdatekeeps every intermediate an exact integer.-ffp-contract=off.Date.UTC()of the resulting fields.JSTests/stresscollection plusLayoutTests/jsc-layout-tests.yamlran in all configurations against the patched shell: 27,176 of 27,319 runs pass. The 143 failures (24 files, e.g.eval-func-decl-*,function-toString-native-one-line.js,intl-datetimeformat-default-timezone-change.js) fail identically on the unpatchedjscbuilt from the same tree, so they are pre-existing in this configuration.--useJIT=0,--useDFGJIT=0and eager tier-up thresholds, and runs in about 10 ms.DateCache::gregorianDateTimeToMS()has no caller left in this tree but stays: Bun calls it fromsrc/jsc/bindings/bindings.cpp(Bun__gregorianDateTimeToMS).timeClip()decide, as itsDate.UTCalways has. V8 caps |year| at 1e6 and |month| at 1e7 first, sonew Date(0).setUTCFullYear(2e9, 0, -730484269514)is 2000-01-01 here (and by the spec's exact arithmetic) butNaNin V8.makeDay()month check: for |month| < 2^53,floor(month / 12)and the remainder are exact, so the remainder is always in [0, 11] and the check never fires. Beyond that the remainder can come out as e.g. 16 or -8, whichdateToDaysFrom1970()folds into the year again inintarithmetic (Date.UTC(-9007197107257351, 108086391056891980, 784353026671)returned a year-2000 date through an INT32_MAX + 1 wrap). The Temporal callers always pass a month in [0, 11].thiswithin hours of the minimum time value returnsNaNwhere V8 keeps the date (TZ=America/New_York,new Date(-8.64e15).setMilliseconds(10)), because the local-to-UTC conversion of an out-of-range local time uses an "equivalent" modern year's offset. That is [JSC] Date: convert a local time to UTC with the zone's own offset when the local time is outside the time value range #615, which touchesJSDateMath.cpp/WTF/DateMath.cpponly, so the two PRs do not overlap.mainhas the samefillStructuresUsingDateArgs(), so the patch applies there unchanged.