Skip to content

fs: coerce FileHandle.read length like fs.read - #65521

Open
bun-unsafe wants to merge 1 commit into
nodejs:mainfrom
bun-unsafe:fs-coerce-filehandle-read-length
Open

fs: coerce FileHandle.read length like fs.read#65521
bun-unsafe wants to merge 1 commit into
nodejs:mainfrom
bun-unsafe:fs-coerce-filehandle-read-length

Conversation

@bun-unsafe

@bun-unsafe bun-unsafe commented Aug 24, 2026

Copy link
Copy Markdown

Summary

fs.promises.FileHandle.read() forwarded a non-number length (for example "5") into binding.read. node::fs::Read requires args[3]->IsInt32() and aborts the process on a debug/CHECK failure.

fs.read / fs.readSync already coerce with length |= 0 after defaulting. This applies the same step on the promises path so a bad length cannot reach C++.

Test plan

  • ./node test/parallel/test-fs-promises-file-handle-read.js
  • Existing FileHandle.read cases (empty length, options object, null/undefined length) still pass on CI
const fh = await fs.promises.open(file, 'r');
await fh.read(Buffer.alloc(20), 0, '5', 0); // must not abort; bytesRead === 5

Non-number length values reached node::fs::Read, which CHECKs
IsInt32() and aborts the process. Apply length |= 0 after defaulting,
matching fs.read and fs.readSync.

Signed-off-by: Xia Chao <236466140+bun-unsafe@users.noreply.github.com>
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run. labels Aug 24, 2026
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2026
@github-actions github-actions Bot added request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Aug 24, 2026
@github-actions

Copy link
Copy Markdown
Contributor
Failed to start CI
- Validating Jenkins credentials
✔  Jenkins credentials valid
- Starting PR CI job
✘  Failed to start PR CI: 400 Bad Request
https://github.com/nodejs/node/actions/runs/32773413890

@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.15%. Comparing base (f509cf1) to head (dcefcbc).
⚠️ Report is 51 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #65521   +/-   ##
=======================================
  Coverage   90.14%   90.15%           
=======================================
  Files         751      751           
  Lines      253585   253586    +1     
  Branches    47772    47773    +1     
=======================================
+ Hits       228596   228620   +24     
+ Misses      16228    16204   -24     
- Partials     8761     8762    +1     
Files with missing lines Coverage Δ
lib/internal/fs/promises.js 92.65% <100.00%> (+<0.01%) ⬆️

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aduh95 aduh95 added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. labels Aug 25, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 25, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@bun-unsafe

Copy link
Copy Markdown
Author

Jenkins parent https://ci.nodejs.org/job/node-test-pull-request/76428/ is red on
rhel8-x64 and rhel9-x64 in the same linux matrix; rhel10, ubuntu, debian, fedora,
windows, and osx passed. I cannot open the consoles from here. If those two look
unrelated to FileHandle.read, a Resume Build would help.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@bun-unsafe
bun-unsafe requested a review from jasnell August 27, 2026 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fs Issues and PRs related to file-system APIs and the fs module. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants