Skip to content

Commit a13500f

Browse files
BridgeARBridgeAR
committed
fs: improve mode and flags validation
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: #27044 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
1 parent a45c1aa commit a13500f

12 files changed

Lines changed: 246 additions & 92 deletions

‎doc/api/fs.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5493,8 +5493,8 @@ On Linux, positional writes don't work when the file is opened in append mode.
54935493
The kernel ignores the position argument and always appends the data to
54945494
the end of the file.
54955495

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

54995499
The behavior of some flags are platform-specific. As such, opening a directory
55005500
on macOS and Linux with the `'a+'` flag, as in the example below, will return an

‎lib/fs.js‎

Lines changed: 41 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ const {
7272
getDirents,
7373
getOptions,
7474
getValidatedPath,
75+
getValidMode,
7576
handleErrorFromBinding,
7677
nullCheck,
7778
preprocessSymlinkDestination,
@@ -99,7 +100,7 @@ const {
99100
} = require('internal/constants');
100101
const {
101102
isUint32,
102-
parseMode,
103+
parseFileMode,
103104
validateBuffer,
104105
validateInteger,
105106
validateInt32,
@@ -183,20 +184,15 @@ function access(path, mode, callback) {
183184
}
184185

185186
path = getValidatedPath(path);
186-
187-
mode = mode | 0;
187+
mode = getValidMode(mode, 'access');
188188
const req = new FSReqCallback();
189189
req.oncomplete = makeCallback(callback);
190190
binding.access(pathModule.toNamespacedPath(path), mode, req);
191191
}
192192

193193
function accessSync(path, mode) {
194194
path = getValidatedPath(path);
195-
196-
if (mode === undefined)
197-
mode = F_OK;
198-
else
199-
mode = mode | 0;
195+
mode = getValidMode(mode, 'access');
200196

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

312308
path = getValidatedPath(path);
309+
const flagsNumber = stringToFlags(options.flags);
313310
binding.open(pathModule.toNamespacedPath(path),
314-
stringToFlags(options.flag || 'r'),
311+
flagsNumber,
315312
0o666,
316313
req);
317314
}
@@ -428,11 +425,10 @@ function open(path, flags, mode, callback) {
428425
} else if (typeof mode === 'function') {
429426
callback = mode;
430427
mode = 0o666;
428+
} else {
429+
mode = parseFileMode(mode, 'mode', 0o666);
431430
}
432431
const flagsNumber = stringToFlags(flags);
433-
if (arguments.length >= 4) {
434-
mode = parseMode(mode, 'mode', 0o666);
435-
}
436432
callback = makeCallback(callback);
437433

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

448444
function openSync(path, flags, mode) {
449445
path = getValidatedPath(path);
450-
const flagsNumber = stringToFlags(flags || 'r');
451-
mode = parseMode(mode, 'mode', 0o666);
446+
const flagsNumber = stringToFlags(flags);
447+
mode = parseFileMode(mode, 'mode', 0o666);
452448

453449
const ctx = { path };
454450
const result = binding.open(pathModule.toNamespacedPath(path),
@@ -814,16 +810,18 @@ function fsyncSync(fd) {
814810
}
815811

816812
function mkdir(path, options, callback) {
813+
let mode = 0o777;
814+
let recursive = false;
817815
if (typeof options === 'function') {
818816
callback = options;
819-
options = {};
820817
} else if (typeof options === 'number' || typeof options === 'string') {
821-
options = { mode: options };
818+
mode = options;
819+
} else if (options) {
820+
if (options.recursive !== undefined)
821+
recursive = options.recursive;
822+
if (options.mode !== undefined)
823+
mode = options.mode;
822824
}
823-
const {
824-
recursive = false,
825-
mode = 0o777
826-
} = options || {};
827825
callback = makeCallback(callback);
828826
path = getValidatedPath(path);
829827

@@ -833,25 +831,27 @@ function mkdir(path, options, callback) {
833831
const req = new FSReqCallback();
834832
req.oncomplete = callback;
835833
binding.mkdir(pathModule.toNamespacedPath(path),
836-
parseMode(mode, 'mode', 0o777), recursive, req);
834+
parseFileMode(mode, 'mode'), recursive, req);
837835
}
838836

839837
function mkdirSync(path, options) {
838+
let mode = 0o777;
839+
let recursive = false;
840840
if (typeof options === 'number' || typeof options === 'string') {
841-
options = { mode: options };
841+
mode = options;
842+
} else if (options) {
843+
if (options.recursive !== undefined)
844+
recursive = options.recursive;
845+
if (options.mode !== undefined)
846+
mode = options.mode;
842847
}
843-
const {
844-
recursive = false,
845-
mode = 0o777
846-
} = options || {};
847-
848848
path = getValidatedPath(path);
849849
if (typeof recursive !== 'boolean')
850850
throw new ERR_INVALID_ARG_TYPE('recursive', 'boolean', recursive);
851851

852852
const ctx = { path };
853853
binding.mkdir(pathModule.toNamespacedPath(path),
854-
parseMode(mode, 'mode', 0o777), recursive, undefined,
854+
parseFileMode(mode, 'mode'), recursive, undefined,
855855
ctx);
856856
handleErrorFromBinding(ctx);
857857
}
@@ -1070,7 +1070,7 @@ function unlinkSync(path) {
10701070

10711071
function fchmod(fd, mode, callback) {
10721072
validateInt32(fd, 'fd', 0);
1073-
mode = parseMode(mode, 'mode');
1073+
mode = parseFileMode(mode, 'mode');
10741074
callback = makeCallback(callback);
10751075

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

10811081
function fchmodSync(fd, mode) {
10821082
validateInt32(fd, 'fd', 0);
1083-
mode = parseMode(mode, 'mode');
1083+
mode = parseFileMode(mode, 'mode');
10841084
const ctx = {};
10851085
binding.fchmod(fd, mode, undefined, ctx);
10861086
handleErrorFromBinding(ctx);
@@ -1120,7 +1120,7 @@ function lchmodSync(path, mode) {
11201120

11211121
function chmod(path, mode, callback) {
11221122
path = getValidatedPath(path);
1123-
mode = parseMode(mode, 'mode');
1123+
mode = parseFileMode(mode, 'mode');
11241124
callback = makeCallback(callback);
11251125

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

11311131
function chmodSync(path, mode) {
11321132
path = getValidatedPath(path);
1133-
mode = parseMode(mode, 'mode');
1133+
mode = parseFileMode(mode, 'mode');
11341134

11351135
const ctx = { path };
11361136
binding.chmod(pathModule.toNamespacedPath(path), mode, undefined, ctx);
@@ -1791,10 +1791,10 @@ function mkdtempSync(prefix, options) {
17911791
}
17921792

17931793

1794-
function copyFile(src, dest, flags, callback) {
1795-
if (typeof flags === 'function') {
1796-
callback = flags;
1797-
flags = 0;
1794+
function copyFile(src, dest, mode, callback) {
1795+
if (typeof mode === 'function') {
1796+
callback = mode;
1797+
mode = 0;
17981798
} else if (typeof callback !== 'function') {
17991799
throw new ERR_INVALID_CALLBACK(callback);
18001800
}
@@ -1804,23 +1804,23 @@ function copyFile(src, dest, flags, callback) {
18041804

18051805
src = pathModule._makeLong(src);
18061806
dest = pathModule._makeLong(dest);
1807-
flags = flags | 0;
1807+
mode = getValidMode(mode, 'copyFile');
18081808
const req = new FSReqCallback();
18091809
req.oncomplete = makeCallback(callback);
1810-
binding.copyFile(src, dest, flags, req);
1810+
binding.copyFile(src, dest, mode, req);
18111811
}
18121812

18131813

1814-
function copyFileSync(src, dest, flags) {
1814+
function copyFileSync(src, dest, mode) {
18151815
src = getValidatedPath(src, 'src');
18161816
dest = getValidatedPath(dest, 'dest');
18171817

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

18201820
src = pathModule._makeLong(src);
18211821
dest = pathModule._makeLong(dest);
1822-
flags = flags | 0;
1823-
binding.copyFile(src, dest, flags, undefined, ctx);
1822+
mode = getValidMode(mode, 'copyFile');
1823+
binding.copyFile(src, dest, mode, undefined, ctx);
18241824
handleErrorFromBinding(ctx);
18251825
}
18261826

‎lib/internal/bootstrap/switches/does_own_process_state.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ if (credentials.implementsPosixCredentials) {
2323
// ---- compare the setups side-by-side -----
2424

2525
const {
26-
parseMode,
26+
parseFileMode,
2727
validateString
2828
} = require('internal/validators');
2929

@@ -119,7 +119,7 @@ function wrappedChdir(directory) {
119119

120120
function wrappedUmask(mask) {
121121
if (mask !== undefined) {
122-
mask = parseMode(mask, 'mask');
122+
mask = parseFileMode(mask, 'mask');
123123
}
124124
return rawMethods.umask(mask);
125125
}

‎lib/internal/fs/promises.js‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ const {
3030
getOptions,
3131
getStatsFromBinding,
3232
getValidatedPath,
33+
getValidMode,
3334
nullCheck,
3435
preprocessSymlinkDestination,
3536
stringToFlags,
@@ -43,7 +44,7 @@ const {
4344
} = require('internal/fs/utils');
4445
const { opendir } = require('internal/fs/dir');
4546
const {
46-
parseMode,
47+
parseFileMode,
4748
validateBuffer,
4849
validateInteger,
4950
validateUint32
@@ -189,27 +190,27 @@ async function readFileHandle(filehandle, options) {
189190
async function access(path, mode = F_OK) {
190191
path = getValidatedPath(path);
191192

192-
mode = mode | 0;
193+
mode = getValidMode(mode, 'access');
193194
return binding.access(pathModule.toNamespacedPath(path), mode,
194195
kUsePromises);
195196
}
196197

197-
async function copyFile(src, dest, flags) {
198+
async function copyFile(src, dest, mode) {
198199
src = getValidatedPath(src, 'src');
199200
dest = getValidatedPath(dest, 'dest');
200-
flags = flags | 0;
201+
mode = getValidMode(mode, 'copyFile');
201202
return binding.copyFile(pathModule.toNamespacedPath(src),
202203
pathModule.toNamespacedPath(dest),
203-
flags, kUsePromises);
204+
mode,
205+
kUsePromises);
204206
}
205207

206208
// Note that unlike fs.open() which uses numeric file descriptors,
207209
// fsPromises.open() uses the fs.FileHandle class.
208210
async function open(path, flags, mode) {
209211
path = getValidatedPath(path);
210-
if (arguments.length < 2) flags = 'r';
211212
const flagsNumber = stringToFlags(flags);
212-
mode = parseMode(mode, 'mode', 0o666);
213+
mode = parseFileMode(mode, 'mode', 0o666);
213214
return new FileHandle(
214215
await binding.openFileHandle(pathModule.toNamespacedPath(path),
215216
flagsNumber, mode, kUsePromises));
@@ -342,7 +343,7 @@ async function mkdir(path, options) {
342343
throw new ERR_INVALID_ARG_TYPE('recursive', 'boolean', recursive);
343344

344345
return binding.mkdir(pathModule.toNamespacedPath(path),
345-
parseMode(mode, 'mode', 0o777), recursive,
346+
parseFileMode(mode, 'mode', 0o777), recursive,
346347
kUsePromises);
347348
}
348349

@@ -410,13 +411,13 @@ async function unlink(path) {
410411

411412
async function fchmod(handle, mode) {
412413
validateFileHandle(handle);
413-
mode = parseMode(mode, 'mode');
414+
mode = parseFileMode(mode, 'mode');
414415
return binding.fchmod(handle.fd, mode, kUsePromises);
415416
}
416417

417418
async function chmod(path, mode) {
418419
path = getValidatedPath(path);
419-
mode = parseMode(mode, 'mode');
420+
mode = parseFileMode(mode, 'mode');
420421
return binding.chmod(pathModule.toNamespacedPath(path), mode, kUsePromises);
421422
}
422423

0 commit comments

Comments
 (0)