Skip to content

text-decoder: snapshot shared buffers before decoding - #31643

Closed
EffortlessSteven wants to merge 1 commit into
oven-sh:mainfrom
EffortlessSteven:claude/textdecoder-shared-buffer-stable-bytes
Closed

text-decoder: snapshot shared buffers before decoding#31643
EffortlessSteven wants to merge 1 commit into
oven-sh:mainfrom
EffortlessSteven:claude/textdecoder-shared-buffer-stable-bytes

Conversation

@EffortlessSteven

Copy link
Copy Markdown
Contributor

Summary

TextDecoder.decode() accepted SharedArrayBuffer-backed inputs and decoded them through a &[u8] built from JS backing storage. Another JS agent can mutate that shared memory while Rust holds the slice, which is undefined behavior under Rust's aliasing rules.

This keeps fixed unshared inputs on the borrowed fast path, and snapshots shared or resizable inputs into a fresh non-shared buffer before decoding.

Changes:

  • snapshot shared/resizable BufferSource inputs before decoding
  • keep fixed unshared inputs zero-copy
  • disable the TextDecoder.decode DOMJIT fast path so SAB-backed typed arrays use the safe path
  • add SharedArrayBuffer-backed UTF-8 / UTF-16 decode coverage and a worker-mutation regression

Test approach

The regression uses a worker mutating SharedArrayBuffer-backed input while TextDecoder.decode() runs. The unpatched debug build crashes in the UTF-8 materializer; the patched build exits cleanly.

Verification

  • cargo fmt --all -- --check
  • git diff --check -- src/runtime/webcore/TextDecoder.rs src/runtime/webcore/encoding.classes.ts test/js/web/encoding/text-decoder.test.js
  • bun bd test test/js/web/encoding/text-decoder.test.js -t "SharedArrayBuffer input"
  • bun bd test test/js/deno/encoding/encoding.test.ts
  • bun bd test/js/node/test/parallel/test-whatwg-encoding-custom-api-basics.js
  • bun bd test/js/node/test/parallel/test-whatwg-encoding-custom-textdecoder-streaming.js
  • Regression confirmed: unpatched debug build crashes on the SharedArrayBuffer decode witness; patched build exits cleanly

Review map

  • src/runtime/webcore/TextDecoder.rs: snapshot shared/resizable BufferSource input before creating the decode slice
  • src/runtime/webcore/encoding.classes.ts: remove the DOMJIT fast path so SAB-backed typed arrays cannot bypass the snapshot path
  • test/js/web/encoding/text-decoder.test.js: cover SAB-backed UTF-8 / UTF-16, offset, zero-length, growable/resizable inputs, and concurrent mutation

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8cb58495-8816-40c1-a655-6f71790e04d8

📥 Commits

Reviewing files that changed from the base of the PR and between 2629789 and 65f8404.

📒 Files selected for processing (3)
  • src/runtime/webcore/TextDecoder.rs
  • src/runtime/webcore/encoding.classes.ts
  • test/js/web/encoding/text-decoder.test.js
💤 Files with no reviewable changes (1)
  • src/runtime/webcore/encoding.classes.ts

Walkthrough

TextDecoder.decode now snapshots shared, growable, and resizable ArrayBuffer inputs into fresh non-shared ArrayBuffers before decoding, preventing unsafe memory borrowing during concurrent mutations or buffer resizing. The fast-path decode_without_type_checks method is removed, and type system metadata is updated accordingly. Test coverage validates both deterministic and concurrent access patterns.

Changes

TextDecoder SharedArrayBuffer Safety

Layer / File(s) Summary
Safe buffer snapshotting and decode logic
src/runtime/webcore/TextDecoder.rs
TextDecoder.decode now checks if the input ArrayBuffer is shared, resizable, or growable; if so, it snapshots the buffer into a fresh non-shared ArrayBuffer via snapshot_shared_array_buffer helper and extern binding to Bun__createArrayBufferForCopy in C++. The JSUint8Array import is removed with the decode_without_type_checks fast path.
DOMJIT metadata removal
src/runtime/webcore/encoding.classes.ts
The decode method's DOMJIT metadata block specifying JSString return type and JSUint8Array argument type is removed, reflecting the removal of the no-options fast path and changed input handling.
SharedArrayBuffer test coverage
test/js/web/encoding/text-decoder.test.js
Test suite verifies TextDecoder.decode handles SharedArrayBuffer inputs, typed-array views with offsets/lengths, UTF-16LE/UTF-16BE variants, zero-length buffers, and growable/resizable buffers. Concurrent mutation test spawns a Worker that continuously mutates shared bytes while main thread decodes, validating memory safety under concurrent access.

Possibly related PRs

  • oven-sh/bun#31438: Modifies the same decode_without_type_checks fast path and decode control flow in src/runtime/webcore/TextDecoder.rs.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and specifically describes the main change: snapshotting shared buffers in TextDecoder before decoding, which is the core fix for the undefined behavior issue.
Description check ✅ Passed The description comprehensively covers the template requirements with a clear summary, detailed changes, test approach, verification steps, and a helpful review map.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@EffortlessSteven
EffortlessSteven force-pushed the claude/textdecoder-shared-buffer-stable-bytes branch from 65f8404 to 7f4bcd3 Compare June 5, 2026 11:45
@robobun

robobun commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this. The same fix landed on main in #33072 (merged July 4): TextDecoder.decode() now copies the input into an owned buffer first whenever the backing store is a SharedArrayBuffer or a resizable ArrayBuffer, and keeps fixed unshared inputs on the zero-copy path, which is the behavior this PR implements. That change also added a worker-mutation test to test/js/web/encoding/text-decoder.test.js ("decodes a stable snapshot of a Uint8Array over a SharedArrayBuffer while another thread writes to it"). The DOMJIT entry in encoding.classes.ts has been inert since #23169, since class-definitions.ts clears DOMJIT on every definition, so there was no fast path left to disable.

I ran the nine "SharedArrayBuffer input" tests from this branch against current main (04148c8, debug build) and they all pass, including the concurrent worker mutation case across several runs.

Closing since main already has this behavior. If you run into a remaining case on a current canary, please open an issue and we will take a look.

@robobun robobun closed this Aug 13, 2026
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.

2 participants