JSTests: stress tests for the int32-boundary double->int UB fixed in #244#245
Open
robobun wants to merge 1 commit into
Open
JSTests: stress tests for the int32-boundary double->int UB fixed in #244#245robobun wants to merge 1 commit into
robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 10 minutes and 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
Collaborator
Author
|
Consumer side: oven-sh/bun#31679 links the non-LTO JSC prebuilt as an immediate mitigation and adds a regression test that runs on bun's Linux release (LTO) CI lanes. Its workaround registry trips on the next WEBKIT_VERSION bump — once this PR merges and a pin includes it, the |
robobun
added a commit
to oven-sh/bun
that referenced
this pull request
Jun 2, 2026
… the pin
Official Linux release builds (--lto=on) link the -lto JSC prebuilt —
bitcode compiled by clang 21 whose final optimization runs in rust-lld
(rustc 1.97 nightly, LLVM 22.1.4). DFG parseIntResult() in that bitcode
does static_cast<int>(input) on out-of-range doubles, which is undefined
behavior, and LLVM 22's InstCombine folds the int32 overflow guard into
a bare integrality test. Once a parseInt call site tiers up to the DFG,
results at or above 2^31 come back as sign-wrapped int32s:
parseInt("80000000", 16) === -2147483648 // after ~10k hot calls
and stay wrong for the life of the process. Lower tiers box through the
hardened jsNumber(double) path, so fresh processes look fine — the flip
only appears after JIT warmup, and only on LTO builds (Linux release
CI). Debug, ASAN, macOS (no LTO / native builds) are unaffected.
The source fix is oven-sh/WebKit#245. Until a WEBKIT_VERSION pin
includes it, select the non-LTO (native object) JSC tarball: clang 21
codegen keeps the guard. Bun's own C++/Rust modules stay LTO. The
workaround registry trips on the next pin bump as the removal reminder.
Also pass the artifact builders' LTO flags (-flto=full,
-fwhole-program-vtables, -fforce-emit-vtables) to local Linux WebKit
builds — with plain -flto=thin the final link fails with 'inconsistent
LTO Unit splitting' because bun-profile links -fwhole-program-vtables.
Test: test/js/bun/jsc/parseint-jit-int32-overflow.test.ts spawns bun
with a small jitPolicyScale and hammers parseInt past tier-up. Fails on
the current canary artifact in ~130 ms; passes on an --lto=on build
with this change and on non-LTO/debug builds (which never had the bug —
the miscompile needs the LTO pipeline, so only release CI lanes
exercise the failure mode).
Preview Builds
|
robobun
added a commit
to oven-sh/bun
that referenced
this pull request
Jun 2, 2026
…ping JSC artifacts Linking the non-LTO (native object) JSC into the -flto=full -fwhole-program-vtables release link broke whole-program devirtualization: vtables living in native archives are invisible to the LTO summary, and CI caught segfaults in jsc-stress and a GC-finalizer regression in bundler_defer on linux-aarch64 plus several x64 shards. aarch64 also never had the parseInt fold in the first place (fcvtzs saturates, so the round-trip guard survives there) — the miscompile was x86-64 only. Revert the prebuiltSuffix change and the workaround-registry entry, and instead pin WEBKIT_VERSION to autobuild-preview-pr-245-88395aed — the preview artifacts of the actual source fix (DFG parseIntResult() using tryConvertToStrictInt32 instead of a UB double->int cast). The build layout is exactly what CI has been shipping; only the one JSC function changes. Re-pin to the merged sha once oven-sh/WebKit#245 lands. Validated on linux-x64 --lto=on against the preview artifacts: parseInt("80000000", 16) stays 2147483648 through 10M iterations with forced FTL tier-up, the new regression test passes (and still fails in ~130 ms on the current canary), and bundler_defer + jsc-stress — the suites that failed with the artifact swap — pass.
Collaborator
Author
|
Pushed a second commit after a systematic scan for the same UB class (raw
Both stress tests pass on a jsc shell built from this branch ( |
robobun
added a commit
to oven-sh/bun
that referenced
this pull request
Jun 2, 2026
…ession tests oven-sh/WebKit#245 grew a second commit after a systematic scan for the same UB class: slow_path_switch_imm did static_cast<int32_t>(value) on the double scrutinee before its range check — switch (2**31) could wrongly match a case -2147483648 label if the optimizer folds the check. parseIntResult and switch_imm were the only two raw round-trips left; Map key normalization (#31080) was the same disease but is already hardened at the current pin — verified the Map repro passes on today's canary. Extend the regression test with Map key coverage (keys >= 2^31 and ±Infinity must survive a Map round-trip, issue #31080) and switch scrutinee coverage, sized to run under debug/ASAN timeouts. Validated on linux-x64 --lto=on against autobuild-preview-pr-245-29422029: parseInt repro clean through 10M iterations with forced FTL, Map and switch repros clean, both regression tests pass (parseInt test still fails in ~130 ms on the current canary), bundler_defer and jsc-stress pass.
parseInt results, and switch-immediate scrutinees, at or above 2^31 must not come back as wrapped int32s once out-of-range double->int casts are involved. #244 fixed the undefined behavior (newer LLVM LTO backends folded the round-trip overflow guards into bare integrality tests, so bun's linux-x64 release builds boxed parseInt("80000000", 16) as -2147483648 after DFG tier-up); these tests pin the behavior: - dfg-parseIntResult-should-not-wrap-above-int32.js: hex/decimal/ negative parses around the int32 boundary plus the Infinity-overflow case, with and without radix. - switch-imm-scrutinee-above-int32.js: dense jump-table and sparse switches with out-of-int32 scrutinees (2^31, 2^32, Infinity), which land exactly on a case label if the scrutinee range check is folded. Both passed on a jsc shell built with the equivalent fixes under -flto=full -fwhole-program-vtables.
robobun
force-pushed
the
fix/dfg-parseint-result-int32-overflow-ub
branch
from
2942202 to
5f4bc26
Compare
robobun
changed the title
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
1 participant
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.
Re-scoped to tests-only. #244 landed equivalent (and more thorough) fixes for the double→int cast UB this PR originally addressed —
parseIntResult,slow_path_switch_imm, plusgetUInt32, LiteralParser,String.fromCodePoint, and NumberPrototype — and oven-sh/bun picked them up via the pin bump in oven-sh/bun#31724. The source changes here are dropped; what remains is the regression coverage #244 didn't include:JSTests/stress/dfg-parseIntResult-should-not-wrap-above-int32.js— parseInt around the int32 boundary (hex/decimal/negative, no-radix path, Infinity overflow viaparseInt("f".repeat(400), 16)), looped past DFG tier-up.JSTests/stress/switch-imm-scrutinee-above-int32.js— dense jump-table and sparse switches with out-of-int32 scrutinees (2^31, 2^32, Infinity), which land exactly on a case label if the scrutinee's range check is folded.Context: newer LLVM LTO backends (LLVM 22 via rust-lld in bun's Linux release link) legally fold
(double)(int)x == xround-trips into bare integrality tests when the cast is UB, which shipped asparseInt("80000000", 16) === -2147483648after JIT warmup on bun's linux-x64 canaries (and the Map-key variant, oven-sh/bun#31080). Both tests passed on a jsc shell built with the equivalent fixes under-flto=full -fwhole-program-vtables; bun-side regression tests covering the same behavior through the full pipeline are in oven-sh/bun#31679.