fs: stop fixing EPERM on Windows

Originally, when rimraf faced an EPERM on Windows, it would try to
change the file permissions of the entry and attempt to remove it
again. However, rm -rf doesn't change any file permissions, so rimraf
shouldn't either.

This change also updates test/common/tmpdir.js to repeatedly try
changing the file permissions of its contents to 0o777 when an EACCES or
an EPERM is faced while attempting to remove them.

Signed-off-by: Darshan Sen <[email protected]>

lib/internal/fs/rimraf.js

@@ -1,9 +1,4 @@
-// This file is a modified version of the rimraf module on npm. It has been
-// modified in the following ways:
-// - Use of the assert module has been replaced with core's error system.
-// - All code related to the glob dependency has been removed.
-// - Bring your own custom fs module is not currently supported.
-// - Some basic code cleanup.
+// This file is a modified version of the rimraf module on npm.
 'use strict';
 
 const {
@@ -15,16 +10,12 @@ const {
 const { Buffer } = require('buffer');
 const fs = require('fs');
 const {
-  chmod,
-  chmodSync,
   lstat,
   lstatSync,
   readdir,
   readdirSync,
   rmdir,
   rmdirSync,
-  stat,
-  statSync,
   unlink,
   unlinkSync
 } = fs;
@@ -34,9 +25,6 @@ const { sleep } = require('internal/util');
 const notEmptyErrorCodes = new SafeSet(['ENOTEMPTY', 'EEXIST', 'EPERM']);
 const retryErrorCodes = new SafeSet(
   ['EBUSY', 'EMFILE', 'ENFILE', 'ENOTEMPTY', 'EPERM']);
-const isWindows = process.platform === 'win32';
-const epermHandler = isWindows ? fixWinEPERM : _rmdir;
-const epermHandlerSync = isWindows ? fixWinEPERMSync : _rmdirSync;
 const readdirEncoding = 'buffer';
 const separator = Buffer.from(sep);
 
@@ -69,10 +57,6 @@ function _rimraf(path, options, callback) {
     if (err) {
       if (err.code === 'ENOENT')
         return callback(null);
-
-      // Windows can EPERM on stat.
-      if (isWindows && err.code === 'EPERM')
-        return fixWinEPERM(path, options, err, callback);
     } else if (stats.isDirectory()) {
       return _rmdir(path, options, err, callback);
     }
@@ -81,11 +65,8 @@ function _rimraf(path, options, callback) {
       if (err) {
         if (err.code === 'ENOENT')
           return callback(null);
-        if (err.code === 'EISDIR')
+        if (err.code === 'EISDIR' || err.code === 'EPERM')
           return _rmdir(path, options, err, callback);
-        if (err.code === 'EPERM') {
-          return epermHandler(path, options, err, callback);
-        }
       }
 
       return callback(err);
@@ -94,24 +75,6 @@ function _rimraf(path, options, callback) {
 }
 
 
-function fixWinEPERM(path, options, originalErr, callback) {
-  chmod(path, 0o666, (err) => {
-    if (err)
-      return callback(err.code === 'ENOENT' ? null : originalErr);
-
-    stat(path, (err, stats) => {
-      if (err)
-        return callback(err.code === 'ENOENT' ? null : originalErr);
-
-      if (stats.isDirectory())
-        _rmdir(path, options, originalErr, callback);
-      else
-        unlink(path, callback);
-    });
-  });
-}
-
-
 function _rmdir(path, options, originalErr, callback) {
   rmdir(path, (err) => {
     if (err) {
@@ -181,10 +144,6 @@ function rimrafSync(path, options) {
   } catch (err) {
     if (err.code === 'ENOENT')
       return;
-
-    // Windows can EPERM on stat.
-    if (isWindows && err.code === 'EPERM')
-      fixWinEPERMSync(path, options, err);
   }
 
   try {
@@ -194,14 +153,11 @@ function rimrafSync(path, options) {
     else
       _unlinkSync(path, options);
   } catch (err) {
-    if (err.code === 'ENOENT')
-      return;
-    if (err.code === 'EPERM')
-      return epermHandlerSync(path, options, err);
-    if (err.code !== 'EISDIR')
-      throw err;
+    if (err.code === 'EISDIR' || err.code === 'EPERM')
+      return _rmdirSync(path, options, err);
 
-    _rmdirSync(path, options, err);
+    if (err.code !== 'ENOENT')
+      throw err;
   }
 }
 
@@ -280,31 +236,4 @@ function _rmdirSync(path, options, originalErr) {
 }
 
 
-function fixWinEPERMSync(path, options, originalErr) {
-  try {
-    chmodSync(path, 0o666);
-  } catch (err) {
-    if (err.code === 'ENOENT')
-      return;
-
-    throw originalErr;
-  }
-
-  let stats;
-
-  try {
-    stats = statSync(path, { throwIfNoEntry: false });
-  } catch {
-    throw originalErr;
-  }
-
-  if (stats === undefined) return;
-
-  if (stats.isDirectory())
-    _rmdirSync(path, options, originalErr);
-  else
-    _unlinkSync(path, options);
-}
-
-
 module.exports = { rimraf, rimrafPromises, rimrafSync };

test/common/tmpdir.js

@@ -6,7 +6,45 @@ const path = require('path');
 const { isMainThread } = require('worker_threads');
 
 function rmSync(pathname) {
-  fs.rmSync(pathname, { maxRetries: 3, recursive: true, force: true });
+  let err = null;
+  const maxRetries = 10;
+
+  for (let retryNumber = 0; retryNumber < maxRetries; ++retryNumber) {
+    err = null;
+
+    try {
+      fs.rmSync(pathname, { maxRetries: 3, recursive: true, force: true });
+    } catch (_err) {
+      err = _err;
+    }
+
+    if (err === null) {
+      return;
+    }
+
+    const errPath = err.path;
+    const errCode = err.code;
+
+    if (errCode === 'EACCES' || errCode === 'EPERM') {
+      const surroundingDir = path.join(errPath, '..');
+
+      try {
+        fs.chmodSync(surroundingDir, 0o777);
+      } catch {
+      }
+
+      try {
+        fs.chmodSync(errPath, 0o777);
+      } catch {
+      }
+    }
+  }
+
+  if (err === null) {
+    return;
+  }
+
+  throw err;
 }
 
 const testRoot = process.env.NODE_TEST_DIR ?
@@ -31,28 +69,37 @@ function refresh() {
   }
 }
 
+function logErrorInfo(err) {
+  console.error('Can\'t clean tmpdir:', tmpPath);
+
+  const files = fs.readdirSync(tmpPath);
+  console.error('Files blocking:', files);
+
+  if (files.some((f) => f.startsWith('.nfs'))) {
+    // Warn about NFS "silly rename"
+    console.error('Note: ".nfs*" might be files that were open and ' +
+                  'unlinked but not closed.');
+    console.error('See http://nfs.sourceforge.net/#faq_d2 for details.');
+  }
+
+  console.error();
+
+  // Additionally logging err, just in case throwing err gets overshadowed by
+  // an error from a test failure.
+  console.error(err);
+
+  throw err;
+}
+
 function onexit() {
   // Change directory to avoid possible EBUSY
   if (isMainThread)
     process.chdir(testRoot);
 
   try {
     rmSync(tmpPath);
-  } catch (e) {
-    console.error('Can\'t clean tmpdir:', tmpPath);
-
-    const files = fs.readdirSync(tmpPath);
-    console.error('Files blocking:', files);
-
-    if (files.some((f) => f.startsWith('.nfs'))) {
-      // Warn about NFS "silly rename"
-      console.error('Note: ".nfs*" might be files that were open and ' +
-                    'unlinked but not closed.');
-      console.error('See http://nfs.sourceforge.net/#faq_d2 for details.');
-    }
-
-    console.error();
-    throw e;
+  } catch (err) {
+    logErrorInfo(err);
   }
 }