Repository navigation
Conversation
…e default length uses it FileHandle.write(view, offset) computed the default length, view.byteLength - offset, before anything had checked offset. A bigint or a symbol made the subtraction throw a TypeError with no code, an object had its valueOf() called before the native write refused it, and a function was taken for a callback by the native parser, so the whole view was written. Check the type of offset where Node calls validateInteger(), before the subtraction. The native write still checks the integer and its range. The rejection is awaited, so it settles on the same turn as the native check and as Node's.
|
Status: closed in favour of #47036, which covers this fix. All 14 tests of this PR's test file pass unchanged on a build of #47036 (6cdda02). How it was reproduced (Node v26.3.0 against Bun 1.4.3 and main): import { open } from "node:fs/promises";
const fh = await open("/tmp/fh-write-offset.txt", "w+");
const buf = Buffer.from("abcd");
// Node: ERR_INVALID_ARG_TYPE. Bun: undefined ("Invalid mix of BigInt and other type in subtraction.")
await fh.write(buf, 1n).catch(e => console.log(e.code));
// Node: 0. Bun: 1
let calls = 0;
const offset = { valueOf: () => (calls++, 1) };
await fh.write(buf, { offset, length: null }).catch(() => {});
console.log(calls);
// Node: ERR_INVALID_ARG_TYPE. Bun: 4 (the whole view is written)
await fh.write(buf, () => {}).then(r => console.log(r.bytesWritten), e => console.log(e.code));
await fh.close();On a build of #47036 the three lines print what Node prints. |
|
Updated 3:20 PM PT - Oct 20th, 2026
✅ @robobun, your commit 80d79ebc625cf213501b8d95b0cb807f9e1825cd passed in 🧪 To try this PR locally: bunx bun-pr 46220That installs a local version of the PR into your bun-46220 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Walkthrough
ChangesOffset validation
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the primary change: rejecting non-number offsets in FileHandle.write before default length calculation. |
| Description check | |
The description is detailed and covers the problem, fix, validation, tests, compatibility behavior, performance impact, limitations, and related work. It does not use the exact template headings, but … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description is detailed and covers the problem, fix, validation, tests, compatibility behavior, performance impact, limitations, and related work. It does not use the exact template headings, but it provides the required information under equivalent sections.
- 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.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked two things that look wrong but are not: { offset: 1n } with no length still rejecting with a code-less TypeError is what Node does too (its options destructure default performs the same subtraction before validateInteger runs), and the test file's fs.mkdtempSync instead of the harness tempDir is required because the file runs under node:test on real Node and cannot import harness, as its existing header already does.
Extended reasoning...
The change adds a three-line typeof offset !== "number" arm in FileHandle.write in src/js/node/fs.promises.ts and seven tests in test/js/node/fs/fs-write-offset-bound.test.ts; it touches no security-sensitive surface. Inline findings cover the rejection-timing inconsistency with the sibling throw sites and the hand-rolled check versus validateInteger, so a human should weigh those; the note above records the two candidate issues examined and ruled out.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/js/node/fs.promises.ts— Callers of FileHandle.write get rejection timing that depends on which invalid argument they passed. The new arm at src/js/node/fs.promises.ts:651 defers the rejection one microtask withreturn await Promise.$reject, while the sibling exits at line 659 and line 661 stillthrowand reject on the same turn. Node routes every one of these throughfsCall()'s await, so all reject one turn later. Fix: give every validation exit in this method the same delivery timing as Node (await the rejection at lines 659 and 661 too, or validate in one place before a single awaited exit), rather than only the new arm.Why this was flagged
Trigger: a user passes a non-string, non-view buffer (e.g. fh.write(123)) or a bad encoding (fh.write("ab", null, "bogus")) through src/js/node/fs.promises.ts:635. Line 659 throws $ERR_INVALID_ARG_TYPE synchronously inside the async function, and line 661 validateEncoding throws likewise, so the returned promise is already rejected when write() returns. The new arm at line 651 instead awaits Promise.$reject, so a bad offset rejects one turn later. The PR's test at test/js/node/fs/fs-write-offset-bound.test.ts:195-203 pins the delayed turn as Node's behaviour, which shows the author considers timing part of the contract; the untouched sibling exits violate that same contract. The base branch already had the line 659 timing, but the PR introduces the disagreement between exit sites of one method. The 'covered' nit at line 651 is about the one-off
return await Promise.$rejectform, not about lines 659/661 being on the wrong turn, so a fix to the nit (switching tothrow) would not fix this and vice versa.Verification: Pre-existing on the two untouched exits. Trigger: fh.write(123) or a bad encoding, observing settlement order. The new arm at src/js/node/fs.promises.ts:651 does
return await Promise.$reject(...), so it rejects one microtask later. The sibling exits at line 659 and line 661 throw synchronously, rejecting at tick 0, unchanged from base, while Node surfaces all one tick later through fsCall's await.
| } else if (typeof offset !== "number") { | ||
| // Type half of Node's validateInteger(). Awaited to reject on the turn of the native integer and range check. | ||
| return await Promise.$reject($ERR_INVALID_ARG_TYPE("offset", "number", offset)); |
There was a problem hiding this comment.
🟡 nit (optional): Maintainers get a hand-rolled offset type check in FileHandle.write that differs from the sibling FileHandle.read, which calls validateInteger(offset, "offset", 0) at src/js/node/fs.promises.ts:553. The new arm at src/js/node/fs.promises.ts:649-651 spells the type half of the validator with typeof and a return await Promise.$reject(...) shape used nowhere else in src/js. Fix: use the shared validator in the same shape as the read sibling (else { validateInteger(offset, "offset", 0); }), or keep the typeof form and align read to it, so both entry points share one error path; the timing test at test/js/node/fs/fs-write-offset-bound.test.ts:148 would go with the validator form.
Why this was flagged
A caller passes a non-number offset to FileHandle.write, e.g. fh.write(buf, 1n), and reaches the new arm at src/js/node/fs.promises.ts:649. The arm rejects with $ERR_INVALID_ARG_TYPE via return await Promise.$reject(...) instead of calling validateInteger as FileHandle.read does at src/js/node/fs.promises.ts:553 for the identical input. Behaviour for users is the Node code and message, so there is no user-visible defect over the base, which threw a code-less TypeError. The cost is a divergent error-path convention between two sibling methods in one file; the PR description itself raises this as an open question and the repository guidance asks for shared validators over typeof checks. This is a style nit, not a correctness bug.
Verification: nit. The new arm at /home/claude/bun/src/js/node/fs.promises.ts:649-651 is a hand-rolled type check that re-spells only the type half of validateInteger, which the sibling FileHandle.read uses for the identical slot at line 550-554. Triggering condition: any non-number offset to FileHandle.write. User-visible behaviour is correct, so nothing breaks; the cost is maintainer-facing divergence from the file's own convention.
There was a problem hiding this comment.
Not changed. The choice is a question for a maintainer, and the PR description lists it (Notes, "Questions for a maintainer"). The trade-offs:
validateInteger(offset, "offset", 0)is Node's statement and the same asFileHandle.read. It adds one JS-to-C++ call to each write that passes an offset, and the native binding then checks the integer a second time. It also rejects one microtask turn before Node, for type errors and for range errors.typeofwith$ERR_INVALID_ARG_TYPEis the shape of thebuffercheck of this same method. A valid write makes no new call.- The awaited rejection keeps the turn that the native check has on main. With a plain
throw, 353 of the 1,333 probed argument combinations that equal Node in every field on main reject one turn early.
Each alternative is a one-line change, and the rejection rows of the test pass with each. The turn test goes with either.
FileHandle.read stays on validateInteger. The C++ validator prints Node's digit grouping for a large out-of-range offset, and the native one does not, so the typeof form there would lose that text.
…e.write The rows for an out-of-range offset asserted the name and the code only. Assert Node's message for each row where Bun prints the same text. The 2 ** 53 row keeps name and code: Node groups the digits of the received value and Bun does not. The buffer row asserts the whole message, and the closed-handle row asserts the syscall.
|
On the timing of the two other validation exits of
The two test suggestions are in 80d79eb. |
There was a problem hiding this comment.
The follow-up commit addresses the message assertions, and I found no bugs in the fix itself; a maintainer still needs to settle the open spelling question (typeof + Promise.$reject versus validateInteger as in FileHandle.read), which is a design call rather than a correctness one.
What was reviewed:
- Checked the one-turn timing claim in the comment at src/js/node/fs.promises.ts:650:
writeisasyncWrap(fs.write), an async wrapper, so a synchronous native throw becomes an already-rejected promise thatawaitsurfaces one turn later;return await Promise.$reject(...)lands on the same turn, so the test at test/js/node/fs/fs-write-offset-bound.test.ts:148 measures a real property. - Confirmed the new arm sits outside the
try { this[kRef]() ... }block, so a refused write never refs the handle. - Confirmed the check uses only
typeofand the$ERR_*/Promise.$rejectintrinsics, with no user-overridable machinery on the path.
Extended reasoning...
The change adds a three-line type check to FileHandle.prototype.write in src/js/node/fs.promises.ts and appends seven node:test-style tests to an existing test file; the follow-up commit 80d79eb only tightens test assertions. It touches no security-sensitive surface, only the error contract for a non-number offset. The code is small and I verified the microtask-timing rationale against asyncWrap, but the PR description itself asks a maintainer to choose between the typeof spelling and the shared validateInteger used by the read sibling, and that unresolved design choice plus the novel return await Promise.$reject shape in src/js is why a human look is still worthwhile. Sibling entry points (fs.writeSync, fs.promises.write) are explicitly excluded in favor of #45837.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
|
Closing: #47036 covers this. It removes the JS length default from |
Problem
filehandle.write(buf, 1n)rejects with aTypeErrorthat has nocode("Invalid mix of BigInt and other type in subtraction."). Node rejects withERR_INVALID_ARG_TYPE. An object offset has itsvalueOfcalled first, and a function offset writes the whole view.src/js/node/fs.promises.ts:650:length = buffer.byteLength - offsetruns before any check ofoffset.Fix
FileHandle.writerejects a non-number offset withERR_INVALID_ARG_TYPE, where Node callsvalidateInteger(offset, 'offset', 0)(lib/internal/fs/promises.jsL1384).test/js/node/fs/fs-write-offset-bound.test.ts(7 new tests, 3 fail on main, all pass on Node v26.3.0). Alsopromises.test.js,fs.test.ts, 7 vendored Node tests.writeSynccheck (fs: read offset, length and position as independent slots in write and writeSync #45837 owns it).Background
FileHandle.writeis JS over a native binding. The JS handles the options form and defaults. The binding throws the type and range errors.validateInteger: one more JS-to-C++ call per write. Considered native defaults: they need fs: read offset, length and position as independent slots in write and writeSync #45837 and an INT32 bound.Downsides
fs.writeSync(fd, view, fn)andfs.promises.write(fd, view, fn)still write the whole view. fs: read offset, length and position as independent slots in write and writeSync #45837 fixes both.Notes
Questions for a maintainer
.claude/docs/landing-prs.mdsays to use the shared validators and no hand-rolledtypeofchecks. Node spells this statementvalidateInteger(offset, 'offset', 0), asFileHandle.readdoes atfs.promises.ts:553. This PR usestypeofwith$ERR_INVALID_ARG_TYPE, so a valid write makes no call into C++ and the integer check runs once, inargs::Write::from_js. If you want Node's spelling, the arm becomeselse { validateInteger(offset, "offset", 0); }.return await Promise.$reject(...)is not used anywhere else insrc/js. A plainthrowis simpler, but it rejects one microtask turn before Node and before main: 353 rows of the matrix below equal Node in every field on main and would then differ in the turn. With the awaited form that count is 0. The testa refused offset rejects one turn after the call, as in nodeholds the turn. If you prefer thethrow, that test goes with it.Repro (Node v26.3.0 against main). With this PR every row equals the Node column.
Reach. The bug needs an offset that is not a number. No valid call changes. Searches of the tracker for
filehandle.write offsetand"Invalid mix of BigInt"find no report of this symptom. The vendoredtest-fs-promises-write-optional-params.jscovers only{ offset: false }and{ offset: true }, which main already rejects, so this PR turns on no new Node test.Rows that still differ from Node, or equal it in a way that looks wrong
{ offset: 1n }with no length is a bareTypeErrorin Node too: its destructuring default does the same subtraction. Unchanged, and tested.-1,1.5,NaN,2**53) is stillERR_OUT_OF_RANGEfrom the native parser. For2**53the message has no digit grouping (9007199254740992, Node prints9_007_199_254_740_992). Unchanged.(view, 0, NaN)writes 0 bytes, Node throwsERR_OUT_OF_RANGE. fs: read offset, length and position as independent slots in write and writeSync #45837 covers the length slot.Matrix (54 offset kinds x 20 argument forms on a Buffer, plus 4 other view kinds: 1,333 rows, Node v26.3.0 against main and this PR)
NaNlength rows)valueOforSymbol.toPrimitivecalls differsRegressions: 0. No row that equals Node on main changes in any recorded field.
Overlap with #45837 (a deduction from the matrix and from that PR's diff. No build of #45837 was run.)
code, 20 rows with an extra hook call, and 32 function rows where a uservalueOforSymbol.toPrimitivestill runs once.fs.write/fs.writeSyncignorepositionwhenoffsetorlengthis omitted #45607).FileHandle.writecannot move into the parser with fs: read offset, length and position as independent slots in write and writeSync #45837 alone. Node bounds a defaulted length by INT32. Bun gets that bound only because the JS default makes the length a number:fh.write(view)with a 2 GiB view rejects withERR_OUT_OF_RANGElike Node, andfs.writeSync(fd, view)returns 2147479552 where Node throws. Both parsers bound a numeric length only.Self-review concerns not taken
fs.writeSyncfor a function offset (1 concern).fs.promises.write(fd, view, fn)calls the binding directly and would still write. Only the parser change in fs: read offset, length and position as independent slots in write and writeSync #45837 reaches all three callers.Cost measurements (main against this PR, production bundle from
bun scripts/build.ts --profile=release --mode=codegen)await, main to PR:(buf, 0, 4, 0)72 to 76,(buf)84 to 85,(buf, 1)82 to 86,(buf, {})98 to 102 (BUN_JSC_dumpGeneratedBytecodes=1). The new instructions on a valid path areresolve_scope,get_from_scope,is_numberandjtrue, or onejmpwhen no offset is passed.FileHandle.writebody: 174 to 196 bytecode instructions, 1,013 to 1,118 bytes. The generator frame keeps 2 slots and 22 callee registers. The async wrapper stays at 43 instructions.validateIntegerhost calls per 100fh.writecalls: 0 (lldb breakpoint count over 5 valid shapes. The same count forfh.read(buf, 0, 4, 0)is 100).fh.write(buf, 0, 4, 0): the same on main and here (Promise 3, JSLexicalEnvironment 2, Function 1, Object 1, Array 1, AsyncFunctionGenerator 1, frombun:jscheapStats over 2,000 calls). A refused write allocates one more Promise than athrowwould.fs.promises.js: 43,573 to 43,712 bytes. The module blob that release builds link (InternalModuleRegistryConstants.bin): 2,710,570 to 2,710,709 bytes.perf,valgrindandstraceare not installed in the build container. The bytecode counts come from the production bundles, loaded by the debug binary. No release binary was built.Related PRs
fs.promises.ts.FileHandle.readonly.Suites run with the debug build
test/js/node/fs/fs-write-offset-bound.test.ts(30 runs in a row),test/js/node/fs/promises.test.js.test/js/node/fs/fs.test.ts: 6 to 7 tests time out per run in this container, on main and on this branch, a different set each run (readdir ... x 100,rmdir,createReadStream, callbackfs.write). Each passes alone with-t, and none callsFileHandle.write.test-fs-promises-write-optional-params.js,test-fs-promises-file-handle-write.js,test-fs-write-optional-params.js,test-fs-promises-file-handle-read.js,test-fs-write-buffer.js,test-fs-promises.jsandtest-fs-promises-file-handle-writer.js.