node:fs: validate buffer type before offset in fs.read/readSync#32301
node:fs: validate buffer type before offset in fs.read/readSync#32301robobun wants to merge 3 commits into
Conversation
args::Read::from_js was validating the offset argument before checking that the buffer argument is a TypedArray. When both arguments were invalid, Bun threw ERR_OUT_OF_RANGE for offset where Node.js throws ERR_INVALID_ARG_TYPE for buffer. The ordering diverged in #31559, which moved the Buffer::from_js call after length.toNumber() so the destination buffer is snapshotted after user code (valueOf on the length argument) could detach it. That move also pushed the type check past offset validation. Restore the type check to its original position (matching Node and the Zig reference) and keep a second Buffer::from_js after toNumber() so the bounds checks and the read itself still observe the post-coercion backing store.
|
Updated 10:08 AM PT - Jun 15th, 2026
❌ @robobun, your commit cc7d065 has some failures in 🧪 To try this PR locally: bunx bun-pr 32301That installs a local version of the PR into your bun-32301 --bun |
|
Warning Review limit reached
More reviews will be available in 2 minutes. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesBuffer argument validation and re-snapshot in fs read
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…n load-bearing The re-snapshot after length.toNumber() now asserts the JSCell-type invariant via .expect() instead of falling back to the pre-coercion snapshot on the unreachable None arm. The second readSync assertion now checks the error message names "buffer"; a string offset also produces ERR_INVALID_ARG_TYPE via validateInteger, so the error code alone did not distinguish which argument was rejected.
There was a problem hiding this comment.
LGTM — both nits from the prior review are addressed (.expect() on the re-snapshot, message-asserting test for the string-offset case).
Extended reasoning...
Overview
This PR reorders argument validation in args::Read::from_js (src/runtime/node/node_fs.rs) so the buffer's type is checked before offset, matching Node.js's validateBuffer → validateInteger ordering. The post-length.toNumber() re-snapshot from #31559 is preserved, now spelled as .expect(...) on a provably-unreachable None arm. Two regression tests in test/js/node/fs/fs.test.ts cover both readSync and callback fs.read, asserting the error code and that the message names "buffer".
Since my previous review, commit bc63519 applied both suggested changes: .unwrap_or(buffer) → .expect("buffer JSCell type is immutable; detached views return Some(len=0)"), and the string-offset assertion now checks message: expect.stringContaining('"buffer"') so it's load-bearing against the pre-fix behavior. Both inline comments are marked resolved.
Security risks
None. The change only affects which validation error surfaces first when arguments are already invalid — execution never reaches the read syscall on these paths. The early Buffer::from_js type check rejects non-TypedArray inputs sooner than before, and the .expect() guards an internal invariant (JSCell type immutability) that, if ever violated, would otherwise risk a stale-pointer write — failing loud is the safer spelling there.
Level of scrutiny
Low-to-moderate. This is a ~15-line Node-compat fix to error-path ordering in a single function, plus ~40 lines of targeted tests. The only subtlety is the interaction with #31559's detach-during-coercion safeguard, which the diff explicitly preserves (re-snapshot still happens after arg.to_number(ctx)?), and the PR description confirms the existing detach test still passes.
Other factors
- Bug-hunting system found nothing on this revision.
- Both prior 🟡 nits were addressed exactly as suggested.
- Author verified the new tests fail on the released build with
ERR_OUT_OF_RANGEand pass with the fix; fullreadSyncdescribe block passes 9/9. - The
.expect()arm is unreachable perJSC__JSValue__asArrayBuffersemantics (detached views returnSome(len=0)), so no new panic surface in practice.
There was a problem hiding this comment.
LGTM — both review nits addressed (.expect() on the re-snapshot, message-asserting second test case).
Extended reasoning...
Overview
Reorders argument validation in args::Read::from_js (src/runtime/node/node_fs.rs) so the buffer type check happens before offset validation, matching Node.js error precedence. The post-length.toNumber() re-snapshot from #31559 is preserved, now spelled with .expect() on the documented invariant. Adds three regression assertions in test/js/node/fs/fs.test.ts.
Security risks
None. The change only affects which validation error fires first; the detach-during-coercion safeguard is intact and now fail-loud rather than silently falling back. No new I/O paths, no auth/crypto/permissions surface.
Level of scrutiny
Low–moderate. ~10 effective lines of Rust in a single argument-parsing function plus straightforward test additions. The only subtlety — keeping the re-snapshot after JS re-entry — is handled correctly and commented.
Other factors
I left two nit-level comments on the previous revision (.unwrap_or(buffer) → .expect(...), and making the string-offset assertion check the message). Commit bc63519 applied both exactly as suggested; both threads are resolved. The bug-hunting pass on the updated revision found nothing. Tests assert both code and message so they're load-bearing against the pre-fix behavior, and the PR description confirms the #31559 detach test still passes.
|
Diff is green: 280 jobs passed on build #62555, The remaining red is three darwin test lanes (darwin-14-aarch64, darwin-14-x64, darwin-26-aarch64) that expired in the agent queue without running, on both this build and the previous one (#62493). That's infra, not something a ~15-line Rust reorder in Ready for a maintainer to merge once the darwin queue recovers or the expired lanes are waived. |
1 participant
What
fs.readSync(fd, "not a buffer", -1, 5)(and the callback-basedfs.readequivalent) threwERR_OUT_OF_RANGEforoffsetinstead ofERR_INVALID_ARG_TYPEforbuffer. Node.js validates the buffer's type first, so when both arguments are invalid the buffer error wins.Why
args::Read::from_jsinsrc/runtime/node/node_fs.rsvalidatedoffset(viavalidate_integer) before callingBuffer::from_json the buffer argument. The ordering diverged in #31559, which movedBuffer::from_jsafterlength.toNumber()so the destination buffer is snapshotted after user code invalueOfcould detach it. That move also pushed the buffer type check pastoffsetvalidation, so the Node error ordering (validateBuffer, then validateInteger(offset)) was no longer matched.Fix
Restore the
Buffer::from_jstype check to immediately followbuffer_value(matching Node and the Zig reference), and re-snapshot the buffer afterlength.toNumber()so the subsequent bounds checks and the read itself still observe the post-coercion backing store. The existing detach-during-coercion test from #31559 continues to pass.Verified
ERR_OUT_OF_RANGE, pass with this change.readSync > rejects the read when the length argument detaches the destination buffer during coercionstill passes.readSyncdescribe block: 9 pass, 0 fail.