Skip to content

node:fs: fs.readv with an empty buffers array throws EINVAL - #34395

Closed
robobun wants to merge 1 commit into
mainfrom
claude/farm/558c20f4/fs-readv-empty-einval
Closed

robobun wants to merge 1 commit into
mainfrom
claude/farm/558c20f4/fs-readv-empty-einval

Conversation

@robobun

@robobun robobun commented Jul 16, 2026 •

Copy link
Copy Markdown
Collaborator

What does this PR do?

fs.readv(fd, []), fs.readvSync(fd, []), and FileHandle#readv([]) now throw EINVAL instead of returning bytesRead: 0, matching Node.js.

Reproduction

import * as fs from "node:fs";
const fd = fs.openSync("/etc/hosts", "r");
try { console.log(fs.readvSync(fd, [])); } catch (e) { console.log(e.code, e.syscall); }

Node v26.3.0: EINVAL read
Bun before: 0
Bun after: EINVAL read

Cause

Node routes readv through uv_fs_read, which has an explicit nbufs == 0 -> UV_EINVAL guard. Node only short-circuits the empty-array case for writev (in JS, before reaching libuv), so writev(fd, []) returns 0 while readv(fd, []) throws. Bun's readv() in src/runtime/node/node_fs.rs was short-circuiting both to 0.

Fix

Change the readv empty-buffers short-circuit to return EINVAL with syscall "read", matching Node's error object exactly ({code: "EINVAL", errno: -22, syscall: "read"}). The Windows libuv async path gets the same guard; without it, uv_fs_read would return UV_EINVAL synchronously rather than via the callback, leaving the request unresolved.

writev(fd, []) is unchanged and still returns 0.

How did you verify your code works?

New tests in test/js/node/fs/fs.test.ts cover all three forms (sync, callback, FileHandle) plus the positional variant, and assert both the readv -> EINVAL and writev -> 0 sides. All fail on current main and pass with the fix. bun run rust:check-all passes for all targets.


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

Node.js passes fs.readv/readvSync/FileHandle#readv through to uv_fs_read,
which returns UV_EINVAL when nbufs == 0. Node only short-circuits the
empty-array case for writev (in JS, before libuv), so the read and write
sides are asymmetric. Bun was returning bytesRead=0 for readv([]) in all
three forms, silently succeeding where Node throws.

Change the readv empty-buffers short-circuit to return an EINVAL error
with syscall "read" (matching Node's error shape exactly), and mirror
the guard in the Windows libuv async path where uv_fs_read would have
returned UV_EINVAL synchronously instead of invoking the callback.
@robobun

robobun commented Jul 16, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 1:10 PM PT - Jul 16th, 2026

⏳ @robobun, your commit 254f2fc is still building in Build #74025, but has 1 failures so far (All Failures):

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c80eec81-f412-4fda-9b7c-111479f85a53

📥 Commits

Reviewing files that changed from the base of the PR and between 6ad467f and 254f2fc.

📒 Files selected for processing (2)
  • src/runtime/node/node_fs.rs
  • test/js/node/fs/fs.test.ts

Walkthrough

Changes

Empty-buffer readv behavior

Layer / File(s) Summary
Readv validation and API coverage
src/runtime/node/node_fs.rs, test/js/node/fs/fs.test.ts
Synchronous and Windows asynchronous readv now return EINVAL with syscall read for empty buffers; corresponding readv and writev APIs are covered by tests.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: empty fs.readv buffers now throw EINVAL.
Description check ✅ Passed The description matches the template and includes the change summary plus verification details with reproduction and tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — small Node.js compat fix matching libuv's uv_fs_read(nbufs==0) → UV_EINVAL behavior.

Checked that the Windows async guard mirrors the existing Writev empty-buffers pattern at node_fs.rs:889-903 (same core::ptr::write + enqueue_task sequence), and that the sync path's sys::Error construction matches neighboring error sites. Verified writev([]) remains a 0-return on both the sync and Windows-async paths. Tests cover sync/callback/FileHandle plus the positional variant, use tempDir with proper cleanup, and pin the unchanged writev contract.

Extended reasoning...

Overview

Two-file change: src/runtime/node/node_fs.rs swaps the empty-buffers short-circuit in readv from Ok(bytes_read: 0) to Err(EINVAL, syscall=read), and adds a matching guard to the Windows libuv async Readv arm so uv_fs_read is never called with nbufs==0 (which would return synchronously and never fire the callback). test/js/node/fs/fs.test.ts gains a describe block covering sync, callback, and FileHandle forms for both readv (now EINVAL) and writev (still 0).

Security risks

None. No new input parsing, no path handling, no allocation sizing — this replaces a success return with an error return on an already-validated empty array.

Level of scrutiny

Low. This is a targeted Node.js compatibility fix with a well-documented upstream rationale (libuv's explicit nbufs==0 → UV_EINVAL guard, which Node surfaces for readv but short-circuits in JS for writev). The Windows async change is a byte-for-byte copy of the existing Writev empty-buffers handling immediately below it (lines 889-903), just writing Err instead of Ok. The sync change is a one-expression swap using the same sys::Error { errno, syscall, fd, ..Default::default() } shape as the neighboring preadv_inner error path.

Other factors

Tests follow harness conventions (tempDir, using/await using, try/finally around fd), assert exact error shape (code + syscall), and cover the positional overload and the negative contract (writev([]) unchanged). The bug hunting system found no issues. rust:check-all reportedly passes for all targets.

@robobun

robobun commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator Author

Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-16, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant