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>

8 files changed, 271 insertions, 152 deletions

diff --git a/test/parallel/test-fs-mkdir-recursive-eaccess.js b/test/parallel/test-fs-mkdir-recursive-eaccess.js
index d0b3b2b950c..c07f8cbd1bf 100644
--- 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"`);
   }
 }
 
diff --git a/test/parallel/test-fs-open-no-close.js b/test/parallel/test-fs-open-no-close.js
index 5e432dd11d8..41b58f5ef79 100644
--- 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);
+  }
+}));
diff --git a/test/parallel/test-fs-promises-file-handle-read-worker.js b/test/parallel/test-fs-promises-file-handle-read-worker.js
index ccf3338d145..a1b7fbd2dfb 100644
--- 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 = '';
diff --git a/test/parallel/test-fs-promises-file-handle-readFile.js b/test/parallel/test-fs-promises-file-handle-readFile.js
index fc28dd23328..880b97d7071 100644
--- 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
diff --git a/test/parallel/test-fs-promises-file-handle-writeFile.js b/test/parallel/test-fs-promises-file-handle-writeFile.js
index 7ad1beb4bcd..44f049f55a9 100644
--- 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()
diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js
index f0140084732..5da1e55ffd2 100644
--- 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 unlink(dest);
-      await handle.close();
+      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);
+      });
     }
 
     // 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());
-      }
-
-      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'
-        });
+        if (!common.isWindows) {
+          await chown(dest, process.getuid(), process.getgid());
+          await handle.chown(process.getuid(), process.getgid());
+        }
 
-      await handle.close();
+        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'
+          });
+
+        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 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();
+      await executeOnHandle(dest, async (handle) => {
+
+        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);
+        }
+      });
     }
 
     // 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);
-
-      assert.rejects(
-        async () => handle.write('abc', 0, 'hex'),
-        {
-          code: 'ERR_INVALID_ARG_VALUE',
-          message: /'encoding' is invalid for data of length 3/
-        }
-      );
+      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/
+          }
+        );
 
-      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();
     }
   }
 
diff --git a/test/parallel/test-fs-read-stream-pos.js b/test/parallel/test-fs-read-stream-pos.js
index c9470cb23dd..c67c2e8a81f 100644
--- 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();
+  }
+};
diff --git a/test/parallel/test-fs-rm.js b/test/parallel/test-fs-rm.js
index 5bb5d2de553..3fdfc842624 100644
--- 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);
+      }
+    }
+  }
+}