Skip to content

node:fs: check that the callback of fs.read and fs.fstat is a function - #47995

Open
robobun wants to merge 2 commits into
mainfrom
robobun/136f5687/fs-read-fstat-callback-check
Open

node:fs: check that the callback of fs.read and fs.fstat is a function#47995
robobun wants to merge 2 commits into
mainfrom
robobun/136f5687/fs-read-fstat-callback-check

Conversation

@robobun

@robobun robobun commented Nov 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Problem

  • fs.read(fd, buffer, 0, 4, 0, 5) returns, reads into buffer, then raises an uncaught TypeError: callback is not a function. Node.js throws ERR_INVALID_ARG_TYPE from the call. fs.fstat has the same fault.
  • fs.read tests !callback (src/js/node/fs.ts:290) and fs.fstat tests nothing (:167). guardCallback then wraps the value in a function.
  • No user reported it: Node.js rejects every such call. fix(fs): match Node read and write argument defaults #44476 fixes fs.read in a wider change and node:fs: honor options.signal in the async stat family #47310 has the fs.fstat line. This PR is the narrow part of both.

Fix

  • fs.fstat calls ensureCallback, as fs.stat does. fs.read tests $isCallable(callback) before the native call.
  • fs.read needs more: Node.js checks fd, options and buffer first. Each JS throw path repeats the earlier checks (throwInvalidReadCallback).
  • Correct: all 41 calls of the order tests give node's exact error (main: 18).
  • Verified: fs-read-buffer-before-offset.test.ts (18 of 20 new tests fail on main, all pass under node --test). test-fs-read.js and test-fs-read-type.js equal upstream again and need this fix. Self-reviewed: 6 concerns raised, 6 addressed.

Background

Downsides

  • Not fixed: fs.read(fd, b, undefined, cb) reads where Node.js throws for options. So fs.read(fd, b, undefined, 5) names cb.
  • Per valid call: fs.read about +90 interpreter instructions (one is_callable), fs.fstat +385 (one ensureCallback call). The JIT hides both. The binary grows 332 bytes.
Notes

Where this comes from. No user reported it. An automated comparison with Node.js v26.3.0 and v26.12.1 found it. The report asked for a fix or for the number of the open PR that has it. The answer to the second part is #44476 for fs.read and #47310 for fs.fstat. Neither has a human review. #12412 added ensureCallback to 35 adapters and left these two out. #43728 lists the gap under "Not in this PR".

Repro. node r.mjs prints threw ERR_INVALID_ARG_TYPE. Debug and release builds of main b1007f1177 print returned, then uncaught TypeError buffer="impo". The full text is TypeError: callback is not a function. (In 'callback(a, b, c)', 'callback' is 5).

import fs from "node:fs";
const fd = fs.openSync(import.meta.filename, "r"), b = Buffer.alloc(4, 46);
process.on("uncaughtException", e => console.log("uncaught", e.code || e.name, "buffer=" + JSON.stringify(b.toString())));
try { fs.read(fd, b, 0, 4, 0, 5); console.log("returned"); } catch (e) { console.log("threw", e.code); }

Probe rows. Each row is a child process. Node.js v26.3.0 and this PR agree in every row.

call main b1007f1177 Node.js, this PR
fs.read(fd, b, 0, 4, 0, X), X is 5, "x", {}, true returns, uncaught TypeError, buffer filled throws ERR_INVALID_ARG_TYPE, buffer untouched
fs.read(fd, { buffer: b }, 5), fs.read(fd, b, { length: 4 }, 5), fs.read(fd, 5) returns, uncaught TypeError throws
fs.fstat(fd), fs.fstat(fd, 5) returns, uncaught TypeError throws
fs.read(fd, b, 0, 4, 0, null), fs.read(fd, b, 0, 4, 0) throws, names "callback" throws, names "cb"
fs.read(fd, b, 0, 4, 0, () => {}) reads reads

Siblings. I put 8 values that are not functions (5, "x", {}, [], true, null, undefined, 0) into 81 callback slots of node:fs (every callback API and overload). Only fs.read (4 slots) and fs.fstat (2 slots) start the operation where Node.js throws. guardCallback has 16 callers (9 in fs.ts, 7 in dns.ts). The other 14 test the value first.

Why the test is in the JS adapter. The native arm has its own test (split_callback, src/runtime/node/node_fs_binding.rs:60-73). It receives the function that guardCallback made, so it cannot fail. It also runs before the native parser reads fd and buffer, so a test there would name cb first too. Node.js has the test in lib/fs.js (read: line 634, after fd at 600 and buffer at 633. fstat: line 1579, first).

Order of the checks in fs.read. Node.js: fd, options, buffer, callback, offset, length, position. Bun: options and the callback in JS, then fd, buffer, offset, length and position in the native parser (Read::from_js, src/runtime/node/node_fs.rs). So each JS throw path repeats the checks that Node.js makes earlier. The callback path calls throwInvalidReadCallback, which runs validateInteger(fd, "fd", 0, 2147483647) and the isArrayBufferView test first. The options path runs the same fd check before it throws (second commit, from a review finding). A valid call runs none of this.

  • The order tests hold 41 calls. 38 have two invalid arguments (of fd, options, buffer, callback, offset, length and position). All 41 give node's code, class and message. Main gives it in 18.
  • The 33 of those calls that have an invalid callback and valid options, with callback = ensureCallback(callback) in fs.read (bundled text patched in a private mount): 12 give node's error, and the 16 that match on main all name cb.
  • The helper does not call the shared validateBuffer. Bun's validateBuffer accepts undefined (src/jsc/bindings/NodeValidatorBodies.h:322), and Node.js does not. The test "an undefined buffer gets the buffer error" fails with that substitution.
  • 32 fd values and 23 buffer values with an invalid callback: the text equals node's in all 55.
  • The same fd values with a valid callback go to the native parser. Its text differs from node's in 3 of 32 (no digit separators for an fd above 2^32). Not changed here.
  • With a valid callback the native parser accepts an ArrayBuffer and a SharedArrayBuffer as the buffer. Node.js does not. Not changed here. With an invalid callback both now get node's buffer error.
  • Each clause of the change has a test that fails without it. Without the fd check of the helper: 2 tests. Without the buffer check: 2. With a buffer check that accepts undefined: 1. Without the fd check before the options error: 1. With !callback in fs.read: 13. With wrapFsCallback in fs.fstat: 3.

Cost (main b1007f1177 against this PR, Linux x64, release builds from one tree, except where a debug build is named).

  • Executed bytecode on a valid call, from a bytecode dump of the debug build. fs.read: 31 to 32 (one is_callable). fs.fstat: its own 18 do not change, and ensureCallback adds 7 and one call. fs.stat and fs.lstat pay the same today.
  • User-space instructions of the JS thread per call with the JIT off (BUN_JSC_useJIT=0). The call has a valid callback and an offset or an fd that the native parser refuses, so the adapter and the parser run and nothing is queued. Slope between 100,000 and 300,000 calls, least of 3, two rounds. fs.read, three rounds: 10,632, 10,666 and 10,636 on main, 10,721, 10,739 and 10,746 here (+90 by the means). fs.fstat: 8,548 and 8,548 on main, 8,933 and 8,930 here (+385).
  • The same count with the JIT on, for calls that run on the pool and call back. Slope between 100,000 and 400,000 calls, least of 5, two rounds. fs.read: 2,868 and 2,923 on main, 2,907 and 2,915 here. fs.fstat: 2,257 and 2,258 on main, 2,257 and 2,262 here. The difference is below the spread of one binary.
  • Objects that a pending valid call keeps alive (heapStats, 1,000 pending calls): fs.read 2 Function and 2 scopes, fs.fstat 1 and 1, 0 Promise. The same as main.
  • First load of node:fs: +3 heap cells (one Function, one FunctionExecutable and one UnlinkedFunctionExecutable for throwInvalidReadCallback) and +308 bytes of heap. The bytecode of the helper is generated only when a call is refused. Instructions to run require("node:fs"), least of 15, two rounds: 12,717,101 and 12,712,643 on main, 12,713,718 and 12,718,154 here.
  • Built-in text: node/fs.js 30,978 to 31,310 bytes (+332). Binary text (size): 140,290,352 to 140,290,684 (+332). data and bss do not change. No host function and no binding member is added.
  • Bytecode size: fs.fstat stays at 80 bytes. The DFG inline limit (maximumFunctionForCallInlineCandidateBytecodeCostForDFG) is 80. fs.read goes from 613 to 648, all of it on the throw paths.
  • The other spelling of fs.fstat, an inline if (!$isCallable(callback)) throw ... before wrapFsCallback, is the statement that node:fs: honor options.signal in the async stat family #47310 has. I built it too, before the second commit: +98 instructions per call with the JIT off (against +385), no difference with the JIT on, 123 bytes of bytecode (over the DFG limit) and 93 more bytes of text. This PR uses ensureCallback because the other stat adapters do.
  • The operation that must not start. Read syscalls of the process (/proc/self/io) around 100 refused calls: 8, with 6 for no call (main: 108). Around 1,000: 9 (main: 1,008). Event-loop references that one refused fs.read and one refused fs.fstat take: 0 (main: 2). node.fs.async trace events for the two: 0 (main: 2 begin and 2 end, Node.js: 0).
  • valgrind and perf are not installed on this machine. The instruction counts come from perf_event_open on the first thread of the process.

Not in this PR. Each row differs from Node.js before and after it.

  • undefined in the options slot of fs.read. The test at fs.ts:263 (on main) looks at params, which is still null there, so fs.read(fd, buffer, undefined, cb) reads where Node.js throws for options. A program can depend on that read today, so this PR leaves it. fix(fs): match Node read and write argument defaults #44476 changes it.
  • fs.readSync(fd, "x", 5) names options where Node.js names buffer. It is the same order fault in the sync function. fix(fs): match Node read and write argument defaults #44476 changes it.
  • 14 other callback APIs test the callback before an operand that Node.js tests first (fs.open(p, "r", {}) names cb, Node.js names mode). Both throw from the call.
  • fs.glob names "callback" where Node.js names "cb". fs.rm(p, {}, 5) throws where Node.js runs the removal.
  • Dir.read(null) and Dir.close(null) return a promise where Node.js throws.
  • A zero-length fs.read queues a job. Node.js calls back from process.nextTick and does not touch the fd.
  • wrapFsCallback and guardCallback still wrap any value. Each of their callers now tests first.
  • The native arm could take the user callback unwrapped and remove the 2 closures that each fs.read allocates. That change touches the binding files that bindgen: delete b.any and .variadic #46789 and Remove libuv on Windows #47776 rewrite.

Other open PRs on these lines (git merge-tree against main b1007f1177 and against this branch).

Tests. test/js/node/fs/fs-read-buffer-before-offset.test.ts uses node:test and node:assert, so the same file runs under Node.js. It had 5 tests. It now has 25.

  • 11 tests for the callback: the 5 call forms of fs.read with the 8 values (code, class and message, then a read at the current position to show that nothing moved it), the argument count, length.valueOf not called, an async function, a bound function and a Proxy still accepted, fs.fstat with 2 and 3 arguments, the callback before the fd in fs.fstat, and util.promisify of both (a rejection where main never settles).
  • 9 tests for the order, with the 41 calls.
$ node --test test/js/node/fs/fs-read-buffer-before-offset.test.ts      # v26.3.0
ℹ tests 25
ℹ pass 25
ℹ fail 0

$ bun bd test test/js/node/fs/fs-read-buffer-before-offset.test.ts      # this PR, debug build
 25 pass
 0 fail

$ bun test test/js/node/fs/fs-read-buffer-before-offset.test.ts         # release build of main b1007f1177
 7 pass
 18 fail
  • Two new tests pass on main. "the fd error comes first" has the rows that main gets right only because it does not test the callback. It fails when the helper has no fd check. "the options error comes before the callback error" holds the rows with a valid fd, which must keep the options error.
  • test-fs-read.js and test-fs-read-type.js had 7 message: assertions commented out since node fs compat pr #2 #16422. All 7 are restored, and both files are byte for byte the files of nodejs/node v26.3.0 (blobs 966185c5 and 81ad7ecb). Both exit 1 on main and 0 here. 2 of the 7 need this change (fs.read(null, buffer, 0, 1, 0) names fd, and fs.read(fd, 4, 0, "utf-8", fn) names buffer). The other 5 already pass on main.
  • Also run with this build: all of test/js/node/fs/fs.test.ts (633 pass, 10 skip, 0 fail), 10 other files of test/js/node/fs/ (179 pass), test/internal/source-lints/ (194 pass), and 397 files of Node's test/parallel (test-fs-*, test-util-promisify*, test-filehandle*, test-trace-events-fs*, test-http2-respond-file*, test-stream-pipeline*, test-readline*, test-file-*), all pass.
  • Not run here: Windows and macOS. The change is JS only.

@robobun
node:fs: check that the callback of fs.read and fs.fstat is a function
5d4d4a0 
fs.read tested its callback with `!callback`, and fs.fstat did not test
it. A truthy value that is not a function (5, "x", {}, true) passed.
guardCallback wrapped it in a function, so the native callback arm saw a
function, ran the read or the stat on the thread pool, and the completion
failed with an uncaught `TypeError: callback is not a function`.

fs.fstat now calls ensureCallback, as fs.stat and fs.lstat do. fs.read
tests `$isCallable(callback)` before the native call. Both throw
ERR_INVALID_ARG_TYPE for "cb" from the call, as Node.js does.

Node.js checks fd and buffer before the callback in fs.read. The native
parser checks them after this test, so the throw path of fs.read repeats
those two checks first (throwInvalidReadCallback). A call with two
invalid arguments names the argument that Node.js names.

test-fs-read.js and test-fs-read-type.js are the upstream v26.3.0 files
again. Two of the seven restored message assertions need this change.
@github-actions github-actions Bot added the claude label Nov 6, 2026
@robobun

robobun commented Nov 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author
Updated 11:09 PM PT - Nov 5th, 2026

❌ @robobun, your commit b2d465b has 38 failures in Build #137919 (All Failures):

🧪   To try this PR locally:

bunx bun-pr 47995

That installs a local version of the PR into your bun-47995 executable, so you can run:

bun-47995 --bun

@robobun

robobun commented Nov 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Status: the fix is pushed and waits for review.

How I reproduced it

// r.mjs
import fs from "node:fs";
const fd = fs.openSync(import.meta.filename, "r"), b = Buffer.alloc(4, 46);
process.on("uncaughtException", e => console.log("uncaught", e.code || e.name, "buffer=" + JSON.stringify(b.toString())));
try { fs.read(fd, b, 0, 4, 0, 5); console.log("returned"); } catch (e) { console.log("threw", e.code); }
  • Node.js v26.3.0: threw ERR_INVALID_ARG_TYPE
  • Main b1007f1177, debug and release builds: returned, then uncaught TypeError buffer="impo"
  • This PR: threw ERR_INVALID_ARG_TYPE

With fs.fstat(fd) in place of the fs.read call, Node.js and this PR print threw ERR_INVALID_ARG_TYPE. Main prints returned, then uncaught TypeError buffer="....".

CI (build 137919, commit b2d465badd, finished)

  • No test of this PR failed on any lane. fs-read-buffer-before-offset.test.ts ran on Linux, Windows, macOS and the ASAN lane (25 of 25). test-fs-read.js and test-fs-read-type.js passed too. No node:fs test failed.
  • The build is red on 28 test files: 27 under test/aot/ and test/bundler/compile-aot-size.test.ts. Each one also fails on main (build 137822, commit b1305ddc7c). This PR does not touch them.

PR: #47995

@coderabbitai

coderabbitai Bot commented Nov 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

fs.read() now validates descriptor and buffer arguments before reporting an invalid callback. fs.fstat() now rejects invalid callbacks. Tests cover callback behavior, validation order, and exact filesystem error messages.

Changes

Filesystem callback validation

Layer / File(s) Summary
Callback validation and behavior tests
src/js/node/fs.ts, test/js/node/fs/fs-read-buffer-before-offset.test.ts		 fs.read() checks the descriptor and buffer before reporting an invalid callback. fs.fstat() validates its callback. Tests cover callback values, overloads, promisified calls, and argument-validation order.
fs.read error-message assertions
test/js/node/test/parallel/test-fs-read-type.js, test/js/node/test/parallel/test-fs-read.js		 Asynchronous and synchronous tests assert exact error messages for invalid buffers, descriptors, and lengths.

Suggested reviewers: jarred-sumner

Merge Risk: Minimal · up to b2d46

No actionable issue is established; the change is mergeable after normal checks.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly describes callback validation for fs.read and fs.fstat.
Description check Passed The description explains the changes and verification. It uses different headings from the template but provides the required information.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 5d4d4a0 and b2d465b.

Files selected for processing: 2

  • src/js/node/fs.ts
  • test/js/node/fs/fs-read-buffer-before-offset.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

claude[bot]
claude Bot reviewed Nov 6, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I also checked the remaining bare wrapFsCallback callers in src/js/node/fs.ts (close, the 4-argument writeFile overload, opendir) for the same unchecked-callback bug — each gates on $isCallable/validateFunction first, so fs.read and fs.fstat were the only two. The fstat callback-before-fd order and the read fd -> buffer -> cb order in throwInvalidReadCallback match Node's makeCallback/getValidatedFd/validateBuffer sequence, so the new ordering assertions certify Node's behavior, not a Bun quirk.

Extended reasoning...

The change adds a 7-line throwInvalidReadCallback helper and two one-line call-site edits in src/js/node/fs.ts, plus ~230 lines of node:test-style tests and re-enabled message assertions in two ported upstream tests; it touches no security-sensitive surface. The only confirmed finding is a pre-existing options-vs-fd ordering gap that the PR narrows but does not close. The re-enabled message assertions in test-fs-read-type.js exercise the native parser (readSync, valid-callback read) rather than the JS edits, and I could not run them, so a human should confirm CI on those files.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟣 src/js/node/fs.ts — pre-existing: callers passing a bad fd together with a bad options argument still get the "options" error from fs.read, where Node reports the fd first. The new throwInvalidReadCallback only orders fd before buffer and cb; the options throws at src/js/node/fs.ts:273 and :294 still run before any fd check, since fd validation is deferred to native. Fix: on every synchronous error path of fs.read, including the two options throws, run validateInteger(fd, "fd", 0, 2147483647) before throwing, so doubly-invalid calls name "fd" as Node does while valid calls keep skipping JS fd validation.

    Why this was flagged

    A user calls fs.read(-1, buffer, 0, 5) or fs.read(-1, buffer, "x", cb) (argc 4, non-object options). At src/js/node/fs.ts:268-274 params is set from offsetOrOptions and at :293-295 the typeof check throws ERR_INVALID_ARG_TYPE for "options". Node runs getValidatedFd(fd) first (the PR's own cited lib/fs.js#L600) and throws ERR_OUT_OF_RANGE for "fd". The base branch behaves the same, and the PR description lists this under Downsides as not fixed; it is reported because the PR adds fd-before-X ordering for the cb path at :299 and :49-55 but leaves the sibling options path in the same function with the wrong winner. The fix is cheap because it only runs on the error path, like throwInvalidReadCallback.

    Verification: pre-existing. A caller passes an out-of-range fd with a non-object options argument, e.g. fs.read(-1, buffer, 0, 5). In /home/claude/bun/src/js/node/fs.ts, line 294 throws the options TypeError before any fd check; throwInvalidReadCallback (lines 48-55) is reached only from line 299, after line 294 has thrown. Base branch has identical lines, so the base already fails by the same route.

@robobun
node:fs: check the fd of fs.read before its options error
b2d465b 
The options test of fs.read runs in JS, and the fd test is in the
native parser. A call with an invalid fd and an invalid options
argument named "options". Node.js checks the fd first.

The options throw path now checks the fd first, as the callback throw
path does. A valid call does not run the check.
@robobun

robobun commented Nov 6, 2026

Copy link
Copy Markdown
Collaborator Author

The finding on the options path is right. b2d465b adds the fd check there.

  • Before: fs.read(-1, buffer, 0, 5) and fs.read(-1, buffer, "x", cb) named options. Node.js v26.3.0 names fd. Main does the same, with or without a valid callback.
  • Now: the options throw path of fs.read runs validateInteger(fd, "fd", 0, 2147483647) first. A valid call does not reach it.
  • Only the second options test needed the line. The first one (fs.ts:272) cannot throw, because params is still null there.
  • Tests: "the fd error comes before the options error" in fs-read-buffer-before-offset.test.ts (6 calls). It fails on main, it fails without the new line, and it passes under node --test. "the options error comes before the callback error" holds the rows with a valid fd.
  • All 41 calls of the order tests now give node's exact error (main: 18).

Two rows in the options slot stay as they are. The PR body lists them.

  • fs.read(fd, buffer, undefined, cb) reads where Node.js throws for options. A program can depend on that read today, so I left it.
  • fs.readSync(fd, "x", 5) names options where Node.js names buffer. That is the sync function, which this PR does not touch.

#44476 changes both.

claude[bot]
claude Bot reviewed Nov 6, 2026
View reviewed changes

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I also checked two things and ruled them out: the edits to the two vendored tests in test/js/node/test/parallel/ only uncomment the upstream message: assertions (the text matches nodejs/node verbatim, and the readSync messages they re-enable come from the shared native formatter in src/jsc/JSGlobalObject.rs, so they are not new assertions); and validateInteger(fd, "fd", 0, 2147483647) in throwInvalidReadCallback is equivalent to Node's getValidatedFd (validateInt32(fd, 'fd', 0)), including -0 passing and the an integer vs range message split.

Extended reasoning...

The change tightens callback validation in fs.read and fs.fstat in src/js/node/fs.ts and adds tests; it touches no security-sensitive surface. The inline finding is a pre-existing dead params check the diff leaves in place, and the items above are the additional checks this run ruled out.

Comment thread src/js/node/fs.ts Resolved
@robobun robobun mentioned this pull request Nov 6, 2026
Open

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@claude claude[bot] claude[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Jarred-Sumner Jarred-Sumner

Labels

claude

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@robobun @Jarred-Sumner