Skip to content

Add DFG/FTL nodes for byte-offset scalar accessors on ArrayBufferView receivers - #330

Open
Jarred-Sumner wants to merge 12 commits into
mainfrom
claude/buffer-jit
Open

Add DFG/FTL nodes for byte-offset scalar accessors on ArrayBufferView receivers#330
Jarred-Sumner wants to merge 12 commits into
mainfrom
claude/buffer-jit

Conversation

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

Buffer accessor nodes (BufferReadInt / BufferReadFloat / BufferWrite)

Adds a DFG/FTL intrinsic for host functions that read/write a fixed-width scalar on an
ArrayBufferView receiver at a byte offset — Node.js's Buffer.prototype.readInt32LE,
writeDoubleBE, etc. (Bun registers its native implementations of those with the new
BufferAccessorRegistry.) Everything is under USE(BUN_JSC_ADDITIONS).

Piece What it is
runtime/BufferAccessorRegistry.{h,cpp} process-global map: native function pointer → { DataViewData, isWrite }
BufferAccessorIntrinsic (Intrinsic.h) one intrinsic for the family; the descriptor comes from the registry at parse time
dfg/DFGDataViewData.h DataViewData moved out of DFGNode.h so the registry can use it
BufferReadInt / BufferReadFloat / BufferWrite vararg nodes shaped like typed-array GetByVal/PutByVal: opInfo1 = ArrayMode (forced Uint8Array), opInfo2 = DataViewData

How the nodes flow through the pipeline (each step reuses existing machinery):

  • ByteCodeParser: read*(offset = 0) / write*(value, offset = 0) call sites on a registered function become the node; bails when the site has BadType/BadIndexingType/OutOfBounds/Overflow exit sites (so a persistently-exiting site keeps the plain call).
  • Fixup: forces ArrayMode(Uint8Array) and calls blessArrayOperation() → the receiver check is a CheckArray, the storage is a GetIndexedPropertyStorage; offset Int32Use (with the same DoubleAsInt32 conversion GetByVal does); write values Int32 / Int52 (uint32) / Double / HeapBigInt.
  • SSA lowering (FTL): GetArrayLength + CheckInBounds(offset) + CheckInBounds(offset + byteSize - 1) appended as untyped children, same as lowerBoundsCheck() — so type check, storage, length and bounds are all separately CSE-able / hoistable / IntegerRange-eliminable.
  • Backends: DFG tier does the length load + two-sided bounds check inline (the compileDataViewGet/Set sequence); FTL loads/stores through the already-checked pointer. Endianness is a per-accessor constant (byte swap, no branch). Writes also range-check the value the way Node requires (narrow ints, uint32, "fits in 64 bits" for BigInt) — those are OSR exits, not throws.
  • Anything not speculated (wrong receiver, non-int32 / out-of-bounds offset, out-of-range value) exits to the host function, which owns the error semantics.

Tests: JSTests/stress/buffer-accessor-jit{,-exits,-resizable,-bigint-write}.js via a $vm.createBufferAccessors() hook; they pass in default, no-cjit, ftl-eager, no-ftl and validateGraph configurations. The existing DataView / Atomics / typed-array stress tests are unaffected.

… receivers

Introduces the BufferReadInt / BufferReadFloat / BufferWrite nodes (all under
USE(BUN_JSC_ADDITIONS)) plus a small BufferAccessorRegistry: an embedder
registers host functions that read or write a fixed-width scalar at a byte
offset on an ArrayBufferView receiver (Node.js's Buffer.prototype.readInt32LE,
writeDoubleBE, ...) and the DFG turns call sites of those functions into
bounds-checked loads and stores on the receiver's storage.

The nodes are shaped like typed-array GetByVal / PutByVal: Fixup forces a
Uint8Array ArrayMode and blesses the array operation (CheckArray +
GetIndexedPropertyStorage), and SSA lowering appends GetArrayLength plus a
CheckInBounds for the first and last byte, so the receiver check, the storage
pointer, the length load and the bounds checks are all ordinary CSE-able /
hoistable / IntegerRange-eliminable nodes. The DFG tier does the length load
and bounds check inline like DataViewGet/Set. Everything the nodes do not
speculate (other receivers, non-int32 or out-of-bounds offsets, out-of-range
write values, oversized BigInts) OSR-exits back to the host function, which
stays the single owner of the error semantics.

DataViewData moves into dfg/DFGDataViewData.h so the registry header can
carry the access descriptor without pulling in DFGNode.h.

Tests: JSTests/stress/buffer-accessor-jit*.js via a $vm.createBufferAccessors()
hook in JSDollarVM.
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Changes

Buffer accessor host functions, metadata registration, DFG and FTL compilation support, and stress tests were added for typed integer, floating-point, and BigInt Buffer reads and writes. Coverage includes bounds, coercion, exits, detached buffers, and resizable or growable backing stores.

Changes

Buffer accessor JIT support

Layer / File(s) Summary
Accessor runtime and metadata
Source/JavaScriptCore/runtime/..., Source/JavaScriptCore/tools/JSDollarVM.cpp, Source/JavaScriptCore/dfg/DFGDataViewData.h, Source/JavaScriptCore/runtime/Intrinsic.h, build lists
The Dollar VM creates fixed- and variable-width accessors, registers packed descriptors, and exposes the intrinsic and build entries.
DFG buffer nodes and analysis
Source/JavaScriptCore/dfg/...
Dedicated buffer read/write nodes are added to parsing, node classification, prediction, clobberization, fixup, GC, cloning, and execution analysis.
DFG bounds checks and JIT code generation
Source/JavaScriptCore/dfg/...
DFG lowering and speculative JIT compilation add receiver, offset, storage, bounds, endian, numeric, and BigInt handling.
FTL buffer lowering
Source/JavaScriptCore/ftl/...
FTL accepts and lowers buffer nodes into endian-aware typed loads and stores with range checks and BigInt conversions.
Stress validation
JSTests/stress/buffer-accessor-jit*.js
Stress tests validate reads, writes, variable widths, BigInt values, coercion, exits, detached buffers, and resizable or growable buffers.

Possibly related PRs

  • oven-sh/WebKit#319: Shares the DataViewData metadata and its DFG integration for typed-memory operations.

Suggested reviewers: webkit-commit-queue, constellation, justinmichaud

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description summarizes the change, but it does not follow the required template or include the Bugzilla link, reviewed-by line, or commit-message sections. Reformat it to the repository template, adding the Bugzilla bug link, Reviewed by line, explanation of why it fixes the bug, and the changed-file bullets.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: new DFG/FTL buffer accessor nodes for byte-offset scalar accessors.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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 `@JSTests/stress/buffer-accessor-jit-resizable.js`:
- Around line 60-66: Replace the self-comparison in the shrink loop’s “read near
the shrunk end” assertion with an explicit expected value of 0, while preserving
the existing offset 6 read and RangeError checks for offsets 7 and 62.

In `@Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp`:
- Around line 5123-5124: Update the isResizable assignment in the affected
DataView accessor to use the exit-site check first, then fall back to
getArrayMode(Array::Read).mayBeResizableOrGrowableSharedTypedArray() when no
UnexpectedResizableArrayBufferView exit exists, matching the sibling DataView
get/set cases.

In `@Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp`:
- Around line 1989-1995: Guard the BufferReadInt, BufferReadFloat, and
BufferWrite switch cases with `#if` USE(BUN_JSC_ADDITIONS), and similarly guard
the compileBufferRead() and compileBufferWrite() method definitions plus
Node::bufferAccessData() with the same conditional. Keep these Bun-only dispatch
and implementation paths unchanged when the feature is enabled while excluding
them from non-Bun builds.
🪄 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: 861241cc-1c43-470f-a89a-4e2723f17809

📥 Commits

Reviewing files that changed from the base of the PR and between 81ea857 and fb957cf.

📒 Files selected for processing (27)
  • JSTests/stress/buffer-accessor-jit-bigint-write.js
  • JSTests/stress/buffer-accessor-jit-exits.js
  • JSTests/stress/buffer-accessor-jit-resizable.js
  • JSTests/stress/buffer-accessor-jit.js
  • Source/JavaScriptCore/CMakeLists.txt
  • Source/JavaScriptCore/Sources.txt
  • Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
  • Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
  • Source/JavaScriptCore/dfg/DFGClobberize.h
  • Source/JavaScriptCore/dfg/DFGCloneHelper.h
  • Source/JavaScriptCore/dfg/DFGDataViewData.h
  • Source/JavaScriptCore/dfg/DFGDoesGC.cpp
  • Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
  • Source/JavaScriptCore/dfg/DFGNode.h
  • Source/JavaScriptCore/dfg/DFGNodeType.h
  • Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
  • Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp
  • Source/JavaScriptCore/dfg/DFGSafeToExecute.h
  • Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
  • Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
  • Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
  • Source/JavaScriptCore/ftl/FTLCapabilities.cpp
  • Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
  • Source/JavaScriptCore/runtime/BufferAccessorRegistry.cpp
  • Source/JavaScriptCore/runtime/BufferAccessorRegistry.h
  • Source/JavaScriptCore/runtime/Intrinsic.h
  • Source/JavaScriptCore/tools/JSDollarVM.cpp

Comment thread JSTests/stress/buffer-accessor-jit-resizable.js
Comment thread Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp Outdated
Comment thread Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/dfg/DFGFixupPhase.cpp`:
- Around line 3753-3775: Move fixEdge<KnownCellUse>(base) and
fixEdge<Int32Use>(offset) out of the non-forceExit branch and execute them after
the forceExit conditional, while preserving the existing ForceExit setup and
normal Uint8Array handling. Ensure both BufferReadInt and BufferWrite edges are
fixed on every path, including when blessArrayOperation() inserts ForceOSRExit.
🪄 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: 3f90886d-e19c-4b6e-be98-7f27706666ec

📥 Commits

Reviewing files that changed from the base of the PR and between fb957cf and abf1dc3.

📒 Files selected for processing (1)
  • Source/JavaScriptCore/dfg/DFGFixupPhase.cpp

Comment thread Source/JavaScriptCore/dfg/DFGFixupPhase.cpp

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any bugs, but this adds three new DFG/FTL node types with ~600 lines of hand-written JIT codegen across both backends — bounds checking, speculation, and OSR-exit paths in a JIT compiler need expert human review.

What was reviewed:

  • Bounds-check shape in both tiers (DFG inline: negative + offset+byteSize-1 < length; FTL via two CheckInBounds children in SSA lowering) — both cover the last byte.
  • BigInt 64-bit range check (length ≤ 1 digit + sign-match on wrapped value) matches the host reference in JSDollarVM.
  • Clobberize/DoesGC/PredictionPropagation/AbstractInterpreter/SafeToExecute entries are consistent with the DataView-node precedents.
  • ForceExit path leaves base/offset untyped (matches GetByVal); AI marks state invalid so the node isn't compiled.
Extended reasoning...

Overview

This PR introduces three new DFG node types (BufferReadInt, BufferReadFloat, BufferWrite) plus a BufferAccessorIntrinsic and a process-global BufferAccessorRegistry, wired through the full DFG/FTL pipeline: bytecode parser, fixup, prediction propagation, abstract interpreter, clobberize, DoesGC, SSA lowering, safe-to-execute, clone helper, and both the DFG SpeculativeJIT64 and FTL LowerDFGToB3 backends. It also adds a ~260-line $vm.createBufferAccessors() test hook and four stress tests. 27 files changed.

Security risks

JIT compilers are among the most security-sensitive code in the engine. This change performs raw loads/stores through a caged storage pointer at a JS-controlled byte offset, guarded by speculation checks. Any gap between the speculation (CheckArray, Int32Use offset, CheckInBounds) and the actual load/store — or any way for the node to be hoisted/CSE'd past its guards — is an out-of-bounds read/write primitive. The design mirrors existing DataViewGet/Set and typed-array GetByVal/PutByVal closely, and the FTL path appends the CheckInBounds nodes as untyped children so LICM cannot separate them, which is the standard pattern. But correctness here depends on subtle interactions across many phases (e.g. SafeToExecute returning true for reads relies on edgesDominate gating LICM; the resizable-length path emits ExitOK after GetArrayLength; the ForceExit path relies on AI invalidating state so backends never compile the node). These need eyes from someone who owns this pipeline.

Level of scrutiny

High. New node types in the DFG/FTL are structural changes with correctness obligations spread across ~15 phase files, and the codegen does direct memory access. This is the opposite of a mechanical change.

Other factors

The PR is well-tested (four stress tests covering happy path, exits, resizable/growable-shared, BigInt writes) and well-documented. All CodeRabbit findings were either addressed in 4fa7b25 or withdrawn as incorrect after author explanation, and the bug-hunting system found nothing. That raises confidence but does not substitute for a human JSC reviewer on a change of this scope.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Source/JavaScriptCore/tools/JSDollarVM.cpp (1)

4773-4777: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Handle NaN before the int64_t cast.

numberValue < min || numberValue > max does not reject NaN, so static_cast<int64_t>(std::trunc(numberValue)) can hit undefined behavior. Convert NaN to 0 first here, matching the fixed-width accessor path.

🤖 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/tools/JSDollarVM.cpp` around lines 4773 - 4777, Update
the buffer accessor conversion near the range check to normalize NaN numberValue
to 0 before calling std::trunc and casting to int64_t. Preserve the existing
range validation and byte-writing behavior for non-NaN values, matching the
fixed-width accessor path.
🤖 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/tools/JSDollarVM.cpp`:
- Around line 4773-4777: Update the buffer accessor conversion near the range
check to normalize NaN numberValue to 0 before calling std::trunc and casting to
int64_t. Preserve the existing range validation and byte-writing behavior for
non-NaN values, matching the fixed-width accessor path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 988fd86c-0b1a-4638-b62a-111332797734

📥 Commits

Reviewing files that changed from the base of the PR and between f2c0e3b and be77ad7.

📒 Files selected for processing (1)
  • Source/JavaScriptCore/tools/JSDollarVM.cpp

Comment thread Source/JavaScriptCore/tools/JSDollarVM.cpp Outdated
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

Preview Builds

Commit Release Date
8debd979 autobuild-preview-pr-330-8debd979 2026-07-25 09:37:33 UTC
93cf9dc7 autobuild-preview-pr-330-93cf9dc7 2026-07-25 04:27:25 UTC
cee81fb3 autobuild-preview-pr-330-cee81fb3 2026-07-24 09:22:57 UTC
6d8df126 autobuild-preview-pr-330-6d8df126 2026-07-24 06:42:33 UTC
be77ad70 autobuild-preview-pr-330-be77ad70 2026-07-24 03:55:45 UTC

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
Source/JavaScriptCore/tools/JSDollarVM.cpp (2)

4773-4780: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Handle NaN before static_cast<int64_t> in the variable-width write path.
NaN bypasses the range check, then std::trunc(numberValue) still leaves NaN, so the integer cast is undefined behavior. The fixed-width path already normalizes NaN to 0; this path should do the same or reject it explicitly.

🤖 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/tools/JSDollarVM.cpp` around lines 4773 - 4780, Update
the variable-width write path guarded by isWrite before the static_cast<int64_t>
conversion to explicitly handle NaN, matching the fixed-width path by
normalizing it to 0 or rejecting it with the appropriate range error. Ensure the
subsequent truncation and integer cast only receive a non-NaN value.

4587-4597: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Clamp buffer accessor offsets to int32. The DFG path treats this offset as Int32, so the host fallback can accept larger integral values on large views and diverge from inlined call sites. Add the INT32_MAX cap to keep both paths aligned.

🤖 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/tools/JSDollarVM.cpp` around lines 4587 - 4597, Update
the buffer accessor offset validation in the shown offset handling path to
reject integral offsetNumber values greater than INT32_MAX, alongside the
existing floor, nonnegative, and byteLength bounds checks. Keep valid in-range
Int32 offsets unchanged so the host fallback matches the DFG path.
🤖 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/tools/JSDollarVM.cpp`:
- Around line 4773-4780: Update the variable-width write path guarded by isWrite
before the static_cast<int64_t> conversion to explicitly handle NaN, matching
the fixed-width path by normalizing it to 0 or rejecting it with the appropriate
range error. Ensure the subsequent truncation and integer cast only receive a
non-NaN value.
- Around line 4587-4597: Update the buffer accessor offset validation in the
shown offset handling path to reject integral offsetNumber values greater than
INT32_MAX, alongside the existing floor, nonnegative, and byteLength bounds
checks. Keep valid in-range Int32 offsets unchanged so the host fallback matches
the DFG path.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 3063ce5d-8c5c-496d-880a-f467aafc2a81

📥 Commits

Reviewing files that changed from the base of the PR and between be77ad7 and 2a16ffb.

📒 Files selected for processing (1)
  • Source/JavaScriptCore/tools/JSDollarVM.cpp

Comment thread Source/JavaScriptCore/tools/JSDollarVM.cpp Outdated
Move the 1- and 2-byte write value range checks out of the backends and
into SSA lowering as a (sign-biased) CheckInBounds, so integer range
analysis / LICM can hoist or eliminate them; the DFG tier keeps its
inline checks since SSA lowering only runs in the FTL pipeline.

BufferWrite no longer has a result. The parser emits Node's
`offset + byteSize` return value as an ordinary ArithAdd before the
store, which DCE removes when the caller ignores it and which stays a
checked int32 add when it is used (loop-carried offsets included).
The reference reader boxed raw loaded bytes, which asserts in jsNumber() for
an impure NaN bit pattern (and encodes a NaN-boxing-colliding JSValue in
release). The variable-width writer let NaN past its range guard and into an
out-of-range float-to-int conversion. Both paths are now covered by the
stress tests.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new issues found this pass, but this adds ~600 lines of hand-written DFG/FTL codegen for bounds-checked typed-array memory access across every compiler phase — JIT-security-sensitive enough that it needs eyes from someone familiar with the DFG pipeline.

What was reviewed:

  • DFG64 and FTL compileBufferRead/Write bounds sequences — both do the offset >= 0 + offset + byteSize - 1 < length pair, matching the DataView pattern.
  • SSA lowering's CheckInBounds pair and value-range CheckInBounds for narrow-int writes — offsets/ranges look consistent with the inline DFG checks.
  • The two earlier JSDollarVM.cpp findings (impure-NaN float reads, NaN falling through the var-width range guard) — both fixed in a4ecdd6 and now covered by tests.
  • clobberize / doesGC / safeToExecute / PredictionPropagation entries — consistent with the sibling DataView node handling.
Extended reasoning...

Overview

This PR adds three new DFG/FTL node types — BufferReadInt, BufferReadFloat, BufferWrite — that let the JIT inline Node.js Buffer.prototype.read*/write* host functions as direct typed-array loads/stores. It touches 28 files: a new process-global BufferAccessorRegistry, a new BufferAccessorIntrinsic, bytecode-parser hookup, and per-phase handling in Fixup, PredictionPropagation, AbstractInterpreter, Clobberize, DoesGC, SafeToExecute, SSALowering, CloneHelper, plus ~300 lines of SpeculativeJIT64 codegen and ~220 lines of FTL B3 lowering. A ~330-line $vm.createBufferAccessors() reference implementation and five stress tests round it out.

Security risks

This is JIT compiler code that emits machine instructions doing bounds-checked reads/writes into ArrayBufferView backing storage. A soundness bug in the bounds check (off-by-one, wrong length source, missing resizable-view refresh, speculation that isn't backed by a check) is an out-of-bounds read/write primitive. The two-sided bounds check, resizable-view handling (loadTypedArrayLength vs. cached length + UnexpectedResizableArrayBufferView speculation), BigInt digit-count/sign checks, and ensureStillAliveHere(base) liveness fence all look correctly patterned after the existing DataView/typed-array paths, but this class of code historically produces CVEs and deserves an expert second pair of eyes rather than a bot sign-off.

Level of scrutiny

High. This is not a mechanical or pattern-following change — it's a new node family with bespoke codegen in two backends, custom SSA-lowering that appends multiple CheckInBounds children, and OSR-exit-based error semantics whose correctness depends on the interaction between speculation checks and the host-function fallback. Even with the feature guarded by USE(BUN_JSC_ADDITIONS), it will ship in production Bun and back the Buffer prototype.

Other factors

All prior review threads (CodeRabbit's four, my two) are resolved, and the author's responses were substantive. Test coverage is good — five stress files exercise every width/endianness/sign combination, resizable/growable-shared views, detach, OSR exits, BigInt range, and the impure-NaN case. The multi-agent bug hunt found nothing new on the current revision. None of that changes the calculus: the size and security surface put this well outside what should be approved without human review.

Under USE(LARGE_TYPED_ARRAYS) the receiver can exceed 2GB, so lower the
length load and bounds checks to GetTypedArrayLengthAsInt52 /
CheckInBoundsInt52 the way lowerBoundsCheck() does for typed array element
accesses. The Int32 GetArrayLength speculation Overflow-exited on every FTL
execution for such receivers, and with Overflow in the parser's exit-site
gate the call site then stopped inlining for good. Overflow is dropped from
that gate as well, and a stress test covers a 3GB receiver end to end.
Comment thread Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Add a seeded JIT-vs-reference differential fuzzer: per-accessor generated
invokers (the reference arm's source must differ from the JIT arm's or
noDFG on the shared executable poisons both), clean-input rounds that
keep the site optimized and compare bytes/results on the fast path, and
dirty-input rounds for error parity, over fixed, subarray, resizable and
growable receivers.

Put Overflow back into the parser's exit-site gate. Removing it let the
remaining Overflow exits (double offsets >= 2^31, the lastByteOffset int32
add near 2^31, DFG-tier value checks) exit and recompile indefinitely;
the Int52 length path already keeps large receivers optimized on its own
since the length load can no longer produce an Overflow exit.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new issues found on this pass — the Overflow exit-site gate is now in place (8debd97). That said, this adds three new DFG/FTL node types with hand-written bounds-check lowering across SpeculativeJIT64, FTLLowerDFGToB3 and SSALowering, so a human pass on the JIT paths is still warranted.

What was reviewed:

  • Bounds-check shape in both tiers: negative-offset guard + zero-extended offset + byteSize - 1 vs. 64-bit length; FTL's separate CheckInBounds on first/last byte with Int52 length under LARGE_TYPED_ARRAYS.
  • Value-range checks (int8/16 min-max, uint32 Int52 upper bound, BigInt length/sign) and that all exit kinds now have a bytecode-parser back-off.
  • ensureStillAliveHere(base) after the FTL loads/stores; Fixup's ForceExit path leaving base/offset untyped while still setting the result flags.
  • $vm reference-implementation NaN handling (purifyNaN on float reads, negated range guard on var-width writes) — both prior findings confirmed fixed.
Extended reasoning...

Overview

This PR introduces three new DFG node types — BufferReadInt, BufferReadFloat, BufferWrite — plus a BufferAccessorIntrinsic and a process-global BufferAccessorRegistry so Bun can JIT-compile Node.js-style Buffer.prototype.read*/write* host functions. It touches ~31 files across the full DFG/FTL pipeline: bytecode parsing, prediction propagation, fixup, abstract interpreter, clobberize, doesGC, safeToExecute, clone helper, SSA lowering, both DFG SpeculativeJIT backends, FTL capabilities, and FTL lowering. Roughly ~300 lines of new SpeculativeJIT64 codegen and ~220 lines of new B3 lowering, plus ~330 lines of $vm reference implementation and eight new stress tests.

Security risks

JIT bounds-check emission on typed-array storage is squarely memory-safety-critical. The checks look sound (negative-offset guard, zero-extend before the 64-bit add, unsigned compare against the 64-bit length; FTL relies on graph-level CheckInBounds/CheckInBoundsInt52 inserted at SSA lowering with the check nodes threaded as vararg children so B3 can't reorder the store above them), and ensureStillAliveHere(base) keeps the receiver live across the load/store. Gigacage caging comes from the shared GetIndexedPropertyStorage path. Resizable/growable-shared handling is delegated to the existing loadTypedArrayLength / GetTypedArrayLengthAsInt52 machinery. I did not find a hole, but a mistake here would be an out-of-bounds read/write from JIT'd code, which is why this shouldn't be auto-approved.

Level of scrutiny

High. New node types must be correctly wired into every DFG phase (missing a case in one switch is a classic source of miscompiles), and the two backend implementations independently reimplement bounds/range checks that must agree with each other and with the host function they OSR-exit to. This is Bun-gated (USE(BUN_JSC_ADDITIONS)) so upstream WebKit is unaffected, but it's production Bun code once merged.

Other factors

All prior automated findings on this PR (impure-NaN boxing in the $vm float reads, NaN fall-through in the var-width writer, missing Overflow exit-site back-off) have been addressed by the author in follow-up commits, and CodeRabbit's threads are resolved. The stress-test coverage is unusually thorough (differential fuzzer, resizable/detached, >2GB and ~4GB byteOffset views, BigInt range, exit-site behavior). The bug-hunting system found nothing on this revision. Deferring rather than approving purely on scope and blast radius.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant