-
Notifications
You must be signed in to change notification settings - Fork 5k
node:fs: keep ArrayBuffer storage alive across worker.terminate() during async write #36818
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
robobun
wants to merge
7
commits into
main
Choose a base branch
from
claude/c51f3b14/fs-write-worker-terminate-buffer-lifetime
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+197
−0
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
10ae0db
node:fs: keep ArrayBuffer storage alive across worker.terminate() dur…
robobun 9b7c4bc
[autofix.ci] apply automated fixes
autofix-ci[bot] 13bb29d
trim pinArrayBuffer comment
robobun 8cc9b96
test: require PASS reads>=1 unconditionally; drop negative-error-stri…
robobun 0946896
test: prove fs.read/readv destination buffer survives worker.terminate()
robobun 92f57e3
[autofix.ci] apply automated fixes
autofix-ci[bot] ddbe327
test(fs-read-worker-terminate): disable LSan and drop negative-string…
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, isASAN, isLinux, tempDir } from "harness"; | ||
|
|
||
| // worker.terminate() while an async node:fs read into a user Buffer is parked | ||
| // in read(2) on an empty FIFO: Heap::lastChanceToFinalize would free the | ||
| // ArrayBufferContents while the kernel still holds the destination address, | ||
| // so a later write to the FIFO copies the peer's bytes into freed (and | ||
| // possibly reused) memory. The kernel's copy_to_user is invisible to ASAN, | ||
| // so the oracle is a direct address probe: the worker reports ptr(buf) before | ||
| // parking, the parent terminates it, and bun:ffi's read.u8 at that address | ||
| // either observes the worker's fill byte (storage alive) or trips ASAN | ||
| // heap-use-after-free (storage freed mid-read). The FIFO is never written, | ||
| // so read(2) stays parked and the separate completion-into-dead-VM crash | ||
| // (#34154) is not reached. | ||
| describe.concurrent.skipIf(!isLinux || !isASAN)( | ||
| "worker.terminate() during parked async node:fs read keeps the destination buffer alive", | ||
| () => { | ||
| for (const api of ["read", "readv"] as const) { | ||
| test(`fs.${api}`, async () => { | ||
| const fixture = ` | ||
| import { Worker } from "node:worker_threads"; | ||
| import fs from "node:fs"; | ||
| import path from "node:path"; | ||
| import { execFileSync } from "node:child_process"; | ||
| import { read as ffiRead } from "bun:ffi"; | ||
|
|
||
| const dir = process.argv[2]; | ||
| const API = process.argv[3]; | ||
| const fifo = path.join(dir, "fifo"); | ||
| execFileSync("mkfifo", [fifo]); | ||
| // O_RDWR on a FIFO (Linux) never blocks on open and never delivers EOF, so | ||
| // the worker's read(2) parks on an empty pipe for as long as we need. | ||
| const fd = fs.openSync(fifo, fs.constants.O_RDWR); | ||
|
|
||
| const src = | ||
| "import { parentPort, workerData as d } from 'node:worker_threads';\\n" + | ||
| "import fs from 'node:fs';\\n" + | ||
| "import { ptr } from 'bun:ffi';\\n" + | ||
| "const buf = Buffer.allocUnsafeSlow(8 << 20).fill(0x42);\\n" + | ||
| "if (d.api === 'read') fs.read(d.fd, buf, 0, buf.length, null, () => {});\\n" + | ||
| "else fs.readv(d.fd, [buf.subarray(0, 4 << 20), buf.subarray(4 << 20)], null, () => {});\\n" + | ||
| "parentPort.postMessage({ addr: ptr(buf), len: buf.length });\\n"; | ||
| const wfile = path.join(dir, "w.mjs"); | ||
| fs.writeFileSync(wfile, src); | ||
|
|
||
| const w = new Worker(wfile, { workerData: { fd, api: API } }); | ||
| w.on("error", e => { console.log("WORKER_ERR", String(e)); process.exit(1); }); | ||
| const { addr, len } = await new Promise(r => w.once("message", r)); | ||
| await w.terminate(); | ||
| // Probe both ends of the range the pool thread handed to read(2). Without | ||
| // the native ref this is the first access after lastChanceToFinalize freed | ||
| // the storage and ASAN aborts with heap-use-after-free. | ||
| const head = ffiRead.u8(addr, 0); | ||
| const tail = ffiRead.u8(addr, len - 1); | ||
| if (head !== 0x42 || tail !== 0x42) { | ||
| console.log("FAIL saw 0x" + head.toString(16) + "/0x" + tail.toString(16) + " at freed destination"); | ||
| process.exit(1); | ||
| } | ||
| console.log("PASS addr=0x" + addr.toString(16) + " len=" + len); | ||
| process.exit(0); | ||
| `; | ||
|
|
||
| using dir = tempDir("fs-read-term", { "fixture.mjs": fixture }); | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "--smol", "fixture.mjs", String(dir), api], | ||
| env: { | ||
| ...bunEnv, | ||
| // Route JSC ArrayBuffer storage through system malloc so ASAN | ||
| // owns the allocation and the ffi probe can observe the free. | ||
| Malloc: "1", | ||
| // detect_leaks=0: Malloc=1 surfaces unrelated pre-existing worker | ||
| // teardown leaks to LSan; the oracle here is the ffi probe above, | ||
| // not leak accounting. symbolize=0: the fail-before report is the | ||
| // signal and symbolization adds seconds for nothing we assert on. | ||
| ASAN_OPTIONS: (bunEnv.ASAN_OPTIONS ? bunEnv.ASAN_OPTIONS + ":" : "") + "symbolize=0:detect_leaks=0", | ||
| }, | ||
| cwd: String(dir), | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout + stderr).not.toContain("FAIL"); | ||
| expect(stdout).toMatch(/^PASS addr=0x[0-9a-f]+ len=8388608$/m); | ||
| }, 20_000); | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
| }, | ||
| ); | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { bunEnv, bunExe, isASAN, isWindows, tempDir } from "harness"; | ||
|
|
||
| // worker.terminate() while an async node:fs write of a user Buffer is still | ||
| // inside write(2) on a pool thread: Heap::lastChanceToFinalize would free the | ||
| // ArrayBufferContents while the kernel is still copying from it, so the fd | ||
| // receives freed-heap bytes instead of the bytes the writer held. The reader | ||
| // is the kernel, so the oracle is content: the worker fills its buffer with | ||
| // one known byte and writes only to one FIFO; the parent flags any byte that | ||
| // writer never held. | ||
| describe.concurrent.skipIf(isWindows || !isASAN)( | ||
| "worker.terminate() during in-flight async node:fs write does not write freed memory", | ||
| () => { | ||
| for (const api of ["write", "writev", "writeFile"] as const) { | ||
| test(`fs.${api}`, async () => { | ||
| const fixture = ` | ||
| import { Worker } from "node:worker_threads"; | ||
| import fs from "node:fs"; | ||
| import path from "node:path"; | ||
| import { execFileSync } from "node:child_process"; | ||
|
|
||
| const dir = process.argv[2]; | ||
| const API = process.argv[3]; | ||
| const fifo = path.join(dir, "fifo"); | ||
| execFileSync("mkfifo", [fifo]); | ||
| const rfd = fs.openSync(fifo, fs.constants.O_RDONLY | fs.constants.O_NONBLOCK); | ||
|
|
||
| const src = | ||
| "import { parentPort, workerData as d } from 'node:worker_threads';\\n" + | ||
| "import fs from 'node:fs';\\n" + | ||
| // 8 MiB malloc-backed Buffer filled with this worker's unique byte | ||
| "const buf = Buffer.allocUnsafe(8 << 20).fill(d.gen);\\n" + | ||
| "let fd;\\n" + | ||
| "async function lane() { for (;;) {\\n" + | ||
| " if (d.api === 'write') await new Promise(r => fs.write(fd ??= fs.openSync(d.fifo, 'w'), buf, 0, buf.length, null, r));\\n" + | ||
| " else if (d.api === 'writev') await new Promise(r => fs.writev(fd ??= fs.openSync(d.fifo, 'w'), [buf.subarray(0, 4 << 20), buf.subarray(4 << 20)], null, r));\\n" + | ||
| " else if (d.api === 'writeFile') await new Promise(r => fs.writeFile(d.fifo, buf, r));\\n" + | ||
| "} }\\n" + | ||
| "parentPort.postMessage('up'); lane().catch(() => {});\\n"; | ||
| const wfile = path.join(dir, "w.mjs"); | ||
| fs.writeFileSync(wfile, src); | ||
|
|
||
| const GEN = 0x42; | ||
| const chunk = Buffer.alloc(1 << 16); | ||
| let foreign = 0; | ||
| function drain() { | ||
| let n; | ||
| try { n = fs.readSync(rfd, chunk, 0, chunk.length, null); } catch { return false; } | ||
| if (n <= 0) return false; | ||
| for (let j = 0; j < n; j++) { | ||
| if (chunk[j] !== GEN) { | ||
| let k = j; while (k < n && chunk[k] === chunk[j]) k++; | ||
| if (++foreign <= 4) | ||
| console.log("FOREIGN fifo byte 0x" + chunk[j].toString(16) + " run=" + (k - j) + " writer-held=0x42"); | ||
| j = k - 1; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| const w = new Worker(wfile, { workerData: { fifo, gen: GEN, api: API } }); | ||
| w.on("error", () => {}); | ||
| await Promise.race([new Promise(res => w.once("message", res)), Bun.sleep(5000)]); | ||
| // Let write(2) park on the full pipe, then tear the VM down while the kernel | ||
| // is still copying from the buffer. | ||
| await Bun.sleep(30); | ||
| await w.terminate(); | ||
| // Drain enough of what the in-flight write(2) streamed to observe the bytes | ||
| // it produced after the VM died. Stop well short of the full 8 MiB so the | ||
| // pool thread stays parked inside write(2) and never reaches the dead-VM | ||
| // completion path (that crash is tracked separately). | ||
| let idle = 0, reads = 0; | ||
| while (idle < 20 && reads < 48) { if (drain()) { idle = 0; reads++; } else { idle++; await Bun.sleep(5); } } | ||
| console.log(foreign ? "FAIL foreign-chunks=" + foreign : "PASS reads=" + reads); | ||
| process.exit(foreign ? 1 : 0); | ||
| `; | ||
|
|
||
| using dir = tempDir("fs-write-term", { "fixture.mjs": fixture }); | ||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "--smol", "fixture.mjs", String(dir), api], | ||
| env: { | ||
| ...bunEnv, | ||
| // Route JSC ArrayBuffer storage through system malloc so ASAN | ||
| // free-fills it on release (bmalloc/Gigacage hides the UAF). | ||
| Malloc: "1", | ||
| ASAN_OPTIONS: (bunEnv.ASAN_OPTIONS ? bunEnv.ASAN_OPTIONS + ":" : "") + "max_free_fill_size=268435456", | ||
| }, | ||
| cwd: String(dir), | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout + stderr).not.toContain("FOREIGN"); | ||
| // Prove the oracle reached its verdict and drained at least one chunk; | ||
| // a crash or worker-spawn failure before this point must fail the test. | ||
| expect(stdout).toMatch(/^PASS reads=[1-9]\d*$/m); | ||
| }, 30_000); | ||
|
robobun marked this conversation as resolved.
|
||
| } | ||
| }, | ||
| ); | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.