fs: streamline EISDIR error on different platforms

lib/fs.js

@@ -317,7 +317,7 @@ function readFile(path, options, callback) {
     return;
   }
 
-  path = getValidatedPath(path);
+  path = getValidatedPath(path, 'path', true);
   const flagsNumber = stringToFlags(options.flags);
   binding.open(pathModule.toNamespacedPath(path),
                flagsNumber,
@@ -365,7 +365,11 @@ function tryReadSync(fd, isUserFd, buffer, pos, len) {
 function readFileSync(path, options) {
   options = getOptions(options, { flag: 'r' });
   const isUserFd = isFd(path); // File descriptor ownership
-  const fd = isUserFd ? path : fs.openSync(path, options.flag, 0o666);
+  let fd = path;
+  if (!isUserFd) {
+    path = getValidatedPath(path, 'path', true);
+    fd = fs.openSync(path, options.flag, 0o666);
+  }
 
   const stats = tryStatSync(fd, isUserFd);
   const size = isFileType(stats, S_IFREG) ? stats[8] : 0;
@@ -429,7 +433,7 @@ function closeSync(fd) {
 }
 
 function open(path, flags, mode, callback) {
-  path = getValidatedPath(path);
+  path = getValidatedPath(path, 'path', true);
   if (arguments.length < 3) {
     callback = flags;
     flags = 'r';
@@ -454,7 +458,7 @@ function open(path, flags, mode, callback) {
 
 
 function openSync(path, flags, mode) {
-  path = getValidatedPath(path);
+  path = getValidatedPath(path, 'path', true);
   const flagsNumber = stringToFlags(flags);
   mode = parseFileMode(mode, 'mode', 0o666);
 
@@ -777,6 +781,7 @@ function truncate(path, len, callback) {
 
   validateInteger(len, 'len');
   callback = maybeCallback(callback);
+  path = getValidatedPath(path, 'path', true);
   fs.open(path, 'r+', (er, fd) => {
     if (er) return callback(er);
     const req = new FSReqCallback();
@@ -798,6 +803,9 @@ function truncateSync(path, len) {
   if (len === undefined) {
     len = 0;
   }
+
+  path = getValidatedPath(path, 'path', true);
+
   // Allow error to be thrown, but still close fd.
   const fd = fs.openSync(path, 'r+');
   let ret;
@@ -1391,6 +1399,7 @@ function writeFile(path, data, options, callback) {
     writeAll(path, isUserFd, data, 0, data.byteLength, callback);
     return;
   }
+  path = getValidatedPath(path, 'path', true);
 
   fs.open(path, flag, options.mode, (openErr, fd) => {
     if (openErr) {
@@ -1413,7 +1422,11 @@ function writeFileSync(path, data, options) {
   const flag = options.flag || 'w';
 
   const isUserFd = isFd(path); // File descriptor ownership
-  const fd = isUserFd ? path : fs.openSync(path, flag, options.mode);
+  let fd = path;
+  if (!isUserFd) {
+    path = getValidatedPath(path, 'path', true);
+    fd = fs.openSync(path, flag, options.mode);
+  }
 
   let offset = 0;
   let length = data.byteLength;
@@ -1916,8 +1929,8 @@ function copyFile(src, dest, mode, callback) {
     throw new ERR_INVALID_CALLBACK(callback);
   }
 
-  src = getValidatedPath(src, 'src');
-  dest = getValidatedPath(dest, 'dest');
+  src = getValidatedPath(src, 'src', true);
+  dest = getValidatedPath(dest, 'dest', true);
 
   src = pathModule._makeLong(src);
   dest = pathModule._makeLong(dest);
@@ -1929,8 +1942,8 @@ function copyFile(src, dest, mode, callback) {
 
 
 function copyFileSync(src, dest, mode) {
-  src = getValidatedPath(src, 'src');
-  dest = getValidatedPath(dest, 'dest');
+  src = getValidatedPath(src, 'src', true);
+  dest = getValidatedPath(dest, 'dest', true);
 
   const ctx = { path: src, dest };  // non-prefixed
 

lib/internal/errors.js

@@ -1483,3 +1483,10 @@ E('ERR_WORKER_UNSUPPORTED_EXTENSION',
 E('ERR_WORKER_UNSUPPORTED_OPERATION',
   '%s is not supported in workers', TypeError);
 E('ERR_ZLIB_INITIALIZATION_FAILED', 'Initialization failed', Error);
+// This error is used in compatibility with what some operating systems
+// already throw. And is therefore put at the end of the list.
+// Eslint disable is needed to disable node-core/documented-errors and
+// node-code/alphabetize-errors and since that doesn't fit in one line
+// we disable all.
+// eslint-disable-next-line
+E('EISDIR', 'illegal operation on a directory: "%s"', Error);

lib/internal/fs/promises.js

@@ -291,8 +291,8 @@ async function access(path, mode = F_OK) {
 }
 
 async function copyFile(src, dest, mode) {
-  src = getValidatedPath(src, 'src');
-  dest = getValidatedPath(dest, 'dest');
+  src = getValidatedPath(src, 'src', true);
+  dest = getValidatedPath(dest, 'dest', true);
   mode = getValidMode(mode, 'copyFile');
   return binding.copyFile(pathModule.toNamespacedPath(src),
                           pathModule.toNamespacedPath(dest),
@@ -303,7 +303,7 @@ async function copyFile(src, dest, mode) {
 // Note that unlike fs.open() which uses numeric file descriptors,
 // fsPromises.open() uses the fs.FileHandle class.
 async function open(path, flags, mode) {
-  path = getValidatedPath(path);
+  path = getValidatedPath(path, 'path', true);
   const flagsNumber = stringToFlags(flags);
   mode = parseFileMode(mode, 'mode', 0o666);
   return new FileHandle(

lib/internal/fs/streams.js

@@ -20,6 +20,7 @@ const { Buffer } = require('buffer');
 const {
   copyObject,
   getOptions,
+  validatePath,
 } = require('internal/fs/utils');
 const { Readable, Writable, finished } = require('stream');
 const { toPathIfFileURL } = require('internal/url');
@@ -114,7 +115,13 @@ function ReadStream(path, options) {
 
   // Path will be ignored when fd is specified, so it can be falsy
   this.path = toPathIfFileURL(path);
-  this.fd = options.fd === undefined ? null : options.fd;
+  if (options.fd !== undefined) {
+    this.fd = options.fd;
+  } else {
+    this.fd = null;
+    validatePath(this.path, 'path', true);
+  }
+
   this.flags = options.flags === undefined ? 'r' : options.flags;
   this.mode = options.mode === undefined ? 0o666 : options.mode;
 
@@ -286,6 +293,13 @@ function WriteStream(path, options) {
 
   // Path will be ignored when fd is specified, so it can be falsy
   this.path = toPathIfFileURL(path);
+  if (options.fd !== undefined) {
+    this.fd = options.fd;
+  } else {
+    this.fd = null;
+    validatePath(this.path, 'path', true);
+  }
+
   this.fd = options.fd === undefined ? null : options.fd;
   this.flags = options.flags === undefined ? 'w' : options.flags;
   this.mode = options.mode === undefined ? 0o666 : options.mode;

lib/internal/fs/utils.js

@@ -16,6 +16,7 @@ const {
 const { Buffer } = require('buffer');
 const {
   codes: {
+    EISDIR,
     ERR_FS_INVALID_SYMLINK_TYPE,
     ERR_INVALID_ARG_TYPE,
     ERR_INVALID_ARG_VALUE,
@@ -608,7 +609,7 @@ const validateOffsetLengthWrite = hideStackFrames(
   }
 );
 
-const validatePath = hideStackFrames((path, propName = 'path') => {
+const validatePath = hideStackFrames((path, propName = 'path', isFile) => {
   if (typeof path !== 'string' && !isUint8Array(path)) {
     throw new ERR_INVALID_ARG_TYPE(propName, ['string', 'Buffer', 'URL'], path);
   }
@@ -618,14 +619,25 @@ const validatePath = hideStackFrames((path, propName = 'path') => {
   if (err !== undefined) {
     throw err;
   }
-});
 
-const getValidatedPath = hideStackFrames((fileURLOrPath, propName = 'path') => {
-  const path = toPathIfFileURL(fileURLOrPath);
-  validatePath(path, propName);
-  return path;
+  if (isFile) {
+    const lastCharacter = path[path.length - 1];
+    if (
+      lastCharacter === '/' || lastCharacter === 47 ||
+      (isWindows && (lastCharacter === '\\' || lastCharacter === 92))
+    ) {
+      throw new EISDIR(path);
+    }
+  }
 });
 
+const getValidatedPath =
+  hideStackFrames((fileURLOrPath, propName = 'path', isFile) => {
+    const path = toPathIfFileURL(fileURLOrPath);
+    validatePath(path, propName, isFile);
+    return path;
+  });
+
 const validateBufferArray = hideStackFrames((buffers, propName = 'buffers') => {
   if (!ArrayIsArray(buffers))
     throw new ERR_INVALID_ARG_TYPE(propName, 'ArrayBufferView[]', buffers);

test/sequential/sequential.status

@@ -35,3 +35,7 @@ test-perf-hooks: SKIP
 [$arch==arm]
 # https://github.com/nodejs/node/issues/26401#issuecomment-613095719
 test-worker-prof: PASS, FLAKY
+# The test itself is not flaky but it cannot finish within timeout on ARM and
+# since there is no way to change test timeout mark it as flaky instead so
+# that it will at least be checked on other platforms
+test-fs-path-dir: PASS, FLAKY

test/sequential/test-fs-path-dir.js

@@ -0,0 +1,159 @@
+'use strict';
+
+const common = require('../common');
+const tmpdir = require('../common/tmpdir');
+const assert = require('assert');
+const path = require('path');
+const fs = require('fs');
+const URL = require('url').URL;
+
+tmpdir.refresh();
+
+let fileCounter = 1;
+const nextFile = () => path.join(tmpdir.path, `file${fileCounter++}`);
+
+const generateStringPath = (file, suffix = '') => file + suffix;
+
+const generateURLPath = (file, suffix = '') =>
+  new URL('file://' + path.resolve(file) + suffix);
+
+const generateUint8ArrayPath = (file, suffix = '') =>
+  new Uint8Array(Buffer.from(file + suffix));
+
+const generatePaths = (file, suffix = '') => [
+  generateStringPath(file, suffix),
+  generateURLPath(file, suffix),
+  generateUint8ArrayPath(file, suffix),
+];
+
+const generateNewPaths = (suffix = '') => [
+  generateStringPath(nextFile(), suffix),
+  generateURLPath(nextFile(), suffix),
+  generateUint8ArrayPath(nextFile(), suffix),
+];
+
+function checkSyncFn(syncFn, p, args, fail) {
+  const result = fail ? 'must fail' : 'must succeed';
+  const failMsg = `failed testing sync ${p} ${syncFn.name} ${result}`;
+  if (!fail) {
+    try {
+      syncFn(p, ...args);
+    } catch (e) {
+      console.log(failMsg, e);
+      throw e;
+    }
+  } else {
+    assert.throws(() => syncFn(p, ...args), {
+      code: 'EISDIR',
+      name: 'Error',
+    }, failMsg);
+  }
+}
+
+function checkAsyncFn(asyncFn, p, args, fail) {
+  const result = fail ? 'must fail' : 'must succeed';
+  const failMsg = `failed testing async ${p} ${asyncFn.name} ${result}`;
+  if (!fail) {
+    try {
+      asyncFn(p, ...args, common.mustCall());
+    } catch (e) {
+      console.log(failMsg, e);
+      throw e;
+    }
+  } else {
+    assert.throws(
+      () => asyncFn(p, ...args, common.mustNotCall()), {
+        code: 'EISDIR',
+        name: 'Error',
+      },
+      failMsg
+    );
+  }
+}
+
+function checkPromiseFn(promiseFn, p, args, fail) {
+  const result = fail ? 'must fail' : 'must succeed';
+  const failMsg = `failed testing promise ${p} ${promiseFn.name} ${result}`;
+  if (!fail) {
+    const r = promiseFn(p, ...args).catch((err) => {
+      console.log(failMsg, err);
+      throw err;
+    });
+    if (r && r.close) r.close();
+  } else {
+    assert.rejects(
+      () => promiseFn(p, ...args), {
+        code: 'EISDIR',
+        name: 'Error',
+      },
+      failMsg
+    );
+  }
+}
+
+function check(asyncFn, syncFn, promiseFn,
+               { suffix, fail, args = [] }) {
+  const file = nextFile();
+  fs.writeFile(file, 'data', (e) => {
+    assert.ifError(e);
+    const paths = generatePaths(file, suffix);
+    for (const p of paths) {
+      if (asyncFn) checkAsyncFn(asyncFn, p, args, fail);
+      if (promiseFn) checkPromiseFn(promiseFn, p, args, fail);
+      if (syncFn) checkSyncFn(syncFn, p, args, fail);
+    }
+  });
+}
+
+function checkManyToMany(asyncFn, syncFn, promiseFn,
+                         { suffix, fail, args = [] }) {
+  const source = nextFile();
+  fs.writeFile(source, 'data', (err) => {
+    assert.ifError(err);
+    if (asyncFn) {
+      generateNewPaths(suffix)
+        .forEach((p) => checkAsyncFn(asyncFn, source, [p, ...args], fail));
+    }
+    if (promiseFn) {
+      generateNewPaths(suffix)
+        .forEach((p) => checkPromiseFn(promiseFn, source, [p, ...args], fail));
+    }
+    if (syncFn) {
+      generateNewPaths(suffix)
+        .forEach((p) => checkSyncFn(syncFn, source, [p, ...args], fail));
+    }
+  });
+}
+
+check(fs.readFile, fs.readFileSync, fs.promises.readFile,
+      { suffix: '', fail: false });
+check(fs.readFile, fs.readFileSync, fs.promises.readFile,
+      { suffix: '/', fail: true });
+check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile,
+      { suffix: '', fail: false, args: ['data'] });
+check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile,
+      { suffix: '/', fail: true, args: ['data'] });
+check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile,
+      { suffix: '', fail: false, args: ['data'] });
+check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile,
+      { suffix: '/', fail: true, args: ['data'] });
+check(undefined, fs.createReadStream, undefined,
+      { suffix: '', fail: false });
+check(undefined, fs.createReadStream, undefined,
+      { suffix: '/', fail: true });
+check(undefined, fs.createWriteStream, undefined,
+      { suffix: '', fail: false });
+check(undefined, fs.createWriteStream, undefined,
+      { suffix: '/', fail: true });
+check(fs.truncate, fs.truncateSync, fs.promises.truncate,
+      { suffix: '', fail: false, args: [42] });
+check(fs.truncate, fs.truncateSync, fs.promises.truncate,
+      { suffix: '/', fail: true, args: [42] });
+
+check(fs.open, fs.openSync, fs.promises.open, { suffix: '', fail: false });
+check(fs.open, fs.openSync, fs.promises.open, { suffix: '/', fail: true });
+
+checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile,
+                { suffix: '', fail: false });
+checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile,
+                { suffix: '/', fail: true });