blob: snapshot shared buffers before materializing bytes - #31648
Open
EffortlessSteven wants to merge 1 commit into
Open
blob: snapshot shared buffers before materializing bytes#31648EffortlessSteven wants to merge 1 commit into
EffortlessSteven wants to merge 1 commit into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesSharedArrayBuffer snapshotting in Blob
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@test/js/bun/s3/s3.test.ts`:
- Around line 1805-1828: Change the test suite declaration for
"SharedArrayBuffer upload (stable bytes)" to run concurrently by using the
concurrent variant of describe (e.g., replace describe(...) with
describe.concurrent(...)); update the block that defines fakeS3 (and any tests
inside that describe) to ensure they still create isolated servers (using port:
0) and rely on the per-test returned server/options/received so tests remain
independent when run in parallel.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0cfa05dc-8eac-4d78-992d-37ef17331035
📒 Files selected for processing (3)
src/runtime/webcore/Blob.rstest/js/bun/s3/s3.test.tstest/js/web/fetch/blob.test.ts
EffortlessSteven
force-pushed
the
claude/s3-write-stable-bytes
branch
from
June 1, 2026 05:30
50f6d5d to
5d5bf19
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 @.factory/review-prompts/README.md:
- Around line 22-24: The README statement claiming security review "lives here
only" is inaccurate because bun-ai-review-gh-hosted.yml already defines a
security lane; update the text in .factory/review-prompts/README.md to remove or
rephrase that claim and accurately reflect that security reviews run from both
.github/workflows/droid-tag.yml (on-demand `@droid` review/security/fill) and
bun-ai-review-gh-hosted.yml (the automated security lane), mentioning the actual
workflow names (`.github/workflows/droid-tag.yml` and
`bun-ai-review-gh-hosted.yml`) and the `security` lane so readers aren’t misled.
In @.factory/skills/review-guidelines/SKILL.md:
- Around line 106-118: The fenced example block shown in the SKILL.md diff is
unlabeled and triggers MD040; update that triple-backtick fence to include the
language label "text" (i.e., change ``` to ```text) so the block is explicitly
marked as illustrative output and stops triggering the MD040 lint rule; locate
the example block in SKILL.md (the one containing the "No blocking finding..."
and "Best failed objection" sections) and add the label accordingly.
In @.github/workflows/bun-ai-review-gh-hosted.yml:
- Around line 246-255: The workflow currently hard-fails when FIREPASS is
missing by exporting FIREPASS in env and running the check `test -n
"${FIREPASS:-}" || { echo "::error::Missing FIREPASS secret"; exit 1; }`; remove
the FIREPASS requirement by either deleting the FIREPASS env line and its `test
-n "${FIREPASS:-}"` check, or change that check to a non-fatal warning (e.g.,
echo a warning without `exit 1`) so missing FIREPASS no longer fails the job;
update the run block where `test -n "${FIREPASS:-}"` appears to implement the
chosen approach.
- Around line 236-242: Replace the tag-based checkout action by pinning
actions/checkout to a specific commit SHA (replace uses: actions/checkout@v6
with uses: actions/checkout@<commit-sha>) so the workflow uses an immutable
release; also address the FIREPASS handling by either making the secret optional
where used (remove or change the unconditional test -n "${FIREPASS:-}" and
conditionalize injection of "apiKey": "${FIREPASS}") or require and document the
secret for jobs with pull-requests: write permissions to avoid injecting an
empty/dormant secret into the generated config.
In @.github/workflows/droid-tag.yml:
- Around line 43-52: The workflow currently declares env secrets GLM, FIREPASS,
and MINIMAX and then requires all three with test -n checks, but FIREPASS and
MINIMAX are unused and block runs; remove the unused secret declarations and
their guards: delete the FIREPASS and MINIMAX entries under the env: block and
remove the corresponding test -n "${FIREPASS:-}" and test -n "${MINIMAX:-}"
checks so only GLM remains enforced; keep the GLM env and its test -n check
intact to preserve the required credential validation.
- Around line 35-39: Update the workflow to pin actions/checkout to a specific
commit SHA instead of the mutable tag `actions/checkout@v6` (replace the
reference in the Checkout step with the chosen commit SHA), and make the
BYOK/FIREPASS/MINIMAX secret usage conditional: change the BYOK config step (the
logic that writes configs for FirePass/MiniMax) so it only requires or writes
FIREPASS/MINIMAX when those models are selected, or mark those secrets optional,
and ensure `review_model` and `security_model` (currently set to
`custom:GLM-5.1-ZAI-Coding-0`) only trigger generation of their respective
configs; in short, pin the checkout action to a commit SHA and guard the BYOK
secret/config generation behind the selected model checks to avoid failing runs
when unused.
In @.github/workflows/ub-review-packet.yml:
- Around line 31-88: Replace the mutable action refs in the UB review packet
workflow with pinned commit SHAs: change actions/checkout@v5,
dtolnay/rust-toolchain@master, actions/cache@v4,
EffortlessMetrics/ub-review@main, and actions/upload-artifact@v7 to the exact
commit SHAs (or tag+sha) used in other workflows (e.g., how
.github/workflows/vscode-release.yml pins actions) so the workflow always uses
immutable commits; update the five `uses:` entries accordingly and verify the
pinned SHAs resolve and pass CI.
- Around line 27-29: The workflow currently uses mutable action refs (e.g.,
actions/checkout@v5, dtolnay/rust-toolchain@master, actions/cache@v4,
EffortlessMetrics/ub-review@main, actions/upload-artifact@v7) which should be
pinned to immutable commit SHAs for supply-chain safety; update each uses:
reference in the workflow to the corresponding full commit SHA (replace the
tag/branch after the @ with the exact commit hash), leaving job-level env keys
like CARGO_TARGET_DIR and CARGO_INSTALL_JOBS unchanged, and ensure all updated
uses refer to the intended pinned commits.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d0fb8f4-189e-4361-a3c6-bf581d66079e
📒 Files selected for processing (5)
.factory/review-prompts/README.md.factory/skills/review-guidelines/SKILL.md.github/workflows/bun-ai-review-gh-hosted.yml.github/workflows/droid-tag.yml.github/workflows/ub-review-packet.yml
EffortlessSteven
force-pushed
the
claude/s3-write-stable-bytes
branch
2 times, most recently
from
June 2, 2026 17:42
5d5bf19 to
00e594b
Compare
Contributor
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 `@src/runtime/webcore/Blob.rs`:
- Around line 3416-3426: The fast-paths that take a raw &[u8] from JS backing
memory must snapshot shared/resizable ArrayBuffers before creating Rust borrows;
update the small-buffer file-write fast path inside write_file_internal so that
instead of passing buffer_view.byte_slice() directly into
write_bytes_to_file_fast you call snapshot_shared_array_buffer(global,
&array_buffer) when array_buffer.shared || array_buffer.resizable and then use
the snapshot's byte_slice(); apply the same pattern used around Blob::try_create
(top_value.as_array_buffer / snapshot_shared_array_buffer) to the
write_bytes_to_file_fast call and the other similar call sites (the ones
referenced around write_file_internal and the other locations) so no
shared/resizable backing ever produces a direct Rust &[u8].
In `@test/js/web/fetch/blob.test.ts`:
- Around line 560-602: Add a new test that constructs a Blob from a
SharedArrayBuffer view (e.g., using new Uint8Array(sab, offset, length) or new
DataView(...)), then mutates the underlying SharedArrayBuffer after Blob
construction, and finally asserts that await blob.arrayBuffer() still yields the
original pre-mutation bytes; reference the existing test patterns like Blob
snapshots a nonzero-offset SharedArrayBuffer view and Blob snapshots DataView
parts over SharedArrayBuffer to locate where to add the test and use the same
arrayBuffer() assertion style to verify immutability.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cce5bebb-1a78-4dd8-9494-d72fe7b5c1ac
📒 Files selected for processing (3)
src/runtime/webcore/Blob.rstest/js/bun/s3/s3.test.tstest/js/web/fetch/blob.test.ts
EffortlessSteven
force-pushed
the
claude/s3-write-stable-bytes
branch
from
June 2, 2026 18:05
00e594b to
43a5090
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/runtime/webcore/Blob.rs (1)
7446-7472:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSnapshot coverage is still incomplete.
This helper fixes Blob materialization, but
write_file_internal()still has a direct fast path at Lines 5194-5221 that feedsbuffer_view.byte_slice()intowrite_bytes_to_file_fast().Bun.write(pathOrFd, sharedOrResizableView)can therefore still materialize a Rust&[u8]over mutable JS backing and bypass this new safeguard entirely.Suggested fix
- } else if let Some(buffer_view) = data.as_array_buffer(global_this) { + } else if let Some(buffer_view) = data.as_array_buffer(global_this) { if buffer_view.byte_len < 256 * 1024 { let pathlike: PathOrFileDescriptor = match &*path_or_blob { PathOrBlob::Path(p) => p.clone(), PathOrBlob::Blob(b) => b .store() .expect("infallible: store present") .data .as_file() .pathlike .clone(), }; + let snapshot = if buffer_view.shared || buffer_view.resizable { + Some(snapshot_shared_array_buffer(global_this, &buffer_view)?) + } else { + None + }; + let bytes = snapshot + .as_ref() + .map(|snapshot| snapshot.byte_slice()) + .unwrap_or_else(|| buffer_view.byte_slice()); let result = if matches!(pathlike, PathOrFileDescriptor::Path(_)) { write_bytes_to_file_fast::<true>( global_this, &pathlike, - buffer_view.byte_slice(), + bytes, &mut needs_async, ) } else { write_bytes_to_file_fast::<false>( global_this, &pathlike, - buffer_view.byte_slice(), + bytes, &mut needs_async, ) };🤖 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 `@src/runtime/webcore/Blob.rs` around lines 7446 - 7472, The fast path in write_file_internal that calls buffer_view.byte_slice() and then write_bytes_to_file_fast can still create a Rust &[u8] over mutable JS backing; update that fast path to detect shared or resizable ArrayBuffer-backed views and snapshot them first using the existing snapshot_shared_array_buffer (which calls Bun__createArrayBufferForCopy) before calling byte_slice() or pass the copied ArrayBuffer into the write path so the Rust code never borrows into shared/mutable JS memory; specifically, in the code handling buffer_view.byte_slice() inside write_file_internal, branch to call snapshot_shared_array_buffer(global, array_buffer) and use the returned ArrayBuffer for subsequent byte slicing/writing instead of the original backing store.
🤖 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.
Duplicate comments:
In `@src/runtime/webcore/Blob.rs`:
- Around line 7446-7472: The fast path in write_file_internal that calls
buffer_view.byte_slice() and then write_bytes_to_file_fast can still create a
Rust &[u8] over mutable JS backing; update that fast path to detect shared or
resizable ArrayBuffer-backed views and snapshot them first using the existing
snapshot_shared_array_buffer (which calls Bun__createArrayBufferForCopy) before
calling byte_slice() or pass the copied ArrayBuffer into the write path so the
Rust code never borrows into shared/mutable JS memory; specifically, in the code
handling buffer_view.byte_slice() inside write_file_internal, branch to call
snapshot_shared_array_buffer(global, array_buffer) and use the returned
ArrayBuffer for subsequent byte slicing/writing instead of the original backing
store.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d367d8d-d10f-4892-bd70-0c39fcde585e
📒 Files selected for processing (3)
src/runtime/webcore/Blob.rstest/js/bun/s3/s3.test.tstest/js/web/fetch/blob.test.ts
EffortlessSteven
force-pushed
the
claude/s3-write-stable-bytes
branch
from
June 2, 2026 19:06
43a5090 to
74972f4
Compare
This was referenced Jun 2, 2026
EffortlessSteven
force-pushed
the
claude/s3-write-stable-bytes
branch
from
June 2, 2026 21:41
74972f4 to
758e462
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
new Blob([view])(and S3 uploads through it) accept aSharedArrayBuffer- or resizable-ArrayBuffer-backed part from safe JS. Materializing the Blob reads it as a Rust&[u8]while another agent can mutate or resize that backing. That is undefined behavior under Rust's aliasing rules.Fix: shared and resizable parts are copied in C++ (
ArrayBuffer::copy_to_unshared) to a fresh non-shared buffer and read as owned bytes, so Rust never slices shared memory. Fixed unshared parts keep the zero-copy borrowed path.Verification
Behavior-preserving: tests assert public behavior and pass on old code by design; the change is the removed
&[u8]over shared backing.fmtclean.Review map
array_buffer.rs:copy_to_unshared, reusing the existing FFIBlob.rs: snapshot shared/resizable parts (single + multi-part); fast path for fixed unsharedblob.test.ts/s3.test.ts: SAB, resizable, DataView, multi-part, S3 input