fs: fix rmsync error swallowing

Linkgoron · jasnell · commit f9447b71a6b4 · 2021-05-25T07:18:05.000-07:00
fix rmsync swallowing errors instead of throwing them. fixes: #38683 fixes: #34580 PR-URL: #38684 Fixes: #38683 Fixes: #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>
diff --git 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;
   }
 }
 
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git a/test/parallel/test-fs-read-stream-pos.js b/test/parallel/test-fs-read-stream-pos.js
diff --git a/test/parallel/test-fs-rm.js b/test/parallel/test-fs-rm.js

Original file line number	Diff line number	Diff line change
`@@ -219,6 +219,11 @@ function _unlinkSync(path, options) {`
`219`	`219`	`i < tries &&`
`220`	`220`	`options.retryDelay > 0) {`
`221`	`221`	`sleep(i * options.retryDelay);`
	`222`	`+ } else if (err.code === 'ENOENT') {`
	`223`	`+ // The file is already gone.`
	`224`	`+ return;`
	`225`	`+ } else if (i === tries) {`
	`226`	`+ throw err;`
`222`	`227`	`}`
`223`	`228`	`}`
`224`	`229`	`}`
`@@ -231,8 +236,9 @@ function _rmdirSync(path, options, originalErr) {`
`231`	`236`	`} catch (err) {`
`232`	`237`	`if (err.code === 'ENOENT')`
`233`	`238`	`return;`
`234`		`- if (err.code === 'ENOTDIR')`
`235`		`- throw originalErr;`
	`239`	`+ if (err.code === 'ENOTDIR') {`
	`240`	`+ throw originalErr \|\| err;`
	`241`	`+ }`
`236`	`242`
`237`	`243`	`if (notEmptyErrorCodes.has(err.code)) {`
`238`	`244`	`// Removing failed. Try removing all children and then retrying the`
`@@ -259,10 +265,17 @@ function _rmdirSync(path, options, originalErr) {`
`259`	`265`	`i < tries &&`
`260`	`266`	`options.retryDelay > 0) {`
`261`	`267`	`sleep(i * options.retryDelay);`
	`268`	`+ } else if (err.code === 'ENOENT') {`
	`269`	`+ // The file is already gone.`
	`270`	`+ return;`
	`271`	`+ } else if (i === tries) {`
	`272`	`+ throw err;`
`262`	`273`	`}`
`263`	`274`	`}`
`264`	`275`	`}`
`265`	`276`	`}`
	`277`	`+`
	`278`	`+ throw originalErr \|\| err;`
`266`	`279`	`}`
`267`	`280`	`}`
`268`	`281`
Original file line number	Diff line number	Diff line change
`@@ -26,8 +26,7 @@ function makeDirectoryReadOnly(dir) {`
`26`	`26`	`let accessErrorCode = 'EACCES';`
`27`	`27`	`if (common.isWindows) {`
`28`	`28`	`accessErrorCode = 'EPERM';`
`29`		- execSync(`icacls ${dir} /inheritance:r`);
`30`		- execSync(`icacls ${dir} /deny "everyone":W`);
	`29`	+ execSync(`icacls ${dir} /deny "everyone:(OI)(CI)(DE,DC,AD,WD)"`);
`31`	`30`	`} else {`
`32`	`31`	`fs.chmodSync(dir, '444');`
`33`	`32`	`}`
`@@ -36,7 +35,7 @@ function makeDirectoryReadOnly(dir) {`
`36`	`35`
`37`	`36`	`function makeDirectoryWritable(dir) {`
`38`	`37`	`if (common.isWindows) {`
`39`		- execSync(`icacls ${dir} /grant "everyone":W`);
	`38`	+ execSync(`icacls ${dir} /remove:d "everyone"`);
`40`	`39`	`}`
`41`	`40`	`}`
`42`	`41`