jsc: don't leak builtin @-identifiers in TypeError messages (debug/ASAN) - #36262
jsc: don't leak builtin @-identifiers in TypeError messages (debug/ASAN)#36262robobun wants to merge 3 commits into
Conversation
In ASSERT_ENABLED builds (debug and ASAN), JSC builtins emit expression info, and when a .@call() inside a builtin (e.g. the WrapForValidIterator next/return prototype functions used by Iterator.from) hits a non-callable value, the Call IC slow path feeds the builtin code block and bytecode index straight into appendSourceToErrorMessage, which splices the private @-identifier source text into the user-visible TypeError: Iterator.from({}).next() // TypeError: @getWrapForValidIteratorInternalField(this, // @wrapForValidIteratorFieldIteratedNextMethod).@call is not a // function. (In '...') Release builds are unaffected: BytecodeGenerator::emitExpressionInfo skips builtins under '#if !ASSERT_ENABLED', so hasExpressionInfo() returns false and appendSourceToErrorMessage bails early. WebKit fix (oven-sh/WebKit#369) adds an isBuiltinFunction() check in appendSourceToErrorMessage so ASSERT_ENABLED output matches release. The preview build here is the current WEBKIT_VERSION (549170099) plus that one commit.
|
Updated 8:15 PM PT - Jul 28th, 2026
❌ @robobun, your commit 6e52dfd has some failures in 🧪 To try this PR locally: bunx bun-pr 36262That installs a local version of the PR into your bun-36262 --bun |
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
There was a problem hiding this comment.
No issues found in the diff, but this bumps WEBKIT_VERSION to a preview-PR tag rather than a merged main sha — per the PR description, that's intentionally temporary pending oven-sh/WebKit#369 landing. A human should decide whether to merge on the preview build or wait for the main sha.
Checked: prebuiltUrl/prebuiltDestDir already handle the autobuild- prefix correctly (tag passed through, full string used as cache key). The new test asserts exact messages (toBe) plus negative @-leak checks across three builtin call-IC paths.
Extended reasoning...
Overview
Two changes: (1) scripts/build/deps/webkit.ts swaps WEBKIT_VERSION from commit sha 549170099... to preview tag autobuild-preview-pr-369-bce3c3e1, and (2) adds test/js/bun/jsc/builtin-error-message.test.ts covering three JSC-builtin TypeError paths that previously leaked @-prefixed private identifiers into user-visible messages under ASSERT_ENABLED (debug/ASAN) builds.
Security risks
None. The change affects error-message text formatting inside JSC and a build-script version constant; no auth, crypto, permissions, or untrusted-input parsing paths.
Level of scrutiny
High — not because the Bun-side diff is complex (it's a one-line version bump plus a small test), but because any WebKit bump swaps the JS engine binary for every platform/profile. The actual behavioral fix lives in oven-sh/WebKit#369, which isn't reviewable from this repo. The PR author explicitly flags the preview tag as temporary and says it should be replaced with the resulting main sha once the WebKit PR merges. That's a merge-timing/process decision a maintainer should make.
Other factors
- The build-script side is safe:
prebuiltUrlalready special-casesversion.startsWith("autobuild-")to avoid double-prefixing the release tag, andprebuiltDestDiruses the full tag (minus theautobuild-prefix) as the cache-dir key, so there's no collision with the previous sha-keyed extraction. - The test file follows repo conventions: exact-value assertions (
toBe), covers three sibling entry points to the same Call IC slow path, and thenot.toContain("@")/not.toContain("WrapForValidIterator")checks make the failure mode obvious if it regresses. It correctly lives undertest/js/bun/jsc/. - No prior claude[bot] review on this PR; nothing to avoid duplicating.
|
The preview tag is temporary as described; oven-sh/WebKit#369 is the one-commit diff (based on the current CI status: build #84438 built the preview WebKit cleanly on every lane and |
What
Bumps WebKit to a preview build containing oven-sh/WebKit#369 (the current
WEBKIT_VERSION549170099 plus one commit) and adds a regression test.Why
In ASSERT_ENABLED builds (debug and ASAN), JSC builtins emit expression info. When a
.@call()inside a builtin hits a non-callable value, the Call IC slow path (throwNotAFunctionErrorFromCallICinRepatchInlines.h) passes the builtin code block and bytecode index straight intoappendSourceToErrorMessage, which splices the private@-identifier source text into the user-visibleTypeErrormessage.Release builds are unaffected because
BytecodeGenerator::emitExpressionInfoskips builtins under#if !ASSERT_ENABLED, sohasExpressionInfo()returns false and the append step bails early.Repro
debug/ASAN before:
after (matches release output):
Also covers
Iterator.from({[Symbol.iterator]: 5})(getIteratorFlattenable) and any other builtin that reaches the Call IC slow path with a non-callable.Verification
Built WebKit from source with the fix applied;
test/js/bun/jsc/builtin-error-message.test.tsfails under a debug build without the WebKit change and passes with it. Release builds pass either way (they never had the leak).Notes
Draft until the WebKit preview build (
autobuild-preview-pr-369-bce3c3e1) finishes publishing; CI will fail fetching WebKit until then. Once oven-sh/WebKit#369 merges,WEBKIT_VERSIONshould be set to the resulting main sha rather than the preview tag.[decide:webkit] gate passed · iteration 1 · 2 files touched
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file