Repository navigation
Conversation
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 finding, I also checked the empty-string encoding path ("" previously hit assert's falsy short-circuit and now misses ENCODING_MAP and falls to unwrap_or — same UTF-8 result either way) and the String-object asymmetry with readFile/writeFile/readdir (pre-existing and disclosed in the description; those calls take an options object in Node, so they need a separate change).
Extended reasoning...
The diff is a 5-line change in args::Write::from_js (src/runtime/node/node_fs.rs:3710-3712) plus two tests. I read Encoding::from_js, Encoding::assert, from_js_with_default_on_empty, in_map_case_insensitive and JSValue::is_string_literal to confirm the new path cannot throw for a primitive string, that non-ASCII names miss the map (UTF-16 narrowing returns None; Latin-1 bytes never case-fold to ASCII keys), and that utf-16le is already in ENCODING_MAP at the stacked base commit so the new test rows pin behavior rather than depending on this PR. The only behavior regression versus base is the "hex\0" name, which is covered by the inline finding; nothing else in the ruled-out set turned up a second issue. No debug build was present, so the tests were assessed by reading rather than run.
|
Updated 2:38 AM PT - Nov 8th, 2026
✅ @robobun, your commit 63df7755a82d9b38f1ef91f448ca26861ec42415 passed in 🧪 To try this PR locally: bunx bun-pr 43435That installs a local version of the PR into your bun-43435 --bun |
|
Status
|
fs.writeSync(fd, "XY", null, "bogus") threw ERR_INVALID_ARG_VALUE and wrote nothing. fs.write and filehandle.write did the same. Node.js through v26 writes the string as UTF-8. For a null or undefined position, Bun 1.3.14 wrote UTF-8 too, and 1.4.0 started to throw (#32813). args::Write::from_js parsed the encoding with Encoding::assert, the port of Node's assertEncoding. Node's WriteString reads that argument with ParseEncoding(args[3], UTF8), which never throws. The parser now does the same: a primitive string that names a known encoding selects it, and every other value means UTF-8. Node.js v27 rejects an unknown name in its JS validateEncoding (nodejs/node#66247). That check is not part of this change.
8ea823e to
63df775
Compare
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 3 minutes. View limit details
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. |
There was a problem hiding this comment.
I reviewed the latest push and didn't find any bugs; because the diff range from the recorded base spans the whole upstream history merged since September (36k files) rather than just this fix, a human look at the two files the PR itself changes would still be worthwhile.
What was reviewed:
- The
is_string()->is_string_literal()switch andEncoding::from_js(..).unwrap_or(default)insrc/runtime/node/node_fs.rsagainst Node'sParseEncoding(args[3], UTF8): aStringobject or unknown name now falls through to UTF-8 and the argument slot is left unconsumed, matching V8'sIsString()semantics; the JS-sidevalidateEncodinginsrc/js/node/fs.tsstill runs first, so the hex even-length check is unchanged. test/js/node/fs/fs-write-unknown-encoding.test.ts: covers sync, callback, promisified, FileHandle andfs.promises.writeforms with known/unknown/object encodings and both null and numeric positions; cleanup is inafter/finallyand no external hosts are touched.- Not done in this run: the test file was read, not executed, so I did not confirm it fails on the pre-fix build.
Extended reasoning...
The PR's own commit changes one argument-parsing branch in src/runtime/node/node_fs.rs (fs.write/writeSync/filehandle.write string form) so an unknown or non-primitive encoding selects UTF-8 instead of throwing ERR_INVALID_ARG_VALUE, and adds a node:test-based regression file. It touches no security-sensitive surface; the only semantic shift is that new String("hex") now means UTF-8 (as in Node) rather than hex. The recorded base SHA is stale, so the review scope covers ~36k upstream files that are not part of this fix, the PR conversation history was incomplete this run, and an earlier inline thread of mine on this exact line was closed by author self-resolution with a reply I cannot see — those facts rule out approving, while the small real change and the lack of findings make a human look an additional step rather than a blocker.
Problem
fs.writeSync(fd, "XY", null, "bogus")throwsTypeError [ERR_INVALID_ARG_VALUE]: encoding 'bogus' is an invalid encoding.fs.writeandfilehandle.writedo the same. Node.js v26.12.1, the version Bun reports, writes UTF-8.nullorundefinedposition this regressed in 1.4.0 (node:fs: apply the string encoding in write/writeSync and other Node compat fixes #32813). No user reported it.args::Write::from_js(src/runtime/node/node_fs.rs:4227) parses the encoding withEncoding::assert, which throws for an unknown name.Fix
WriteStringdoes. A known name in a primitive string selects that encoding. Every other value means UTF-8.WriteStringusesParseEncoding(args[3], UTF8), which never throws.test/js/node/fs/fs-write-unknown-encoding.test.ts(node:testform). 44 of 51 tests fail on main. Node v26.12.1 passes the file. Also ranfs.test.tsand 20 vendored Node tests.Background
writeSync,fs.write,util.promisify(fs.write),filehandle.writeand Bun'sfs.promises.writeshare this parser.Encoding::assertports Node'sassertEncoding, whichreadFile,writeFileandreaddirkeep.ParseEncodingport (one caller) and a JS-wrapper check (one more native call per write, as in fix(fs): match Node read and write argument defaults #44476). node:crypto, webcrypto: a fourth batch of Node compatibility fixes #47671 fixedcipher.updatethis way.Downsides
ERR_UNKNOWN_ENCODING(crypto,fs: reject invalid encodings nodejs/node#66247). Until Bun ports that check, a mistyped name such as"base-64"writes the text with no error. A maintainer must choose: the pin (this PR) or Node 27.Stringobject is no longer read as an encoding:new String("hex")writes UTF-8, as in Node.Notes
The decision: the pin or Node 27. Bun reports Node.js v26.12.1. Node.js v27.0.0-alpha.0.0.0 (2026-10-29) changed this behaviour on purpose: nodejs/node#66247 (semver-major) added
if (normalizedEncoding === undefined) throw new ERR_UNKNOWN_ENCODING(encoding)to the JSvalidateEncoding(https://github.com/nodejs/node/blob/v27.0.0-alpha.0.0.0/lib/internal/validators.js#L419-L423). The nativeWriteStringis the same in both (ParseEncoding(args[3], UTF8))."bogus"ERR_UNKNOWN_ENCODING, nothing writtenERR_INVALID_ARG_VALUE, nothing written5ERR_UNKNOWN_ENCODINGnew String("hex"), data"abcd"cipher.updateon 2026-11-02, against Node v26.12.1. The port of the v27 check then comes with the move to v27.validateEncoding(src/jsc/bindings/NodeValidatorBodies.h:279). The vendored v26test-fs-promises-file-handle-write.jsthen fails, because it passes a number in this argument. Under Node v27.0.0-alpha it fails withUnknown encoding: 16. Upstream's newtest-fs-write-invalid-encoding.jsfails on main (other error code) and with this PR (no error).Repro. Node v24.0.0 to v26.13.0 and Bun 1.3.14 print
2 "XY..". Bun 1.4.0, 1.4.4 and main throwERR_INVALID_ARG_VALUE. Node v27.0.0-alpha throwsERR_UNKNOWN_ENCODING.....writeSync(fd, "XY", null, "bogus")XY..XY..XY..writeSync(fd, "XY", 0, "bogus")XY..XY..fs.write(fd, "XY", null, "bogus", cb)(null, 2, "XY")filehandle.write("XY", null, "bogus")writeSync(fd, "é", null, "bogus")writeSync(fd, "XY", "bogus")XY..XY..XY.."", encoding5XY..writeFileSync(p, "XY", "bogus")Node v24.0.0, v24.22.2, v26.3.0 and v26.13.0 give the v26.12.1 column. These 5 releases are the ones I ran.
Node source.
WriteStringreads the encoding withParseEncoding(isolate, args[3], UTF8): https://github.com/nodejs/node/blob/v26.12.1/src/node_file.cc#L3204.ParseEncodingreturns the default for a value that is not a string and for an unknown name: https://github.com/nodejs/node/blob/v26.12.1/src/api/encoding.cc#L125-L136.History. Through v1.1.33 this argument was read with
Encoding.fromJS(current) orelse Encoding.utf8. #14937 (v1.1.34) made the encoding parses innode_fsstrict withEncoding.assert, this one included. Its tests port Node'stest-fs-assert-encoding-error, which does not coverfs.write. Until 1.4.0 the parser reached the encoding only after a numeric position, so a call with anullposition never ran the check. #32813 (1.4.0) made the parser consume the position whatever its type, as Node does, and the check then ran for every call.This PR before. It was stacked on #43373, which replaces the
utf16-lekey of Bun's name table withutf-16le. #47559 addedutf-16leto the table, so this PR now targets main and does not touch the table. The tests moved fromfs.test.tsto their own file innode:testform, the form a maintainer asked for on #37632, the previous fix to this parser.Tests.
test/js/node/fs/fs-write-unknown-encoding.test.tshas 43 tests that Node can run and 8 for Bun'sfs.promises.write.test-fs-write.js,test-fs-write-sync.js,test-fs-write-sync-optional-params.js,test-fs-write-optional-params.js,test-fs-write-buffer.js,test-fs-write-negativeoffset.js,test-fs-write-no-fd.js,test-fs-write-reuse-callback.js,test-fs-writesync-crash.js,test-fs-assert-encoding-error.js,test-fs-internal-assertencoding.js,test-fs-read-file-assert-encoding.js,test-fs-promises-file-handle-write.js,test-fs-promises-file-handle-writeFile.js,test-fs-promises-writefile.js,test-fs-append-file-sync.js,test-fs-append-file.js,test-fs-write-file.js,test-fs-write-file-sync.js,test-fs-write-stream-encoding.js.fs.test.tson the debug build with the fix: the 39 tests that matchencodingand the 191 that matchritepass. On release builds of 7ca0042 with and without the change, every other test of the file passes, exceptkeeps a non-throwing callback in the same place in the event loop. That test fails in about half of the runs on both builds (anfs.statcallback against asetImmediate).BUN_JSC_validateExceptionChecks=1 BUN_JSC_dumpSimulatedThrows=1on the new file: 51 pass, 0 reports of an unchecked exception.fs.writewith an unknown name in Node's v26test-fs-write*.js, so there is no upstream v26 test to copy.uv_fs_writeon Windows). The read-only fd tests expectEBADF, which the existing testwhen the call fails with EBADFalready expects on Windows for the callback form.Self-review. 13 concerns, 12 addressed: the base and the body, the Node version (v26.12.1, not v26.3.0), the Node 27 reversal, the reason for the NUL names, the open PRs on the same lines, the tests in
node:testform, and the cost text. Not taken: a test that spawnsnodeon this file in every CI run, as six http and tls test files do. On a cold debug build here that one test took 4.7 s and 5.1 s against the 5 s limit, andfs-write-offset-bound.test.ts, the file from #37632, has no such test. Upstream'stest-fs-write-invalid-encoding.jsis the test that fails when the v27 check is missing.Matrix. 1428 calls of
fs.writeSync(fd, data, position, encoding): 102 values in the encoding argument, 7 strings, positionsnulland0. Compared by return value and file bytes with Node v26.12.1, on release builds of 7ca0042 with and without the change.new String("hex")ornew String("utf16le"). 18 of the 24 now match Node. In the other 6 Node throws (see below).Not in this PR. The 150 calls that still differ, and the related differences I found:
writeSync(fd, s, 0, "utf16-le")(36 calls)utf16-lekey ofENCODING_MAP,src/runtime/node/types.rs:593"hex\0"(96 calls)writeSync(fd, "abc", 0, new String("hex")), and an odd-length string with another object whose string form ishex(18 calls)ERR_INVALID_ARG_VALUEvalidateEncoding,src/jsc/bindings/NodeValidatorBodies.h:279Stringobject whosetoStringthrowswriteFileSync(p, "6162", new String("hex"))36 31 36 3261 62WriteFile::from_js_with_default_flag, and the same gate inReadFile::from_js,Readdir::from_js,parse_encoding_argandEncoding::assertutf16-le: the key stays. node:crypto, webcrypto: assorted Node compatibility fixes #47559 addedutf-16lebeside it. node: accept the utf-16le encoding alias and reject utf16-le #43373 (open, conflicts with main) would remove it. Other lookups share the table (Bun.CryptoHasher,readFile,writeFileand more)."hex\0"ishexthere. Bun writes UTF-8, and this is a deliberate difference. Node v27 rejects these names. No other encoding reader in Bun stops at a NUL:Buffer.from("abcd", "hex\0")throwsERR_UNKNOWN_ENCODING, as in Node. A cut at the NUL would match only the v26 line.validateEncodingis duck-typed. It readsenc.lengthand callstoStringon a value of length 3 to 9. Bun's reads primitive strings only. In a probe of 17Stringobjects and other objects at 2 positions (34 calls), main matches Node on 14 and this PR on 18: 12 calls that differed now match, and 8 calls that matched now differ. All 8 pass aStringobject with atoStringthat throws, which main called in the native parser. I found no open PR that changes this validator.Stringobjects in the other fs calls: Node'sgetOptionstakes an encoding only from a primitive string. Those parsers must keep their throw for an unknown name, so they need a different change. node:fs: reject a non-string, non-object options argument like node #41928 and node:fs: take a function in the options slot as the default options #46369 (open) rework the same parsers.Cost, measured. Release builds (ThinLTO) of main 7ca0042 and of the change on Linux x64. Instructions and calls are counted inside
args::Write::from_js, callees included, withgdbstepiand the other threads held. 5 calls of each kind, all 5 counts equal.writeSync(fd, "x")await filehandle.write("x")writeSync(fd, "x", null, "utf8")fs.write(fd, "x", cb)(the JS wrapper passes"utf8")writeSync(fd, "x", null, name),nameis"utf8"in a 16-bit stringmi_mallocand 1mi_freewriteSync(fd, "x", null, "")writeSync(fd, "x", null, "bogus")TypeErrorsizereports text 140,290,979 on main and 140,291,235 with the change (+256 bytes). Data and bss are equal, and the stripped file has the same size. Of 161,833 function symbols, 2 differ:args::Write::from_jsshrinks from 2852 to 2833 bytes, and a standalone copy ofEncoding::from_js(142 bytes) is new.gdbsyscall catchpoint: the unknown-name call makes 0writecalls on main and 1 with the change. The known-name call makes 1 on both.Related PRs.
normalizeEncoding(length)from the JS wrappers ofwriteSyncand callbackfs.write, which gives those two forms the UTF-8 fallback.filehandle.writeandfs.promises.writedo not go through it.Encoding::asserton it. The one that lands second must keep this parse. Fixfs.write()ignoringposition, and match Node.js range and type errors #47036 (open) changes the buffer form and the position line of the same function, and not this line.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/fs/fs.test.ts