From 758edd373d991cb6bd9a0dca6d9a986bb01a167b Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 23 Jan 2018 10:23:46 +0800 Subject: [PATCH 1/2] fs: throw errors on invalid paths synchronously - Throw getPathFromURL() and nullCheck() errors synchronously instead of deferring them to the next tick, since we already throw validatePath() errors synchronously. - Merge nullCheck() into validatePath() - Never throws in `fs.exists()`, instead, invoke the callback with false, or emit a warning when the callback is not a function. This is to bring it inline with fs.existsSync(), which never throws. - Updates the comment of rethrow() - Throw ERR_INVALID_ARG_VALUE for null checks --- lib/fs.js | 341 +++++++----------- lib/internal/url.js | 14 +- test/parallel/test-fs-copyfile.js | 20 - test/parallel/test-fs-exists.js | 25 +- test/parallel/test-fs-null-bytes.js | 28 +- test/parallel/test-fs-whatwg-url.js | 93 +++-- test/parallel/test-repl-persistent-history.js | 2 +- 7 files changed, 231 insertions(+), 292 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 04d3d0a5bd4357..0a1f979824df5c 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -108,8 +108,9 @@ function copyObject(source) { return target; } +// TODO(joyeecheung): explore how the deprecation could be solved via linting +// rules. See https://github.com/nodejs/node/pull/12976 function rethrow() { - // TODO(thefourtheye) Throw error instead of warning in major version > 7 process.emitWarning( 'Calling an asynchronous function without callback is deprecated.', 'DeprecationWarning', 'DEP0013', rethrow @@ -207,6 +208,34 @@ function validateOffsetLengthWrite(offset, length, byteLength) { } } +// Check if the path contains null types if it is a string nor Uint8Array, +// otherwise return silently. +function nullCheck(path, propName, throwError = true) { + const pathIsString = typeof path === 'string'; + const pathIsUint8Array = isUint8Array(path); + + // We can only perform meaningful checks on strings and Uint8Arrays. + if (!pathIsString && !pathIsUint8Array) { + return; + } + + if (pathIsString && path.indexOf('\u0000') === -1) { + return; + } else if (pathIsUint8Array && path.indexOf(0) === -1) { + return; + } + + const err = new errors.Error( + 'ERR_INVALID_ARG_VALUE', propName, path, + 'must be a string or Uint8Array without null bytes'); + + if (throwError) { + Error.captureStackTrace(err, nullCheck); + throw err; + } + return err; +} + function validatePath(path, propName) { let err; @@ -217,6 +246,8 @@ function validatePath(path, propName) { if (typeof path !== 'string' && !isUint8Array(path)) { err = new errors.TypeError('ERR_INVALID_ARG_TYPE', propName, ['string', 'Buffer', 'URL']); + } else { + err = nullCheck(path, propName, false); } if (err !== undefined) { @@ -255,21 +286,6 @@ function makeStatsCallback(cb) { }; } -function nullCheck(path, callback) { - if (('' + path).indexOf('\u0000') !== -1) { - const er = new errors.Error('ERR_INVALID_ARG_TYPE', - 'path', - 'string without null bytes', - path); - - if (typeof callback !== 'function') - throw er; - process.nextTick(callback, er); - return false; - } - return true; -} - function isFd(path) { return (path >>> 0) === path; } @@ -361,16 +377,6 @@ Object.defineProperties(fs, { X_OK: { enumerable: true, value: constants.X_OK || 0 }, }); -function handleError(val, callback) { - if (val instanceof Error) { - if (typeof callback === 'function') { - process.nextTick(callback, val); - return true; - } else throw val; - } - return false; -} - fs.access = function(path, mode, callback) { if (typeof mode === 'function') { callback = mode; @@ -379,14 +385,9 @@ fs.access = function(path, mode, callback) { throw new errors.TypeError('ERR_INVALID_CALLBACK'); } - if (handleError((path = getPathFromURL(path)), callback)) - return; - + path = getPathFromURL(path); validatePath(path); - if (!nullCheck(path, callback)) - return; - mode = mode | 0; var req = new FSReqWrap(); req.oncomplete = makeCallback(callback); @@ -394,9 +395,8 @@ fs.access = function(path, mode, callback) { }; fs.accessSync = function(path, mode) { - handleError((path = getPathFromURL(path))); + path = getPathFromURL(path); validatePath(path); - nullCheck(path); if (mode === undefined) mode = fs.F_OK; @@ -411,17 +411,30 @@ fs.accessSync = function(path, mode) { } }; +// fs.exists never throws even when the arguments are invalid - if there is +// a callback it would invoke it with false, otherwise it emits a warning +// (see the comments of rethrow()). +// This is to bring it inline with fs.existsSync, which never throws. +// TODO(joyeecheung): deprecate the never-throw-on-invalid-arguments behavior fs.exists = function(path, callback) { - if (handleError((path = getPathFromURL(path)), cb)) + if (typeof callback !== 'function') { + rethrow(); return; - validatePath(path); - if (!nullCheck(path, cb)) return; + } + + function suppressedCallback(err) { + callback(err ? false : true); + } + + try { + path = getPathFromURL(path); + validatePath(path); + } catch (err) { + return callback(false); + } var req = new FSReqWrap(); - req.oncomplete = cb; + req.oncomplete = suppressedCallback; binding.stat(pathModule.toNamespacedPath(path), req); - function cb(err) { - if (callback) callback(err ? false : true); - } }; Object.defineProperty(fs.exists, internalUtil.promisify.custom, { @@ -432,16 +445,16 @@ Object.defineProperty(fs.exists, internalUtil.promisify.custom, { } }); - +// fs.existsSync never throws, it only returns true or false. +// Since fs.existsSync never throws, users have established +// the expectation that passing invalid arguments to it, even like +// fs.existsSync(), would only get a false in return, so we cannot signal +// validation errors to users properly out of compatibility concerns. +// TODO(joyeecheung): deprecate the never-throw-on-invalid-arguments behavior fs.existsSync = function(path) { try { - handleError((path = getPathFromURL(path))); - try { - validatePath(path); - } catch (e) { - return false; - } - nullCheck(path); + path = getPathFromURL(path); + validatePath(path); const ctx = { path }; binding.stat(pathModule.toNamespacedPath(path), undefined, ctx); if (ctx.errno !== undefined) { @@ -456,12 +469,6 @@ fs.existsSync = function(path) { fs.readFile = function(path, options, callback) { callback = maybeCallback(callback || options); options = getOptions(options, { flag: 'r' }); - - if (handleError((path = getPathFromURL(path)), callback)) - return; - if (!nullCheck(path, callback)) - return; - var context = new ReadFileContext(callback, options.encoding); context.isUserFd = isFd(path); // file descriptor ownership var req = new FSReqWrap(); @@ -475,8 +482,8 @@ fs.readFile = function(path, options, callback) { return; } + path = getPathFromURL(path); validatePath(path); - binding.open(pathModule.toNamespacedPath(path), stringToFlags(options.flag || 'r'), 0o666, @@ -754,9 +761,7 @@ fs.open = function(path, flags, mode, callback_) { var callback = makeCallback(arguments[arguments.length - 1]); mode = modeNum(mode, 0o666); - if (handleError((path = getPathFromURL(path)), callback)) - return; - if (!nullCheck(path, callback)) return; + path = getPathFromURL(path); validatePath(path); validateUint32(mode, 'mode'); @@ -771,8 +776,7 @@ fs.open = function(path, flags, mode, callback_) { fs.openSync = function(path, flags, mode) { mode = modeNum(mode, 0o666); - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); validateUint32(mode, 'mode'); @@ -901,15 +905,9 @@ fs.writeSync = function(fd, buffer, offset, length, position) { fs.rename = function(oldPath, newPath, callback) { callback = makeCallback(callback); - if (handleError((oldPath = getPathFromURL(oldPath)), callback)) - return; - - if (handleError((newPath = getPathFromURL(newPath)), callback)) - return; - - if (!nullCheck(oldPath, callback)) return; - if (!nullCheck(newPath, callback)) return; + oldPath = getPathFromURL(oldPath); validatePath(oldPath, 'oldPath'); + newPath = getPathFromURL(newPath); validatePath(newPath, 'newPath'); const req = new FSReqWrap(); req.oncomplete = callback; @@ -919,11 +917,9 @@ fs.rename = function(oldPath, newPath, callback) { }; fs.renameSync = function(oldPath, newPath) { - handleError((oldPath = getPathFromURL(oldPath))); - handleError((newPath = getPathFromURL(newPath))); - nullCheck(oldPath); - nullCheck(newPath); + oldPath = getPathFromURL(oldPath); validatePath(oldPath, 'oldPath'); + newPath = getPathFromURL(newPath); validatePath(newPath, 'newPath'); const ctx = { path: oldPath, dest: newPath }; binding.rename(pathModule.toNamespacedPath(oldPath), @@ -1005,9 +1001,7 @@ fs.ftruncateSync = function(fd, len = 0) { fs.rmdir = function(path, callback) { callback = maybeCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; - if (!nullCheck(path, callback)) return; + path = getPathFromURL(path); validatePath(path); const req = new FSReqWrap(); req.oncomplete = callback; @@ -1015,8 +1009,7 @@ fs.rmdir = function(path, callback) { }; fs.rmdirSync = function(path) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); return binding.rmdir(pathModule.toNamespacedPath(path)); }; @@ -1056,10 +1049,7 @@ fs.fsyncSync = function(fd) { fs.mkdir = function(path, mode, callback) { if (typeof mode === 'function') callback = mode; callback = makeCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; - if (!nullCheck(path, callback)) return; - + path = getPathFromURL(path); validatePath(path); mode = modeNum(mode, 0o777); validateUint32(mode, 'mode'); @@ -1070,8 +1060,7 @@ fs.mkdir = function(path, mode, callback) { }; fs.mkdirSync = function(path, mode) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); mode = modeNum(mode, 0o777); validateUint32(mode, 'mode'); @@ -1081,10 +1070,7 @@ fs.mkdirSync = function(path, mode) { fs.readdir = function(path, options, callback) { callback = makeCallback(typeof options === 'function' ? options : callback); options = getOptions(options, {}); - if (handleError((path = getPathFromURL(path)), callback)) - return; - if (!nullCheck(path, callback)) return; - + path = getPathFromURL(path); validatePath(path); const req = new FSReqWrap(); @@ -1094,8 +1080,7 @@ fs.readdir = function(path, options, callback) { fs.readdirSync = function(path, options) { options = getOptions(options, {}); - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); return binding.readdir(pathModule.toNamespacedPath(path), options.encoding); }; @@ -1109,9 +1094,7 @@ fs.fstat = function(fd, callback) { fs.lstat = function(path, callback) { callback = makeStatsCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; - if (!nullCheck(path, callback)) return; + path = getPathFromURL(path); validatePath(path); const req = new FSReqWrap(); req.oncomplete = callback; @@ -1120,9 +1103,7 @@ fs.lstat = function(path, callback) { fs.stat = function(path, callback) { callback = makeStatsCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; - if (!nullCheck(path, callback)) return; + path = getPathFromURL(path); validatePath(path); const req = new FSReqWrap(); req.oncomplete = callback; @@ -1140,8 +1121,7 @@ fs.fstatSync = function(fd) { }; fs.lstatSync = function(path) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); const ctx = { path }; binding.lstat(pathModule.toNamespacedPath(path), undefined, ctx); @@ -1152,8 +1132,7 @@ fs.lstatSync = function(path) { }; fs.statSync = function(path) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); const ctx = { path }; binding.stat(pathModule.toNamespacedPath(path), undefined, ctx); @@ -1166,9 +1145,7 @@ fs.statSync = function(path) { fs.readlink = function(path, options, callback) { callback = makeCallback(typeof options === 'function' ? options : callback); options = getOptions(options, {}); - if (handleError((path = getPathFromURL(path)), callback)) - return; - if (!nullCheck(path, callback)) return; + path = getPathFromURL(path); validatePath(path, 'oldPath'); const req = new FSReqWrap(); req.oncomplete = callback; @@ -1177,8 +1154,7 @@ fs.readlink = function(path, options, callback) { fs.readlinkSync = function(path, options) { options = getOptions(options, {}); - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path, 'oldPath'); const ctx = { path }; const result = binding.readlink(pathModule.toNamespacedPath(path), @@ -1229,14 +1205,8 @@ fs.symlink = function(target, path, type_, callback_) { var type = (typeof type_ === 'string' ? type_ : null); var callback = makeCallback(arguments[arguments.length - 1]); - if (handleError((target = getPathFromURL(target)), callback)) - return; - - if (handleError((path = getPathFromURL(path)), callback)) - return; - - if (!nullCheck(target, callback)) return; - if (!nullCheck(path, callback)) return; + target = getPathFromURL(target); + path = getPathFromURL(path); validatePath(target, 'target'); validatePath(path); @@ -1250,11 +1220,8 @@ fs.symlink = function(target, path, type_, callback_) { fs.symlinkSync = function(target, path, type) { type = (typeof type === 'string' ? type : null); - handleError((target = getPathFromURL(target))); - handleError((path = getPathFromURL(path))); - nullCheck(target); - nullCheck(path); - + target = getPathFromURL(target); + path = getPathFromURL(path); validatePath(target, 'target'); validatePath(path); const flags = stringToSymlinkType(type); @@ -1276,15 +1243,8 @@ fs.symlinkSync = function(target, path, type) { fs.link = function(existingPath, newPath, callback) { callback = makeCallback(callback); - if (handleError((existingPath = getPathFromURL(existingPath)), callback)) - return; - - if (handleError((newPath = getPathFromURL(newPath)), callback)) - return; - - if (!nullCheck(existingPath, callback)) return; - if (!nullCheck(newPath, callback)) return; - + existingPath = getPathFromURL(existingPath); + newPath = getPathFromURL(newPath); validatePath(existingPath, 'existingPath'); validatePath(newPath, 'newPath'); @@ -1297,10 +1257,8 @@ fs.link = function(existingPath, newPath, callback) { }; fs.linkSync = function(existingPath, newPath) { - handleError((existingPath = getPathFromURL(existingPath))); - handleError((newPath = getPathFromURL(newPath))); - nullCheck(existingPath); - nullCheck(newPath); + existingPath = getPathFromURL(existingPath); + newPath = getPathFromURL(newPath); validatePath(existingPath, 'existingPath'); validatePath(newPath, 'newPath'); @@ -1316,9 +1274,7 @@ fs.linkSync = function(existingPath, newPath) { fs.unlink = function(path, callback) { callback = makeCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; - if (!nullCheck(path, callback)) return; + path = getPathFromURL(path); validatePath(path); const req = new FSReqWrap(); req.oncomplete = callback; @@ -1326,8 +1282,7 @@ fs.unlink = function(path, callback) { }; fs.unlinkSync = function(path) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); const ctx = { path }; binding.unlink(pathModule.toNamespacedPath(path), undefined, ctx); @@ -1394,10 +1349,7 @@ if (constants.O_SYMLINK !== undefined) { fs.chmod = function(path, mode, callback) { callback = makeCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; - if (!nullCheck(path, callback)) return; - + path = getPathFromURL(path); validatePath(path); mode = modeNum(mode); validateUint32(mode, 'mode'); @@ -1408,8 +1360,7 @@ fs.chmod = function(path, mode, callback) { }; fs.chmodSync = function(path, mode) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); mode = modeNum(mode); validateUint32(mode, 'mode'); @@ -1466,10 +1417,7 @@ fs.fchownSync = function(fd, uid, gid) { fs.chown = function(path, uid, gid, callback) { callback = makeCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; - if (!nullCheck(path, callback)) return; - + path = getPathFromURL(path); validatePath(path); validateUint32(uid, 'uid'); validateUint32(gid, 'gid'); @@ -1480,8 +1428,7 @@ fs.chown = function(path, uid, gid, callback) { }; fs.chownSync = function(path, uid, gid) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); validateUint32(uid, 'uid'); validateUint32(gid, 'gid'); @@ -1515,10 +1462,7 @@ fs._toUnixTimestamp = toUnixTimestamp; fs.utimes = function(path, atime, mtime, callback) { callback = makeCallback(callback); - if (handleError((path = getPathFromURL(path)), callback)) - return; - if (!nullCheck(path, callback)) return; - + path = getPathFromURL(path); validatePath(path); const req = new FSReqWrap(); @@ -1530,8 +1474,7 @@ fs.utimes = function(path, atime, mtime, callback) { }; fs.utimesSync = function(path, atime, mtime) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); binding.utimes(pathModule.toNamespacedPath(path), toUnixTimestamp(atime), @@ -1685,12 +1628,17 @@ function FSWatcher() { } util.inherits(FSWatcher, EventEmitter); +// FIXME(joyeecheung): this method is not documented. +// At the moment if filename is undefined, we +// 1. Throw an Error from C++ land if it's the first time .start() is called +// 2. Return silently from C++ land if .start() has already been called +// on a valid filename and the wrap has been initialized FSWatcher.prototype.start = function(filename, persistent, recursive, encoding) { - handleError((filename = getPathFromURL(filename))); - nullCheck(filename); + filename = getPathFromURL(filename); + nullCheck(filename, 'filename'); var err = this._handle.start(pathModule.toNamespacedPath(filename), persistent, recursive, @@ -1708,9 +1656,6 @@ FSWatcher.prototype.close = function() { }; fs.watch = function(filename, options, listener) { - handleError((filename = getPathFromURL(filename))); - nullCheck(filename); - if (typeof options === 'function') { listener = options; } @@ -1777,9 +1722,14 @@ function StatWatcher() { util.inherits(StatWatcher, EventEmitter); +// FIXME(joyeecheung): this method is not documented. +// At the moment if filename is undefined, we +// 1. Throw an Error from C++ land if it's the first time .start() is called +// 2. Return silently from C++ land if .start() has already been called +// on a valid filename and the wrap has been initialized StatWatcher.prototype.start = function(filename, persistent, interval) { - handleError((filename = getPathFromURL(filename))); - nullCheck(filename); + filename = getPathFromURL(filename); + nullCheck(filename, 'filename'); this._handle.start(pathModule.toNamespacedPath(filename), persistent, interval); }; @@ -1793,8 +1743,8 @@ StatWatcher.prototype.stop = function() { const statWatchers = new Map(); fs.watchFile = function(filename, options, listener) { - handleError((filename = getPathFromURL(filename))); - nullCheck(filename); + filename = getPathFromURL(filename); + validatePath(filename); filename = pathModule.resolve(filename); var stat; @@ -1833,8 +1783,8 @@ fs.watchFile = function(filename, options, listener) { }; fs.unwatchFile = function(filename, listener) { - handleError((filename = getPathFromURL(filename))); - nullCheck(filename); + filename = getPathFromURL(filename); + validatePath(filename); filename = pathModule.resolve(filename); var stat = statWatchers.get(filename); @@ -1903,12 +1853,11 @@ fs.realpathSync = function realpathSync(p, options) { options = emptyObj; else options = getOptions(options, emptyObj); + p = getPathFromURL(p); if (typeof p !== 'string') { - handleError((p = getPathFromURL(p))); - if (typeof p !== 'string') - p += ''; + p += ''; } - nullCheck(p); + validatePath(p); p = pathModule.resolve(p); const cache = options[internalFS.realpathCacheKey]; @@ -2046,8 +1995,8 @@ fs.realpathSync = function realpathSync(p, options) { fs.realpathSync.native = function(path, options) { options = getOptions(options, {}); - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); + validatePath(path); return binding.realpath(path, options.encoding); }; @@ -2058,14 +2007,11 @@ fs.realpath = function realpath(p, options, callback) { options = emptyObj; else options = getOptions(options, emptyObj); + p = getPathFromURL(p); if (typeof p !== 'string') { - if (handleError((p = getPathFromURL(p)), callback)) - return; - if (typeof p !== 'string') - p += ''; + p += ''; } - if (!nullCheck(p, callback)) - return; + validatePath(p); p = pathModule.resolve(p); const seenLinks = Object.create(null); @@ -2192,14 +2138,13 @@ fs.realpath = function realpath(p, options, callback) { fs.realpath.native = function(path, options, callback) { callback = maybeCallback(callback || options); options = getOptions(options, {}); - if (handleError((path = getPathFromURL(path)), callback)) return; - if (!nullCheck(path, callback)) return; + path = getPathFromURL(path); + validatePath(path); const req = new FSReqWrap(); req.oncomplete = callback; return binding.realpath(path, options.encoding, req); }; - fs.mkdtemp = function(prefix, options, callback) { callback = makeCallback(typeof options === 'function' ? options : callback); options = getOptions(options, {}); @@ -2209,26 +2154,22 @@ fs.mkdtemp = function(prefix, options, callback) { 'string', prefix); } - if (!nullCheck(prefix, callback)) { - return; - } - + nullCheck(prefix, 'prefix'); var req = new FSReqWrap(); req.oncomplete = callback; - binding.mkdtemp(`${prefix}XXXXXX`, options.encoding, req); }; fs.mkdtempSync = function(prefix, options) { + options = getOptions(options, {}); if (!prefix || typeof prefix !== 'string') { throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'prefix', 'string', prefix); } - options = getOptions(options, {}); - nullCheck(prefix); + nullCheck(prefix, 'prefix'); return binding.mkdtemp(`${prefix}XXXXXX`, options.encoding); }; @@ -2248,21 +2189,7 @@ fs.copyFile = function(src, dest, flags, callback) { } src = getPathFromURL(src); - - if (handleError(src, callback)) - return; - - if (!nullCheck(src, callback)) - return; - dest = getPathFromURL(dest); - - if (handleError(dest, callback)) - return; - - if (!nullCheck(dest, callback)) - return; - validatePath(src, 'src'); validatePath(dest, 'dest'); @@ -2277,13 +2204,7 @@ fs.copyFile = function(src, dest, flags, callback) { fs.copyFileSync = function(src, dest, flags) { src = getPathFromURL(src); - handleError(src); - nullCheck(src); - dest = getPathFromURL(dest); - handleError(dest); - nullCheck(dest); - validatePath(src, 'src'); validatePath(dest, 'dest'); @@ -2320,7 +2241,8 @@ function ReadStream(path, options) { Readable.call(this, options); - handleError((this.path = getPathFromURL(path))); + // path will be ignored when fd is specified, so it can be falsy + this.path = getPathFromURL(path); this.fd = options.fd === undefined ? null : options.fd; this.flags = options.flags === undefined ? 'r' : options.flags; this.mode = options.mode === undefined ? 0o666 : options.mode; @@ -2483,7 +2405,8 @@ function WriteStream(path, options) { Writable.call(this, options); - handleError((this.path = getPathFromURL(path))); + // path will be ignored when fd is specified, so it can be falsy + this.path = getPathFromURL(path); this.fd = options.fd === undefined ? null : options.fd; this.flags = options.flags === undefined ? 'w' : options.flags; this.mode = options.mode === undefined ? 0o666 : options.mode; diff --git a/lib/internal/url.js b/lib/internal/url.js index b395e77b046f10..7055667718beed 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -1321,7 +1321,7 @@ function getPathFromURLWin32(url) { var third = pathname.codePointAt(n + 2) | 0x20; if ((pathname[n + 1] === '2' && third === 102) || // 2f 2F / (pathname[n + 1] === '5' && third === 99)) { // 5c 5C \ - return new errors.TypeError( + throw new errors.TypeError( 'ERR_INVALID_FILE_URL_PATH', 'must not include encoded \\ or / characters'); } @@ -1342,8 +1342,8 @@ function getPathFromURLWin32(url) { var sep = pathname[2]; if (letter < 97 || letter > 122 || // a..z A..Z (sep !== ':')) { - return new errors.TypeError('ERR_INVALID_FILE_URL_PATH', - 'must be absolute'); + throw new errors.TypeError('ERR_INVALID_FILE_URL_PATH', + 'must be absolute'); } return pathname.slice(1); } @@ -1351,15 +1351,15 @@ function getPathFromURLWin32(url) { function getPathFromURLPosix(url) { if (url.hostname !== '') { - return new errors.TypeError('ERR_INVALID_FILE_URL_HOST', platform); + throw new errors.TypeError('ERR_INVALID_FILE_URL_HOST', platform); } var pathname = url.pathname; for (var n = 0; n < pathname.length; n++) { if (pathname[n] === '%') { var third = pathname.codePointAt(n + 2) | 0x20; if (pathname[n + 1] === '2' && third === 102) { - return new errors.TypeError('ERR_INVALID_FILE_URL_PATH', - 'must not include encoded / characters'); + throw new errors.TypeError('ERR_INVALID_FILE_URL_PATH', + 'must not include encoded / characters'); } } } @@ -1372,7 +1372,7 @@ function getPathFromURL(path) { return path; } if (path.protocol !== 'file:') - return new errors.TypeError('ERR_INVALID_URL_SCHEME', 'file'); + throw new errors.TypeError('ERR_INVALID_URL_SCHEME', 'file'); return isWindows ? getPathFromURLWin32(path) : getPathFromURLPosix(path); } diff --git a/test/parallel/test-fs-copyfile.js b/test/parallel/test-fs-copyfile.js index a335065bf43cb7..8b910ba046ec48 100644 --- a/test/parallel/test-fs-copyfile.js +++ b/test/parallel/test-fs-copyfile.js @@ -102,26 +102,6 @@ common.expectsError(() => { ); }); -// Throws if the source path is an invalid path. -common.expectsError(() => { - fs.copyFileSync('\u0000', dest); -}, { - code: 'ERR_INVALID_ARG_TYPE', - type: Error, - message: 'The "path" argument must be of type string without null bytes.' + - ' Received type string' -}); - -// Throws if the destination path is an invalid path. -common.expectsError(() => { - fs.copyFileSync(src, '\u0000'); -}, { - code: 'ERR_INVALID_ARG_TYPE', - type: Error, - message: 'The "path" argument must be of type string without null bytes.' + - ' Received type string' -}); - // Errors if invalid flags are provided. assert.throws(() => { fs.copyFileSync(src, dest, -1); diff --git a/test/parallel/test-fs-exists.js b/test/parallel/test-fs-exists.js index a526373c6a876d..c52679764b4558 100644 --- a/test/parallel/test-fs-exists.js +++ b/test/parallel/test-fs-exists.js @@ -26,30 +26,33 @@ const fs = require('fs'); const { URL } = require('url'); const f = __filename; +// Only warnings are emitted when the callback is invalid +assert.doesNotThrow(() => fs.exists(f)); +assert.doesNotThrow(() => fs.exists()); +assert.doesNotThrow(() => fs.exists(f, {})); + fs.exists(f, common.mustCall(function(y) { assert.strictEqual(y, true); })); -assert.doesNotThrow(() => fs.exists(f)); - fs.exists(`${f}-NO`, common.mustCall(function(y) { assert.strictEqual(y, false); })); +// If the path is invalid, fs.exists will still invoke the callback with false +// instead of throwing errors fs.exists(new URL('https://foo'), common.mustCall(function(y) { assert.strictEqual(y, false); })); +fs.exists({}, common.mustCall(function(y) { + assert.strictEqual(y, false); +})); + assert(fs.existsSync(f)); assert(!fs.existsSync(`${f}-NO`)); -common.expectsError( - () => { fs.exists(() => {}); }, - { - code: 'ERR_INVALID_ARG_TYPE', - message: 'The "path" argument must be one of type string, Buffer, or URL', - type: TypeError - } -); - +// fs.existsSync() never throws assert(!fs.existsSync()); +assert(!fs.existsSync({})); +assert(!fs.existsSync(new URL('https://foo'))); diff --git a/test/parallel/test-fs-null-bytes.js b/test/parallel/test-fs-null-bytes.js index 44defc782ee139..6ffaef571ac8d6 100644 --- a/test/parallel/test-fs-null-bytes.js +++ b/test/parallel/test-fs-null-bytes.js @@ -27,16 +27,7 @@ const URL = require('url').URL; function check(async, sync) { const argsSync = Array.prototype.slice.call(arguments, 2); - const argsAsync = argsSync.concat((er) => { - common.expectsError( - () => { - throw er; - }, - { - code: 'ERR_INVALID_ARG_TYPE', - type: Error - }); - }); + const argsAsync = argsSync.concat(common.mustNotCall()); if (sync) { common.expectsError( @@ -44,13 +35,20 @@ function check(async, sync) { sync.apply(null, argsSync); }, { - code: 'ERR_INVALID_ARG_TYPE', + code: 'ERR_INVALID_ARG_VALUE', type: Error, }); } if (async) { - async.apply(null, argsAsync); + common.expectsError( + () => { + async.apply(null, argsAsync); + }, + { + code: 'ERR_INVALID_ARG_VALUE', + type: Error + }); } } @@ -59,6 +57,8 @@ check(fs.access, fs.accessSync, 'foo\u0000bar', fs.F_OK); check(fs.appendFile, fs.appendFileSync, 'foo\u0000bar', 'abc'); check(fs.chmod, fs.chmodSync, 'foo\u0000bar', '0644'); check(fs.chown, fs.chownSync, 'foo\u0000bar', 12, 34); +check(fs.copyFile, fs.copyFileSync, 'foo\u0000bar', 'abc'); +check(fs.copyFile, fs.copyFileSync, 'abc', 'foo\u0000bar'); check(fs.link, fs.linkSync, 'foo\u0000bar', 'foobar'); check(fs.link, fs.linkSync, 'foobar', 'foo\u0000bar'); check(fs.lstat, fs.lstatSync, 'foo\u0000bar'); @@ -90,6 +90,8 @@ check(fs.access, fs.accessSync, fileUrl, fs.F_OK); check(fs.appendFile, fs.appendFileSync, fileUrl, 'abc'); check(fs.chmod, fs.chmodSync, fileUrl, '0644'); check(fs.chown, fs.chownSync, fileUrl, 12, 34); +check(fs.copyFile, fs.copyFileSync, fileUrl, 'abc'); +check(fs.copyFile, fs.copyFileSync, 'abc', fileUrl); check(fs.link, fs.linkSync, fileUrl, 'foobar'); check(fs.link, fs.linkSync, 'foobar', fileUrl); check(fs.lstat, fs.lstatSync, fileUrl); @@ -118,6 +120,8 @@ check(fs.access, fs.accessSync, fileUrl2, fs.F_OK); check(fs.appendFile, fs.appendFileSync, fileUrl2, 'abc'); check(fs.chmod, fs.chmodSync, fileUrl2, '0644'); check(fs.chown, fs.chownSync, fileUrl2, 12, 34); +check(fs.copyFile, fs.copyFileSync, fileUrl2, 'abc'); +check(fs.copyFile, fs.copyFileSync, 'abc', fileUrl2); check(fs.link, fs.linkSync, fileUrl2, 'foobar'); check(fs.link, fs.linkSync, 'foobar', fileUrl2); check(fs.lstat, fs.lstatSync, fileUrl2); diff --git a/test/parallel/test-fs-whatwg-url.js b/test/parallel/test-fs-whatwg-url.js index 21eb82700aeb13..cfdfbe0dde01b8 100644 --- a/test/parallel/test-fs-whatwg-url.js +++ b/test/parallel/test-fs-whatwg-url.js @@ -29,46 +29,75 @@ fs.readFile(url, common.mustCall((err, data) => { // Check that using a non file:// URL reports an error const httpUrl = new URL('http://example.org'); -fs.readFile(httpUrl, common.expectsError({ - code: 'ERR_INVALID_URL_SCHEME', - type: TypeError, - message: 'The URL must be of scheme file' -})); -// pct-encoded characters in the path will be decoded and checked -fs.readFile(new URL('file:///c:/tmp/%00test'), common.mustCall((err) => { - common.expectsError( - () => { - throw err; - }, - { - code: 'ERR_INVALID_ARG_TYPE', - type: Error - }); -})); +common.expectsError( + () => { + fs.readFile(httpUrl, common.mustNotCall()); + }, + { + code: 'ERR_INVALID_URL_SCHEME', + type: TypeError, + message: 'The URL must be of scheme file' + }); +// pct-encoded characters in the path will be decoded and checked if (common.isWindows) { // encoded back and forward slashes are not permitted on windows ['%2f', '%2F', '%5c', '%5C'].forEach((i) => { - fs.readFile(new URL(`file:///c:/tmp/${i}`), common.expectsError({ - code: 'ERR_INVALID_FILE_URL_PATH', - type: TypeError, - message: 'File URL path must not include encoded \\ or / characters' - })); + common.expectsError( + () => { + fs.readFile(new URL(`file:///c:/tmp/${i}`), common.mustNotCall()); + }, + { + code: 'ERR_INVALID_FILE_URL_PATH', + type: TypeError, + message: 'File URL path must not include encoded \\ or / characters' + } + ); }); + common.expectsError( + () => { + fs.readFile(new URL('file:///c:/tmp/%00test'), common.mustNotCall()); + }, + { + code: 'ERR_INVALID_ARG_VALUE', + type: Error, + message: 'The argument \'path\' must be a string or Uint8Array without ' + + 'null bytes. Received \'c:/tmp/\\u0000test\'' + } + ); } else { // encoded forward slashes are not permitted on other platforms ['%2f', '%2F'].forEach((i) => { - fs.readFile(new URL(`file:///c:/tmp/${i}`), common.expectsError({ - code: 'ERR_INVALID_FILE_URL_PATH', - type: TypeError, - message: 'File URL path must not include encoded / characters' - })); + common.expectsError( + () => { + fs.readFile(new URL(`file:///c:/tmp/${i}`), common.mustNotCall()); + }, + { + code: 'ERR_INVALID_FILE_URL_PATH', + type: TypeError, + message: 'File URL path must not include encoded / characters' + }); }); - - fs.readFile(new URL('file://hostname/a/b/c'), common.expectsError({ - code: 'ERR_INVALID_FILE_URL_HOST', - type: TypeError, - message: `File URL host must be "localhost" or empty on ${os.platform()}` - })); + common.expectsError( + () => { + fs.readFile(new URL('file://hostname/a/b/c'), common.mustNotCall()); + }, + { + code: 'ERR_INVALID_FILE_URL_HOST', + type: TypeError, + message: `File URL host must be "localhost" or empty on ${os.platform()}` + } + ); + common.expectsError( + () => { + fs.readFile(new URL('file:///tmp/%00test'), common.mustNotCall()); + }, + { + code: 'ERR_INVALID_ARG_VALUE', + type: Error, + message: 'The argument \'path\' must be a string or Uint8Array without ' + + 'null bytes. Received \'/tmp/\\u0000test\'' + } + ); } diff --git a/test/parallel/test-repl-persistent-history.js b/test/parallel/test-repl-persistent-history.js index 396203d949f4ba..43c868f8326f3f 100644 --- a/test/parallel/test-repl-persistent-history.js +++ b/test/parallel/test-repl-persistent-history.js @@ -57,7 +57,7 @@ const CLEAR = { ctrl: true, name: 'u' }; // File paths const historyFixturePath = fixtures.path('.node_repl_history'); const historyPath = path.join(tmpdir.path, '.fixture_copy_repl_history'); -const historyPathFail = path.join(tmpdir.path, '.node_repl\u0000_history'); +const historyPathFail = fixtures.path('nonexistent_folder', 'filename'); const oldHistoryPathObj = fixtures.path('old-repl-history-file-obj.json'); const oldHistoryPathFaulty = fixtures.path('old-repl-history-file-faulty.json'); const oldHistoryPath = fixtures.path('old-repl-history-file.json'); From 76bad33b43b6a5dfebc7c7eaf119706f85ed91f3 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 7 Feb 2018 19:06:44 +0800 Subject: [PATCH 2/2] squash! update error handling in promise APIs --- lib/fs.js | 63 +++++++++++++++++++------------------------------------ 1 file changed, 21 insertions(+), 42 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 0a1f979824df5c..56acc657912bbe 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -2706,8 +2706,7 @@ async function readFileHandle(filehandle, options) { // thrown synchronously const promises = { async access(path, mode = fs.F_OK) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); mode = mode | 0; @@ -2716,10 +2715,8 @@ const promises = { }, async copyFile(src, dest, flags) { - handleError((src = getPathFromURL(src))); - handleError((dest = getPathFromURL(dest))); - nullCheck(src); - nullCheck(dest); + src = getPathFromURL(src); + dest = getPathFromURL(dest); validatePath(src, 'src'); validatePath(dest, 'dest'); flags = flags | 0; @@ -2732,8 +2729,7 @@ const promises = { // promises.open() uses the fs.FileHandle class. async open(path, flags, mode) { mode = modeNum(mode, 0o666); - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); validateUint32(mode, 'mode'); return new FileHandle( @@ -2800,10 +2796,8 @@ const promises = { }, async rename(oldPath, newPath) { - handleError((oldPath = getPathFromURL(oldPath))); - handleError((newPath = getPathFromURL(newPath))); - nullCheck(oldPath); - nullCheck(newPath); + oldPath = getPathFromURL(oldPath); + newPath = getPathFromURL(newPath); validatePath(oldPath, 'oldPath'); validatePath(newPath, 'newPath'); return binding.rename(pathModule.toNamespacedPath(oldPath), @@ -2823,8 +2817,7 @@ const promises = { }, async rmdir(path) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); return binding.rmdir(pathModule.toNamespacedPath(path), kUsePromises); }, @@ -2841,8 +2834,7 @@ const promises = { async mkdir(path, mode) { mode = modeNum(mode, 0o777); - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); validateUint32(mode, 'mode'); return binding.mkdir(pathModule.toNamespacedPath(path), mode, kUsePromises); @@ -2850,8 +2842,7 @@ const promises = { async readdir(path, options) { options = getOptions(options, {}); - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); return binding.readdir(pathModule.toNamespacedPath(path), options.encoding, kUsePromises); @@ -2859,8 +2850,7 @@ const promises = { async readlink(path, options) { options = getOptions(options, {}); - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path, 'oldPath'); return binding.readlink(pathModule.toNamespacedPath(path), options.encoding, kUsePromises); @@ -2868,10 +2858,8 @@ const promises = { async symlink(target, path, type_) { const type = (typeof type_ === 'string' ? type_ : null); - handleError((target = getPathFromURL(target))); - handleError((path = getPathFromURL(path))); - nullCheck(target); - nullCheck(path); + target = getPathFromURL(target); + path = getPathFromURL(path); validatePath(target, 'target'); validatePath(path); return binding.symlink(preprocessSymlinkDestination(target, type, path), @@ -2886,26 +2874,22 @@ const promises = { }, async lstat(path) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); return statsFromValues( await binding.lstat(pathModule.toNamespacedPath(path), kUsePromises)); }, async stat(path) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); return statsFromValues( await binding.stat(pathModule.toNamespacedPath(path), kUsePromises)); }, async link(existingPath, newPath) { - handleError((existingPath = getPathFromURL(existingPath))); - handleError((newPath = getPathFromURL(newPath))); - nullCheck(existingPath); - nullCheck(newPath); + existingPath = getPathFromURL(existingPath); + newPath = getPathFromURL(newPath); validatePath(existingPath, 'existingPath'); validatePath(newPath, 'newPath'); return binding.link(pathModule.toNamespacedPath(existingPath), @@ -2914,8 +2898,7 @@ const promises = { }, async unlink(path) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); return binding.unlink(pathModule.toNamespacedPath(path), kUsePromises); }, @@ -2930,8 +2913,7 @@ const promises = { }, async chmod(path, mode) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); mode = modeNum(mode); validateUint32(mode, 'mode'); @@ -2964,8 +2946,7 @@ const promises = { }, async chown(path, uid, gid) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); validateUint32(uid, 'uid'); validateUint32(gid, 'gid'); @@ -2974,8 +2955,7 @@ const promises = { }, async utimes(path, atime, mtime) { - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); return binding.utimes(pathModule.toNamespacedPath(path), toUnixTimestamp(atime), @@ -2992,8 +2972,7 @@ const promises = { async realpath(path, options) { options = getOptions(options, {}); - handleError((path = getPathFromURL(path))); - nullCheck(path); + path = getPathFromURL(path); validatePath(path); return binding.realpath(path, options.encoding, kUsePromises); },