Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

errors: validate input arguments #19924

Closed
wants to merge 2 commits into from
Closed

errors: validate input arguments #19924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76a8ce5
errors: validate input arguments
BridgeAR Apr 11, 2018
edf2852
fixup: add comments
BridgeAR Apr 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,7 +920,7 @@ Buffer.prototype.write = function write(string, offset, length, encoding) {
length = remaining;

if (string.length > 0 && (length < 0 || offset < 0))
throw new ERR_BUFFER_OUT_OF_BOUNDS('length', true);
throw new ERR_BUFFER_OUT_OF_BOUNDS();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain this and the change to fs? I didn't expect to see those here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The length was never used in this case. It was a weird API that I fixed.

Originally the implementation was like:

function a (a, b) {
  if (b === true) {
    return 'foo'
  }
  return `${a} foo`
}

Now it is:

function a(a = undefined) {
  if (a === undefined) {
    return 'foo'
  }
  return `${a} foo`
}

} else {
// if someone is still calling the obsolete form of write(), tell them.
// we don't want eg buf.write("foo", "utf8", 10) to silently turn into
Expand Down
4 changes: 2 additions & 2 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1019,7 +1019,7 @@ fs.fchmod = function(fd, mode, callback) {
validateUint32(mode, 'mode');
// Values for mode < 0 are already checked via the validateUint32 function
if (mode > 0o777)
throw new ERR_OUT_OF_RANGE('mode');
throw new ERR_OUT_OF_RANGE('mode', undefined, mode);

const req = new FSReqWrap();
req.oncomplete = makeCallback(callback);
Expand All @@ -1032,7 +1032,7 @@ fs.fchmodSync = function(fd, mode) {
validateUint32(mode, 'mode');
// Values for mode < 0 are already checked via the validateUint32 function
if (mode > 0o777)
throw new ERR_OUT_OF_RANGE('mode');
throw new ERR_OUT_OF_RANGE('mode', undefined, mode);
const ctx = {};
binding.fchmod(fd, mode, undefined, ctx);
handleErrorFromBinding(ctx);
Expand Down
2 changes: 1 addition & 1 deletion lib/fs/promises.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ async function fchmod(handle, mode) {
validateFileHandle(handle);
validateUint32(mode, 'mode');
if (mode > 0o777)
throw new ERR_OUT_OF_RANGE('mode');
throw new ERR_OUT_OF_RANGE('mode', undefined, mode);
return binding.fchmod(handle.fd, mode, kUsePromises);
}

Expand Down
2 changes: 1 addition & 1 deletion lib/internal/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ function boundsError(value, length, type) {
}

if (length < 0)
throw new ERR_BUFFER_OUT_OF_BOUNDS(null, true);
throw new ERR_BUFFER_OUT_OF_BOUNDS();

throw new ERR_OUT_OF_RANGE(type || 'offset',
`>= ${type ? 1 : 0} and <= ${length}`,
Expand Down
7 changes: 2 additions & 5 deletions lib/internal/crypto/pbkdf2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,8 @@ function _pbkdf2(password, salt, iterations, keylen, digest, callback) {
if (typeof keylen !== 'number')
throw new ERR_INVALID_ARG_TYPE('keylen', 'number', keylen);

if (keylen < 0 ||
!Number.isFinite(keylen) ||
keylen > INT_MAX) {
throw new ERR_OUT_OF_RANGE('keylen');
}
if (keylen < 0 || !Number.isInteger(keylen) || keylen > INT_MAX)
throw new ERR_OUT_OF_RANGE('keylen', `>= 0 && <= ${INT_MAX}`, keylen);

const encoding = getDefaultEncoding();

Expand Down
61 changes: 37 additions & 24 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,20 +416,30 @@ function internalAssert(condition, message) {
}
}

function message(key, args) {
function message(key, args = []) {
const msg = messages.get(key);
internalAssert(msg, `An invalid error message key was used: ${key}.`);
let fmt;
if (util === undefined) util = require('util');

if (typeof msg === 'function') {
fmt = msg;
} else {
fmt = util.format;
if (args === undefined || args.length === 0)
return msg;
args.unshift(msg);
internalAssert(
msg.length <= args.length, // Default options do not count.
`Code: ${key}; The provided arguments length (${args.length}) does not ` +
`match the required ones (${msg.length}).`
);
return msg.apply(null, args);
}
return String(fmt.apply(null, args));

const expectedLength = (msg.match(/%[dfijoOs]/g) || []).length;
internalAssert(
expectedLength === args.length,
`Code: ${key}; The provided arguments length (${args.length}) does not ` +
`match the required ones (${expectedLength}).`
);
if (args.length === 0)
return msg;

args.unshift(msg);
return util.format.apply(null, args);
}

/**
Expand Down Expand Up @@ -739,7 +749,7 @@ E('ERR_HTTP2_INVALID_SETTING_VALUE',
'Invalid value for setting "%s": %s', TypeError, RangeError);
E('ERR_HTTP2_INVALID_STREAM', 'The stream has been destroyed', Error);
E('ERR_HTTP2_MAX_PENDING_SETTINGS_ACK',
'Maximum number of pending settings acknowledgements (%s)', Error);
'Maximum number of pending settings acknowledgements', Error);
E('ERR_HTTP2_NO_SOCKET_MANIPULATION',
'HTTP/2 sockets should not be directly manipulated (e.g. read and written)',
Error);
Expand Down Expand Up @@ -792,7 +802,7 @@ E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => {
}, TypeError, RangeError);
E('ERR_INVALID_ARRAY_LENGTH',
(name, len, actual) => {
internalAssert(typeof actual === 'number', 'actual must be a number');
internalAssert(typeof actual === 'number', 'actual must be of type number');
return `The array "${name}" (length ${actual}) must be of length ${len}.`;
}, TypeError);
E('ERR_INVALID_ASYNC_ID', 'Invalid %s value: %s', RangeError);
Expand Down Expand Up @@ -924,7 +934,9 @@ E('ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET',
Error);
E('ERR_UNESCAPED_CHARACTERS', '%s contains unescaped characters', TypeError);
E('ERR_UNHANDLED_ERROR',
(err) => {
// Using a default argument here is important so the argument is not counted
// towards `Function#length`.
(err = undefined) => {
const msg = 'Unhandled error.';
if (err === undefined) return msg;
return `${msg} (${err})`;
Expand Down Expand Up @@ -959,7 +971,6 @@ E('ERR_VM_MODULE_STATUS', 'Module status %s', Error);
E('ERR_ZLIB_INITIALIZATION_FAILED', 'Initialization failed', Error);

function invalidArgType(name, expected, actual) {
internalAssert(arguments.length === 3, 'Exactly 3 arguments are required');
internalAssert(typeof name === 'string', 'name must be a string');

// determiner: 'must be' or 'must not be'
Expand Down Expand Up @@ -1006,8 +1017,7 @@ function missingArgs(...args) {
}

function oneOf(expected, thing) {
internalAssert(expected, 'expected is required');
internalAssert(typeof thing === 'string', 'thing is required');
internalAssert(typeof thing === 'string', '`thing` has to be of type string');
if (Array.isArray(expected)) {
const len = expected.length;
internalAssert(len > 0,
Expand All @@ -1026,25 +1036,28 @@ function oneOf(expected, thing) {
}
}

function bufferOutOfBounds(name, isWriting) {
if (isWriting) {
return 'Attempt to write outside buffer bounds';
} else {
// Using a default argument here is important so the argument is not counted
// towards `Function#length`.
function bufferOutOfBounds(name = undefined) {
if (name) {
return `"${name}" is outside of buffer bounds`;
}
return 'Attempt to write outside buffer bounds';
}

function invalidChar(name, field) {
// Using a default argument here is important so the argument is not counted
// towards `Function#length`.
function invalidChar(name, field = undefined) {
let msg = `Invalid character in ${name}`;
if (field) {
if (field !== undefined) {
msg += ` ["${field}"]`;
}
return msg;
}

function outOfRange(name, range, value) {
let msg = `The value of "${name}" is out of range.`;
if (range) msg += ` It must be ${range}.`;
if (value !== undefined) msg += ` Received ${value}`;
if (range !== undefined) msg += ` It must be ${range}.`;
msg += ` Received ${value}`;
return msg;
}
14 changes: 9 additions & 5 deletions lib/internal/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,9 +368,10 @@ function validateOffsetLengthRead(offset, length, bufferLength) {
let err;

if (offset < 0 || offset >= bufferLength) {
err = new ERR_OUT_OF_RANGE('offset');
err = new ERR_OUT_OF_RANGE('offset', `>= 0 && <= ${bufferLength}`, offset);
} else if (length < 0 || offset + length > bufferLength) {
err = new ERR_OUT_OF_RANGE('length');
err = new ERR_OUT_OF_RANGE('length',
`>= 0 && <= ${bufferLength - offset}`, length);
}

if (err !== undefined) {
Expand All @@ -383,9 +384,12 @@ function validateOffsetLengthWrite(offset, length, byteLength) {
let err;

if (offset > byteLength) {
err = new ERR_OUT_OF_RANGE('offset');
} else if (offset + length > byteLength || offset + length > kMaxLength) {
err = new ERR_OUT_OF_RANGE('length');
err = new ERR_OUT_OF_RANGE('offset', `<= ${byteLength}`, offset);
} else {
const max = byteLength > kMaxLength ? kMaxLength : byteLength;
if (length > max - offset) {
err = new ERR_OUT_OF_RANGE('length', `<= ${max - offset}`, length);
}
}

if (err !== undefined) {
Expand Down
8 changes: 5 additions & 3 deletions lib/internal/http2/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1312,8 +1312,10 @@ class ServerHttp2Session extends Http2Session {
if (origin === 'null')
throw new ERR_HTTP2_ALTSVC_INVALID_ORIGIN();
} else if (typeof originOrStream === 'number') {
if (originOrStream >>> 0 !== originOrStream || originOrStream === 0)
throw new ERR_OUT_OF_RANGE('originOrStream');
if (originOrStream >>> 0 !== originOrStream || originOrStream === 0) {
throw new ERR_OUT_OF_RANGE('originOrStream',
`> 0 && < ${2 ** 32}`, originOrStream);
}
stream = originOrStream;
} else if (originOrStream !== undefined) {
// Allow origin to be passed a URL or object with origin property
Expand Down Expand Up @@ -1766,7 +1768,7 @@ class Http2Stream extends Duplex {
if (typeof code !== 'number')
throw new ERR_INVALID_ARG_TYPE('code', 'number', code);
if (code < 0 || code > kMaxInt)
throw new ERR_OUT_OF_RANGE('code');
throw new ERR_OUT_OF_RANGE('code', `>= 0 && <= ${kMaxInt}`, code);
if (callback !== undefined && typeof callback !== 'function')
throw new ERR_INVALID_CALLBACK();

Expand Down
26 changes: 13 additions & 13 deletions test/parallel/test-buffer-arraybuffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use strict';

const common = require('../common');
require('../common');
const assert = require('assert');

const LENGTH = 16;
Expand Down Expand Up @@ -58,14 +58,14 @@ assert.throws(function() {
buf[0] = 9;
assert.strictEqual(ab[1], 9);

common.expectsError(() => Buffer.from(ab.buffer, 6), {
assert.throws(() => Buffer.from(ab.buffer, 6), {
code: 'ERR_BUFFER_OUT_OF_BOUNDS',
type: RangeError,
name: 'RangeError [ERR_BUFFER_OUT_OF_BOUNDS]',
message: '"offset" is outside of buffer bounds'
});
common.expectsError(() => Buffer.from(ab.buffer, 3, 6), {
assert.throws(() => Buffer.from(ab.buffer, 3, 6), {
code: 'ERR_BUFFER_OUT_OF_BOUNDS',
type: RangeError,
name: 'RangeError [ERR_BUFFER_OUT_OF_BOUNDS]',
message: '"length" is outside of buffer bounds'
});
}
Expand All @@ -86,14 +86,14 @@ assert.throws(function() {
buf[0] = 9;
assert.strictEqual(ab[1], 9);

common.expectsError(() => Buffer(ab.buffer, 6), {
assert.throws(() => Buffer(ab.buffer, 6), {
code: 'ERR_BUFFER_OUT_OF_BOUNDS',
type: RangeError,
name: 'RangeError [ERR_BUFFER_OUT_OF_BOUNDS]',
message: '"offset" is outside of buffer bounds'
});
common.expectsError(() => Buffer(ab.buffer, 3, 6), {
assert.throws(() => Buffer(ab.buffer, 3, 6), {
code: 'ERR_BUFFER_OUT_OF_BOUNDS',
type: RangeError,
name: 'RangeError [ERR_BUFFER_OUT_OF_BOUNDS]',
message: '"length" is outside of buffer bounds'
});
}
Expand All @@ -111,11 +111,11 @@ assert.throws(function() {
assert.deepStrictEqual(Buffer.from(ab, [1]), Buffer.from(ab, 1));

// If byteOffset is Infinity, throw.
common.expectsError(() => {
assert.throws(() => {
Buffer.from(ab, Infinity);
}, {
code: 'ERR_BUFFER_OUT_OF_BOUNDS',
type: RangeError,
name: 'RangeError [ERR_BUFFER_OUT_OF_BOUNDS]',
message: '"offset" is outside of buffer bounds'
});
}
Expand All @@ -133,11 +133,11 @@ assert.throws(function() {
assert.deepStrictEqual(Buffer.from(ab, 0, [1]), Buffer.from(ab, 0, 1));

// If length is Infinity, throw.
common.expectsError(() => {
assert.throws(() => {
Buffer.from(ab, 0, Infinity);
}, {
code: 'ERR_BUFFER_OUT_OF_BOUNDS',
type: RangeError,
name: 'RangeError [ERR_BUFFER_OUT_OF_BOUNDS]',
message: '"length" is outside of buffer bounds'
});
}
Expand Down
16 changes: 12 additions & 4 deletions test/parallel/test-child-process-fork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,26 @@ const n = fork(fixtures.path('child-process-spawn-node.js'), args);
assert.strictEqual(n.channel, n._channel);
assert.deepStrictEqual(args, ['foo', 'bar']);

n.on('message', function(m) {
n.on('message', (m) => {
console.log('PARENT got message:', m);
assert.ok(m.foo);
});

// https://github.com/joyent/node/issues/2355 - JSON.stringify(undefined)
// returns "undefined" but JSON.parse() cannot parse that...
assert.throws(function() { n.send(undefined); }, TypeError);
assert.throws(function() { n.send(); }, TypeError);
assert.throws(() => n.send(undefined), {
name: 'TypeError [ERR_MISSING_ARGS]',
message: 'The "message" argument must be specified',
code: 'ERR_MISSING_ARGS'
});
assert.throws(() => n.send(), {
name: 'TypeError [ERR_MISSING_ARGS]',
message: 'The "message" argument must be specified',
code: 'ERR_MISSING_ARGS'
});

n.send({ hello: 'world' });

n.on('exit', common.mustCall(function(c) {
n.on('exit', common.mustCall((c) => {
assert.strictEqual(c, 0);
}));
Loading