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.

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

Migrate process errors from C++ to JS #19973

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1597,6 +1597,11 @@ A string that contained unescaped characters was received.
An unhandled error occurred (for instance, when an `'error'` event is emitted
by an [`EventEmitter`][] but an `'error'` handler is not registered).

<a id="ERR_UNKNOWN_CREDENTIAL"></a>
### ERR_UNKNOWN_CREDENTIAL

A Unix group or user identifier that does not exist was passed.

<a id="ERR_UNKNOWN_ENCODING"></a>
### ERR_UNKNOWN_ENCODING

Expand Down
16 changes: 9 additions & 7 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ const internalUtil = require('internal/util');
const {
copyObject,
getOptions,
isUint32,
modeNum,
nullCheck,
preprocessSymlinkDestination,
Expand All @@ -62,16 +61,19 @@ const {
stringToSymlinkType,
toUnixTimestamp,
validateBuffer,
validateLen,
validateOffsetLengthRead,
validateOffsetLengthWrite,
validatePath,
validateUint32
validatePath
} = internalFS;
const {
CHAR_FORWARD_SLASH,
CHAR_BACKWARD_SLASH,
} = require('internal/constants');
const {
isUint32,
validateInt32,
validateUint32
} = require('internal/validators');

Object.defineProperty(exports, 'constants', {
configurable: false,
Expand Down Expand Up @@ -755,8 +757,8 @@ fs.ftruncate = function(fd, len = 0, callback) {
// TODO(BridgeAR): This does not seem right.
// There does not seem to be any validation before and if there is any, it
// should work similar to validateUint32 or not have a upper cap at all.
// This applies to all usage of `validateLen`.
validateLen(len);
// This applies to all usage of `validateInt32(len, 'len')`.
validateInt32(len, 'len');
len = Math.max(0, len);
const req = new FSReqWrap();
req.oncomplete = makeCallback(callback);
Expand All @@ -765,7 +767,7 @@ fs.ftruncate = function(fd, len = 0, callback) {

fs.ftruncateSync = function(fd, len = 0) {
validateUint32(fd, 'fd');
validateLen(len);
validateInt32(len, 'len');
len = Math.max(0, len);
const ctx = {};
binding.ftruncate(fd, len, undefined, ctx);
Expand Down
12 changes: 7 additions & 5 deletions lib/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,22 @@ const {
copyObject,
getOptions,
getStatsFromBinding,
isUint32,
modeNum,
nullCheck,
preprocessSymlinkDestination,
stringToFlags,
stringToSymlinkType,
toUnixTimestamp,
validateBuffer,
validateLen,
validateOffsetLengthRead,
validateOffsetLengthWrite,
validatePath,
validateUint32
validatePath
} = require('internal/fs');
const {
isUint32,
validateInt32,
validateUint32
} = require('internal/validators');
const pathModule = require('path');

const kHandle = Symbol('handle');
Expand Down Expand Up @@ -275,7 +277,7 @@ async function truncate(path, len = 0) {

async function ftruncate(handle, len = 0) {
validateFileHandle(handle);
validateLen(len);
validateInt32(len, 'len');
len = Math.max(0, len);
return binding.ftruncate(handle.fd, len, kUsePromises);
}
Expand Down
1 change: 1 addition & 0 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
NativeModule.require('internal/process/warning').setup();
NativeModule.require('internal/process/next_tick').setup();
NativeModule.require('internal/process/stdio').setup();
NativeModule.require('internal/process/methods').setup();

const perf = process.binding('performance');
const {
Expand Down
1 change: 1 addition & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1007,6 +1007,7 @@ E('ERR_UNHANDLED_ERROR',
if (err === undefined) return msg;
return `${msg} (${err})`;
}, Error);
E('ERR_UNKNOWN_CREDENTIAL', '%s identifier does not exist: %s', Error);
E('ERR_UNKNOWN_ENCODING', 'Unknown encoding: %s', TypeError);

// This should probably be a `TypeError`.
Expand Down
45 changes: 1 addition & 44 deletions lib/internal/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,6 @@ function getOptions(options, defaultOptions) {
return options;
}

function isInt32(n) { return n === (n | 0); }
function isUint32(n) { return n === (n >>> 0); }

function modeNum(m, def) {
if (typeof m === 'number')
return m;
Expand Down Expand Up @@ -346,26 +343,6 @@ function validateBuffer(buffer) {
}
}

function validateLen(len) {
let err;

if (!isInt32(len)) {
if (typeof len !== 'number') {
err = new ERR_INVALID_ARG_TYPE('len', 'number', len);
} else if (!Number.isInteger(len)) {
err = new ERR_OUT_OF_RANGE('len', 'an integer', len);
} else {
// 2 ** 31 === 2147483648
err = new ERR_OUT_OF_RANGE('len', '> -2147483649 && < 2147483648', len);
}
}

if (err !== undefined) {
Error.captureStackTrace(err, validateLen);
throw err;
}
}

function validateOffsetLengthRead(offset, length, bufferLength) {
let err;

Expand Down Expand Up @@ -415,28 +392,10 @@ function validatePath(path, propName = 'path') {
}
}

function validateUint32(value, propName) {
if (!isUint32(value)) {
let err;
if (typeof value !== 'number') {
err = new ERR_INVALID_ARG_TYPE(propName, 'number', value);
} else if (!Number.isInteger(value)) {
err = new ERR_OUT_OF_RANGE(propName, 'an integer', value);
} else {
// 2 ** 32 === 4294967296
err = new ERR_OUT_OF_RANGE(propName, '>= 0 && < 4294967296', value);
}
Error.captureStackTrace(err, validateUint32);
throw err;
}
}

module.exports = {
assertEncoding,
copyObject,
getOptions,
isInt32,
isUint32,
modeNum,
nullCheck,
preprocessSymlinkDestination,
Expand All @@ -449,9 +408,7 @@ module.exports = {
SyncWriteStream,
toUnixTimestamp,
validateBuffer,
validateLen,
validateOffsetLengthRead,
validateOffsetLengthWrite,
validatePath,
validateUint32
validatePath
};
137 changes: 137 additions & 0 deletions lib/internal/process/methods.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
'use strict';

const {
ERR_INVALID_ARG_TYPE,
ERR_INVALID_ARG_VALUE,
ERR_UNKNOWN_CREDENTIAL
} = require('internal/errors').codes;
const {
validateUint32
} = require('internal/validators');

function setupProcessMethods() {
// Non-POSIX platforms like Windows don't have certain methods.
if (process.setgid !== undefined) {
setupPosixMethods();
}

const {
chdir: _chdir,
umask: _umask,
} = process;

process.chdir = chdir;
process.umask = umask;

function chdir(directory) {
if (typeof directory !== 'string') {
throw new ERR_INVALID_ARG_TYPE('directory', 'string', directory);
}
return _chdir(directory);
}

const octalReg = /^[0-7]+$/;
function umask(mask) {
if (typeof mask === 'undefined') {
return _umask(mask);
}

if (typeof mask === 'number') {
validateUint32(mask, 'mask');
return _umask(mask);
}

if (typeof mask === 'string') {
if (!octalReg.test(mask)) {
throw new ERR_INVALID_ARG_VALUE('mask', mask,
'must be an octal string');
}
const octal = Number.parseInt(mask, 8);
validateUint32(octal, 'mask');
Copy link
Member

@BridgeAR BridgeAR Apr 23, 2018

Choose a reason for hiding this comment

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

Suggestion: This is actually only testing for <= 2 ** 32. All other checks are already done due to the RegExp above.
I personally would either remove the extra check or make the whole function stricter to only accept valid entries.
The reason validateUint32 is used for numbers is only about making sure no negative numbers got passed in (as far as I know).

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you know what is the largest allowed value?

Copy link
Member

@joyeecheung joyeecheung Apr 23, 2018

Choose a reason for hiding this comment

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

@targos In some fs methods we throw if the mode is larger than 0o777. It is not strictly documented wether we mask off or throw if the mode_t type of value passed to any API is larger than that though.

Copy link
Member Author

Choose a reason for hiding this comment

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

What happens in the system if the value is larger than 0o777 ?
For example if you pass 0o12345 ? is it truncated to 0o345 ?

Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: add a TODO to investigate and land the PR. In fs there are a couple of validations that need improvements, especially having more consistent checks. I am also working on some of those.

Copy link
Member

Choose a reason for hiding this comment

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

@targos Yes it gets truncated/masked. http://man7.org/linux/man-pages/man2/umask.2.html

       umask() sets the calling process's file mode creation mask (umask) to
       mask & 0777 (i.e., only the file permission bits of mask are used),
       and returns the previous value of the mask.

return _umask(octal);
}

throw new ERR_INVALID_ARG_TYPE('mask', ['number', 'string', 'undefined'],
mask);
}
}

function setupPosixMethods() {
const {
initgroups: _initgroups,
setegid: _setegid,
seteuid: _seteuid,
setgid: _setgid,
setuid: _setuid,
setgroups: _setgroups
} = process;

process.initgroups = initgroups;
process.setegid = setegid;
process.seteuid = seteuid;
process.setgid = setgid;
process.setuid = setuid;
process.setgroups = setgroups;

function initgroups(user, extraGroup) {
validateId(user, 'user');
validateId(extraGroup, 'extraGroup');
// Result is 0 on success, 1 if user is unknown, 2 if group is unknown.
const result = _initgroups(user, extraGroup);
if (result === 1) {
throw new ERR_UNKNOWN_CREDENTIAL('User', user);
} else if (result === 2) {
throw new ERR_UNKNOWN_CREDENTIAL('Group', extraGroup);
}
}

function setegid(id) {
return execId(id, 'Group', _setegid);
}

function seteuid(id) {
return execId(id, 'User', _seteuid);
}

function setgid(id) {
return execId(id, 'Group', _setgid);
}

function setuid(id) {
return execId(id, 'User', _setuid);
}

function setgroups(groups) {
if (!Array.isArray(groups)) {
throw new ERR_INVALID_ARG_TYPE('groups', 'Array', groups);
}
for (var i = 0; i < groups.length; i++) {
validateId(groups[i], `groups[${i}]`);
}
// Result is 0 on success. A positive integer indicates that the
// corresponding group was not found.
const result = _setgroups(groups);
if (result > 0) {
throw new ERR_UNKNOWN_CREDENTIAL('Group', groups[result - 1]);
}
}

function execId(id, type, method) {
validateId(id, 'id');
// Result is 0 on success, 1 if credential is unknown.
const result = method(id);
if (result === 1) {
throw new ERR_UNKNOWN_CREDENTIAL(type, id);
}
}

function validateId(id, name) {
if (typeof id === 'number') {
validateUint32(id, name);
} else if (typeof id !== 'string') {
throw new ERR_INVALID_ARG_TYPE(name, ['number', 'string'], id);
}
}
}

exports.setup = setupProcessMethods;
58 changes: 58 additions & 0 deletions lib/internal/validators.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
'use strict';

Copy link
Member

Choose a reason for hiding this comment

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

I like the consolidation of validation functions!

const {
ERR_INVALID_ARG_TYPE,
ERR_OUT_OF_RANGE
} = require('internal/errors').codes;

function isInt32(value) {
return value === (value | 0);
}

function isUint32(value) {
return value === (value >>> 0);
}

function validateInt32(value, name) {
if (!isInt32(value)) {
let err;
if (typeof value !== 'number') {
err = new ERR_INVALID_ARG_TYPE(name, 'number', value);
} else if (!Number.isInteger(value)) {
err = new ERR_OUT_OF_RANGE(name, 'an integer', value);
} else {
// 2 ** 31 === 2147483648
err = new ERR_OUT_OF_RANGE(name, '> -2147483649 && < 2147483648', value);
Copy link
Member

Choose a reason for hiding this comment

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

Just as a note: as far as I know, this function should actually check >= 0 && ... in all call cases. So we might want to rename the function / add a new one but this does not have to be in this PR.

}
Error.captureStackTrace(err, validateInt32);
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: I think it is best to actually keep the internal structure visible in most cases. So I personally would not remove this frame. The same applies to the other validation functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer not to change this in this PR.

throw err;
}
}

function validateUint32(value, name, positive) {
if (!isUint32(value)) {
let err;
if (typeof value !== 'number') {
err = new ERR_INVALID_ARG_TYPE(name, 'number', value);
} else if (!Number.isInteger(value)) {
err = new ERR_OUT_OF_RANGE(name, 'an integer', value);
} else {
const min = positive ? 1 : 0;
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: it is not much overhead but the default argument is slower than not having a default in all cases. The check itself could stay as it is, since it already tests for truthy values only.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the default value

// 2 ** 32 === 4294967296
err = new ERR_OUT_OF_RANGE(name, `>= ${min} && < 4294967296`, value);
}
Error.captureStackTrace(err, validateUint32);
throw err;
} else if (positive && value === 0) {
const err = new ERR_OUT_OF_RANGE(name, '>= 1 && < 4294967296', value);
Error.captureStackTrace(err, validateUint32);
throw err;
}
}

module.exports = {
isInt32,
isUint32,
validateInt32,
validateUint32
};
Loading