Node.js primordials: tamper-proof built-in references for builtin JS - #341
Node.js primordials: tamper-proof built-in references for builtin JS#341robobun wants to merge 7 commits into
Conversation
Expose 435 tamper-proof references to original ECMAScript built-in
functions as link-time constants named after Node.js's primordials
(@ArrayPrototypePush, @ObjectDefineProperty, @MapPrototypeGetSize, ...)
so Bun's builtin JavaScript can call them in JSC style:
@ArrayPrototypePush.@call(array, value);
@ObjectDefineProperty(obj, key, desc);
@MapPrototypeGetSize.@call(map);
@-name resolution at bytecode generation emits moveLinkTimeConstant
(constant-pool slot, swapped in at CodeBlock link), and .@call in a
builtin compiles to a bare op_call with no property load, so the cost
is the same as an unpolluted direct call.
Each primordial is captured into m_linkTimeConstants when its holder
object is created: eager holders in JSGlobalObject::init(), lazy ones
(LazyClassStructure, LazyProperty, PropertyCallback) inside their own
init bodies, so capture is exactly as lazy as the holder and always
happens before user code can mutate it. The link-time constant slots
for lazy holders also get an initLater fallback that forces the holder
so a builtin referencing @MapPrototypeGet can link before anything
touches Map.
The per-holder macro table lives in runtime/JSCPrimordials.h and drives
the LinkTimeConstant enum, private builtin names, bytecode-intrinsic
registry entries, and capturePrimordials dispatch from one source.
WalkthroughChangesPrimordial holder and name catalogs are added behind ChangesPrimordial Capture
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 2
🤖 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 `@Source/JavaScriptCore/bytecode/LinkTimeConstant.h`:
- Around line 166-184: Monitor the per-realm memory impact of the additional
primordial entries counted by numberOfLinkTimeConstants and stored in
m_linkTimeConstants; no code change is requested unless measurements show the
increased allocation is problematic.
In `@Source/JavaScriptCore/runtime/JSGlobalObject.h`:
- Around line 503-506: Make capturePrimordials publicly callable from the free
functions in JSGlobalObject.cpp by moving its declaration out of the private
section into public, or by granting equivalent friend access. Preserve its
existing signature and USE(BUN_JSC_ADDITIONS) guard.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 11b26707-d435-4cf3-880b-06bf3c1c25c1
📒 Files selected for processing (11)
Source/JavaScriptCore/CMakeLists.txtSource/JavaScriptCore/Sources.txtSource/JavaScriptCore/builtins/BuiltinNames.cppSource/JavaScriptCore/builtins/BuiltinNames.hSource/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.cppSource/JavaScriptCore/bytecode/LinkTimeConstant.cppSource/JavaScriptCore/bytecode/LinkTimeConstant.hSource/JavaScriptCore/runtime/JSCPrimordials.cppSource/JavaScriptCore/runtime/JSCPrimordials.hSource/JavaScriptCore/runtime/JSGlobalObject.cppSource/JavaScriptCore/runtime/JSGlobalObject.h
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Preview Builds
|
Use the autobuild-preview-pr-341-6d9faf57 prerelease until the WebKit PR merges and a main-branch autobuild is available.
Each primordial link-time constant is now armed lazily at the start of
JSGlobalObject::init() and materialized on the first CodeBlock link that
references it, from two pristine sources:
- the holder's own properties, snapshotted (getDirect + pointer store,
no reification or allocation) at the holder's creation: end of
init() for the eager holders, and the LazyClassStructure /
LazyProperty / PropertyCallback that creates each lazy holder;
- the holder's ClassInfo static hash tables, which user code cannot
mutate. A reified value is reused for identity only when it still
matches its table entry; otherwise a fresh copy is built from the
entry.
A key present in neither (a runtime-flagged builtin) becomes a function
that throws when called rather than a RELEASE_ASSERT at capture time, and
the previous prototype-chain get() fallback that could capture a planted
Object.prototype value on lazy holders is gone.
Adds m_symbolConstructor / m_bigIntConstructor and accessors so every
holder is reachable from the global object, an overridePrimordialsFromHolder
hook for embedders that replace a builtin during their own global setup,
and JSGlobalObject::auditPrimordials() which returns the full manifest and
values for testing.
|
Pushed a4e834c reworking the mechanism after review; summary of what changed and why: Design change: lazy link-time materialization instead of eager capture. Every primordial slot is now armed lazily at the start of Fixes:
Additions: Verified against a local build with the paired Bun PR (test suite exercises all 435 entries; details there). |
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 `@Source/JavaScriptCore/runtime/JSCPrimordials.cpp`:
- Around line 85-100: Add a compile-time static_assert in
firstLinkTimeConstantIndex() or immediately before its unsigned subtraction to
enforce that numberOfLinkTimeConstants is at least numberOfPrimordials. Keep the
existing index calculations unchanged once this invariant is validated, so
invalid enum ordering fails during compilation rather than underflowing.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6801c17b-8937-429c-b520-d1f575c610b1
📒 Files selected for processing (6)
Source/JavaScriptCore/runtime/JSCPrimordials.cppSource/JavaScriptCore/runtime/JSCPrimordials.hSource/JavaScriptCore/runtime/JSGlobalObject.cppSource/JavaScriptCore/runtime/JSGlobalObject.hSource/JavaScriptCore/runtime/PropertyDescriptor.cppSource/JavaScriptCore/runtime/PropertyDescriptor.h
There was a problem hiding this comment.
Thanks — verified all three prior findings are addressed in a4e834c: the initLater slots are now armed at the top of init() before any lazy holder can fire, missing keys materialize as a throwing function instead of RELEASE_ASSERT, the proto-chain get() fallback is gone, and <cstdint> is included. No new issues found in the rework. Given the scope (a new ~1200-line lazy-materialization mechanism threaded through JSGlobalObject::init(), LinkTimeConstant, and static-table reification), this still warrants a human pass.
What was reviewed:
initializePrimordialLinkTimeConstants()ordering vs. every lazy-holder force path ininit()— no clobber window remains.materializePrimordial/pristineFromHolderOwnProperty— reified values are only reused when they still match the ClassInfo table entry; user-installed values are not adopted.pristineNamespaceObject— reifies the global's own static entry directly (nogetOwnPropertySlotoverride / sandbox trap can run), and rejects namespace objects from another global.- Feature-gated entries (
Atomics.waitAsync,JSON.rawJSON) now fall through tounavailableFunctionrather than crashing.
Extended reasoning...
Overview
This PR adds 435 Node.js-style primordial references (@ArrayPrototypePush, @ObjectDefineProperty, etc.) as link-time constants for Bun's builtin JavaScript. It introduces two new files (JSCPrimordials.h — a ~780-line macro table, and JSCPrimordials.cpp — ~360 lines of materialization logic) and threads capture/materialization hooks through JSGlobalObject::init(), LinkTimeConstant, BuiltinNames, BytecodeIntrinsicRegistry, and every lazy-holder creation site (LazyClassStructure, LazyProperty, PropertyCallback). Everything is guarded by #if USE(BUN_JSC_ADDITIONS).
The current revision (a4e834c) is a substantial rework of the original mechanism after my first-round review: it moved from eager capture to lazy link-time materialization backed by ClassInfo static hash tables, added snapshotPrimordialsFromHolder / overridePrimordialsFromHolder / auditPrimordials, and added m_symbolConstructor / m_bigIntConstructor members with GC visitation.
Security risks
Primordials exist specifically to be tamper-proof — a bug that lets user-mutated values reach a primordial slot would let prototype pollution affect Bun's internal builtins. The rework is careful here: own-property snapshots happen at holder creation before user code can reach the holder; static-table entries are read from immutable ClassInfo tables and a reified value is only reused when matchesEntry confirms it still points at the original native/builtin function; the old prototype-chain get() fallback (which could pick up a planted Object.prototype.<key>) is gone; and pristineNamespaceObject reifies via reifyStaticProperty rather than getOwnPropertySlot, so a node:vm sandbox override cannot run user code mid-link. I did not find a path where user-controlled values are adopted, but the invariants are subtle and spread across several call sites.
Level of scrutiny
High. This is core JSC runtime infrastructure: it extends the LinkTimeConstant enum by ~435 entries, hooks into JSGlobalObject::init() ordering (where the previous revision had a real crash bug), and adds a bespoke lazy-materialization path that interacts with LazyProperty, LazyClassStructure, static hash-table reification, and GC. The first revision had two correctness bugs I flagged that the author reproduced and fixed; the rework that fixed them is itself a significant redesign that a human familiar with JSC's lazy-init and static-table machinery should look over.
Other factors
- All three of my prior findings and both CodeRabbit findings are addressed and resolved; the author confirmed each with a specific explanation.
- The paired Bun PR reportedly exercises all 435 entries plus option-off configurations; there are no in-tree JSC tests added here.
- New
WriteBarriermembers are correctly visited invisitChildrenImpl. - The change is additive and fully
#if-guarded, so non-Bun builds are unaffected.
Deferring to human review given the scope and the subtlety of the init-ordering / tamper-proofing invariants; not because of any known defect in this revision.
There was a problem hiding this comment.
The rework in a4e834c addresses all three issues I raised on the original revision (Error-structure init ordering, feature-gated table entries crashing, and the <cstdint> include), and this pass found no new problems — but the change is large and touches JSGlobalObject::init() ordering, LazyProperty semantics, and ClassInfo static-table introspection across ~50 holders, so it warrants a human look.
What was reviewed:
- Init ordering:
initializePrimordialLinkTimeConstants()now runs first ininit(), so any holder forced mid-init.set()s over an armed lazy slot rather than being clobbered. - Missing-key path: absent entries (vmType-gated
Atomics.waitAsync, option-gatedJSON.rawJSON) materialize as a throwing stub instead ofRELEASE_ASSERT; no proto-chainget()remains, so pollution can't be captured. materializePrimordialre-checksslot.isInitialized()after forcing the holder, so the snapshot taken inside the holder's creation hook wins.JSC_FOREACH_PRIMORDIAL_NAMEexpansion order matchesJSC_FOREACH_PRIMORDIAL_HOLDER, sos_holderBegin/holderFor()index math lines up.
Extended reasoning...
Overview
This PR adds ~435 Node.js-style primordial references as link-time constants under USE(BUN_JSC_ADDITIONS). It touches 13 files: a new ~780-line macro table header (JSCPrimordials.h), a ~360-line implementation (JSCPrimordials.cpp), and integration into BuiltinNames, BytecodeIntrinsicRegistry, LinkTimeConstant, JSGlobalObject::{init,visitChildren}, and PropertyDescriptor. Since my previous review the author pushed a4e834c, a substantial redesign: instead of eagerly capturing every primordial at holder creation, each slot is armed lazily at the top of init() and materialized on first CodeBlock link, either from an own-property snapshot or directly from the holder's immutable ClassInfo static hash tables. d0f433e adds the <cstdint> include.
Security risks
Low. The whole mechanism is guarded by USE(BUN_JSC_ADDITIONS) and is engine-internal plumbing for builtin JS — it exposes no new user-facing surface. The point of the feature is to reduce prototype-pollution exposure in Bun's builtins. The reworked capture path is careful not to run user code: pristineNamespaceObject reifies the global's own static entry via reifyStaticProperty rather than get(), and materializePrimordialFromTables reads ClassInfo tables (immutable C++ data) and only reuses a reified value if it still matches the table entry. I didn't find any path where user-controlled values are adopted as primordials.
Level of scrutiny
High. Despite the feature guard, this threads through JSGlobalObject::init() — the most order-sensitive function in JSC — and interacts with LazyProperty / LazyClassStructure init semantics, reifyStaticProperty, exception scopes inside init, and GC visiting (m_symbolConstructor/m_bigIntConstructor are new WriteBarriers, correctly appended in visitChildrenImpl). The original revision had two real ordering/gating bugs; the redesign is materially different code. That combination — critical path, subtle invariants, recent large rework — is exactly what a human reviewer should sign off on.
Other factors
All prior review comments (mine and CodeRabbit's) are resolved. One open CodeRabbit nit suggests a defensive static_assert(numberOfPrimordials <= numberOfLinkTimeConstants); it's trivially true by construction (primordials are appended to the enum) and non-blocking. The author reports the paired Bun PR's test suite exercises all 435 entries plus option-off configurations. I verified the two macro enumeration orders (JSC_FOREACH_PRIMORDIAL_NAME vs JSC_FOREACH_PRIMORDIAL_HOLDER) are consistent so holderFor()'s prefix-sum lookup is sound, and that snapshotPrimordialsFromHolder guards against re-run PropertyCallback holders via isInitialized() && !overrideExisting.
…pers Expand the primordial set from a fixed 435-name list to the complete Node.js primordials manifest (644 entries across 99 holders), described by a generated table (JSCPrimordialsTable.h) rather than hand-written lists. Holders now include RegExp/Function/native-error/per-type typed-array constructors and prototypes, ProxyObject, WeakRef and FinalizationRegistry constructors, and DataViewConstructor; entry kinds grow from Method/Getter to also cover Setter, Value (own objects and symbols), and Self (a holder object itself). The mechanism keeps every slot lazy: slots are armed at the start of JSGlobalObject::init(), own properties are snapshotted at each holder's creation (a key lookup + getDirect + pointer store per entry), and static-table properties materialize on first link from the immutable ClassInfo tables. Materialization now handles the entry types the larger manifest reaches: JS GetterSetter and C++ CustomGetterSetter accessors (including CustomValue data-like properties, whose getter is read at snapshot time), LazyCellProperty / LazyClassStructure / PropertyCallback entries (recomputed from their own pristine source), and symbol-valued and Self entries. A property present in neither an own property nor a table becomes a per-name throwing "unavailable" function. Correctness fixes: the identity check for a reified static-table function now also requires the same realm (a foreign realm's builtin shares the generator/native pointer); the %TypedArray% prototype and constructor snapshots both happen in the constructor initializer, after prototype.constructor is installed, so either lazy-init order is correct; a null holder or an un-buildable entry type is a RELEASE_ASSERT rather than a cached stub. primordialHolderObject and the holder enum are generated from the table. PropertyDescriptor's cached custom getter/setter function constructors are exported directly instead of through wrappers.
There was a problem hiding this comment.
All three issues I flagged on the first revision are addressed: the Error-holder initLater ordering crash is fixed by arming all primordial slots at the top of init() before any lazy holder can fire; the RELEASE_ASSERT on feature-gated keys is replaced by an unavailable-throwing stub; and <cstdint> is now included. The automated pass over the reworked lazy-materialization mechanism found no new issues. Deferring to a human given the scope.
Checked: initializePrimordialLinkTimeConstants runs before any LazyClassStructure registration, so the mid-init() Error force no longer clobbers set slots. materializePrimordialFromTables reads only own properties and ClassInfo static tables — the old proto-chain get() pollution vector is gone. The four new constructor WriteBarrier members are visited in visitChildrenImpl. pristineNamespaceObject reifies the global's own static entry directly rather than going through getOwnPropertySlot, so a node:vm sandbox override can't run mid-link.
Extended reasoning...
Overview
This PR adds ~1800 lines across 14 files to expose 435 Node.js-style primordials (@ArrayPrototypePush, @ObjectDefineProperty, etc.) as link-time constants for Bun's builtin JavaScript. The core additions are three new files (JSCPrimordials.{h,cpp}, JSCPrimordialsTable.h), plus wiring into LinkTimeConstant, BuiltinNames, BytecodeIntrinsicRegistry, and JSGlobalObject init/visit paths. Everything is gated on USE(BUN_JSC_ADDITIONS). After my first-round review, the author reworked the mechanism from eager capture to lazy link-time materialization (a4e834c), then split the generated table into its own header (39d7bcf).
Security risks
The whole point of primordials is tamper-proofing against prototype pollution, so the correctness bar here is security-relevant: a captured value that user code can influence defeats the purpose. The rework specifically closed the two vectors I'd flagged — proto-chain get() fallback (which could capture a planted Object.prototype.<key>) and the PropertyCallback fallback conjuring a namespace object via a user-visible read. The new path reads only own properties at holder creation time and immutable ClassInfo static tables at link time, with a realm check in matchesEntry to reject foreign-realm builtins. No injection/auth/data-exposure surface beyond that.
Level of scrutiny
High. This threads through JSGlobalObject::init() ordering (where the first-revision bug lived), LazyProperty/LazyClassStructure re-entrancy semantics, static hash table reification (reifyStaticProperty, findPropertyHashEntry), custom-getter wrapping, and GC visiting. The generated 1200-line macro table is mechanically produced but drives enum values, identifier registration, and holder dispatch — a mismatch between JSC_FOREACH_PRIMORDIAL_HOLDER order and JSC_FOREACH_PRIMORDIAL_NAME order would silently mis-index. The initLater lambda in initializePrimordialLinkTimeConstants computes its own slot index via pointer arithmetic against m_linkTimeConstants.begin(), which is correct for FixedVector but subtle. This is well beyond the "simple/mechanical" threshold for auto-approval.
Other factors
The author reproduced and fixed all three of my prior findings with detailed responses, and the paired Bun PR reportedly exercises all 435 entries plus an option-off test. One unresolved CodeRabbit nit (a defensive static_assert on numberOfPrimordials <= numberOfLinkTimeConstants) remains open — it's trivial and the invariant holds by construction since primordials are appended to the enum, but it's a reasonable one-liner the author may want to take. CI preview builds passed for both d0f433e and 6d9faf5. Given the size, the depth of the rework since my last look, and the runtime-core surface area, a human should sign off.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Source/JavaScriptCore/runtime/JSCPrimordials.cpp (1)
265-269: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMaterialize custom
Valueentries as values, not getter functions.
cellForOwnProperty()correctly evaluates aCustomGetterSetterforPrimordialKind::Value, but the lazy/table path skips custom entries and then treats every non-Setterentry as a getter. A custom value primordial therefore becomes a callable getter wrapper instead of the getter’s returnedJSCell, producing the wrong intrinsic type and behavior when the holder was not eagerly snapshotted.Pass the holder into
materializeFromEntry()and evaluateentry.propertyGetter()forPrimordialKind::Value, reusing the same exception handling ascellForOwnProperty(); only create getter/setter wrapper functions forGetterandSetterkinds.Also applies to: 289-292, 389-403
🤖 Prompt for 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. In `@Source/JavaScriptCore/runtime/JSCPrimordials.cpp` around lines 265 - 269, The lazy primordial materialization path must treat custom entries of PrimordialKind::Value as evaluated values rather than getter wrappers. Update materializeFromEntry() to receive the holder, evaluate entry.propertyGetter() for Value using the same exception handling as cellForOwnProperty(), and return the resulting JSCell; restrict createCustomGetterFunction() and createCustomSetterFunction() to Getter and Setter kinds.
🤖 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.
Outside diff comments:
In `@Source/JavaScriptCore/runtime/JSCPrimordials.cpp`:
- Around line 265-269: The lazy primordial materialization path must treat
custom entries of PrimordialKind::Value as evaluated values rather than getter
wrappers. Update materializeFromEntry() to receive the holder, evaluate
entry.propertyGetter() for Value using the same exception handling as
cellForOwnProperty(), and return the resulting JSCell; restrict
createCustomGetterFunction() and createCustomSetterFunction() to Getter and
Setter kinds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 209038ac-a5e8-487c-b841-3d17d4214bb6
📒 Files selected for processing (1)
Source/JavaScriptCore/runtime/JSCPrimordials.cpp
There was a problem hiding this comment.
No new issues found on the reworked revision — the three findings from the initial pass (Error-holder initLater clobber, feature-gated-key RELEASE_ASSERT, missing <cstdint>) all look addressed. That said, this is a large change to core JSC runtime init (JSGlobalObject::init, link-time-constant plumbing, ClassInfo-table materialization) with a substantially redesigned mechanism since the first revision, so it warrants a human sign-off.
What was reviewed:
initializePrimordialLinkTimeConstants()now runs at the top ofinit()before any lazy holder can fire, andsnapshotPrimordialsFromHolderskips already-initialized slots — the mid-init-force ordering hazard is gone.- Missing keys now materialize as a throwing stub via
unavailableFunction; the proto-chainget()fallback is removed, so pollution can't be captured. matchesEntry/pristineFromHolderOwnPropertygate reified values on same-realm + same native/generator pointer before trusting them;pristineNamespaceObjectreifies from the global's own static entry rather than doing a user-visible[[Get]].- New
m_symbolConstructor/m_bigIntConstructor/m_weakObjectRefConstructor/m_finalizationRegistryConstructorare visited invisitChildrenImpl.
Extended reasoning...
Overview
Adds ~435 Node.js-style primordial references as lazy link-time constants under USE(BUN_JSC_ADDITIONS). New files JSCPrimordials.{h,cpp} and generated JSCPrimordialsTable.h define per-holder macro tables that drive the LinkTimeConstant enum, BuiltinNames private identifiers, and the bytecode-intrinsic registry. JSGlobalObject grows four constructor WriteBarrier members plus public snapshotPrimordialsFromHolder/overridePrimordialsFromHolder/auditPrimordials APIs, and init() / lazy-holder callbacks are threaded to snapshot own properties at holder creation. PropertyDescriptor.cpp's createCustomGetterFunction/createCustomSetterFunction are un-static'd and exported.
Security risks
The mechanism's purpose is to be a tamper-proof source of built-ins for Bun's builtin JS. Correctness of matchesEntry (realm + native-pointer / builtin-generator identity check) and the static-table materialization path determines whether user-installed replacements can ever be captured. The rework removed the prototype-chain get() fallback that could previously capture polluted values. cellForOwnProperty invokes a CustomGetterSetter's getter under DeferTerminationForAWhile — this only runs at holder creation (pristine) or via the host override API, so user code shouldn't be reachable, but it is exactly the kind of subtle invariant that benefits from a maintainer's eye. No auth/crypto/permission surfaces touched.
Level of scrutiny
High. This is not a mechanical change: it hooks into JSGlobalObject::init() ordering, every LazyClassStructure / LazyProperty / PropertyCallback holder's creation path, and the link-time-constant machinery used by all builtin JS. The mechanism was substantially redesigned after the first review round (eager capture → lazy materialization from immutable ClassInfo tables), so the current shape hasn't been reviewed by a human yet.
Other factors
All prior review comments (mine and CodeRabbit's) are resolved. Preview builds are green through 39d7bcfd. The paired Bun PR reportedly exercises all 435 entries via auditPrimordials. Everything is guarded by #if USE(BUN_JSC_ADDITIONS) so upstream WebKit builds are unaffected. Given the scope, novel design, and that the initial revision had real crash bugs, deferring to a human reviewer.
…ials-link-time-constants Conflicts resolved: - scripts/build/deps/webkit.ts: kept the autobuild-preview-pr-341 pin (this PR depends on oven-sh/WebKit#341). - src/js/internal/primordials.js: kept the generated file; main's #36318 removed SafePromiseAll from the old hand-written module, which the generated module supersedes entirely.
There was a problem hiding this comment.
No issues found in the reworked (a4e834c+) revision, but given the scope — ~450 lines of new lazy-materialization logic wired into JSGlobalObject::init(), link-time-constant resolution, and BuiltinNames — this warrants a human pass.
Checked in this revision:
initializePrimordialLinkTimeConstants()now runs before any lazy holder can fire, so the earlier initLater-clobber crash is fixed;snapshotPrimordialsFromHolderguards onisInitialized().- Missing/feature-gated keys (
Atomics.waitAsync,JSON.rawJSON) now materialize as a throwing stub instead ofRELEASE_ASSERT; no proto-chainget()remains, so pollution can't be captured. - New
WriteBarriermembers (m_symbolConstructor/m_bigIntConstructor/m_weakObjectRefConstructor/m_finalizationRegistryConstructor) are visited invisitChildrenImpl. materializeFromEntry/matchesEntrypaths cross-checked againstreifyStaticProperty's attribute cases;pristineNamespaceObjectreifies via the static table (no user[[Get]]).
Extended reasoning...
Overview
This PR adds 435 Node.js-style primordial references as link-time constants, gated on USE(BUN_JSC_ADDITIONS). It touches: the LinkTimeConstant enum and count, BuiltinNames private-symbol registration, BytecodeIntrinsicRegistry, JSGlobalObject::init() and multiple initLater/PropertyCallback bodies, plus a new ~450-line JSCPrimordials.cpp implementing lazy materialization from ClassInfo static hash tables and a ~1200-line generated table header. Four new WriteBarrier constructor members are added to JSGlobalObject with matching GC visitation.
The design was substantially reworked in a4e834c after the first review round: eager capture was replaced with lazy per-slot materialization armed at the top of init(), own-property snapshot at holder creation, and static-table fallback that verifies identity via matchesEntry before reusing a reified value.
Security risks
The point of primordials is tamper-proofness — if the "pristine" guarantee can be defeated (e.g. a user-installed function captured as a primordial), Bun's builtin JS that relies on @ArrayPrototypePush etc. becomes pollutable. The reworked design mitigates this by (a) snapshotting only own properties at holder creation before user code can reach the holder, (b) materializing lazily from immutable ClassInfo static tables, and (c) validating any already-reified value against its table entry (same native/builtin pointer AND same realm) before reusing it. pristineNamespaceObject reifies the global's own static entry directly rather than doing a [[Get]], so a node:vm sandbox proxy trap can't run mid-link. I did not find a path where user-controlled values leak into a primordial slot, but the surface is large and the invariants are subtle.
Level of scrutiny
High. This is core JSC runtime infrastructure: JSGlobalObject::init() runs on every realm creation, link-time-constant resolution runs on every CodeBlock link that references a builtin, and a bug here manifests as either a startup crash, a per-realm memory regression, or a silent tamper-proofness bypass. The mechanism composes LazyProperty, LazyClassStructure, PropertyCallback, and static HashTable reification in ways that depend on precise init ordering — the first review round found two real crash paths from ordering assumptions, both now fixed.
Other factors
- All prior review comments (mine and CodeRabbit's) are resolved; the
static_assertonnumberOfPrimordials <= numberOfLinkTimeConstantsand the<cstdint>include landed in follow-up commits. - Everything is behind
#if USE(BUN_JSC_ADDITIONS), so upstream WebKit builds are unaffected. - Test coverage lives in the paired Bun PR (all 435 entries exercised via
auditPrimordials, plus an option-off test), not in this repo. - The generated table (
JSCPrimordialsTable.h) and theJSC_FOREACH_PRIMORDIAL_HOLDER_ACCESSORexpression list encode assumptions about which JSGlobalObject accessors exist and which holders are eager vs. lazy — a human familiar with Bun's JSGlobalObject subclass should sanity-check those mappings.
Given the scope, the design rework, and the criticality of the code paths involved, I'm deferring rather than approving.
oven-sh/WebKit#341 synced with WebKit main; autobuild-preview-pr-341-01de5fd4 is published with all 38 assets.
Regenerated against the upgraded upstream base: Float16Array's constructor and prototype are manifest members (Bun's ported inspect indexes primordials by a typed array's Symbol.toStringTag), and the newly-added Iterator.prototype.includes builtin is picked up by the manifest algorithm.
| }; | ||
|
|
||
| static JSCustomSetterFunction* createCustomSetterFunction(JSGlobalObject* globalObject, VM& vm, PropertyName propertyName, PutValueFunc putValueFunc) | ||
| JSCustomSetterFunction* createCustomSetterFunction(JSGlobalObject* globalObject, VM& vm, PropertyName propertyName, PutValueFunc putValueFunc) |
There was a problem hiding this comment.
🟡 nit: the static removal here (and at line 72) is unconditional, but the corresponding declarations in PropertyDescriptor.h are inside #if USE(BUN_JSC_ADDITIONS) — this is the only unguarded change in an otherwise fully-guarded PR. In a non-Bun Xcode build (which sets GCC_WARN_ABOUT_MISSING_PROTOTYPES=YES + warnings-as-errors) these would become external-linkage definitions with no prior declaration and fail to compile; Bun's own CMake CI is unaffected. Consider wrapping the static removal in the same guard, or moving the header declarations outside it.
Extended reasoning...
What the inconsistency is
Every other change in this PR is guarded by #if USE(BUN_JSC_ADDITIONS), but the diff to PropertyDescriptor.cpp drops the static qualifier from createCustomSetterFunction (line 60) and createCustomGetterFunction (line 72) unconditionally, giving them external linkage in every build configuration. The matching header declarations added to PropertyDescriptor.h (lines 36–44), however, are wrapped in #if USE(BUN_JSC_ADDITIONS). So when USE(BUN_JSC_ADDITIONS) is off, the two definitions have external linkage but no prior declaration.
Why it would break an upstream-style Xcode build
JSC's Xcode configuration in Source/JavaScriptCore/Configurations/Base.xcconfig sets GCC_WARN_ABOUT_MISSING_PROTOTYPES = YES (line 84) and GCC_TREAT_WARNINGS_AS_ERRORS = YES (line 70). Clang's -Wmissing-prototypes fires on C++ free functions with external linkage that lack a prior declaration — the tree already carries IGNORE_WARNINGS_BEGIN("missing-prototypes") suppressions in e.g. SIMDUTF.cpp and Zydis.c for exactly this diagnostic. So an Xcode build of this fork with USE(BUN_JSC_ADDITIONS) disabled would error on both definitions.
Why it does not affect any build this repo actually runs
This is the Bun fork. Every documented and CI build path (build.ts, mac-release.bash, release.sh, windows-release.ps1, and the GitHub Actions matrix) builds via CMake with -DUSE_BUN_JSC_ADDITIONS=ON, so the header declarations are always visible. And Source/cmake/WebKitCompilerFlags.cmake does not enable -Wmissing-prototypes for C++, so even a hypothetical CMake build with the Bun flag off would compile cleanly — the only effect would be harmless external linkage on two functions. No shipped configuration breaks.
Step-by-step proof
- Build this tree with Xcode /
xcodebuildusing JSC's ownBase.xcconfig, without definingUSE_BUN_JSC_ADDITIONS. PropertyDescriptor.his included; the#if USE(BUN_JSC_ADDITIONS)block is skipped, so no prototype forcreateCustomGetterFunction/createCustomSetterFunctionis seen.PropertyDescriptor.cppis compiled. At line 60,JSCustomSetterFunction* createCustomSetterFunction(...)has external linkage (nostatic) and no prior declaration in scope.- Clang emits
-Wmissing-prototypes("no previous prototype for function 'createCustomSetterFunction'"); withGCC_TREAT_WARNINGS_AS_ERRORS = YESthis is a hard error. Same at line 72.
Fix
Either guard the linkage change so non-Bun builds keep internal linkage:
#if !USE(BUN_JSC_ADDITIONS)
static
#endif
JSCustomSetterFunction* createCustomSetterFunction(...)or (simpler) move the two declarations in PropertyDescriptor.h outside the #if USE(BUN_JSC_ADDITIONS) guard so a prototype is always visible. Either restores consistency with the rest of the PR's careful guarding.
dylan-conway
changed the title
2 participants
Give JSC realms a set of primordials: per-global references to the original ECMAScript built-ins that user code cannot replace, delete, or shadow. Builtin JavaScript reaches them as
@-prefixed link-time constants named after Node.js'sprimordials, so runtime-internal code keeps working afterArray.prototype.push = evilordelete globalThis.Math.@-name resolution emits a link-time constant load and.@callcompiles to a bareop_callwith no property lookup, so a call costs the same as an unpolluted direct call.How a primordial gets its pristine value
Every primordial is a lazy link-time constant slot on the
JSGlobalObject. The slot is filled the first time a CodeBlock links against it, from one of three untamperable sources:JSGlobalObject::init()for eager holders, and inside the lazy initializer (LazyClassStructure/LazyProperty/ property callback) for holders likeDate,Map, typed arrays, orMath. This is a key lookup,getDirect, and a pointer store per entry — no reification, allocation, or structure transition on the holder — and it always runs before user code can reach the object.ClassInfostatic hash tables, which user code cannot mutate. A property still unlinked at first use is looked up there; if the object still holds the untouched entry it is reified once so the primordial and the visible property are the same function, otherwise a fresh, un-installed copy is built from the immutable table.Selfentries (e.g.@ArrayPrototype), reached through JSC's own accessors rather than the mutable global binding.A key present in neither an own property nor a table is a builtin this configuration does not ship (runtime-flagged); its primordial is a function that throws a clear
TypeErrorwhen called, so an absent feature is a defined error rather than a crash.overridePrimordialsFromHolder()lets an embedder that replaces a builtin during its own global setup make the primordial track its replacement. Identity is realm-checked: a foreign realm's builtin is never adopted even if installed before first link.The manifest
Which primordials exist is a generated table (
runtime/JSCPrimordialsTable.h), not a hand-written list: 647 entries across 99 holders, eachV(name, key, kind)withkind∈Method | Getter | Setter | Value | Self. It is produced by the paired Bun PR's generator, which applies Node.js'sprimordialsconstruction to this engine's actual built-ins, so the manifest tracks the engine's surface automatically. The header also drives theLinkTimeConstantenum, private names, and the intrinsic registry.JSGlobalObject::auditPrimordials()returns a per-entry manifest (name, holder, kind, value, availability) for tests. Everything is under#if USE(BUN_JSC_ADDITIONS).Paired Bun PR: oven-sh/bun#35567