MarkedArrayBuffer.destroy: don't free the struct itself#30166
Conversation
…on error The MarkedArrayBuffer structs appended to the entries ArrayList by Buffer.fromString() are stored by value, so calling item.destroy() passes an interior ArrayList pointer to allocator.destroy(). For index 0 this frees entries.items.ptr, which entries.deinit() then frees again. Match the existing ResultListEntry.Value.deinit() pattern and free only the owned byte slice.
|
Updated 2:28 AM PT - May 3rd, 2026
❌ @robobun, your commit 3b06a51 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 30166That installs a local version of the PR into your bun-30166 --bun |
WalkthroughFixed memory cleanup in recursive ChangesRecursive Readdir Buffer Cleanup Fix
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/node/fs/fs.test.ts`:
- Around line 1227-1229: The test currently ignores stderr and checks exitCode
immediately; change the Promise.all destructure to capture stderr (use
proc.stdout.text(), proc.stderr.text(), proc.exited) into e.g. [stdout, stderr,
exitCode], then assert stdout.trim() === "ELOOP", assert stderr.trim() === ""
(or empty string) next, and finally assert exitCode === 0 so stderr is
explicitly checked and the exit-code assertion remains last; update references
to stdout, stderr, and exitCode accordingly in the test.
🪄 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: 5ed1211a-8d3f-4071-9948-3deb8c998855
📒 Files selected for processing (2)
src/bun.js/node/node_fs.zigtest/js/node/fs/fs.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — correct, minimal fix that matches the existing async cleanup pattern; the flagged Dirent leak is pre-existing and can be folded in or handled separately.
Extended reasoning...
Overview
This PR fixes a heap corruption / double-free in two error-cleanup paths of fs.readdir (readdirWithEntries and readdirInner) when encoding: 'buffer' is used. The previous code called MarkedArrayBuffer.destroy() on structs stored by value inside entries.items. destroy() (array_buffer.zig:590-597) does allocator.free(buffer.slice()) followed by allocator.destroy(this) — the latter is only valid when this was created via MarkedArrayBuffer.init() (which heap-allocates the struct). Here, entries come from fromString() which returns by value, so this is an interior ArrayList pointer; for index 0 it equals entries.items.ptr, leading to UAF on the next iteration and a double-free in entries.deinit(). The fix replaces both sites with bun.default_allocator.free(item.buffer.byteSlice()), identical to the already-correct async cleanup at node_fs.zig:991. A regression test reproduces the path via a self-referential symlink triggering ELOOP and asserts the subprocess exits cleanly (previously SIGABRT under ASAN).
Security risks
None introduced. This is strictly a correctness fix on an error-handling path — it removes a heap corruption rather than adding any new attack surface. No user-controlled data flows change.
Level of scrutiny
Moderate — it's manual memory management in Zig, but the change is mechanical and self-evidently correct once you read MarkedArrayBuffer.destroy() / fromString() / init(). I verified: (1) fromString() returns by value with only the byte buffer heap-allocated, (2) MarkedArrayBuffer.slice() is an alias for buffer.byteSlice() so the freed slice is identical to what destroy() would have freed, and (3) the new code is byte-for-byte the same pattern as the existing async path at line 991.
Other factors
The bug-hunter flagged a pre-existing leak in the adjacent Dirent arm of the same switch (line 5014: result.name.deref() leaks result.path). That's a valid observation and a one-token fix (result.deref()), but it predates this PR, is a leak rather than a crash, and only fires on the withFileTypes: true recursive error path. It's a reasonable fold-in but not a blocker. The new test is well-constructed (subprocess isolation, ASAN symbolize=0 for fast abort, skipIf(isWindows) for symlink semantics).
MarkedArrayBuffer is passed and stored by value (fromString/fromBytes/fromJS all return by value); the struct is never individually heap-allocated by any caller. Calling allocator.destroy(this) on a by-value instance passes an interior pointer to the allocator - in the readdir error path this is an ArrayList slot, corrupting the heap and double-freeing entries.items.ptr. Remove the allocator.destroy(this) call and the unused init() constructor (the only function that heap-allocated the struct, with zero callers). The readdir call sites go back to plain .destroy(), which now correctly checks this.allocator before freeing bytes so JS-owned buffers remain untouched. Also release Dirent.path on the recursive-sync error path to match the non-recursive and async cleanup sites.
|
Reworked per review in 3b06a51:
Gate re-verified locally against the branch base: unfixed → subprocess aborts (ASAN use-after-poison, exit 134); fixed → |
robobun
changed the title
There was a problem hiding this comment.
LGTM — the revised fix to MarkedArrayBuffer.destroy() is cleaner than per-call-site frees, and the Dirent.path leak I flagged is now addressed via result.deref().
Extended reasoning...
Overview
This PR fixes a use-after-poison / double-free in the fs.readdirSync({encoding: 'buffer'}) error-cleanup path. The original symptom: MarkedArrayBuffer.destroy() called allocator.destroy(this) on a struct stored by-value inside an ArrayList, freeing the list's backing storage mid-iteration.
After my earlier review the author revised the approach: rather than inlining allocator.free(byteSlice()) at each readdir call site, commit 3b06a51 removes allocator.destroy(this) from MarkedArrayBuffer.destroy() itself and deletes the unused MarkedArrayBuffer.init() (the only constructor that ever heap-allocated the struct). The readdir call sites go back to calling .destroy(), which is now safe and retains the this.allocator null-check that protects JS-owned buffers. The same commit also fixes the pre-existing Dirent arm in readdirInner to call result.deref() instead of result.name.deref(), releasing both name and path — addressing the leak I flagged.
Security risks
None. This is an internal memory-management fix on an error path; no new inputs, parsing, auth, or external surface area.
Level of scrutiny
Moderate — it changes the semantics of a shared helper (MarkedArrayBuffer.destroy). I verified:
MarkedArrayBuffer.inithad zero callers (grep confirms), so removing it is safe.- There are no
allocator.create(MarkedArrayBuffer)or heap-allocated*MarkedArrayBuffersites anywhere in the tree, so droppingallocator.destroy(this)cannot introduce a leak. - The only
.destroy()callers are the two readdir cleanup loops (node_fs.zig:4565, 5013), both operating on by-valueentries.items— exactly the case this fix targets.
Other factors
- A regression test spawns a subprocess hitting the ELOOP path twice and asserts clean exit; the PR description shows it SIGABRTs under ASAN without the fix and passes with it.
- The two CI failures (
bake/dev-and-prod.test.tson win-aarch64,fetch-http2-client.test.tson linux-asan) are unrelated to fs/readdir. - CodeRabbit's remaining nit (assert
stderrseparately) is stylistic and non-blocking; debug builds can emit benign stderr and the test already pins behavior via stdout + exit code.
## Problem
`MarkedArrayBuffer.destroy()` did two things:
```zig
allocator.free(content.buffer.slice()); // free the bytes
allocator.destroy(this); // free *this
```
Every constructor that is actually used (`fromString`, `fromBytes`,
`fromJS`, `fromTypedArray`, `fromArrayBuffer`) returns
`MarkedArrayBuffer` **by value**, so `this` is never an individually
heap-allocated struct — it's a stack local, an embedded field, or an
ArrayList slot. The `allocator.destroy(this)` call passes that interior
pointer to mimalloc.
In the readdir Buffer error-cleanup path (`readdirWithEntries` /
`readdirInner`), entries are appended by value via
`Buffer.fromString()`:
- `allocator.destroy(&entries.items[0])` frees `entries.items.ptr`
- the next loop iteration reads `this.*` from poisoned memory
- `entries.deinit()` frees the same pointer again
## Repro
```js
const fs = require('fs');
// dir contains regular files + a self-referential symlink 'loop -> loop'
fs.readdirSync(dir, { encoding: 'buffer', recursive: true });
```
The recursive walk collects Buffer entries for the root, then fails with
`ELOOP` opening the symlink (not in the swallowed `NOENT/NOTDIR/PERM`
set), and enters the cleanup loop. Under ASAN:
```
==3593==ERROR: AddressSanitizer: use-after-poison on address 0x737ec6e50040
READ of size 64 at 0x737ec6e50040 thread T0
oven-sh#1 MarkedArrayBuffer.destroy array_buffer.zig:591
oven-sh#2 NodeFS.readdirInner node_fs.zig:5013
oven-sh#3 NodeFS.readdir node_fs.zig:4518
```
## Fix
- Drop `allocator.destroy(this)` from `MarkedArrayBuffer.destroy()`. The
struct is passed/stored by value; callers own its storage.
- Remove the unused `MarkedArrayBuffer.init()` (the only function that
heap-allocated the struct, zero callers) so there's no pairing that
would leak.
- The readdir call sites keep calling `.destroy()`, which still checks
`this.allocator` before freeing bytes — JS-owned buffers remain
untouched.
Also fixed the adjacent `Dirent` arm of the recursive-sync error
cleanup: `result.name.deref()` → `result.deref()` so `Dirent.path` is
released too (matching the non-recursive and async cleanup sites).
## Verification
New test in `test/js/node/fs/fs.test.ts` creates a temp dir with files +
a self-referential symlink, spawns a subprocess that calls
`readdirSync({encoding:'buffer', recursive:true})`, and asserts it
throws `ELOOP` and exits 0.
```
# without fix
(fail) readdirSync({encoding: 'buffer', recursive: true}) frees entries safely ...
{ exitCode: 134, stdout: "" } # SIGABRT from ASAN
# with fix
(pass) readdirSync({encoding: 'buffer', recursive: true}) frees entries safely ... [1.5s]
{ exitCode: 0, stdout: "ELOOP" }
```
`zig:check-all` passes on all targets.
---------
Co-authored-by: robobun <robobun@users.noreply.github.com>
2 participants
Problem
MarkedArrayBuffer.destroy()did two things:Every constructor that is actually used (
fromString,fromBytes,fromJS,fromTypedArray,fromArrayBuffer) returnsMarkedArrayBufferby value, sothisis never an individually heap-allocated struct — it's a stack local, an embedded field, or an ArrayList slot. Theallocator.destroy(this)call passes that interior pointer to mimalloc.In the readdir Buffer error-cleanup path (
readdirWithEntries/readdirInner), entries are appended by value viaBuffer.fromString():allocator.destroy(&entries.items[0])freesentries.items.ptrthis.*from poisoned memoryentries.deinit()frees the same pointer againRepro
The recursive walk collects Buffer entries for the root, then fails with
ELOOPopening the symlink (not in the swallowedNOENT/NOTDIR/PERMset), and enters the cleanup loop. Under ASAN:Fix
allocator.destroy(this)fromMarkedArrayBuffer.destroy(). The struct is passed/stored by value; callers own its storage.MarkedArrayBuffer.init()(the only function that heap-allocated the struct, zero callers) so there's no pairing that would leak..destroy(), which still checksthis.allocatorbefore freeing bytes — JS-owned buffers remain untouched.Also fixed the adjacent
Direntarm of the recursive-sync error cleanup:result.name.deref()→result.deref()soDirent.pathis released too (matching the non-recursive and async cleanup sites).Verification
New test in
test/js/node/fs/fs.test.tscreates a temp dir with files + a self-referential symlink, spawns a subprocess that callsreaddirSync({encoding:'buffer', recursive:true}), and asserts it throwsELOOPand exits 0.zig:check-allpasses on all targets.