fs: keep readFile abortable on non-regular files

`ReadFileJob` treats a file that is not a regular file as size 0 and
reads it to EOF inside one thread pool job, so the `AbortSignal` is
only checked once the job ends. `fs.readFile()` and
`fs.promises.readFile()` on a FIFO whose writer stays open therefore
do not settle until the writer closes, and on `/dev/zero` they keep
growing the buffer until the process runs out of memory.

Hand the fd back to the chunked reader for non-regular files, the path
files larger than one chunk already take. It checks the signal before
each read. Regular files that report size 0, such as procfs files, are
still read in one job.

Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
PR-URL: #66940
Refs: #65327
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Author: christianaurichzm

lib/fs.js

@@ -452,10 +452,11 @@ function readFile(path, options, callback) {
   const flagsNumber = stringToFlags(options.flag, 'options.flag');
   path = getValidatedPath(path);
   if (options.buffer === undefined) {
-    // Open + fstat + read + close in one thread pool round trip for files of
-    // up to one chunk; larger files come back as an open fd + size and take
-    // the chunked reader below (readFileAfterOneShot). `true`: a handed-back
-    // fd will be closed through fs.close(), so track it as unmanaged.
+    // Open + fstat + read + close in one thread pool round trip for regular
+    // files of up to one chunk; larger and non-regular files (size 0) come
+    // back as an open fd + size and take the chunked reader below
+    // (readFileAfterOneShot). `true`: a handed-back fd will be closed through
+    // fs.close(), so track it as unmanaged.
     const job = new ReadFileJob(path, flagsNumber, kReadFileBufferLength, true);
     job.context = context;
     job.ondone = readFileAfterOneShot;
@@ -480,7 +481,8 @@ function readFileAfterOneShot(err, buffer, fd, size, closeErr) {
   }
   if (fd !== -1) {
     // (context.read() below performs the abort check for this case.)
-    // Larger than one chunk: continue exactly like after open + fstat.
+    // Larger than one chunk or not a regular file: continue exactly like
+    // after open + fstat.
     context.fd = fd;
     context.size = size;
     if (size > kIoMaxLength) {

lib/internal/fs/promises.js

@@ -2305,9 +2305,9 @@ async function readFile(path, options) {
   checkAborted(options.signal);
 
   if (options.buffer === undefined && vfsState.handlers === null) {
-    // Open + fstat + read + close in one thread pool round trip for files of
-    // up to one chunk; larger files come back as an open fd + size and are
-    // read by readFileHandle() as before.
+    // Open + fstat + read + close in one thread pool round trip for regular
+    // files of up to one chunk; larger and non-regular files (size 0) come
+    // back as an open fd + size and are read by readFileHandle() as before.
     path = getValidatedPath(path);
     const { 0: buffer, 1: fd, 2: size } = await readFileInOneRoundTrip(path, stringToFlags(flag));
     if (fd === -1) {

src/node_file.cc

@@ -3587,18 +3587,19 @@ static void ReadFileUtf8(const FunctionCallbackInfo<Value>& args) {
 // read + close -- instead of one round trip per step, which is what dominates
 // fs.readFile() for the typical small file and multiplies thread pool
 // contention when many files are read at once. Files whose size exceeds
-// `limit` are not read here: the job hands the open fd and the size back so
-// that the caller continues with the chunked reader (which stays fair and
-// abortable for large files); it then only saved the fstat() round trip.
+// `limit`, and files that are not regular files (a FIFO or a device may never
+// reach EOF), are not read here: the job hands the open fd and the size back
+// so that the caller continues with the chunked reader (which stays fair and
+// abortable between chunks); it then only saved the fstat() round trip.
 //
 // JS: const job = new ReadFileJob(path, flags, limit, trackFd);
 //     job.ondone = (err, buffer, fd, size, closeErr) => {...}; job.run(path);
 // Exactly one of these outcomes is reported:
 //   err            -- open/fstat/read failed (any fd opened here was closed);
-//   fd >= 0, size  -- file is larger than `limit`: caller owns fd now (it is
-//                     registered as an unmanaged fd iff trackFd, i.e. when the
-//                     caller will close it through fs.close() rather than a
-//                     FileHandle);
+//   fd >= 0, size  -- file is larger than `limit`, or not a regular file and
+//                     size is 0: caller owns fd now (it is registered as an
+//                     unmanaged fd iff trackFd, i.e. when the caller will
+//                     close it through fs.close() rather than a FileHandle);
 //   buffer         -- the whole content; closeErr set if only close() failed.
 class ReadFileJob final : public AsyncWrap, public ThreadPoolWork {
  public:
@@ -3663,8 +3664,9 @@ class ReadFileJob final : public AsyncWrap, public ThreadPoolWork {
     const uint64_t size = regular ? static_cast<uint64_t>(st->st_size) : 0;
     uv_fs_req_cleanup(&req);
 
-    if (size > limit_) {
-      // Too large to read in one go here: hand the fd back.
+    if (size > limit_ || !regular) {
+      // Too large to read in one go here, or possibly endless: hand the fd
+      // back.
       fd_ = fd;
       size_ = size;
       return;

test/parallel/test-fs-readfile-fifo-abort.js

@@ -0,0 +1,48 @@
+'use strict';
+// Env: UV_THREADPOOL_SIZE=2
+// fs.readFile() and fs.promises.readFile() stay abortable on a FIFO whose
+// writer stays open, so the read never reaches EOF.
+//
+// The reader's open() holds a thread pool thread until a writer opens the
+// FIFO, and the writer's open() needs a second one.
+const common = require('../common');
+if (common.isWindows)
+  common.skip('no mkfifo on Windows');
+const tmpdir = require('../common/tmpdir');
+const assert = require('assert');
+const { execFileSync } = require('child_process');
+const fs = require('fs');
+const { promisify } = require('util');
+
+tmpdir.refresh();
+
+async function testAbort(name, read) {
+  const fifo = tmpdir.resolve(`${name}.fifo`);
+  execFileSync('mkfifo', [fifo]);
+  const ac = new AbortController();
+  const settled = assert.rejects(read(fifo, { signal: ac.signal }), { name: 'AbortError' });
+  const writer = await fs.promises.open(fifo, 'w');
+  ac.abort();
+  // A reader that honors the abort closes the FIFO after at most one more
+  // read, and the next write fails with EPIPE. One that ignores it keeps
+  // draining the FIFO until the writer closes.
+  let closedByReader = false;
+  const chunk = Buffer.alloc(1024);
+  for (let i = 0; i < 1000; i++) {
+    try {
+      await writer.write(chunk);
+    } catch (err) {
+      assert.strictEqual(err.code, 'EPIPE');
+      closedByReader = true;
+      break;
+    }
+  }
+  await writer.close();
+  assert.ok(closedByReader, `${name} kept reading after the abort`);
+  await settled;
+}
+
+(async () => {
+  await testAbort('callback', promisify(fs.readFile));
+  await testAbort('promises', fs.promises.readFile);
+})().then(common.mustCall());