fs: fix rmsync error swallowing

fix rmsync swallowing errors instead of throwing them. fixes: https://github.com/nodejs/node/issues/38683 fixes: https://github.com/nodejs/node/issues/34580 PR-URL: https://github.com/nodejs/node/pull/38684 Fixes: https://github.com/nodejs/node/issues/38683 Fixes: https://github.com/nodejs/node/issues/34580 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
2026-01-15 12:15:26 +00:00 · 2021-05-14 02:26:20 +03:00
parent fa7cdd6fc9
commit f9447b71a6
9 changed files with 282 additions and 150 deletions
--- a/lib/internal/fs/rimraf.js
+++ b/lib/internal/fs/rimraf.js
@@ -219,6 +219,11 @@ function _unlinkSync(path, options) {
          i < tries &&
          options.retryDelay > 0) {
        sleep(i * options.retryDelay);
+      } else if (err.code === 'ENOENT') {
+        // The file is already gone.
+        return;
+      } else if (i === tries) {
+        throw err;
      }
    }
  }
@@ -231,8 +236,9 @@ function _rmdirSync(path, options, originalErr) {
  } catch (err) {
    if (err.code === 'ENOENT')
      return;
-    if (err.code === 'ENOTDIR')
-      throw originalErr;
+    if (err.code === 'ENOTDIR') {
+      throw originalErr || err;
+    }

    if (notEmptyErrorCodes.has(err.code)) {
      // Removing failed. Try removing all children and then retrying the
@@ -259,10 +265,17 @@ function _rmdirSync(path, options, originalErr) {
              i < tries &&
              options.retryDelay > 0) {
            sleep(i * options.retryDelay);
+          } else if (err.code === 'ENOENT') {
+            // The file is already gone.
+            return;
+          } else if (i === tries) {
+            throw err;
          }
        }
      }
    }
+
+    throw originalErr || err;
  }
 }

--- a/test/parallel/test-fs-mkdir-recursive-eaccess.js
+++ b/test/parallel/test-fs-mkdir-recursive-eaccess.js
@@ -26,8 +26,7 @@ function makeDirectoryReadOnly(dir) {
  let accessErrorCode = 'EACCES';
  if (common.isWindows) {
    accessErrorCode = 'EPERM';
-    execSync(`icacls ${dir} /inheritance:r`);
-    execSync(`icacls ${dir} /deny "everyone":W`);
+    execSync(`icacls ${dir} /deny "everyone:(OI)(CI)(DE,DC,AD,WD)"`);
  } else {
    fs.chmodSync(dir, '444');
  }
@@ -36,7 +35,7 @@ function makeDirectoryReadOnly(dir) {

 function makeDirectoryWritable(dir) {
  if (common.isWindows) {
-    execSync(`icacls ${dir} /grant "everyone":W`);
+    execSync(`icacls ${dir} /remove:d "everyone"`);
  }
 }

--- a/test/parallel/test-fs-open-no-close.js
+++ b/test/parallel/test-fs-open-no-close.js
@@ -15,10 +15,17 @@ const debuglog = (arg) => {
 const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();

-{
-  fs.open(`${tmpdir.path}/dummy`, 'wx+', common.mustCall((err, fd) => {
-    debuglog('fs open() callback');
-    assert.ifError(err);
-  }));
-  debuglog('waiting for callback');
-}
+let openFd;
+
+fs.open(`${tmpdir.path}/dummy`, 'wx+', common.mustCall((err, fd) => {
+  debuglog('fs open() callback');
+  assert.ifError(err);
+  openFd = fd;
+}));
+debuglog('waiting for callback');
+
+process.on('beforeExit', common.mustCall(() => {
+  if (openFd) {
+    fs.closeSync(openFd);
+  }
+}));
--- a/test/parallel/test-fs-promises-file-handle-read-worker.js
+++ b/test/parallel/test-fs-promises-file-handle-read-worker.js
@@ -19,16 +19,20 @@ if (isMainThread || !workerData) {
      transferList: [handle]
    });
  });
-  fs.promises.open(file, 'r').then((handle) => {
-    fs.createReadStream(null, { fd: handle });
-    assert.throws(() => {
-      new Worker(__filename, {
-        workerData: { handle },
-        transferList: [handle]
+  fs.promises.open(file, 'r').then(async (handle) => {
+    try {
+      fs.createReadStream(null, { fd: handle });
+      assert.throws(() => {
+        new Worker(__filename, {
+          workerData: { handle },
+          transferList: [handle]
+        });
+      }, {
+        code: 25,
      });
-    }, {
-      code: 25,
-    });
+    } finally {
+      await handle.close();
+    }
  });
 } else {
  let output = '';
--- a/test/parallel/test-fs-promises-file-handle-readFile.js
+++ b/test/parallel/test-fs-promises-file-handle-readFile.js
@@ -56,13 +56,16 @@ async function doReadAndCancel() {
  {
    const filePathForHandle = path.resolve(tmpDir, 'dogs-running.txt');
    const fileHandle = await open(filePathForHandle, 'w+');
-    const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8');
-    fs.writeFileSync(filePathForHandle, buffer);
-    const signal = AbortSignal.abort();
-    await assert.rejects(readFile(fileHandle, { signal }), {
-      name: 'AbortError'
-    });
-    await fileHandle.close();
+    try {
+      const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8');
+      fs.writeFileSync(filePathForHandle, buffer);
+      const signal = AbortSignal.abort();
+      await assert.rejects(readFile(fileHandle, { signal }), {
+        name: 'AbortError'
+      });
+    } finally {
+      await fileHandle.close();
+    }
  }

  // Signal aborted on first tick
--- a/test/parallel/test-fs-promises-file-handle-writeFile.js
+++ b/test/parallel/test-fs-promises-file-handle-writeFile.js
@@ -30,13 +30,17 @@ async function validateWriteFile() {
 async function doWriteAndCancel() {
  const filePathForHandle = path.resolve(tmpDir, 'dogs-running.txt');
  const fileHandle = await open(filePathForHandle, 'w+');
-  const buffer = Buffer.from('dogs running'.repeat(512 * 1024), 'utf8');
-  const controller = new AbortController();
-  const { signal } = controller;
-  process.nextTick(() => controller.abort());
-  await assert.rejects(writeFile(fileHandle, buffer, { signal }), {
-    name: 'AbortError'
-  });
+  try {
+    const buffer = Buffer.from('dogs running'.repeat(512 * 1024), 'utf8');
+    const controller = new AbortController();
+    const { signal } = controller;
+    process.nextTick(() => controller.abort());
+    await assert.rejects(writeFile(fileHandle, buffer, { signal }), {
+      name: 'AbortError'
+    });
+  } finally {
+    await fileHandle.close();
+  }
 }

 validateWriteFile()
--- a/test/parallel/test-fs-promises.js
+++ b/test/parallel/test-fs-promises.js
@@ -90,6 +90,18 @@ async function getHandle(dest) {
  return open(dest, 'r+');
 }

+async function executeOnHandle(dest, func) {
+  let handle;
+  try {
+    handle = await getHandle(dest);
+    await func(handle);
+  } finally {
+    if (handle) {
+      await handle.close();
+    }
+  }
+}
+
 {
  async function doTest() {
    tmpdir.refresh();
@@ -98,140 +110,138 @@ async function getHandle(dest) {

    // handle is object
    {
-      const handle = await getHandle(dest);
-      assert.strictEqual(typeof handle, 'object');
-      await handle.close();
+      await executeOnHandle(dest, async (handle) => {
+        assert.strictEqual(typeof handle, 'object');
+      });
    }

    // file stats
    {
-      const handle = await getHandle(dest);
-      let stats = await handle.stat();
-      verifyStatObject(stats);
-      assert.strictEqual(stats.size, 35);
+      await executeOnHandle(dest, async (handle) => {
+        let stats = await handle.stat();
+        verifyStatObject(stats);
+        assert.strictEqual(stats.size, 35);

-      await handle.truncate(1);
+        await handle.truncate(1);

-      stats = await handle.stat();
-      verifyStatObject(stats);
-      assert.strictEqual(stats.size, 1);
+        stats = await handle.stat();
+        verifyStatObject(stats);
+        assert.strictEqual(stats.size, 1);

-      stats = await stat(dest);
-      verifyStatObject(stats);
+        stats = await stat(dest);
+        verifyStatObject(stats);

-      stats = await handle.stat();
-      verifyStatObject(stats);
+        stats = await handle.stat();
+        verifyStatObject(stats);

-      await handle.datasync();
-      await handle.sync();
-      await handle.close();
+        await handle.datasync();
+        await handle.sync();
+      });
    }

    // Test fs.read promises when length to read is zero bytes
    {
      const dest = path.resolve(tmpDir, 'test1.js');
-      const handle = await getHandle(dest);
-      const buf = Buffer.from('DAWGS WIN');
-      const bufLen = buf.length;
-      await handle.write(buf);
-      const ret = await handle.read(Buffer.alloc(bufLen), 0, 0, 0);
-      assert.strictEqual(ret.bytesRead, 0);
+      await executeOnHandle(dest, async (handle) => {
+        const buf = Buffer.from('DAWGS WIN');
+        const bufLen = buf.length;
+        await handle.write(buf);
+        const ret = await handle.read(Buffer.alloc(bufLen), 0, 0, 0);
+        assert.strictEqual(ret.bytesRead, 0);

-      await unlink(dest);
-      await handle.close();
+        await unlink(dest);
+      });
    }

    // Use fallback buffer allocation when input not buffer
    {
-      const handle = await getHandle(dest);
-      const ret = await handle.read(0, 0, 0, 0);
-      assert.strictEqual(ret.buffer.length, 16384);
+      await executeOnHandle(dest, async (handle) => {
+        const ret = await handle.read(0, 0, 0, 0);
+        assert.strictEqual(ret.buffer.length, 16384);
+      });
    }

    // Bytes written to file match buffer
    {
-      const handle = await getHandle(dest);
-      const buf = Buffer.from('hello fsPromises');
-      const bufLen = buf.length;
-      await handle.write(buf);
-      const ret = await handle.read(Buffer.alloc(bufLen), 0, bufLen, 0);
-      assert.strictEqual(ret.bytesRead, bufLen);
-      assert.deepStrictEqual(ret.buffer, buf);
-      await handle.close();
+      await executeOnHandle(dest, async (handle) => {
+        const buf = Buffer.from('hello fsPromises');
+        const bufLen = buf.length;
+        await handle.write(buf);
+        const ret = await handle.read(Buffer.alloc(bufLen), 0, bufLen, 0);
+        assert.strictEqual(ret.bytesRead, bufLen);
+        assert.deepStrictEqual(ret.buffer, buf);
+      });
    }

    // Truncate file to specified length
    {
-      const handle = await getHandle(dest);
-      const buf = Buffer.from('hello FileHandle');
-      const bufLen = buf.length;
-      await handle.write(buf, 0, bufLen, 0);
-      const ret = await handle.read(Buffer.alloc(bufLen), 0, bufLen, 0);
-      assert.strictEqual(ret.bytesRead, bufLen);
-      assert.deepStrictEqual(ret.buffer, buf);
-      await truncate(dest, 5);
-      assert.deepStrictEqual((await readFile(dest)).toString(), 'hello');
-      await handle.close();
+      await executeOnHandle(dest, async (handle) => {
+        const buf = Buffer.from('hello FileHandle');
+        const bufLen = buf.length;
+        await handle.write(buf, 0, bufLen, 0);
+        const ret = await handle.read(Buffer.alloc(bufLen), 0, bufLen, 0);
+        assert.strictEqual(ret.bytesRead, bufLen);
+        assert.deepStrictEqual(ret.buffer, buf);
+        await truncate(dest, 5);
+        assert.deepStrictEqual((await readFile(dest)).toString(), 'hello');
+      });
    }

    // Invalid change of ownership
    {
-      const handle = await getHandle(dest);
+      await executeOnHandle(dest, async (handle) => {
+        await chmod(dest, 0o666);
+        await handle.chmod(0o666);

-      await chmod(dest, 0o666);
-      await handle.chmod(0o666);
+        await chmod(dest, (0o10777));
+        await handle.chmod(0o10777);

-      await chmod(dest, (0o10777));
-      await handle.chmod(0o10777);
+        if (!common.isWindows) {
+          await chown(dest, process.getuid(), process.getgid());
+          await handle.chown(process.getuid(), process.getgid());
+        }

-      if (!common.isWindows) {
-        await chown(dest, process.getuid(), process.getgid());
-        await handle.chown(process.getuid(), process.getgid());
-      }
+        await assert.rejects(
+          async () => {
+            await chown(dest, 1, -2);
+          },
+          {
+            code: 'ERR_OUT_OF_RANGE',
+            name: 'RangeError',
+            message: 'The value of "gid" is out of range. ' +
+                     'It must be >= -1 && <= 4294967295. Received -2'
+          });

-      assert.rejects(
-        async () => {
-          await chown(dest, 1, -2);
-        },
-        {
-          code: 'ERR_OUT_OF_RANGE',
-          name: 'RangeError',
-          message: 'The value of "gid" is out of range. ' +
-                  'It must be >= -1 && <= 4294967295. Received -2'
-        });
-
-      assert.rejects(
-        async () => {
-          await handle.chown(1, -2);
-        },
-        {
-          code: 'ERR_OUT_OF_RANGE',
-          name: 'RangeError',
-          message: 'The value of "gid" is out of range. ' +
-                    'It must be >= -1 && <= 4294967295. Received -2'
-        });
-
-      await handle.close();
+        await assert.rejects(
+          async () => {
+            await handle.chown(1, -2);
+          },
+          {
+            code: 'ERR_OUT_OF_RANGE',
+            name: 'RangeError',
+            message: 'The value of "gid" is out of range. ' +
+                      'It must be >= -1 && <= 4294967295. Received -2'
+          });
+      });
    }

    // Set modification times
    {
-      const handle = await getHandle(dest);
+      await executeOnHandle(dest, async (handle) => {

-      await utimes(dest, new Date(), new Date());
+        await utimes(dest, new Date(), new Date());

-      try {
-        await handle.utimes(new Date(), new Date());
-      } catch (err) {
-        // Some systems do not have futimes. If there is an error,
-        // expect it to be ENOSYS
-        common.expectsError({
-          code: 'ENOSYS',
-          name: 'Error'
-        })(err);
-      }
-
-      await handle.close();
+        try {
+          await handle.utimes(new Date(), new Date());
+        } catch (err) {
+          // Some systems do not have futimes. If there is an error,
+          // expect it to be ENOSYS
+          common.expectsError({
+            code: 'ENOSYS',
+            name: 'Error'
+          })(err);
+        }
+      });
    }

    // Set modification times with lutimes
@@ -438,29 +448,28 @@ async function getHandle(dest) {

    // Regression test for https://github.com/nodejs/node/issues/38168
    {
-      const handle = await getHandle(dest);
+      await executeOnHandle(dest, async (handle) => {
+        await assert.rejects(
+          async () => handle.write('abc', 0, 'hex'),
+          {
+            code: 'ERR_INVALID_ARG_VALUE',
+            message: /'encoding' is invalid for data of length 3/
+          }
+        );

-      assert.rejects(
-        async () => handle.write('abc', 0, 'hex'),
-        {
-          code: 'ERR_INVALID_ARG_VALUE',
-          message: /'encoding' is invalid for data of length 3/
-        }
-      );
-
-      const ret = await handle.write('abcd', 0, 'hex');
-      assert.strictEqual(ret.bytesWritten, 2);
-      await handle.close();
+        const ret = await handle.write('abcd', 0, 'hex');
+        assert.strictEqual(ret.bytesWritten, 2);
+      });
    }

    // Test prototype methods calling with contexts other than FileHandle
    {
-      const handle = await getHandle(dest);
-      assert.rejects(() => handle.stat.call({}), {
-        code: 'ERR_INTERNAL_ASSERTION',
-        message: /handle must be an instance of FileHandle/
+      await executeOnHandle(dest, async (handle) => {
+        await assert.rejects(() => handle.stat.call({}), {
+          code: 'ERR_INTERNAL_ASSERTION',
+          message: /handle must be an instance of FileHandle/
+        });
      });
-      await handle.close();
    }
  }

--- a/test/parallel/test-fs-read-stream-pos.js
+++ b/test/parallel/test-fs-read-stream-pos.js
@@ -16,7 +16,7 @@ fs.writeFileSync(file, '');

 let counter = 0;

-setInterval(() => {
+const writeInterval = setInterval(() => {
  counter = counter + 1;
  const line = `hello at ${counter}\n`;
  fs.writeFileSync(file, line, { flag: 'a' });
@@ -28,7 +28,7 @@ let isLow = false;
 let cur = 0;
 let stream;

-setInterval(() => {
+const readInterval = setInterval(() => {
  if (stream) return;

  stream = fs.createReadStream(file, {
@@ -49,7 +49,7 @@ setInterval(() => {
          return false;
        });
      assert.strictEqual(brokenLines.length, 0);
-      process.exit();
+      exitTest();
      return;
    }
    if (chunk.length !== hwm) {
@@ -64,6 +64,20 @@ setInterval(() => {
 }, 10);

 // Time longer than 90 seconds to exit safely
-setTimeout(() => {
-  process.exit();
+const endTimer = setTimeout(() => {
+  exitTest();
 }, 90000);
+
+const exitTest = () => {
+  clearInterval(readInterval);
+  clearInterval(writeInterval);
+  clearTimeout(endTimer);
+  if (stream && !stream.destroyed) {
+    stream.on('close', () => {
+      process.exit();
+    });
+    stream.destroy();
+  } else {
+    process.exit();
+  }
+};
--- a/test/parallel/test-fs-rm.js
+++ b/test/parallel/test-fs-rm.js
@@ -5,6 +5,8 @@ const tmpdir = require('../common/tmpdir');
 const assert = require('assert');
 const fs = require('fs');
 const path = require('path');
+const { execSync } = require('child_process');
+
 const { validateRmOptionsSync } = require('internal/fs/utils');

 tmpdir.refresh();
@@ -280,3 +282,80 @@ function removeAsync(dir) {
    message: /^The value of "options\.maxRetries" is out of range\./
  });
 }
+
+{
+  // IBMi has a different access permission mechanism
+  // This test should not be run as `root`
+  if (!common.isIBMi && (common.isWindows || process.getuid() !== 0)) {
+    function makeDirectoryReadOnly(dir, mode) {
+      let accessErrorCode = 'EACCES';
+      if (common.isWindows) {
+        accessErrorCode = 'EPERM';
+        execSync(`icacls ${dir} /deny "everyone:(OI)(CI)(DE,DC)"`);
+      } else {
+        fs.chmodSync(dir, mode);
+      }
+      return accessErrorCode;
+    }
+
+    function makeDirectoryWritable(dir) {
+      if (fs.existsSync(dir)) {
+        if (common.isWindows) {
+          execSync(`icacls ${dir} /remove:d "everyone"`);
+        } else {
+          fs.chmodSync(dir, 0o777);
+        }
+      }
+    }
+
+    {
+      // Check that deleting a file that cannot be accessed using rmsync throws
+      // https://github.com/nodejs/node/issues/38683
+      const dirname = nextDirPath();
+      const filePath = path.join(dirname, 'text.txt');
+      try {
+        fs.mkdirSync(dirname, { recursive: true });
+        fs.writeFileSync(filePath, 'hello');
+        const code = makeDirectoryReadOnly(dirname, 0o444);
+        assert.throws(() => {
+          fs.rmSync(filePath, { force: true });
+        }, {
+          code,
+          name: 'Error',
+        });
+      } finally {
+        makeDirectoryWritable(dirname);
+      }
+    }
+
+    {
+      // Check endless recursion.
+      // https://github.com/nodejs/node/issues/34580
+      const dirname = nextDirPath();
+      fs.mkdirSync(dirname, { recursive: true });
+      const root = fs.mkdtempSync(path.join(dirname, 'fs-'));
+      const middle = path.join(root, 'middle');
+      fs.mkdirSync(middle);
+      fs.mkdirSync(path.join(middle, 'leaf')); // Make `middle` non-empty
+      try {
+        const code = makeDirectoryReadOnly(middle, 0o555);
+        try {
+          assert.throws(() => {
+            fs.rmSync(root, { recursive: true });
+          }, {
+            code,
+            name: 'Error',
+          });
+        } catch (err) {
+          // Only fail the test if the folder was not deleted.
+          // as in some cases rmSync succesfully deletes read-only folders.
+          if (fs.existsSync(root)) {
+            throw err;
+          }
+        }
+      } finally {
+        makeDirectoryWritable(middle);
+      }
+    }
+  }
+}