Skip to content

fs: only back Blobs with regular files - #66204

Open
xia-chao wants to merge 1 commit into
nodejs:mainfrom
xia-chao:fs-openasblob-regular-files
Open

xia-chao wants to merge 1 commit into
nodejs:mainfrom
xia-chao:fs-openasblob-regular-files

Conversation

@xia-chao

Copy link
Copy Markdown
Contributor

A file-backed Blob is supposed to read the given file, but FdEntry::Create only checked that stat succeeded. So openAsBlob returned a Blob for a directory, with the size of the directory, and reading it failed later with NotReadableError and code 0, which hides the real EISDIR.

A fifo was worse. openAsBlobSync returned a zero size Blob right away, and the first read then blocked the main thread. ReaderImpl::Create opens the path synchronously, and a fifo with no writer never completes that open. Timers and abort signals never run, so the process just hangs. I hit this with a probe that prints a heartbeat every 50 ms; the heartbeat never fired and the probe had to be killed with timeout.

The fix checks the stat mode and rejects anything that is not a regular file, so directories report EISDIR and everything else EINVAL, through the same UVException that #65517 added for missing paths. The TODO on FdEntry already said this entry should only allow regular files.

The test covers a directory and a fifo.

Refs #65514, where the directory case was left out on purpose as a separate behavior change.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Sep 22, 2026
FdEntry::Create() accepted any path that stat'd, so openAsBlob() handed
out a Blob for directories and fifos. Reading a directory Blob failed
later with NotReadableError and code 0, hiding the EISDIR. A fifo was
worse: opening it with no writer blocked the main thread, so the event
loop stopped.

Only a regular file can be read through the file descriptor this entry
uses, so reject everything else, reporting EISDIR for directories and
EINVAL otherwise.

Signed-off-by: Xia Chao <shapirolutts@gmail.com>
@xia-chao
xia-chao force-pushed the fs-openasblob-regular-files branch from c8a4a8b to dece64d Compare September 22, 2026 12:01
@codecov

codecov Bot commented Sep 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.27%. Comparing base (addcba2) to head (dece64d).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/dataqueue/queue.cc 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66204      +/-   ##
==========================================
- Coverage   90.29%   90.27%   -0.02%     
==========================================
  Files         790      790              
  Lines      272529   272535       +6     
  Branches    52043    52033      -10     
==========================================
- Hits       246079   246040      -39     
- Misses      16906    16954      +48     
+ Partials     9544     9541       -3     
Files with missing lines Coverage Δ
src/dataqueue/queue.cc 66.05% <75.00%> (+0.06%) ⬆️

... and 33 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.

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

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants