Repository navigation
Conversation
JSValueToStringSafe writes a value into ERR_OUT_OF_RANGE, ERR_INVALID_ARG_VALUE and the %s codes such as ERR_UNKNOWN_ENCODING. It used JS ToString for a number, so -0 printed as 0. Node uses util.inspect or the %s of util.format there, and both print -0.
|
Status: the fix and its test are pushed. This PR is #43069. How I reproduced it. Run with const { createHistogram } = require("perf_hooks");
const tests = {
readUIntBE: () => Buffer.alloc(8).readUIntBE(0, -0),
percentile: () => createHistogram().percentile(-0),
figures: () => createHistogram({ figures: -0 }),
};
for (const [k, fn] of Object.entries(tests)) {
try { fn(); } catch (e) { console.log(k, "|", e.code, "|", e.message); }
}
Test: The sites that this change does not reach are listed in #43070. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughChangesNegative-zero error handling
Suggested reviewers: Priority: ⬇️ Low Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The negative-zero formatting change has focused regression coverage and no actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/jsc/bindings/ErrorCode.cpp— Users who pass -0 as an encoding get two different ERR_UNKNOWN_ENCODING texts from Bun after this merges, and two of them still disagree with Node. The JSValue overload at ErrorCode.cpp:1283 now prints "Unknown encoding: -0", but JSBuffer.cpp:327 and JSStringDecoder.cpp:589 call toString on the value first and then the StringView overload at ErrorCode.cpp:1270, so Buffer#toString(-0), new StringDecoder(-0) and Readable#setEncoding(-0) still print "Unknown encoding: 0". Fix: every ERR_UNKNOWN_ENCODING site that holds the original JSValue must pass it to the JSValue overload (or the StringView overload must accept the value), so one error code renders -0 the same way at every entry point. [also at: src/jsc/bindings/ErrorCode.cpp:1943 - Users calling http.validateHeaderName(-0) or res.setHeader(-0, "x") still getHeader name must be a valid HTTP token ["0"]after this merges, while Node prints ["-0"].; src/jsc/bindings/ErrorCode.cpp:443 - Users hitting ERR_INVALID_ARG_TYPE with -0 (process.chdir(-0), Buffer.from(-0), fs.readFile(-0)) still seeReceived type number (0)after this merges, while Node printstype number (-0).; +1 more]Extended reasoning...
The PR body itself lists these three sites as still printing 0 and calls them excluded. REVIEW.md says same-class sites are one concern and must be fixed in the same PR, preferring the shared helper; the dismissing finder accepted the author's exclusion as author_intended without weighing that rule. I opened JSBuffer.cpp:678-682: it does
arg1.toString(lexicalGlobalObject), takes the view, and callsBun::ERR::UNKNOWN_ENCODING(scope, lexicalGlobalObject, view). JSStringDecoder.cpp:585-589 does the same. Both have the original JSValue in hand (arg1,jsEncoding) and a JSValue overload already exists at ErrorCode.cpp:1279. Node's ERR_UNKNOWN_ENCODING is'Unknown encoding: %s'and util.format's %s prints -0, so Node prints…Verification: pre-existing; acknowledged in diff: the PR body's Notes list
Buffer.alloc(1).toString(-0),new StringDecoder(-0)andnew Readable().setEncoding(-0)as still printingUnknown encoding: 0after this change, and that claim is accurate. Trigger: a user passes -0 as an encoding to any of those APIs (orbuf.indexOf(str, off, -0)). Mechanism verified.Bun::ERR::UNKNOWN_ENCODINGhas two…
|
These sites are excluded on purpose. The PR body names them in the Notes, and #43070 tracks them.
The swap fixes four values and breaks four that match Node today. The correct fix is in the
|
These rows come from #43069, which carries the same JSValueToStringSafe change. They cover ERR_OUT_OF_RANGE from C++ and from JS, ERR_INVALID_ARG_VALUE, ERR_UNKNOWN_ENCODING from a JS builtin and ReadableStream.from.
|
Closing: #43087 carries the same
The work continues in #43087. |
1 participant
Problem
Buffer.alloc(8).readUIntBE(0, -0)throwsThe value of "byteLength" is out of range. It must be >= 1 and <= 6. Received 0. Node v26.3.0 saysReceived -0. A differential run against Node found it. No user reported it.JSValueToStringSafe(src/jsc/bindings/ErrorCode.cpp:350). It renders a number withtoWTFStringForConsole, which is JSToString.ToString(-0)is"0".Fix
JSValueToStringSafeappends-0when the value is negative zero. Nothing else changes.util.inspect, or with the%sofutil.format. Both print-0(formatNumber).test/js/node/errors/error-code-messages.test.ts(new test, the released bun fails six of seven rows). Alsobuffer.test.js,util.test.js,perf_hooks.test.tsand 23 vendored node tests.-0case ofdetermineSpecificType, because node errors: render -0 and constructor-less objects in determineSpecificType like node #38642 has it.Background
JSValueToStringSafeis a shared C++ routine that writes a JS value into a Node-style error message. It has its own cases for strings, symbols and functions. It passes objects toBun.inspect, which already prints a nested-0.JSC::isNegativeZerotests the sign bit.-0sites do not use this function and stay unchanged:determineSpecificType(node errors: render -0 and constructor-less objects in determineSpecificType like node #38642), the Rustout_of_rangeformatter (node: render the Rust validators' ERR_OUT_OF_RANGE values like JS #40737, which coverscrypto.pbkdf2Sync), and the sites in Node error messages print a received -0 as 0 at several sites (Node prints -0) #43070.Notes
Messages for a
-0input. Node v26.3.0 and this branch print the same text. The new test covers these routes: the C++Bun::ERR::OUT_OF_RANGEoverloads (numeric bounds, range string), the C++INVALID_ARG_VALUEoverload,$ERR_OUT_OF_RANGEand$ERR_UNKNOWN_ENCODINGfrom the JS builtins, andReadableStream.from.Buffer.alloc(8).readUIntBE(0, -0)... >= 1 and <= 6. Received -0Received 0createHistogram().percentile(-0)... > 0 && <= 100. Received -0Received 0createHistogram({ figures: -0 })... >= 1 && <= 5. Received -0Received 0generateKeyPairSync("ec", { namedCurve: "P-256", paramEncoding: -0 })The property 'options.paramEncoding' is invalid. Received -0Received 0require.resolve("./x", { paths: -0 })The property 'options.paths' is invalid. Received -0Received 0new Writable().setDefaultEncoding(-0)Unknown encoding: -0Unknown encoding: 0ReadableStream.from(-0)-0 must be iterable0 must be iterableA computed negative zero (
0 * -1,Math.round(-0.4)) prints-0in Node too.0,NaN,-Infinity,1e+21and-1e-7print the same text before and after this change.How Node renders the value:
ERR_OUT_OF_RANGEandERR_INVALID_ARG_VALUEcallinspect(value)(lib/internal/errors.js).ERR_UNKNOWN_ENCODING,ERR_UNKNOWN_SIGNAL,ERR_OPERATION_FAILEDandERR_ARG_NOT_ITERABLEuse%s.formatWithOptionssends a number throughformatNumber.String(v)is the list of allowed values invalidateOneOf. Those lists are literals in Bun's own code, and none holds-0.Sites that still print
0for-0after this change (tracked in #43070). None of them reachesJSValueToStringSafewith the number:Buffer.alloc(1).toString(-0),new StringDecoder(-0),new Readable().setEncoding(-0)(Unknown encoding: 0):JSBuffer.cppandJSStringDecoder.cppcalltoStringon the value before they throw.crypto.randomInt(-0):src/runtime/node/node_crypto_binding.rsconvertsmaxtoi64before it formats the message.crypto.pbkdf2Sync("a", "b", -0, 1, "sha1"): the Rustout_of_rangeformatter. node: render the Rust validators' ERR_OUT_OF_RANGE values like JS #40737 fixes it.process.chdir(-0),Buffer.from(-0)(Received type number (0)):determineSpecificType. node errors: render -0 and constructor-less objects in determineSpecificType like node #38642 fixes it.src/js/node/http2.tshas a JS copy of that function with the same defect.http.validateHeaderName(-0)(["0"]): the table of fixed-template codes usesToString.child_process.spawnSync("true", [], { killSignal: -0 }):ERR_UNKNOWN_SIGNALinsrc/js/node/child_process.tsis a JS template string.Two differences that this change does not touch:
ERR_SOCKET_BAD_PORTnow printsReceived -0fordgram.createSocket("udp4").connect(-0). Node printsReceived type number (-0).there. The format was different before this change too.Object(-0)goes toBun.inspect, which prints[Number: 0]. Node prints[Number: -0].Suites run with the debug build:
test/js/node/errors/,test/js/node/buffer.test.js,test/js/node/util/util.test.js,test/js/node/perf_hooks/perf_hooks.test.ts,test/js/node/crypto/pbkdf2.test.ts,crypto-random.test.ts,argon2.test.ts,test/js/node/zlib/zlib-handle-bounds-check.test.ts,buffer-compare-bounds.test.ts,buffer-jit.test.ts,test/js/bun/util/error-code-mirror.test.ts,fs-write-offset-bound.test.ts,fs-read-buffer-before-offset.test.ts,readline.node.test.ts.test-buffer-alloc,test-buffer-fill,test-buffer-readint,test-buffer-readuint,test-buffer-writeint,test-buffer-writeuint,test-buffer-readdouble,test-buffer-readfloat,test-buffer-tostring-range,test-crypto-keygen,test-crypto-random,test-crypto-pbkdf2,test-crypto-scrypt,test-fs-opendir,test-string-decoder,test-whatwg-readablestream,test-zlib-deflate-constructors,test-stream-writable-invalid-chunk,test-stream-writable-decoded-encoding,test-child-process-spawnsync-kill-signal,test-child-process-kill,test-dgram-connect,test-net-connect-options-port.BUN_JSC_validateExceptionChecks=1.buffer-jit.test.ts"differential fuzzer" needs 177 s with the debug build in my container, above its 120 s timeout. It passes with a longer timeout, and the JIT and the interpreter produce the same digest.[auto-merge] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file