Skip to content

Commit

Permalink
fs,win: fix bug in paths with trailing slashes
Browse files Browse the repository at this point in the history
  • Loading branch information
huseyinacacak-janea committed Aug 1, 2024
1 parent 5210cd8 commit 733558f
Show file tree
Hide file tree
Showing 5 changed files with 211 additions and 20 deletions.
18 changes: 9 additions & 9 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,7 @@ function readFile(path, options, callback) {
const req = new FSReqCallback();
req.context = context;
req.oncomplete = readFileAfterOpen;
binding.open(getValidatedPath(path), flagsNumber, 0o666, req);
binding.open(getValidatedPath(path, 'path', true), flagsNumber, 0o666, req);
}

function tryStatSync(fd, isUserFd) {
Expand Down Expand Up @@ -436,7 +436,7 @@ function readFileSync(path, options) {

if (options.encoding === 'utf8' || options.encoding === 'utf-8') {
if (!isInt32(path)) {
path = getValidatedPath(path);
path = getValidatedPath(path, 'path', true);
}
return binding.readFileUtf8(path, stringToFlags(options.flag));
}
Expand Down Expand Up @@ -530,7 +530,7 @@ function closeSync(fd) {
* @returns {void}
*/
function open(path, flags, mode, callback) {
path = getValidatedPath(path);
path = getValidatedPath(path, 'path', true);
if (arguments.length < 3) {
callback = flags;
flags = 'r';
Expand Down Expand Up @@ -559,7 +559,7 @@ function open(path, flags, mode, callback) {
*/
function openSync(path, flags, mode) {
return binding.open(
getValidatedPath(path),
getValidatedPath(path, 'path', true),
stringToFlags(flags),
parseFileMode(mode, 'mode', 0o666),
);
Expand Down Expand Up @@ -2336,7 +2336,7 @@ function writeFileSync(path, data, options) {
// C++ fast path for string data and UTF8 encoding
if (typeof data === 'string' && (options.encoding === 'utf8' || options.encoding === 'utf-8')) {
if (!isInt32(path)) {
path = getValidatedPath(path);
path = getValidatedPath(path, 'path', true);
}

return binding.writeFileUtf8(
Expand Down Expand Up @@ -2981,8 +2981,8 @@ function copyFile(src, dest, mode, callback) {
mode = 0;
}

src = getValidatedPath(src, 'src');
dest = getValidatedPath(dest, 'dest');
src = getValidatedPath(src, 'src', true);
dest = getValidatedPath(dest, 'dest', true);
callback = makeCallback(callback);

const req = new FSReqCallback();
Expand All @@ -3000,8 +3000,8 @@ function copyFile(src, dest, mode, callback) {
*/
function copyFileSync(src, dest, mode) {
binding.copyFile(
getValidatedPath(src, 'src'),
getValidatedPath(dest, 'dest'),
getValidatedPath(src, 'src', true),
getValidatedPath(dest, 'dest', true),
mode,
);
}
Expand Down
6 changes: 3 additions & 3 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -620,8 +620,8 @@ async function cp(src, dest, options) {
async function copyFile(src, dest, mode) {
return await PromisePrototypeThen(
binding.copyFile(
getValidatedPath(src, 'src'),
getValidatedPath(dest, 'dest'),
getValidatedPath(src, 'src', 'path', true),
getValidatedPath(dest, 'dest', 'path', true),
mode,
kUsePromises,
),
Expand All @@ -633,7 +633,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(await PromisePrototypeThen(
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/fs/streams.js
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ function ReadStream(path, options) {
this.flags = options.flags === undefined ? 'r' : options.flags;
this.mode = options.mode === undefined ? 0o666 : options.mode;

validatePath(this.path);
validatePath(this.path, 'path', true);
} else {
this.fd = getValidatedFd(importFd(this, options));
}
Expand Down Expand Up @@ -337,7 +337,7 @@ function WriteStream(path, options) {
this.flags = options.flags === undefined ? 'w' : options.flags;
this.mode = options.mode === undefined ? 0o666 : options.mode;

validatePath(this.path);
validatePath(this.path, 'path', true);
} else {
this.fd = getValidatedFd(importFd(this, options));
}
Expand Down
29 changes: 23 additions & 6 deletions lib/internal/fs/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -709,14 +709,30 @@ 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.HideStackFramesError(propName, ['string', 'Buffer', 'URL'], path);
}

const pathIsString = typeof path === 'string';
const pathIsUint8Array = isUint8Array(path);

if (isFile) {
const lastCharacter = path[path.length - 1];
if (
lastCharacter === '/' || lastCharacter === 47 ||
(isWindows && (lastCharacter === '\\' || lastCharacter === 92))
) {
throw new ERR_FS_EISDIR({
code: 'ERR_FS_EISDIR',
message: 'is a directory',
path,
syscall: 'validatePath',
errno: ERR_FS_EISDIR,
});
}
}

// We can only perform meaningful checks on strings and Uint8Arrays.
if ((!pathIsString && !pathIsUint8Array) ||
(pathIsString && !StringPrototypeIncludes(path, '\u0000')) ||
Expand All @@ -731,11 +747,12 @@ const validatePath = hideStackFrames((path, propName = 'path') => {
);
});

const getValidatedPath = hideStackFrames((fileURLOrPath, propName = 'path') => {
const path = toPathIfFileURL(fileURLOrPath);
validatePath(path, propName);
return path;
});
const getValidatedPath =
hideStackFrames((fileURLOrPath, propName = 'path', isFile = false) => {
const path = toPathIfFileURL(fileURLOrPath);
validatePath(path, propName, isFile);
return path;
});

const getValidatedFd = hideStackFrames((fd, propName = 'fd') => {
if (ObjectIs(fd, -0)) {
Expand Down
174 changes: 174 additions & 0 deletions test/sequential/test-fs-path-dir.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
'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: 'ERR_FS_EISDIR',
}, 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: 'ERR_FS_EISDIR',
},
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: 'ERR_FS_EISDIR',
},
failMsg
).then(common.mustCall());
}
}

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 });

if (common.isWindows) {
check(fs.readFile, fs.readFileSync, fs.promises.readFile,
{ suffix: '\\', fail: true });
check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile,
{ suffix: '\\', fail: true, args: ['data'] });
check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile,
{ suffix: '\\', fail: true, args: ['data'] });
check(undefined, fs.createReadStream, undefined,
{ suffix: '\\', fail: true });
check(undefined, fs.createWriteStream, undefined,
{ suffix: '\\', fail: true });
check(fs.truncate, fs.truncateSync, fs.promises.truncate,
{ suffix: '\\', fail: true, args: [42] });
check(fs.open, fs.openSync, fs.promises.open, { suffix: '\\', fail: true });
checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile,
{ suffix: '\\', fail: true });
}

0 comments on commit 733558f

Please sign in to comment.