Skip to content

Commit

Permalink
fs: streamline EISDIR error on different platforms
Browse files Browse the repository at this point in the history
Previously upon calling file-only fs functions with a directory (or
presumably a directory of path ending with a forward slash '/') it would
result in EISDIR error on Linux but Windows would completely ignore trailing '/'
and perform operation on a file excluding it.

This introduces additional checks in fs code to make sure the trailing
slash is handled the same way across systems.

Fixes: nodejs#17801
  • Loading branch information
lundibundi committed Aug 22, 2020
1 parent ac3049d commit 0a9e6d8
Show file tree
Hide file tree
Showing 6 changed files with 224 additions and 19 deletions.
31 changes: 22 additions & 9 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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';
Expand All @@ -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);

Expand Down Expand Up @@ -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();
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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

Expand Down
7 changes: 7 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
6 changes: 3 additions & 3 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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(
Expand Down
16 changes: 15 additions & 1 deletion lib/internal/fs/streams.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand Down
24 changes: 18 additions & 6 deletions lib/internal/fs/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
Expand Down
159 changes: 159 additions & 0 deletions test/parallel/test-fs-path-dir.js
Original file line number Diff line number Diff line change
@@ -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 });

0 comments on commit 0a9e6d8

Please sign in to comment.