ai slop - #37944
ai slop#37944Jarred-Sumner wants to merge 2 commits into
Conversation
Off-thread I/O (async node:fs read/write/readv/writev and buffer arguments, zlib/brotli/zstd async writes, scrypt/pbkdf2, CompressionStream, HTTP parser, Bun.markdown, shell redirects) borrows caller ArrayBuffers by pinning them. A pin does not hold the pages of a non-shared WebAssembly.Memory or of a resizable ArrayBuffer, so those are now reported as not pinnable and every borrower works on a private copy of the requested window instead, writing produced bytes back into the object's current storage on completion.
WalkthroughChangesThe PR adds pin-or-copy handling for ArrayBuffer, WebAssembly memory, resizable buffers, asynchronous filesystem operations, compression streams, shell redirections, and related runtime consumers. Tests cover buffer movement, growth, detachment, write-back, and compression results. ArrayBuffer lifecycle and consumers
Possibly related PRs
Suggested reviewers: Mergeability Score: 🟡 Moderate · up to The PR changes async buffer handling to copy storage that can move, but shell redirection can still silently drop subprocess output when a resizable buffer changes during execution. Because this can cause concrete data loss for affected commands, merge should wait for that path to be fixed or explicitly accepted; the remaining issues are bounded follow-ups. 🚥 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/shell/subproc.rs (1)
1600-1610: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRefreshed-to-empty storage now silently discards output bytes.
buf.refresh(...)setsarray_bufferto the default empty descriptor when the held value has been detached.slice_mut()is then empty,idx >= array_buf_slice.len()is true, andappendreturns without writing and without advancingi. The subprocess output is dropped with no error.Before this change the descriptor was a pin, so it could not become empty mid-run. With resizable buffers and WebAssembly memory the detached case is now reachable, which makes the existing TODO on Line 1603 a live data-loss path rather than a theoretical one.
Record the failure so the shell can report it, for example by setting an error on the
PipeReaderwhenrefreshyields an empty buffer while bytes remain. Do you want me to open an issue to track this?🤖 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/shell/subproc.rs` around lines 1600 - 1610, Update the append logic around buf.refresh, slice_mut, and the idx bounds check to record an error on the relevant PipeReader when refresh leaves the buffer empty while bytes still need to be written, instead of silently returning. Preserve normal copying and index advancement for valid storage, and use the existing PipeReader error-reporting mechanism.
🤖 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/image/Image.rs`:
- Around line 742-743: Update the method documentation near the storage pinning
logic to replace the outdated “We deliberately DON'T copy” statement with the
actual policy: use zero-copy for pinned storage, while duplicating storage that
cannot remain pinned, including FastTypedArray and WebAssembly.Memory. Keep the
comment focused on this durable, non-obvious behavior.
In `@src/runtime/node/node_fs.rs`:
- Around line 3850-3868: Update NodeFS::read so zero-length reads return before
evaluating args.buffer.slice() or otherwise accessing the backing store.
Preserve the existing behavior for nonzero reads, and ensure Read::from_js does
not leave a zero-length buffer vulnerable to background access.
In `@src/runtime/node/types.rs`:
- Around line 1379-1404: Update stand_in_for_spans to distinguish the Windows
total-size limit from genuine allocation failures, using a small error type or
equivalent variant that preserves both cases. Propagate that distinction to its
caller so the u32::MAX overflow maps to a range/size error, while reservation
and allocation failures continue mapping to status 2 out-of-memory.
- Around line 1493-1500: Replace the local magic status value 2 in the status
normalization block with a clearly named constant representing the pin-copy
failure condition, declared near the related status handling. Update the
subsequent match arms to use the same named constant while preserving the
existing FFI status values and behavior.
- Around line 1364-1375: Add a debug_assert_eq! in Node::release immediately
before the views/pins drain loop to verify both vectors have equal lengths,
preserving the existing unpin and unprotect behavior.
In `@src/runtime/shell/subproc.rs`:
- Around line 1576-1592: Refactor ArrayBufferStrong to expose a shared accessor
for current bytes that handles both pinned and unpinned states, then make both
slice and refresh use it instead of duplicating held/as_array_buffer lookup
logic. Because slice obtains the global internally, document on
BufferedOutput::slice that it must run on the JS thread, or pass the caller’s
JSGlobalObject through as append does.
In `@test/js/node/crypto/scrypt.test.ts`:
- Around line 94-118: Restrict the test around the fixture’s fs.read blockers to
non-Windows platforms, since Windows uses a separate libuv pool and may not
enforce the intended ordering. Add the platform guard using the test suite’s
existing platform-detection convention, while preserving the current WorkPool
synchronization and assertions on supported platforms.
In `@test/js/node/fs/fs.test.ts`:
- Around line 5100-5132: Extract the duplicated parked-stdin subprocess protocol
from the run helper in test/js/node/fs/fs.test.ts (lines 5100-5132) into a
shared test utility accepting fixture source, argv, and stdin byte counts for
the park and ready sentinels, returning { report, exitCode }. Replace the inline
driver in test/js/node/crypto/scrypt.test.ts (lines 119-145) with this utility
call; update both sites as described.
- Around line 5134-5187: Expand the test matrix around the run helper to cover
the missing resizable-buffer scenarios: add shrinking cases for write and readv,
plus stable move:false cases for write and writev. Assert each result, detached
state, and written or memory contents consistently with the existing
read/readv/write/writev tests.
In `@test/js/node/zlib/zlib-reset-race.test.ts`:
- Around line 166-190: Extend the resizableWriteFixture in
test/js/node/zlib/zlib-reset-race.test.ts:166-190 with a variant that resizes
resizable.buffer after h.write and before await done, then verify all
DeflateRaw, Brotli, and Zstd engines recover the original 64-byte input. Add the
matching resize-during-write case in
test/js/node/zlib/zlib-handle-bounds-check.test.ts:460-486, resizing between
h.write and the h.cb callback and asserting the recovered input remains
unchanged.
---
Outside diff comments:
In `@src/runtime/shell/subproc.rs`:
- Around line 1600-1610: Update the append logic around buf.refresh, slice_mut,
and the idx bounds check to record an error on the relevant PipeReader when
refresh leaves the buffer empty while bytes still need to be written, instead of
silently returning. Preserve normal copying and index advancement for valid
storage, and use the existing PipeReader error-reporting mechanism.
🪄 Autofix
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: 00a79915-2ab7-49f2-8dfb-8d08238dbd90
📒 Files selected for processing (29)
src/js/node/zlib.tssrc/jsc/JSValue.rssrc/jsc/array_buffer.rssrc/jsc/bindings/bindings.cppsrc/jsc/bindings/headers-handwritten.hsrc/jsc/bindings/node/http/JSHTTPParserPrototype.cppsrc/jsc/lib.rssrc/jsc/node_path.rssrc/runtime/api/MarkdownObject.rssrc/runtime/api/bun/spawn/stdio.rssrc/runtime/image/Image.rssrc/runtime/node/node_fs.rssrc/runtime/node/node_zlib_binding.rssrc/runtime/node/types.rssrc/runtime/node/zlib/NativeBrotli.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/node/zlib/NativeZstd.rssrc/runtime/server/NodeHTTPResponse.rssrc/runtime/shell/Builtin.rssrc/runtime/shell/states/Cmd.rssrc/runtime/shell/subproc.rssrc/runtime/webcore/CompressionStreamCoder.rssrc/sql_jsc/mysql/MySQLValue.rstest/js/bun/shell/bunshell.test.tstest/js/node/crypto/scrypt.test.tstest/js/node/fs/fs.test.tstest/js/node/zlib/zlib-handle-bounds-check.test.tstest/js/node/zlib/zlib-reset-race.test.tstest/js/node/zlib/zlib.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no issues. Given the breadth — an FFI ABI change to the pin primitive, new copy/write-back lifetimes threaded through fs/zlib/crypto/shell/HTTP off-thread paths, a new Drop on ArrayBufferStrong, and user-visible behavior changes (zlib handle.write() now returns a substitute buffer; shell blob-redirect failure now fails the command instead of throwing) — a human pass on the design and the memory-ownership edges is still worthwhile.
What was reviewed:
Bun::tryPin/ArrayBufferPinABI (repr(u8) ↔enum class : uint8_t) and every Rust/C++ caller updated in lockstep, includingcollectBufferSpansandborrowBytesForOffThread.- Pin/unpin balance: new
ArrayBufferStrong::Dropvs. the removed manual unpins insubproc.rs/Builtin.rs;defuse_array_buffer_unpinsnow clearspinnedso the finalizer-time drop is a no-op. from_js_pinned_rangewindow clamping and the write-back path (write_back/write_back_into) — bounds are clamped against the object's current storage, detached targets no-op.- zlib
WriteBufferslifecycle: buffers held until next write/close so the stream context's dangling pointers stay valid;release_write_buffersruns on every completion arm includingthis_value-null and VM-teardown.
Extended reasoning...
Overview
This PR reworks how async native operations borrow caller-supplied ArrayBuffer storage when a pin cannot actually hold that storage in place (non-shared WebAssembly.Memory buffers and resizable ArrayBuffers). It introduces a tri-state ArrayBufferPin returned across the C++/Rust FFI boundary, adds MarkedArrayBuffer::from_js_pinned{,_range} / private_copy / private_zeroed / write_back helpers, and threads the new must-copy path through ~15 call sites: async node:fs read/readv/write/writev and path buffers, the zlib/brotli/zstd native handle, scrypt/pbkdf2 (via StringOrBuffer), CompressionStream, NodeHTTPResponse, Bun.markdown, HTTPParser.execute, shell stdin/stdout redirects, and VectorArrayBuffer (readv/writev). It also adds Drop to ArrayBufferStrong, a new WriteBuffers field on the compression-stream mixin, and changes shell blob-redirect refusal from a thrown JS exception into a per-command stderr + exit-1.
Security risks
The change is squarely in the memory-safety category the repo's review guide flags as most-blocked: raw pointers into JS-owned storage held across thread boundaries, pin/unpin balance, and Drop ordering during VM finalization. The direction of the change is defensive (copy instead of holding a pointer that can be unmapped), but the surface area for getting an unpin/free wrong is large. No auth/crypto correctness or injection surface is touched; scrypt/pbkdf2 only change how the input bytes are borrowed.
Level of scrutiny
High. This is not mechanical: it changes an FFI signature, adds a Drop impl to a widely-held type, introduces new ownership states (owns_buffer + buffer.value set) on MarkedArrayBuffer, and reorders flush_write_result before release_write_buffers in the zlib completion path. It also makes user-visible behavior changes that are effectively API decisions — zlib handle.write() now returns undefined-or-a-Uint8Array and the JS wrapper consumes it, and $…` > ${blob}`` no longer rejects the whole shell promise. Those are reasonable choices but a maintainer should sign off on them.
Other factors
Test coverage is thorough (per-subsystem fixtures that force the worker to run after the storage moves via a parked-threadpool trick, plus no-move variants asserting the bytes land). The bug-hunting pass examined pin/unpin pairing on every early-return, the VectorArrayBuffer stand-in path (pins on already-pinned elements are still released via release()), the private_owned len == 0 dangling-ptr case (owns_buffer=false so destroy() skips it), and the ArrayBufferStrong finalizer defuse — none surfaced a defect. Deferring to a human because the scope and the number of interacting lifetimes exceed what an automated approval should clear on its own.
…in zlib tests, named stand-in errors, shared ArrayBufferStrong::current - test/harness.ts: runParkedFixture drives the park/ready stdin protocol (stdout and stderr drained concurrently); fs, zlib and scrypt tests use it - fs tests: complete the resizable/stay matrix - zlib tests: resize the resizable buffer while the write is in flight - scrypt/zlib parked tests skip Windows, where fs.read runs on libuv's pool - zero-length async fs.read no longer carries a borrowed descriptor - VectorArrayBuffer: assert views/pins pairing; a stand-in too large for one Windows I/O vector is a range error, not out-of-memory - ArrayBufferStrong::current shared by refresh() and BufferedOutput::slice - Image.rs: bring the copy-policy doc in line
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/node/types.rs (1)
1142-1147: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject bare
ArrayBufferpath arguments.
Buffer::from_typed_arrayunwrapsvalue.as_array_buffer(ctx), so a populated, non-NULArrayBufferpasses both validators and becomes a path. Node rejects bareArrayBufferwithERR_INVALID_ARG_TYPE. RemoveJSType::ArrayBufferfrom this arm and add a regression test.🤖 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/node/types.rs` around lines 1142 - 1147, Update the path argument match arm in the relevant validation function to exclude JSType::ArrayBuffer, so bare ArrayBuffer values no longer reach Buffer::from_typed_array or become valid paths. Preserve the Uint8Array and DataView handling, and add a regression test asserting that a bare ArrayBuffer is rejected with Node’s ERR_INVALID_ARG_TYPE behavior.
🤖 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/image/Image.rs`:
- Around line 709-711: Update the documentation near source_from_js to remove
the claim that resizable buffers are duplicated, since source_from_js rejects
resizable and shared ArrayBuffers before storage; document only the supported
non-shared buffer behavior.
In `@test/harness.ts`:
- Around line 628-648: Wrap the proc.stdout drain loop in the helper containing
stderr, stdout, parked, and released so any rejection from stdin.flush() or
stdin.end() cannot bypass cleanup. Ensure the stderr promise is awaited on both
successful and error paths, preserving the existing return values while
propagating the original stdin error after stderr has been drained.
In `@test/js/node/zlib/zlib.test.js`:
- Line 803: Update the child-process command around the `cmd` array to pass an
absolute path for `fixture.mjs`, resolving it from the existing `dir` variable
before spawning the child. Preserve the current executable and arguments while
ensuring the fixture path is suitable for file operations regardless of the
working directory.
---
Outside diff comments:
In `@src/runtime/node/types.rs`:
- Around line 1142-1147: Update the path argument match arm in the relevant
validation function to exclude JSType::ArrayBuffer, so bare ArrayBuffer values
no longer reach Buffer::from_typed_array or become valid paths. Preserve the
Uint8Array and DataView handling, and add a regression test asserting that a
bare ArrayBuffer is rejected with Node’s ERR_INVALID_ARG_TYPE behavior.
🪄 Autofix
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: 6a2c88b3-820f-4115-aca3-e9035b1331a2
📒 Files selected for processing (11)
src/jsc/array_buffer.rssrc/runtime/image/Image.rssrc/runtime/node/node_fs.rssrc/runtime/node/types.rssrc/runtime/shell/subproc.rstest/harness.tstest/js/node/crypto/scrypt.test.tstest/js/node/fs/fs.test.tstest/js/node/zlib/zlib-handle-bounds-check.test.tstest/js/node/zlib/zlib-reset-race.test.tstest/js/node/zlib/zlib.test.js
| /// We DON'T copy storage a pin can hold in place (small `FastTypedArray`s | ||
| /// and non-shared `WebAssembly.Memory`/resizable buffers are duplicated | ||
| /// instead): the encoded input can be tens of MB and |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find where Image construction inspects `shared`/`resizable` on the source ArrayBuffer.
set -euo pipefail
rg -nP -C 12 '\bresizable\b|\bshared\b' src/runtime/image/Image.rs
# Locate the constructor / source-assignment path for classification context.
ast-grep outline src/runtime/image/Image.rs --items allRepository: oven-sh/bun
Length of output: 11570
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '331,438p' src/runtime/image/Image.rs
sed -n '680,818p' src/runtime/image/Image.rsRepository: oven-sh/bun
Length of output: 11665
Document one resizable-buffer policy. source_from_js rejects ab.resizable and ab.shared before storage. Remove the claim that resizable buffers are duplicated.
🤖 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/image/Image.rs` around lines 709 - 711, Update the documentation
near source_from_js to remove the claim that resizable buffers are duplicated,
since source_from_js rejects resizable and shared ArrayBuffers before storage;
document only the supported non-shared buffer behavior.
Source: Coding guidelines
| const stderr = proc.stderr.text(); | ||
| let stdout = ""; | ||
| let parked = false; | ||
| let released = false; | ||
| const decoder = new TextDecoder(); | ||
| for await (const chunk of proc.stdout) { | ||
| stdout += decoder.decode(chunk, { stream: true }); | ||
| if (!parked && stdout.includes("park\n")) { | ||
| parked = true; | ||
| proc.stdin.write(Buffer.alloc(options.parkBytes ?? 16, "c")); | ||
| await proc.stdin.flush(); | ||
| } | ||
| if (!released && stdout.includes("ready\n")) { | ||
| released = true; | ||
| proc.stdin.write(Buffer.alloc(options.readyBytes, "c")); | ||
| await proc.stdin.end(); | ||
| } | ||
| } | ||
| const exitCode = await proc.exited; | ||
| const tail = stdout.slice(stdout.indexOf("ready\n") + "ready\n".length).trim(); | ||
| return { report: tail ? JSON.parse(tail) : undefined, stderr: await stderr, exitCode }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Await stderr on every path so child diagnostics survive a stdin failure.
Line 628 starts draining stderr without awaiting it. The only await on that promise is in the return statement at line 648. If the child exits between the park\n sentinel and the stdin write, proc.stdin.flush() or proc.stdin.end() rejects with EPIPE, and that rejection propagates out of the for await loop. The helper then never awaits the stderr promise, so the caller sees the pipe error instead of the child's actual failure output, and the abandoned promise can surface as an unrelated unhandled rejection.
Wrap the drain loop so the helper always awaits stderr.
This helper now backs test/js/node/fs/fs.test.ts, test/js/node/crypto/scrypt.test.ts, and test/js/node/zlib/zlib.test.js, so the diagnostics matter for three suites.
♻️ Proposed change to always await the stderr drain
const stderr = proc.stderr.text();
let stdout = "";
let parked = false;
let released = false;
const decoder = new TextDecoder();
- for await (const chunk of proc.stdout) {
- stdout += decoder.decode(chunk, { stream: true });
- if (!parked && stdout.includes("park\n")) {
- parked = true;
- proc.stdin.write(Buffer.alloc(options.parkBytes ?? 16, "c"));
- await proc.stdin.flush();
- }
- if (!released && stdout.includes("ready\n")) {
- released = true;
- proc.stdin.write(Buffer.alloc(options.readyBytes, "c"));
- await proc.stdin.end();
- }
- }
- const exitCode = await proc.exited;
+ let driveError: unknown;
+ try {
+ for await (const chunk of proc.stdout) {
+ stdout += decoder.decode(chunk, { stream: true });
+ if (!parked && stdout.includes("park\n")) {
+ parked = true;
+ proc.stdin.write(Buffer.alloc(options.parkBytes ?? 16, "c"));
+ await proc.stdin.flush();
+ }
+ if (!released && stdout.includes("ready\n")) {
+ released = true;
+ proc.stdin.write(Buffer.alloc(options.readyBytes, "c"));
+ await proc.stdin.end();
+ }
+ }
+ } catch (e) {
+ driveError = e;
+ }
+ const [text, exitCode] = await Promise.all([stderr, proc.exited]);
+ if (driveError !== undefined && exitCode === 0) throw driveError;
const tail = stdout.slice(stdout.indexOf("ready\n") + "ready\n".length).trim();
- return { report: tail ? JSON.parse(tail) : undefined, stderr: await stderr, exitCode };
+ return { report: tail ? JSON.parse(tail) : undefined, stderr: text, exitCode };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const stderr = proc.stderr.text(); | |
| let stdout = ""; | |
| let parked = false; | |
| let released = false; | |
| const decoder = new TextDecoder(); | |
| for await (const chunk of proc.stdout) { | |
| stdout += decoder.decode(chunk, { stream: true }); | |
| if (!parked && stdout.includes("park\n")) { | |
| parked = true; | |
| proc.stdin.write(Buffer.alloc(options.parkBytes ?? 16, "c")); | |
| await proc.stdin.flush(); | |
| } | |
| if (!released && stdout.includes("ready\n")) { | |
| released = true; | |
| proc.stdin.write(Buffer.alloc(options.readyBytes, "c")); | |
| await proc.stdin.end(); | |
| } | |
| } | |
| const exitCode = await proc.exited; | |
| const tail = stdout.slice(stdout.indexOf("ready\n") + "ready\n".length).trim(); | |
| return { report: tail ? JSON.parse(tail) : undefined, stderr: await stderr, exitCode }; | |
| const stderr = proc.stderr.text(); | |
| let stdout = ""; | |
| let parked = false; | |
| let released = false; | |
| const decoder = new TextDecoder(); | |
| let driveError: unknown; | |
| try { | |
| for await (const chunk of proc.stdout) { | |
| stdout += decoder.decode(chunk, { stream: true }); | |
| if (!parked && stdout.includes("park\n")) { | |
| parked = true; | |
| proc.stdin.write(Buffer.alloc(options.parkBytes ?? 16, "c")); | |
| await proc.stdin.flush(); | |
| } | |
| if (!released && stdout.includes("ready\n")) { | |
| released = true; | |
| proc.stdin.write(Buffer.alloc(options.readyBytes, "c")); | |
| await proc.stdin.end(); | |
| } | |
| } | |
| } catch (e) { | |
| driveError = e; | |
| } | |
| const [text, exitCode] = await Promise.all([stderr, proc.exited]); | |
| if (driveError !== undefined && exitCode === 0) throw driveError; | |
| const tail = stdout.slice(stdout.indexOf("ready\n") + "ready\n".length).trim(); | |
| return { report: tail ? JSON.parse(tail) : undefined, stderr: text, exitCode }; |
🤖 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 `@test/harness.ts` around lines 628 - 648, Wrap the proc.stdout drain loop in
the helper containing stderr, stdout, parked, and released so any rejection from
stdin.flush() or stdin.end() cannot bypass cleanup. Ensure the stderr promise is
awaited on both successful and error paths, preserving the existing return
values while propagating the original stdin error after stderr has been drained.
| async function run(body) { | ||
| using dir = tempDir("zlib-wasm-memory-view", { "fixture.mjs": prologue + body }); | ||
| const { report, stderr, exitCode } = await runParkedFixture({ | ||
| cmd: [bunExe(), "fixture.mjs"], |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use an absolute fixture path.
Line 803 passes fixture.mjs as a relative path. Build the fixture path from dir before spawning the child.
Proposed fix
+import { join } from "node:path";
import { bunEnv, bunExe, isWindows, runParkedFixture, tempDir, tmpdirSync } from "harness";
- cmd: [bunExe(), "fixture.mjs"],
+ cmd: [bunExe(), join(String(dir), "fixture.mjs")],As per coding guidelines: “Use absolute paths in file operations.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cmd: [bunExe(), "fixture.mjs"], | |
| cmd: [bunExe(), join(String(dir), "fixture.mjs")], |
🤖 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 `@test/js/node/zlib/zlib.test.js` at line 803, Update the child-process command
around the `cmd` array to pass an absolute path for `fixture.mjs`, resolving it
from the existing `dir` variable before spawning the child. Preserve the current
executable and arguments while ensuring the fixture path is suitable for file
operations regardless of the working directory.
Source: Coding guidelines
There was a problem hiding this comment.
I reviewed this PR and the bug hunting system found no issues. Given the scope — a new pin-or-copy contract across the C++/Rust FFI boundary, buffer lifetime changes in async fs/zlib/shell/HTTP paths, and new/removed Drop impls on ArrayBufferStrong/BufferedOutput — a human look at the memory-ownership and write-back semantics is still worthwhile.
What was reviewed:
Bun::tryPin/ArrayBufferPinFFI contract and every Rust call site's handling ofMustCopy.MarkedArrayBuffer::from_js_pinned_rangewindow clamping,write_backbounds against detached/shrunk targets, andprivate_owned'sowns_buffer = len != 0edge.- Zlib
WriteBufferslifecycle:flush_write_resultnow runs beforerelease_write_buffers, and stand-ins persist until the next write/close. ArrayBufferStrong::Dropvs the shell'sdefuse_array_buffer_unpinsfinalizer path;VectorArrayBufferrelease/unpin pairing.
Extended reasoning...
Overview
This PR reworks how Bun borrows caller-supplied ArrayBuffer bytes for operations that outlive the calling JS turn. It introduces a three-state ArrayBufferPin (NotPinned/Pinned/MustCopy) returned by Bun::tryPin in C++ and mirrored in Rust, replacing the previous boolean pin. Storage a pin cannot hold in place (non-shared WebAssembly.Memory, resizable ArrayBuffer) is now copied into a private stand-in; for output buffers, results are written back into the JS object's current storage on completion. The change fans out across ~30 files: bindings.cpp, array_buffer.rs, async fs (node_fs.rs, types.rs), the zlib/brotli/zstd native handle, CompressionStream, the HTTP parser, NodeHTTPResponse, shell redirects (Builtin.rs, Cmd.rs, subproc.rs), Bun.markdown, PathLike, and VectorArrayBuffer. It also adds a Drop impl to ArrayBufferStrong, removes one from BufferedOutput, and changes shell error handling for immutable-blob redirects from a thrown exception to a per-command failure.
Security risks
No new attack surface is introduced; this is a memory-safety hardening. The risks are all in the direction the PR is trying to fix: use-after-free/write-to-freed-pages if a pin state is mishandled, double-unpin if a Drop and an explicit unpin() both fire, or leaked pins/roots on an error path. The review checked that MustCopy never reaches unpinArrayBuffer, that write_back_into clamps to the destination's live length, that VectorArrayBuffer::release only unpins Pinned entries, and that the shell finalizer's pinned = false defuse still short-circuits the new ArrayBufferStrong::Drop.
Level of scrutiny
High. Per REVIEW.md, native memory safety is the most-blocked category, and this PR touches exactly the patterns it calls out: pointers held across JS re-entry, cross-thread buffer ownership, refcount/pin balancing on every terminal path, and GC-visible Drop ordering. The FFI signature change (pinArrayBuffer bool → enum, collectBufferSpans callback gained a parameter) means every caller had to be updated correctly. The zlib change reorders flush_write_result relative to buffer release and introduces a substitute-input Uint8Array handed back to JS. The shell change replaces a bespoke PinnedArrayBuf wrapper with ArrayBufferStrong and its new Drop. These are all correct-looking but non-mechanical.
Other factors
The PR ships extensive new tests (fs/zlib/scrypt/shell) that exercise the mid-operation move for both wasm-memory and resizable-buffer storage, including move: false control cases, and the author addressed the substantive CodeRabbit feedback in commit 822a435 (harness helper extraction, debug_assert_eq! on views/pins, resize-mid-write zlib coverage, StandInError enum, ArrayBufferStrong::current shared accessor, zero-length read fix). The remaining unresolved CodeRabbit comments are trivial nits (magic status constant, variant-matrix completeness). No bugs were found by the multi-agent hunt. Still, a change of this breadth in pin/unpin lifecycle across a dozen subsystems warrants a maintainer's eyes before merge.
|
Updated 7:50 PM PT - Aug 12th, 2026
❌ @Jarred-Sumner, your commit 822a435 has some failures in 🧪 To try this PR locally: bunx bun-pr 37944That installs a local version of the PR into your bun-37944 --bun |
|
This PR has been closed because it was flagged as AI slop. Many AI-generated PRs are fine, but this one was identified as having one or more of the following issues:
If you believe this was done in error, please leave a comment explaining why. |
github-actions
Bot
changed the title
2 participants
This PR has been marked as AI slop and the description has been updated to avoid confusion or misleading reviewers.
Many AI PRs are fine, but sometimes they submit a PR too early, fail to test if the problem is real, fail to reproduce the problem, or fail to test that the problem is fixed. If you think this PR is not AI slop, please leave a comment.