fs: stop fixing EPERM on Windows

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,29 @@ 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) {
+    try {
+      fs.rmSync(pathname, { maxRetries: 3, recursive: true, force: true });
+      return;
+    } catch (_err) {
+      err = _err;
+    }
+
+    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 {}
+    }
+  }
+
+  throw err;
 }
 
 const testRoot = process.env.NODE_TEST_DIR ?
@@ -38,7 +60,7 @@ function onexit() {
 
   try {
     rmSync(tmpPath);
-  } catch (e) {
+  } catch (err) {
     console.error('Can\'t clean tmpdir:', tmpPath);
 
     const files = fs.readdirSync(tmpPath);
@@ -51,8 +73,11 @@ function onexit() {
       console.error('See http://nfs.sourceforge.net/#faq_d2 for details.');
     }
 
-    console.error();
-    throw e;
+    // Additionally logging err, just in case throwing err gets overshadowed by
+    // an error from a test failure.
+    console.error(err);
+
+    throw err;
   }
 }
 

test/parallel/test-fs-rm.js

@@ -5,7 +5,6 @@ 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');
 
@@ -287,74 +286,134 @@ function removeAsync(dir) {
   // 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);
+    class InvalidStateError extends Error {
+      constructor(err) {
+        super('Invalid state');
+        this.cause = err;
       }
-      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
+    {
+      function validateState(exists, err) {
+        // On Windows, we are allowed to access and modify the contents of a
+        // read-only folder.
+        const isValidState =
+          common.isWindows ?
+            (exists === false && err === null) :
+            (exists === true && err?.code === 'EACCES');
+
+        if (!isValidState) {
+          throw new InvalidStateError(err);
         }
       }
-    }
 
-    {
-      // 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 {
+      {
+        const dirname = nextDirPath();
+        const filePath = path.join(dirname, 'text.txt');
+
         fs.mkdirSync(dirname, { recursive: true });
         fs.writeFileSync(filePath, 'hello');
-        const code = makeDirectoryReadOnly(dirname, 0o444);
-        assert.throws(() => {
+
+        fs.chmodSync(filePath, 0o444);
+        fs.chmodSync(dirname, 0o444);
+
+        let err = null;
+
+        try {
           fs.rmSync(filePath, { force: true });
-        }, {
-          code,
-          name: 'Error',
-        });
-      } finally {
-        makeDirectoryWritable(dirname);
+        } catch (_err) {
+          err = _err;
+        }
+
+        try { fs.chmodSync(dirname, 0o777); } catch {}
+        try { fs.chmodSync(filePath, 0o777); } catch {}
+
+        validateState(fs.existsSync(filePath), err);
+      }
+
+      {
+        const dirname = nextDirPath();
+        const filePath = path.join(dirname, 'text.txt');
+
+        fs.mkdirSync(dirname, { recursive: true });
+        fs.writeFileSync(filePath, 'hello');
+
+        fs.chmodSync(filePath, 0o444);
+        fs.chmodSync(dirname, 0o444);
+
+        fs.rm(filePath, { force: true }, common.mustCall((err) => {
+          try { fs.chmodSync(dirname, 0o777); } catch {}
+          try { fs.chmodSync(filePath, 0o777); } catch {}
+
+          validateState(fs.existsSync(filePath), err);
+        }));
       }
     }
 
+    // Check endless recursion.
+    // https://github.com/nodejs/node/issues/34580
     {
-      // 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);
+      function validateState(exists, err) {
+        // On Windows, we are not allowed to delete a read-only folder yet.
+        // TODO(RaisinTen): Remove Windows special-casing if this lands:
+        // https://github.com/libuv/libuv/pull/3193
+        const isValidState =
+          exists === true &&
+          err?.code === common.isWindows ? 'EPERM' : 'EACCES';
+
+        if (!isValidState) {
+          throw new InvalidStateError(err);
+        }
+      }
+
+      {
+        const dirname = nextDirPath();
+        fs.mkdirSync(dirname, { recursive: true });
+        const root = fs.mkdtempSync(path.join(dirname, 'fs-'));
+        const middle = path.join(root, 'middle');
+        const leaf = path.join(middle, 'leaf');
+
+        fs.mkdirSync(middle);
+        fs.mkdirSync(leaf);
+
+        fs.chmodSync(leaf, 0o555);
+        fs.chmodSync(middle, 0o555);
+
+        let err = null;
+
         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;
-          }
+          fs.rmSync(root, { recursive: true });
+        } catch (_err) {
+          err = _err;
         }
-      } finally {
-        makeDirectoryWritable(middle);
+
+        try { fs.chmodSync(middle, 0o777); } catch {}
+        try { fs.chmodSync(leaf, 0o777); } catch {}
+
+        validateState(fs.existsSync(root), err);
+      }
+
+      {
+        const dirname = nextDirPath();
+        fs.mkdirSync(dirname, { recursive: true });
+        const root = fs.mkdtempSync(path.join(dirname, 'fs-'));
+        const middle = path.join(root, 'middle');
+        const leaf = path.join(middle, 'leaf');
+
+        fs.mkdirSync(middle);
+        fs.mkdirSync(leaf);
+
+        fs.chmodSync(leaf, 0o555);
+        fs.chmodSync(middle, 0o555);
+
+        fs.rm(root, { recursive: true }, common.mustCall((err) => {
+          try { fs.chmodSync(middle, 0o777); } catch {}
+          try { fs.chmodSync(leaf, 0o777); } catch {}
+
+          validateState(fs.existsSync(root), err);
+        }));
       }
     }
   }