Repository navigation
Conversation
validateInteger, validateInt32, validateUint32 and Buffer's validateOffset tested a number for integrality with std::fmod(x, 1.0) != 0. On linux-gnu and windows-msvc that is a library call for each validated number. On linux-gnu the call lands in Rust's compiler_builtins fmod, which loops once per exponent bit below 2**32. Buffer#fill with offsets (since 1.4.0) and zlib.crc32 with a seed at or above 2**31 (since 1.4.4) pay for it. JSC::isInteger(x) is x - trunc(x) == 0.0. It is the same predicate for every double, NaN and the infinities included, and it compiles to one roundsd (x64) or frintz (arm64) with no call. validatePort gets the same spelling. clang already emitted the inline form there. No behaviour change. The new test cases pin the non-integer results of Buffer#write and of validateInteger, validateInt32 and validateUint32. They pass before and after.
|
Updated 4:43 AM PT - Oct 28th, 2026
❌ @robobun, your commit 4817fab has 6 failures in
🧪 To try this PR locally: bunx bun-pr 47144That installs a local version of the PR into your bun-47144 --bun |
|
Status How to reproduce, on linux-x64 with a release build ( cd bench && bun install
../build/release/bun snippets/validate-integer.mjsOne iteration makes 16 calls. Per call, CI: build 134315 for PR: #47144 |
|
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 11 seconds. View limit details
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Replace fractional-value checks src/jsc/bindings/JSBuffer.cpp, src/jsc/bindings/NodeValidator.cpp, src/jsc/bindings/NodeValidatorBodies.h |
The validation functions use JSC::isInteger for integer checks. Existing range checks and error handling remain unchanged. |
Add validation tests and benchmarks test/js/node/buffer.test.js, test/js/node/util/util.test.js, bench/snippets/validate-integer.mjs |
Tests cover integer acceptance, non-integer rejection, and range errors. The benchmark measures five APIs across integer, double, and rejected inputs. |
Suggested reviewers: jarred-sumner, dylan-conway
Priority:
Merge Risk: 
Minimal · previous review d6e4f
-
The validation behavior appears preserved. The change is ready to merge after normal checks.
Pre-merge checks: 
4
-
Full details: Description check
-
Explanation
The description follows the repository template. It has a 'What does this PR do?' section (Problem, Fix, Background, Downsides) and a 'How did you verify your code works?' section (measurements, byte-identical output comparison, unit test runs on debug build, platform-specific tests not run). It is detailed and specific, covering the performance numbers, the behaviour-change claim, and the tradeoffs. The main gap is that the verification section says Windows and linux-arm64 were not timed, and the Windows path was not run. A bot note also flags that platform-specific tests in test/js/node/buffer.test.js did not run locally and defer to CI. Those gaps are disclosed, so the description is still mostly complete.
-
- 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.
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.
LGTM — eight identical one-line substitutions of the integrality predicate, with the error ordering (the "an integer" message before the min/max range message) unchanged at every site. The inline note on the now-redundant isnan/isinf guards in validatePort is a cleanup nit, not a correctness issue.
Beyond that, I checked that NaN and +/-Infinity still reach the "an integer" branch rather than falling through to the range check: the new buffer.test.js and util.test.js cases assert the exact message for both, so any divergence in JSC::isInteger's handling of non-finite values would fail CI rather than merge silently.
Extended reasoning...
The diff replaces std::fmod(x, 1.0) != 0 with !JSC::isInteger(x) at eight sites across src/jsc/bindings/JSBuffer.cpp (validateOffset), src/jsc/bindings/NodeValidator.cpp (validateInteger, validateInt32, validateUint32 overloads) and src/jsc/bindings/NodeValidatorBodies.h (validatePort), adds the MathCommon.h include to the two files that lacked it, and ships a mitata benchmark plus tests pinning exact ERR_OUT_OF_RANGE messages for 1.5, -1.5, NaN, +/-Infinity, -0, integral doubles and 2**53. It touches no security-sensitive surface; the validators sit before unchanged range checks and no exception-check or accessor ordering changed. The decision to approve rests on the change being purely mechanical with no control-flow edits, and on the added tests covering the one semantic risk (non-finite inputs) with exact-message assertions; the single inline finding is a dead-guard cleanup nit.
JSC::isInteger is false for NaN and for both infinities, and all three returns raised the same ERR_SOCKET_BAD_PORT.
There was a problem hiding this comment.
LGTM — the second commit addresses the earlier note on validatePort, and the swap to JSC::isInteger is behavior-preserving at all eight sites.
What was reviewed:
- Compared
std::fmod(x, 1.0) != 0against!JSC::isInteger(x)(isfinite && trunc(x) == x) on NaN, ±Infinity, -0 and 2**53: same verdict, so the "an integer" vs range-message ordering is unchanged, andvalidatePortstill returns SOCKET_BAD_PORT for NaN/Infinity. - Grepped src/ for remaining
fmod(x, 1.0)integrality tests — none left;<cmath>stays live in both files (std::isnan/std::round/std::trunc elsewhere). - Checked the new tests' exact messages against ErrorCode.cpp's
appendOutOfRangeReceivedseparator logic andconjunctiveRange, and confirmedexposedInternals["internal/validators"]is bindgen-routed to the changed C++ bodies.
Extended reasoning...
The diff replaces an fmod-based integer test with JSC::isInteger in V::validateInteger/validateInt32/validateUint32 (NodeValidator.cpp), validateOffset (JSBuffer.cpp) and validatePort (NodeValidatorBodies.h, also dropping its now-redundant isnan/isinf returns), and adds exact-message tests in buffer.test.js and util.test.js plus a mitata benchmark. It touches argument validation for Buffer, zlib and net port inputs but no auth, crypto or injection surface; the new check is strictly equivalent on every double class the old one handled, so no new acceptance path is opened. Approve was decided by the small mechanical scope, the tests pinning code, class, verbatim message and NaN/Infinity precedence, the only prior inline finding being resolved by the newer commit, no CODEOWNERS entry for these paths, and the bug hunt ending on a dry streak with no outstanding third-party objections.
Problem
buf.filltakes 33 ns at offset 4096 (v1.3.14: 15 ns).zlib.crc32with a seed at or above 2**31 takes 45 ns (v1.3.14: 17 ns).std::fmod(x, 1.0) != 0(src/jsc/bindings/NodeValidator.cpp:49, 6 more lines,JSBuffer.cpp:370). On linux-gnu that calls Rust'scompiler_builtinsfmod, a bit-by-bit loop.Fix
!JSC::isInteger(x)(x - trunc(x) == 0.0): oneroundsdorfrintz, no call.filldrops to 15 ns,crc32to 19 ns.fmodissue, an unconfirmedstd::powneighbour, and leavingvalidatePortout (Notes).Background
lib/internal/validators.js.Buffermethods and bindgen arguments call them from C++. JS callers skip them for valid integers (Compile TypeScript to machine code ahead of time:bun build --compile --aot#45631).errno(glibc, MSVC). macOS, musl andvalidatePortalready get the inline form.validatePortloses itsisnanandisinfreturns, whichisIntegercovers.fmodto libm (the 10 calls stay) and-fno-math-errno(changes every libm call).Downsides
.textgrows by 256 bytes on linux-x64.minormaxbound whose conversion throws costs 2 to 24 more instructions. No call site passes one.errno = EDOMbehind forbsd.c:1587.Behaviour change: none
Notes
The 8 sites.
src/jsc/bindings/NodeValidator.cpp:49and:67(V::validateInteger, both name overloads),:226and:252(V::validateInt32),:268and:287(V::validateUint32),src/jsc/bindings/NodeValidatorBodies.h:207(validatePort),src/jsc/bindings/JSBuffer.cpp:370(validateOffset).fmodon main.validatePorthas nofmodcall on main: itsisnanandisinfreturns rule out theerrnocases, so clang already emits the inline form. It gets the same spelling.isnanandisinfreturns ofvalidatePort.isIntegeris false for NaN and for both infinities, and all three returns raised the sameERR_SOCKET_BAD_PORT. Its two instantiations shrink by 41 and 43 bytes.fmodcalls insrc/jsc/bindingscompute a remainder and stay:bindings.cpp:6486,wtf-bindings.cpp:283,webcore/JSDOMConvertNumbers.cpp:146and:192.Who gains, and who does not. No issue reports this cost.
buf.fill,buf.writeandbuf.comparewith offsets, andzlib.crc32with a seed that is not an int32.crc32int32 rows), and JS callers ofinternal/validatorswith a valid integer (Compile TypeScript to machine code ahead of time:bun build --compile --aot#45631 answers those before the C++ validator, see thesetDefaultHighWaterMarkrows). The JS-facingvalidateIntegerhost functions disassemble to the same instructions on both builds.node:httprequests make 0fmodcalls on main (gdb breakpoint count).fillreached this test in v1.4.0 (node:buffer: fix six Node compatibility divergences #32784 changed its offsets fromvalidateNumbertovalidateInteger). Thecrc32seed goes through bindgen'svalidateIntegerconversion since Move 58 small natives ofnode:*andBunto bindgen schemas #45851 (v1.4.4): an int32 in range converts inline, any other seed callsV::validateInteger.writeandcomparehave paid for the test since before v1.3.14.How it was measured. linux-x64, AMD EPYC (Zen 5), glibc 2.41. Release builds of main
11c367163a4and of this branch, the release binaries of v1.3.14 and v1.4.4, and node v26.3.0. Windows and linux-arm64 are not timed.bench/snippets/validate-integer.mjsunder mitata. The five binaries ran interleaved for 7 rounds. The tables show the lowest median, divided by the 16 calls of one iteration. Between rounds the median of a case moves by 0.9 ns (typical) to 7.1 ns on this branch, and by 2.1 ns to 11.1 ns on main (two outliers of 42 and 53 ns).perf_event_open(instructions:u, main thread), slope between 2e7 and 1e8 calls, two runs per case.d6e4f3e0). The second commit only deletes the twovalidatePortreturns: the other validator functions and theBufferhost functions disassemble to the same instructions in both builds.crc32with an int32 seed,setDefaultHighWaterMark) differ by -0.2 to +0.3 ns. Their instruction counts differ by up to 3.1, less than the 8.7 between two runs of one build.nis an int32. ns per call, then instructions per call (user mode, main thread).nbuf.fill(1, n, n + 1)buf.fill(1, n, n + 1)buf.fill(1, n, n + 1)buf.fill(1, n, n + 1)buf.write('x', n, 1)buf.write('x', n, 1)buf.write('x', n, 1)buf.write('x', n, 1)zlib.crc32(chunk, n)zlib.crc32(chunk, n)zlib.crc32(chunk, n)zlib.crc32(chunk, n)zlib.crc32(chunk, n)buf.compare(t, n, n + 1, n, n + 1)buf.compare(t, n, n + 1, n, n + 1)buf.compare(t, n, n + 1, n, n + 1)buf.compare(t, n, n + 1, n, n + 1)setDefaultHighWaterMark(false, n)setDefaultHighWaterMark(false, n)setDefaultHighWaterMark(false, n)setDefaultHighWaterMark(false, n)setDefaultHighWaterMark(false, n)nis a double (read from aFloat64Array). ns per call, then instructions per call (user mode, main thread).nbuf.fill(1, n, n + 1)buf.fill(1, n, n + 1)buf.fill(1, n, n + 1)buf.fill(1, n, n + 1)buf.write('x', n, 1)buf.write('x', n, 1)buf.write('x', n, 1)buf.write('x', n, 1)zlib.crc32(chunk, n)zlib.crc32(chunk, n)zlib.crc32(chunk, n)zlib.crc32(chunk, n)zlib.crc32(chunk, n)zlib.crc32(chunk, n)zlib.crc32(chunk, n)zlib.crc32(chunk, n)buf.compare(t, n, n + 1, n, n + 1)buf.compare(t, n, n + 1, n, n + 1)buf.compare(t, n, n + 1, n, n + 1)buf.compare(t, n, n + 1, n, n + 1)setDefaultHighWaterMark(false, n)setDefaultHighWaterMark(false, n)setDefaultHighWaterMark(false, n)setDefaultHighWaterMark(false, n)setDefaultHighWaterMark(false, n)setDefaultHighWaterMark(false, n)setDefaultHighWaterMark(false, n)setDefaultHighWaterMark(false, n)Rejected inputs (
1.5,NaN,Infinity, the call throws). The 15 cases take 651 to 789 ns on main and 632 to 760 ns on this branch. The spread between rounds is 105 to 504 ns, more than the difference between the builds. By instruction count every case is shorter: 6,261 to 6,979 per call on main, 6,218 to 6,936 on this branch (24 to 98 fewer). node takes 6,312 to 7,546 ns. v1.3.14 accepted a fractionalfilloffset, so that case is not a rejection there.Evidence for no behaviour change.
fmod(x, 1.0) != 0against!(x - trunc(x) == 0.0)for 3,070,437,977 doubles from a fixed seed. The set has named edge cases, every power of two and its two neighbours, every integer up to 222 with its neighbours, 217 mantissas for each sign and exponent, 1.5e9 random bit patterns and 1e9 random integers and near-integers. 0 disagreements against thecompiler_builtinsfmodof this build. 0 against glibc 2.41fmod.internal/validatorsfunctions,buf.compare,buf.fill,buf.write,Buffer.copyBytesFrom,process.exitCode, thevmtimeout,zlib.crc32,crypto.getCipherInfo,checkPrimeSync,createHashwithoutputLength, andhkdfSync. The 496,500 output lines are byte-identical on main and on this branch, in 3 runs each.test/parallel/test-validators.jsis not vendored. It stops at itsvalidateArrayblock on main and on this branch (node: match Node's validateArray minLength error message #38410 has that fix), after its integer block passes.util.test.jsnow carries a port of that integer block.Where the call exists today. clang 23.1.2
-O3 -Son the reduced expression with bun's-marchflags,fmodcalls before and after:Binary (llvm-objdump and llvm-size on the two release binaries).
fmodcall sites in the 10 validator functions: 10 to 0. In the whole binary: 52 to 42..text: 108,646,665 to 108,646,921 bytes (+256). The 10 functions: 11,241 to 11,658 bytes. The stripped file keeps its size (138,193,048 bytes).The longer path. When
minormaxis not a number and its conversion throws, the validator runs 2 to 24 more instructions before it returns. The number comes from a probe that compilesNodeValidator.cppwith the release flags and calls the validators directly (12 cells: 4 functions, 3 values). The accept cells of the same probe are 32 to 296 instructions shorter. The C++ callers passjsNumber(...)orjsUndefined(). The 89validateIntegerandvalidateInt32call sites insrc/jspass number literals, numeric constants, or a value validated on the line above.Windows. Not run. The reasoning: the MSVC runtime's
fmodcan seterrnotoEDOMfor an infinite argument, and the inline test sets nothing.bsd_create_listen_socket_unix(packages/bun-usockets/src/bsd.c:1587) readserrnoafter a failure that, on Windows, only callsSetLastError. So after a validator rejected an infinity, that path could reportEDOMon main and no code on this branch.Self-review items that were rejected.
fmodbinding: it is not part of this change. What remains is below.bufferWriteVarWidthCheckValue(JSBuffer.cpp) computes its bounds withstd::pow(2.0, ...). A review note called that a libm call perwriteUIntLE. It is a different pattern, and the release binary did not confirm it here.validatePortout. That avoided a conflict with Compile TypeScript to machine code ahead of time:bun build --compile --aot#45631 while it was open. Compile TypeScript to machine code ahead of time:bun build --compile --aot#45631 has merged, so the line is in.Not in this PR.
fmodstill resolves tocompiler_builtinsfor the other 42 call sites. JavaScriptCore'sfmodDoubleis one of them:(1761580000123.5 + i) % 1000takes 45 ns here and 13 ns on node. With a fastfmodthis PR would still save the call: glibc 2.41's ownfmod(x, 1.0)takes 3.4 ns here, the inline test 1.1 ns.JSBuffer.cppandbuffer.test.js.Tests run on a debug build of this branch.
test/js/node/util/util.test.js,test/js/node/buffer.test.js,test/js/node/buffer-compare-bounds.test.ts,test/internal/bindgen.test.ts,test/js/node/zlib/zlib.test.js,zlib-handle-bounds-check.test.ts,test/js/node/crypto/pbkdf2.test.ts,scrypt.test.ts,argon2.test.ts,hkdf-zero-length.test.ts,crypto-random.test.ts,node-crypto.test.js,test/js/node/os/os.test.js,test/js/node/dgram/node-dgram.test.js,test/js/node/net/node-net-server.test.ts,test/js/bun/perf_hooks/histogram.test.ts,test/js/bun/util/password.test.ts,test/js/bun/udp/udp_socket.test.ts.test/js/node/test/parallelthat assert integer or port validation (buffer, crypto, dgram, dns, fs, net, process, sqlite, stream, tls, vm, zlib).test/js/node/process/process.test.js,test/js/node/vm/vm.test.tsandtest/js/node/net/node-net.test.tshave tests that spawn processes and pass the 5 s limit on this machine's debug build. Those tests pass when run alone. On the release builds, main and this branch fail the same tests: one inprocess.test.js(it reads the host's node version) and two innode-net.test.ts.bindgen.test.tshas one GC accounting test (estimated_size is reported to the GC) that fails in about half of the full-file runs on this machine's debug build, and on a debug build of main too (3 of 6 runs). It passes when run alone.zlib.test.jshad 2 timeouts once while other jobs ran and passes on an idle re-run.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/buffer.test.js