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 8 commits into
Code review found 1 important issue
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1995-2006 |
Error primordial initLater overwrites already-captured slots when holder is forced early |
| 🟡 Nit | Source/JavaScriptCore/runtime/JSCPrimordials.cpp:42-52 |
primordialMethod RELEASE_ASSERTs on feature-gated properties (Atomics.waitAsync, JSON.rawJSON/isRawJSON) |
| 🟡 Nit | Source/JavaScriptCore/runtime/JSCPrimordials.h:26-28 |
JSCPrimordials.h uses uint8_t without including |
Annotations
Check failure on line 2006 in Source/JavaScriptCore/runtime/JSGlobalObject.cpp
claude / Claude Code Review
Error primordial initLater overwrites already-captured slots when holder is forced early
This unconditionally calls `.initLater()` on the Error* link-time-constant slots, but `m_errorStructure` can already have been forced earlier in `init()` (line 1633 forces it via `SuppressedError` when `Options::useExplicitResourceManagement()` is on), which already `.set()` those slots via `capturePrimordials()`. `initLater()` clobbers the stored cell with `lazyTag|&theFunc`; when a builtin later links `@ErrorCaptureStackTrace`, the fallback lambda's `errorConstructor()` returns immediately (al
Check warning on line 52 in Source/JavaScriptCore/runtime/JSCPrimordials.cpp
claude / Claude Code Review
primordialMethod RELEASE_ASSERTs on feature-gated properties (Atomics.waitAsync, JSON.rawJSON/isRawJSON)
`primordialMethod()` has no soft-fail path — if a listed key is absent from the holder it `RELEASE_ASSERT`s — but several table entries are feature-gated on the holder side: `Atomics.waitAsync` is only installed when `vm.vmType == VMType::Default` (AtomicsObject.cpp:95-96), and `JSON.isRawJSON`/`rawJSON` only when `Options::useJSONSourceTextAccess()` (JSONObject.cpp:81-84). On a C-API-created VM (`JSContextGroupCreate` / `JSGlobalContextCreate` → `VMType::APIContextGroup`), the first read of `gl
Check warning on line 28 in Source/JavaScriptCore/runtime/JSCPrimordials.h
claude / Claude Code Review
JSCPrimordials.h uses uint8_t without including <cstdint>
nit: this header uses `uint8_t` (in `enum class PrimordialHolder : uint8_t`) but has no `#include <cstdint>`. Comparable enum-only headers in this directory (`ConstructorKind.h`, `ImplementationVisibility.h`, `InlineAttribute.h`) all include it explicitly. It compiles fine today via the prefix header, so this is purely header-hygiene.