Skip to content

Commit

Permalink
fs: improve mode and flags validation
Browse files Browse the repository at this point in the history 
This fixes a few bugs in `fs`. E.g., `fs.promises.access` accepted
strings as mode. It should have only accepted numbers. It will now
always validate the flags and the mode argument in an consistent way.

PR-URL: nodejs#27044
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
  • Loading branch information
@BridgeAR
BridgeAR committed Jan 12, 2020
1 parent a45c1aa commit a13500f
Show file tree
Hide file tree
Showing 12 changed files with 246 additions and 92 deletions.
4 changes: 2 additions & 2 deletions doc/api/fs.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5493,8 +5493,8 @@ On Linux, positional writes don't work when the file is opened in append mode.
The kernel ignores the position argument and always appends the data to
the end of the file.

Modifying a file rather than replacing it may require a flags mode of `'r+'`
rather than the default mode `'w'`.
Modifying a file rather than replacing it may require the `flag` option to be
set to `'r+'` rather than the default `'w'`.

The behavior of some flags are platform-specific. As such, opening a directory
on macOS and Linux with the `'a+'` flag, as in the example below, will return an
Expand Down
82 changes: 41 additions & 41 deletions lib/fs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ const {
getDirents,
getOptions,
getValidatedPath,
getValidMode,
handleErrorFromBinding,
nullCheck,
preprocessSymlinkDestination,
Expand Down Expand Up @@ -99,7 +100,7 @@ const {
} = require('internal/constants');
const {
isUint32,
parseMode,
parseFileMode,
validateBuffer,
validateInteger,
validateInt32,
Expand Down Expand Up @@ -183,20 +184,15 @@ function access(path, mode, callback) {
}

path = getValidatedPath(path);

mode = mode | 0;
mode = getValidMode(mode, 'access');
const req = new FSReqCallback();
req.oncomplete = makeCallback(callback);
binding.access(pathModule.toNamespacedPath(path), mode, req);
}

function accessSync(path, mode) {
path = getValidatedPath(path);

if (mode === undefined)
mode = F_OK;
else
mode = mode | 0;
mode = getValidMode(mode, 'access');

const ctx = { path };
binding.access(pathModule.toNamespacedPath(path), mode, undefined, ctx);
Expand Down Expand Up @@ -310,8 +306,9 @@ function readFile(path, options, callback) {
}

path = getValidatedPath(path);
const flagsNumber = stringToFlags(options.flags);
binding.open(pathModule.toNamespacedPath(path),
stringToFlags(options.flag || 'r'),
flagsNumber,
0o666,
req);
}
Expand Down Expand Up @@ -428,11 +425,10 @@ function open(path, flags, mode, callback) {
} else if (typeof mode === 'function') {
callback = mode;
mode = 0o666;
} else {
mode = parseFileMode(mode, 'mode', 0o666);
}
const flagsNumber = stringToFlags(flags);
if (arguments.length >= 4) {
mode = parseMode(mode, 'mode', 0o666);
}
callback = makeCallback(callback);

const req = new FSReqCallback();
Expand All @@ -447,8 +443,8 @@ function open(path, flags, mode, callback) {

function openSync(path, flags, mode) {
path = getValidatedPath(path);
const flagsNumber = stringToFlags(flags || 'r');
mode = parseMode(mode, 'mode', 0o666);
const flagsNumber = stringToFlags(flags);
mode = parseFileMode(mode, 'mode', 0o666);

const ctx = { path };
const result = binding.open(pathModule.toNamespacedPath(path),
Expand Down Expand Up @@ -814,16 +810,18 @@ function fsyncSync(fd) {
}

function mkdir(path, options, callback) {
let mode = 0o777;
let recursive = false;
if (typeof options === 'function') {
callback = options;
options = {};
} else if (typeof options === 'number' || typeof options === 'string') {
options = { mode: options };
mode = options;
} else if (options) {
if (options.recursive !== undefined)
recursive = options.recursive;
if (options.mode !== undefined)
mode = options.mode;
}
const {
recursive = false,
mode = 0o777
} = options || {};
callback = makeCallback(callback);
path = getValidatedPath(path);

Expand All @@ -833,25 +831,27 @@ function mkdir(path, options, callback) {
const req = new FSReqCallback();
req.oncomplete = callback;
binding.mkdir(pathModule.toNamespacedPath(path),
parseMode(mode, 'mode', 0o777), recursive, req);
parseFileMode(mode, 'mode'), recursive, req);
}

function mkdirSync(path, options) {
let mode = 0o777;
let recursive = false;
if (typeof options === 'number' || typeof options === 'string') {
options = { mode: options };
mode = options;
} else if (options) {
if (options.recursive !== undefined)
recursive = options.recursive;
if (options.mode !== undefined)
mode = options.mode;
}
const {
recursive = false,
mode = 0o777
} = options || {};

path = getValidatedPath(path);
if (typeof recursive !== 'boolean')
throw new ERR_INVALID_ARG_TYPE('recursive', 'boolean', recursive);

const ctx = { path };
binding.mkdir(pathModule.toNamespacedPath(path),
parseMode(mode, 'mode', 0o777), recursive, undefined,
parseFileMode(mode, 'mode'), recursive, undefined,
ctx);
handleErrorFromBinding(ctx);
}
Expand Down Expand Up @@ -1070,7 +1070,7 @@ function unlinkSync(path) {

function fchmod(fd, mode, callback) {
validateInt32(fd, 'fd', 0);
mode = parseMode(mode, 'mode');
mode = parseFileMode(mode, 'mode');
callback = makeCallback(callback);

const req = new FSReqCallback();
Expand All @@ -1080,7 +1080,7 @@ function fchmod(fd, mode, callback) {

function fchmodSync(fd, mode) {
validateInt32(fd, 'fd', 0);
mode = parseMode(mode, 'mode');
mode = parseFileMode(mode, 'mode');
const ctx = {};
binding.fchmod(fd, mode, undefined, ctx);
handleErrorFromBinding(ctx);
Expand Down Expand Up @@ -1120,7 +1120,7 @@ function lchmodSync(path, mode) {

function chmod(path, mode, callback) {
path = getValidatedPath(path);
mode = parseMode(mode, 'mode');
mode = parseFileMode(mode, 'mode');
callback = makeCallback(callback);

const req = new FSReqCallback();
Expand All @@ -1130,7 +1130,7 @@ function chmod(path, mode, callback) {

function chmodSync(path, mode) {
path = getValidatedPath(path);
mode = parseMode(mode, 'mode');
mode = parseFileMode(mode, 'mode');

const ctx = { path };
binding.chmod(pathModule.toNamespacedPath(path), mode, undefined, ctx);
Expand Down Expand Up @@ -1791,10 +1791,10 @@ function mkdtempSync(prefix, options) {
}


function copyFile(src, dest, flags, callback) {
if (typeof flags === 'function') {
callback = flags;
flags = 0;
function copyFile(src, dest, mode, callback) {
if (typeof mode === 'function') {
callback = mode;
mode = 0;
} else if (typeof callback !== 'function') {
throw new ERR_INVALID_CALLBACK(callback);
}
Expand All @@ -1804,23 +1804,23 @@ function copyFile(src, dest, flags, callback) {

src = pathModule._makeLong(src);
dest = pathModule._makeLong(dest);
flags = flags | 0;
mode = getValidMode(mode, 'copyFile');
const req = new FSReqCallback();
req.oncomplete = makeCallback(callback);
binding.copyFile(src, dest, flags, req);
binding.copyFile(src, dest, mode, req);
}


function copyFileSync(src, dest, flags) {
function copyFileSync(src, dest, mode) {
src = getValidatedPath(src, 'src');
dest = getValidatedPath(dest, 'dest');

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

src = pathModule._makeLong(src);
dest = pathModule._makeLong(dest);
flags = flags | 0;
binding.copyFile(src, dest, flags, undefined, ctx);
mode = getValidMode(mode, 'copyFile');
binding.copyFile(src, dest, mode, undefined, ctx);
handleErrorFromBinding(ctx);
}

Expand Down
4 changes: 2 additions & 2 deletions lib/internal/bootstrap/switches/does_own_process_state.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ if (credentials.implementsPosixCredentials) {
// ---- compare the setups side-by-side -----

const {
parseMode,
parseFileMode,
validateString
} = require('internal/validators');

Expand Down Expand Up @@ -119,7 +119,7 @@ function wrappedChdir(directory) {

function wrappedUmask(mask) {
if (mask !== undefined) {
mask = parseMode(mask, 'mask');
mask = parseFileMode(mask, 'mask');
}
return rawMethods.umask(mask);
}
Expand Down
21 changes: 11 additions & 10 deletions lib/internal/fs/promises.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ const {
getOptions,
getStatsFromBinding,
getValidatedPath,
getValidMode,
nullCheck,
preprocessSymlinkDestination,
stringToFlags,
Expand All @@ -43,7 +44,7 @@ const {
} = require('internal/fs/utils');
const { opendir } = require('internal/fs/dir');
const {
parseMode,
parseFileMode,
validateBuffer,
validateInteger,
validateUint32
Expand Down Expand Up @@ -189,27 +190,27 @@ async function readFileHandle(filehandle, options) {
async function access(path, mode = F_OK) {
path = getValidatedPath(path);

mode = mode | 0;
mode = getValidMode(mode, 'access');
return binding.access(pathModule.toNamespacedPath(path), mode,
kUsePromises);
}

async function copyFile(src, dest, flags) {
async function copyFile(src, dest, mode) {
src = getValidatedPath(src, 'src');
dest = getValidatedPath(dest, 'dest');
flags = flags | 0;
mode = getValidMode(mode, 'copyFile');
return binding.copyFile(pathModule.toNamespacedPath(src),
pathModule.toNamespacedPath(dest),
flags, kUsePromises);
mode,
kUsePromises);
}

// 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);
if (arguments.length < 2) flags = 'r';
const flagsNumber = stringToFlags(flags);
mode = parseMode(mode, 'mode', 0o666);
mode = parseFileMode(mode, 'mode', 0o666);
return new FileHandle(
await binding.openFileHandle(pathModule.toNamespacedPath(path),
flagsNumber, mode, kUsePromises));
Expand Down Expand Up @@ -342,7 +343,7 @@ async function mkdir(path, options) {
throw new ERR_INVALID_ARG_TYPE('recursive', 'boolean', recursive);

return binding.mkdir(pathModule.toNamespacedPath(path),
parseMode(mode, 'mode', 0o777), recursive,
parseFileMode(mode, 'mode', 0o777), recursive,
kUsePromises);
}

Expand Down Expand Up @@ -410,13 +411,13 @@ async function unlink(path) {

async function fchmod(handle, mode) {
validateFileHandle(handle);
mode = parseMode(mode, 'mode');
mode = parseFileMode(mode, 'mode');
return binding.fchmod(handle.fd, mode, kUsePromises);
}

async function chmod(path, mode) {
path = getValidatedPath(path);
mode = parseMode(mode, 'mode');
mode = parseFileMode(mode, 'mode');
return binding.chmod(pathModule.toNamespacedPath(path), mode, kUsePromises);
}

Expand Down
Loading

0 comments on commit a13500f

Please sign in to comment.