WebKit: fix Set iteration corrupting large-integer keys to INT32_MIN (#30757) - #30760
WebKit: fix Set iteration corrupting large-integer keys to INT32_MIN (#30757)#30760robobun wants to merge 1 commit into
Conversation
|
Updated 3:19 AM PT - May 23rd, 2026
❌ @robobun, your commit 3314ce3 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 30760That installs a local version of the PR into your bun-30760 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds three regression tests validating Set/Map behavior for numeric values outside the Int32 range; wraps OVERLAY_CSS with JSON.stringify in the build define for bundler JSON parsing; and adds an explicit include for assert() usage. ChangesSet/Map numeric key regression tests
CSS overlay build definition fix
C++ assert include
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/regression/issue/30757.test.ts`:
- Around line 1-24: Replace the multi-line header comment at the top of the test
(the long prose block describing bug history and root cause) with the two-line
regression-test header pattern: first the GitHub issue URL
(https://github.com/oven-sh/bun/issues/30757) and on the next line a single
brief description like "Fix normalization of large integer-valued doubles that
collapsed to INT32_MIN"; remove the extra paragraphs about normalizeMapKey,
INT32_MIN, WebKit history, and the long root-cause explanation so only the URL
and one-line summary remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78518986-fa75-4612-a9e3-6a575458b789
📒 Files selected for processing (1)
test/regression/issue/30757.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Gate check can't mechanically verify this PR because the bug is LTO-specific: vs. the shipped canary (same source, same WebKit pin as this PR's parent commit, same LTO, no fix): The Linux release-LTO Buildkite lane on this PR will exercise the same assertion and gate the fix — needs a maintainer to override the mechanical gate and merge. |
e4a69f1 to
2901f25
Compare
|
Rebased on main (WebKit bumped to oven-sh/WebKit#232 has been rebased onto the new WebKit main (commit |
15a0aae to
785bcc3
Compare
785bcc3 to
274434d
Compare
a7fdaed to
1e43bb2
Compare
1e43bb2 to
a806eb8
Compare
|
Picks up #31080 too (same |
|
Picks up #31171 as well — same |
a806eb8 to
a5dba02
Compare
There was a problem hiding this comment.
LGTM — test-only addition, all prior feedback addressed, and the WebKit fix already landed on main via the 0d85951a bump.
Extended reasoning...
Overview
After several rebase rounds, this PR now adds exactly one file: test/regression/issue/30757.test.ts, a regression test for the WebKit normalizeMapKey UB that corrupted oversized integer-valued double Set/Map keys to INT32_MIN under LTO. The actual fix landed upstream in WebKit and was picked up on main when WEBKIT_VERSION was bumped to 0d85951a (verified at scripts/build/deps/webkit.ts:6); this PR no longer touches the version pin or any production code. The earlier bake-codegen.ts and wtf-bindings.cpp changes were dropped during rebase.
Security risks
None. Test-only change that spawns hermetic bun -e subprocesses with bunEnv and asserts on JSON stdout. No auth, crypto, network, filesystem, or user-input handling.
Level of scrutiny
Low. This is a new regression test following established repo conventions: two-line header (issue URL + one-line summary), test.concurrent for independent subprocess spawns, bunExe()/bunEnv, await using proc, stderr-empty assertion (acceptable with bunEnv per house style), and exitCode asserted last. The test logic itself is straightforward — round-trip values through Set/Map and compare against expected literals.
Other factors
Every review thread I raised across the PR's lifecycle has been addressed and resolved: the missing/preview-tag WebKit pin (resolved by main's 0d85951a bump containing truncateDoubleToInt32), test.concurrent, the condensed header, the ±Infinity in-subprocess identity checks, and the dropped bake-codegen.ts/wtf-bindings.cpp commits. The bug-hunting system found no issues. robobun's final comment confirms 0d85951a contains the fix and the PR now carries only the test against a durable 40-char SHA pin.
Covers the WebKit `normalizeMapKey` regression where under LTO the
`static_cast<int>(double)` UB was exploited by the optimizer, collapsing
every oversized integer-valued double Set/Map key to `jsNumber(INT32_MIN)`:
[...new Set([1751241600000])][0] // was: -2147483648
new Set([1751241600000]).has(-1 << 31) // was: true
Fixed in WebKit by replacing the UB cast with `truncateDoubleToInt32`
(hardware cvttsd2si on x86_64, fcvtzs on arm64), which returns INT32_MIN
on overflow so the subsequent `i == d` filter works as intended. Picked
up in main via WebKit bump to 0d85951a.
Fixes #30757
[skip size check]
a5dba02 to
3314ce3
Compare
There was a problem hiding this comment.
LGTM — test-only addition, all prior feedback addressed, and main's WebKit pin (0d85951a) now carries the normalizeMapKey fix this test covers.
Extended reasoning...
Overview
After several rounds of rebasing and feedback, this PR has converged to a single new file: test/regression/issue/30757.test.ts (131 lines). It adds three test.concurrent cases that spawn bunExe() subprocesses to verify Set/Map iteration preserves oversized integer-valued double keys (timestamps, INT32_MAX+1, 2^32, MAX_SAFE_INTEGER, ±Infinity, NaN) and that .has(-2147483648) no longer spuriously matches. The actual fix landed in WebKit and was picked up on main via the 0d85951a bump; this PR now only contributes the regression coverage.
Security risks
None. This is a test-only change that spawns hermetic bun -e subprocesses with inline scripts and bunEnv. No network, filesystem, auth, crypto, or user-input handling is touched.
Level of scrutiny
Low. The change is additive test coverage with no production code paths affected. The test follows house conventions (two-line header, test.concurrent, await using proc, bunExe()/bunEnv, stderr asserted before parsed stdout, exitCode asserted last, ±Infinity/NaN identity-checked in-subprocess to avoid JSON lossiness). I verified scripts/build/deps/webkit.ts on the current base pins WEBKIT_VERSION = "0d85951a…", the durable SHA the author identified as containing the truncateDoubleToInt32 fix — so the merge-blocker I raised in earlier rounds (missing/preview-tag WebKit bump) is fully resolved.
Other factors
Every prior review thread (mine and CodeRabbit's) is resolved and reflected in the final diff: header condensed, test.concurrent adopted, ±Infinity identity-checked, the bake-codegen.ts and wtf-bindings.cpp side-changes dropped, and the preview-PR WebKit tag removed in favor of main's durable pin. The bug-hunting system found no issues. The only CI red on the latest build is the binary-size check showing +11 MB on Linux targets, which a test-only change cannot cause — that delta is an artifact of the WebKit bump on main vs. the canary comparison baseline, not this PR.
|
Closing: #30757 was closed after the upstream WebKit fix for normalizeMapKey (WebKit/WebKit@a3aa752) reached main through the WebKit bump; vendor/WebKit/Source/JavaScriptCore/runtime/HashMapHelper.h now uses truncateDoubleToInt32. After that bump this PR only carried the regression test, and that test passes on current main (f426a8e) with both a debug build and the release canary (1.4.0-canary.1+da3851e57), so the fix is already on main. |
1 participant
Fixes #30757.
Root cause
WebKit
HashMapHelper.h::normalizeMapKeydidint i = static_cast<int>(d)before itsi == dround-trip check. Out-of-rangestatic_cast<int>(double)is UB (LLVMfptosi→ poison); under LTO the optimizer folded the branch and the integer path was always taken, normalizing every oversized integer-valued double key tojsNumber(INT32_MIN). Non-LTO builds got away with it because hardwarecvttsd2sireturns INT32_MIN on overflow and the compare filtered the bad case..has(originalValue)still worked because lookup hashed the stored (corrupted) normalized key against itself. Tagged Int32s, fractional doubles, strings — all unaffected.Fix
Fixed in WebKit by replacing the UB cast with
truncateDoubleToInt32(Source/WTF/wtf/MathExtras.h— hardwarecvttsd2sion x86_64,fcvtzson arm64), which returns INT32_MIN on overflow so the subsequenti == dfilter works as intended. Picked up in bun when main bumped WebKit to0d85951a.Test
test/regression/issue/30757.test.ts— three concurrent tests spawning subprocesses to exercise the real iteration path:Setround-trip preserves every value in the failure table (timestamp, ±(INT32_MAX+1), 2^32, MAX_SAFE_INTEGER, ±Infinity (identity-checked in-subprocess), NaN)s.sizestays 4 with mixed small + oversized keys ands.has(-2147483648)no longer spuriously matches oversized keysMap(same normalizer)Coverage note: the Set/Map corruption only manifests in LTO release builds (UB-under-LTO), so
bun bdpasses with or without the WebKit pin. Real coverage lands in the Linux release-LTO CI lane, which exercised the failing path against the pre-fix WebKit and now passes against0d85951a.