ReviewOS

home-lang/home

Native fs: pin backing stores for the readv/writev, read and path argument paths

#623
Closed glennmichael123 opened this 10 days ago ·
10 days ago

Scope

Async fs operations hand a raw pointer into a JavaScript typed array's backing store to a thread-pool worker. Nothing stops JavaScript from transferring that store while the operation is in flight, so the worker reads or writes freed memory. Bun pins the backing store for the async path and releases it in the JS-thread completion hook.

Two of the five paths are done; three remain.

Landed (889d4275f, 2b6ec0a09): Write.fromJS and WriteFile.fromJS pin their source buffer and release it in deinitAndUnprotect. js/node/fs/fs.test.ts went 251 pass / 11 fail → 254 pass / 8 fail.

Remaining:

  • fs.writev / writevSync — the vector path takes its buffers through a different struct, so each element needs rooting and pinning. Bun does this by passing will_be_async into VectorArrayBuffer::from_js and calling buffers.release() on the error paths that never reach the unprotect hook. Covers "fs.writev keeps buffers attached while the write is in flight" and "writevSync does not write bytes from a buffer detached by an index getter during argument conversion" (the latter currently writes 24 bytes where 8 are expected — it is reading a buffer that a getter detached mid-conversion).
  • fs.read — the destination view. Covers "fs.read keeps filling the caller's view when its ArrayBuffer is transferred while the read is pending", which currently reports viewByteLength: 0 where Bun reports 65536.
  • readSync — must reject when the length argument's coercion detaches the destination. Currently returns 22 instead of throwing.
  • A Buffer used as the path argument, pinned while the options object's getters run. Covers "fs.promises.writeFile keeps a buffer path argument attached while options are read".

A trap worth knowing before starting

Adding the same pinned field to Arguments.Read and pinning in Read.fromJS regresses the read path: fs.read > should work with (fd, options, callback) and five siblings, plus three createReadStream cases, all start failing. The pin is never released, so the buffer stays permanently non-detachable and subsequent operations on it fail.

Read.deinit is a no-op (pub fn deinit(_: Read) void {}), and the async read completion evidently does not route through Read.deinitAndUnprotect the way the write paths route through theirs. Find the read path's actual release hook first, then pin. I reverted that attempt rather than leave the regression; the write-side pins are unaffected.

Acceptance criteria

  • The four unchanged upstream cases above pass, matching the pinned Bun 1.4.0-canary.1+4982b91 control.
  • No pin outlives its operation: a buffer is detachable again once the call settles, on the success, error and early-return paths alike.
  • No corpus file, fixture, assertion or workload is edited.

Not in scope

Two of the eight remaining fs.test.ts failures are not Home gaps — stat > folder metadata is correct and lstat > folder metadata is correct resolve ../../../../test, a sibling of Bun's own test/ that does not exist where the corpus is copied to. The pinned Bun control fails them identically. lchown succeeds on every platform fails with EPERM and looks like a sandbox/permission property of this machine rather than a port gap; it needs confirming on another host before being treated as either.

Parent parity tracker: #66. Corpus baseline: #621.

10 days ago

Superseded and fully resolved by #637 / d8cdf06.

The remaining writev/read/path backing-store lifetime cases tracked here were all implemented with balanced async completion and rejection release. The unchanged upstream js/node/fs/fs.test.ts now passes 262/262 runnable tests (5,405 assertions) in Debug and true ReleaseFast, with only its 7 upstream platform skips. No corpus source, assertion, timeout, workload, or skip was changed.

Sign in to comment on this issue.