Skip to content

Commit 0a9e6d8

Browse files
committed
fs: streamline EISDIR error on different platforms
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: #17801
1 parent ac3049d commit 0a9e6d8

6 files changed

Lines changed: 224 additions & 19 deletions

File tree

lib/fs.js

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,7 @@ function readFile(path, options, callback) {
317317
return;
318318
}
319319

320-
path = getValidatedPath(path);
320+
path = getValidatedPath(path, 'path', true);
321321
const flagsNumber = stringToFlags(options.flags);
322322
binding.open(pathModule.toNamespacedPath(path),
323323
flagsNumber,
@@ -365,7 +365,11 @@ function tryReadSync(fd, isUserFd, buffer, pos, len) {
365365
function readFileSync(path, options) {
366366
options = getOptions(options, { flag: 'r' });
367367
const isUserFd = isFd(path); // File descriptor ownership
368-
const fd = isUserFd ? path : fs.openSync(path, options.flag, 0o666);
368+
let fd = path;
369+
if (!isUserFd) {
370+
path = getValidatedPath(path, 'path', true);
371+
fd = fs.openSync(path, options.flag, 0o666);
372+
}
369373

370374
const stats = tryStatSync(fd, isUserFd);
371375
const size = isFileType(stats, S_IFREG) ? stats[8] : 0;
@@ -429,7 +433,7 @@ function closeSync(fd) {
429433
}
430434

431435
function open(path, flags, mode, callback) {
432-
path = getValidatedPath(path);
436+
path = getValidatedPath(path, 'path', true);
433437
if (arguments.length < 3) {
434438
callback = flags;
435439
flags = 'r';
@@ -454,7 +458,7 @@ function open(path, flags, mode, callback) {
454458

455459

456460
function openSync(path, flags, mode) {
457-
path = getValidatedPath(path);
461+
path = getValidatedPath(path, 'path', true);
458462
const flagsNumber = stringToFlags(flags);
459463
mode = parseFileMode(mode, 'mode', 0o666);
460464

@@ -777,6 +781,7 @@ function truncate(path, len, callback) {
777781

778782
validateInteger(len, 'len');
779783
callback = maybeCallback(callback);
784+
path = getValidatedPath(path, 'path', true);
780785
fs.open(path, 'r+', (er, fd) => {
781786
if (er) return callback(er);
782787
const req = new FSReqCallback();
@@ -798,6 +803,9 @@ function truncateSync(path, len) {
798803
if (len === undefined) {
799804
len = 0;
800805
}
806+
807+
path = getValidatedPath(path, 'path', true);
808+
801809
// Allow error to be thrown, but still close fd.
802810
const fd = fs.openSync(path, 'r+');
803811
let ret;
@@ -1391,6 +1399,7 @@ function writeFile(path, data, options, callback) {
13911399
writeAll(path, isUserFd, data, 0, data.byteLength, callback);
13921400
return;
13931401
}
1402+
path = getValidatedPath(path, 'path', true);
13941403

13951404
fs.open(path, flag, options.mode, (openErr, fd) => {
13961405
if (openErr) {
@@ -1413,7 +1422,11 @@ function writeFileSync(path, data, options) {
14131422
const flag = options.flag || 'w';
14141423

14151424
const isUserFd = isFd(path); // File descriptor ownership
1416-
const fd = isUserFd ? path : fs.openSync(path, flag, options.mode);
1425+
let fd = path;
1426+
if (!isUserFd) {
1427+
path = getValidatedPath(path, 'path', true);
1428+
fd = fs.openSync(path, flag, options.mode);
1429+
}
14171430

14181431
let offset = 0;
14191432
let length = data.byteLength;
@@ -1916,8 +1929,8 @@ function copyFile(src, dest, mode, callback) {
19161929
throw new ERR_INVALID_CALLBACK(callback);
19171930
}
19181931

1919-
src = getValidatedPath(src, 'src');
1920-
dest = getValidatedPath(dest, 'dest');
1932+
src = getValidatedPath(src, 'src', true);
1933+
dest = getValidatedPath(dest, 'dest', true);
19211934

19221935
src = pathModule._makeLong(src);
19231936
dest = pathModule._makeLong(dest);
@@ -1929,8 +1942,8 @@ function copyFile(src, dest, mode, callback) {
19291942

19301943

19311944
function copyFileSync(src, dest, mode) {
1932-
src = getValidatedPath(src, 'src');
1933-
dest = getValidatedPath(dest, 'dest');
1945+
src = getValidatedPath(src, 'src', true);
1946+
dest = getValidatedPath(dest, 'dest', true);
19341947

19351948
const ctx = { path: src, dest }; // non-prefixed
19361949

lib/internal/errors.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1483,3 +1483,10 @@ E('ERR_WORKER_UNSUPPORTED_EXTENSION',
14831483
E('ERR_WORKER_UNSUPPORTED_OPERATION',
14841484
'%s is not supported in workers', TypeError);
14851485
E('ERR_ZLIB_INITIALIZATION_FAILED', 'Initialization failed', Error);
1486+
// This error is used in compatibility with what some operating systems
1487+
// already throw. And is therefore put at the end of the list.
1488+
// Eslint disable is needed to disable node-core/documented-errors and
1489+
// node-code/alphabetize-errors and since that doesn't fit in one line
1490+
// we disable all.
1491+
// eslint-disable-next-line
1492+
E('EISDIR', 'illegal operation on a directory: "%s"', Error);

lib/internal/fs/promises.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -291,8 +291,8 @@ async function access(path, mode = F_OK) {
291291
}
292292

293293
async function copyFile(src, dest, mode) {
294-
src = getValidatedPath(src, 'src');
295-
dest = getValidatedPath(dest, 'dest');
294+
src = getValidatedPath(src, 'src', true);
295+
dest = getValidatedPath(dest, 'dest', true);
296296
mode = getValidMode(mode, 'copyFile');
297297
return binding.copyFile(pathModule.toNamespacedPath(src),
298298
pathModule.toNamespacedPath(dest),
@@ -303,7 +303,7 @@ async function copyFile(src, dest, mode) {
303303
// Note that unlike fs.open() which uses numeric file descriptors,
304304
// fsPromises.open() uses the fs.FileHandle class.
305305
async function open(path, flags, mode) {
306-
path = getValidatedPath(path);
306+
path = getValidatedPath(path, 'path', true);
307307
const flagsNumber = stringToFlags(flags);
308308
mode = parseFileMode(mode, 'mode', 0o666);
309309
return new FileHandle(

lib/internal/fs/streams.js

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ const { Buffer } = require('buffer');
2020
const {
2121
copyObject,
2222
getOptions,
23+
validatePath,
2324
} = require('internal/fs/utils');
2425
const { Readable, Writable, finished } = require('stream');
2526
const { toPathIfFileURL } = require('internal/url');
@@ -114,7 +115,13 @@ function ReadStream(path, options) {
114115

115116
// Path will be ignored when fd is specified, so it can be falsy
116117
this.path = toPathIfFileURL(path);
117-
this.fd = options.fd === undefined ? null : options.fd;
118+
if (options.fd !== undefined) {
119+
this.fd = options.fd;
120+
} else {
121+
this.fd = null;
122+
validatePath(this.path, 'path', true);
123+
}
124+
118125
this.flags = options.flags === undefined ? 'r' : options.flags;
119126
this.mode = options.mode === undefined ? 0o666 : options.mode;
120127

@@ -286,6 +293,13 @@ function WriteStream(path, options) {
286293

287294
// Path will be ignored when fd is specified, so it can be falsy
288295
this.path = toPathIfFileURL(path);
296+
if (options.fd !== undefined) {
297+
this.fd = options.fd;
298+
} else {
299+
this.fd = null;
300+
validatePath(this.path, 'path', true);
301+
}
302+
289303
this.fd = options.fd === undefined ? null : options.fd;
290304
this.flags = options.flags === undefined ? 'w' : options.flags;
291305
this.mode = options.mode === undefined ? 0o666 : options.mode;

lib/internal/fs/utils.js

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ const {
1616
const { Buffer } = require('buffer');
1717
const {
1818
codes: {
19+
EISDIR,
1920
ERR_FS_INVALID_SYMLINK_TYPE,
2021
ERR_INVALID_ARG_TYPE,
2122
ERR_INVALID_ARG_VALUE,
@@ -608,7 +609,7 @@ const validateOffsetLengthWrite = hideStackFrames(
608609
}
609610
);
610611

611-
const validatePath = hideStackFrames((path, propName = 'path') => {
612+
const validatePath = hideStackFrames((path, propName = 'path', isFile) => {
612613
if (typeof path !== 'string' && !isUint8Array(path)) {
613614
throw new ERR_INVALID_ARG_TYPE(propName, ['string', 'Buffer', 'URL'], path);
614615
}
@@ -618,14 +619,25 @@ const validatePath = hideStackFrames((path, propName = 'path') => {
618619
if (err !== undefined) {
619620
throw err;
620621
}
621-
});
622622

623-
const getValidatedPath = hideStackFrames((fileURLOrPath, propName = 'path') => {
624-
const path = toPathIfFileURL(fileURLOrPath);
625-
validatePath(path, propName);
626-
return path;
623+
if (isFile) {
624+
const lastCharacter = path[path.length - 1];
625+
if (
626+
lastCharacter === '/' || lastCharacter === 47 ||
627+
(isWindows && (lastCharacter === '\\' || lastCharacter === 92))
628+
) {
629+
throw new EISDIR(path);
630+
}
631+
}
627632
});
628633

634+
const getValidatedPath =
635+
hideStackFrames((fileURLOrPath, propName = 'path', isFile) => {
636+
const path = toPathIfFileURL(fileURLOrPath);
637+
validatePath(path, propName, isFile);
638+
return path;
639+
});
640+
629641
const validateBufferArray = hideStackFrames((buffers, propName = 'buffers') => {
630642
if (!ArrayIsArray(buffers))
631643
throw new ERR_INVALID_ARG_TYPE(propName, 'ArrayBufferView[]', buffers);

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

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const tmpdir = require('../common/tmpdir');
5+
const assert = require('assert');
6+
const path = require('path');
7+
const fs = require('fs');
8+
const URL = require('url').URL;
9+
10+
tmpdir.refresh();
11+
12+
let fileCounter = 1;
13+
const nextFile = () => path.join(tmpdir.path, `file${fileCounter++}`);
14+
15+
const generateStringPath = (file, suffix = '') => file + suffix;
16+
17+
const generateURLPath = (file, suffix = '') =>
18+
new URL('file://' + path.resolve(file) + suffix);
19+
20+
const generateUint8ArrayPath = (file, suffix = '') =>
21+
new Uint8Array(Buffer.from(file + suffix));
22+
23+
const generatePaths = (file, suffix = '') => [
24+
generateStringPath(file, suffix),
25+
generateURLPath(file, suffix),
26+
generateUint8ArrayPath(file, suffix),
27+
];
28+
29+
const generateNewPaths = (suffix = '') => [
30+
generateStringPath(nextFile(), suffix),
31+
generateURLPath(nextFile(), suffix),
32+
generateUint8ArrayPath(nextFile(), suffix),
33+
];
34+
35+
function checkSyncFn(syncFn, p, args, fail) {
36+
const result = fail ? 'must fail' : 'must succeed';
37+
const failMsg = `failed testing sync ${p} ${syncFn.name} ${result}`;
38+
if (!fail) {
39+
try {
40+
syncFn(p, ...args);
41+
} catch (e) {
42+
console.log(failMsg, e);
43+
throw e;
44+
}
45+
} else {
46+
assert.throws(() => syncFn(p, ...args), {
47+
code: 'EISDIR',
48+
name: 'Error',
49+
}, failMsg);
50+
}
51+
}
52+
53+
function checkAsyncFn(asyncFn, p, args, fail) {
54+
const result = fail ? 'must fail' : 'must succeed';
55+
const failMsg = `failed testing async ${p} ${asyncFn.name} ${result}`;
56+
if (!fail) {
57+
try {
58+
asyncFn(p, ...args, common.mustCall());
59+
} catch (e) {
60+
console.log(failMsg, e);
61+
throw e;
62+
}
63+
} else {
64+
assert.throws(
65+
() => asyncFn(p, ...args, common.mustNotCall()), {
66+
code: 'EISDIR',
67+
name: 'Error',
68+
},
69+
failMsg
70+
);
71+
}
72+
}
73+
74+
function checkPromiseFn(promiseFn, p, args, fail) {
75+
const result = fail ? 'must fail' : 'must succeed';
76+
const failMsg = `failed testing promise ${p} ${promiseFn.name} ${result}`;
77+
if (!fail) {
78+
const r = promiseFn(p, ...args).catch((err) => {
79+
console.log(failMsg, err);
80+
throw err;
81+
});
82+
if (r && r.close) r.close();
83+
} else {
84+
assert.rejects(
85+
() => promiseFn(p, ...args), {
86+
code: 'EISDIR',
87+
name: 'Error',
88+
},
89+
failMsg
90+
);
91+
}
92+
}
93+
94+
function check(asyncFn, syncFn, promiseFn,
95+
{ suffix, fail, args = [] }) {
96+
const file = nextFile();
97+
fs.writeFile(file, 'data', (e) => {
98+
assert.ifError(e);
99+
const paths = generatePaths(file, suffix);
100+
for (const p of paths) {
101+
if (asyncFn) checkAsyncFn(asyncFn, p, args, fail);
102+
if (promiseFn) checkPromiseFn(promiseFn, p, args, fail);
103+
if (syncFn) checkSyncFn(syncFn, p, args, fail);
104+
}
105+
});
106+
}
107+
108+
function checkManyToMany(asyncFn, syncFn, promiseFn,
109+
{ suffix, fail, args = [] }) {
110+
const source = nextFile();
111+
fs.writeFile(source, 'data', (err) => {
112+
assert.ifError(err);
113+
if (asyncFn) {
114+
generateNewPaths(suffix)
115+
.forEach((p) => checkAsyncFn(asyncFn, source, [p, ...args], fail));
116+
}
117+
if (promiseFn) {
118+
generateNewPaths(suffix)
119+
.forEach((p) => checkPromiseFn(promiseFn, source, [p, ...args], fail));
120+
}
121+
if (syncFn) {
122+
generateNewPaths(suffix)
123+
.forEach((p) => checkSyncFn(syncFn, source, [p, ...args], fail));
124+
}
125+
});
126+
}
127+
128+
check(fs.readFile, fs.readFileSync, fs.promises.readFile,
129+
{ suffix: '', fail: false });
130+
check(fs.readFile, fs.readFileSync, fs.promises.readFile,
131+
{ suffix: '/', fail: true });
132+
check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile,
133+
{ suffix: '', fail: false, args: ['data'] });
134+
check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile,
135+
{ suffix: '/', fail: true, args: ['data'] });
136+
check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile,
137+
{ suffix: '', fail: false, args: ['data'] });
138+
check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile,
139+
{ suffix: '/', fail: true, args: ['data'] });
140+
check(undefined, fs.createReadStream, undefined,
141+
{ suffix: '', fail: false });
142+
check(undefined, fs.createReadStream, undefined,
143+
{ suffix: '/', fail: true });
144+
check(undefined, fs.createWriteStream, undefined,
145+
{ suffix: '', fail: false });
146+
check(undefined, fs.createWriteStream, undefined,
147+
{ suffix: '/', fail: true });
148+
check(fs.truncate, fs.truncateSync, fs.promises.truncate,
149+
{ suffix: '', fail: false, args: [42] });
150+
check(fs.truncate, fs.truncateSync, fs.promises.truncate,
151+
{ suffix: '/', fail: true, args: [42] });
152+
153+
check(fs.open, fs.openSync, fs.promises.open, { suffix: '', fail: false });
154+
check(fs.open, fs.openSync, fs.promises.open, { suffix: '/', fail: true });
155+
156+
checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile,
157+
{ suffix: '', fail: false });
158+
checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile,
159+
{ suffix: '/', fail: true });

0 commit comments

Comments
 (0)