Skip to content

Migrate process errors from C++ to JS #19973

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

Closed
targos wants to merge 3 commits into from
Closed

Migrate process errors from C++ to JS #19973

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8a80bf7
lib: introduce internal/validators
targos Apr 15, 2018
0ea5929
process: migrate methods to throw errors with code
targos Apr 12, 2018
6e43560
fixup! fix Windows
targos Apr 19, 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
Prev Previous commit
fixup! fix Windows
  • Loading branch information
@targos
targos committed Apr 19, 2018
commit 6e43560b1c7cda24e7e2cd42bf94264e8c580e67
83 changes: 47 additions & 36 deletions lib/internal/process/methods.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,32 +10,68 @@ const {
} = 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
Copy Markdown
Member

@BridgeAR BridgeAR Apr 23, 2018 •
edited
Loading

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
Copy Markdown
Member Author

@targos targos Apr 23, 2018

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
Copy Markdown
Member

@joyeecheung joyeecheung Apr 23, 2018 •
edited
Loading

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
Copy Markdown
Member Author

@targos targos Apr 23, 2018

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
Copy Markdown
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: 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
Copy Markdown
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 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,
umask: _umask,
setgroups: _setgroups
} = process;

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

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

function initgroups(user, extraGroup) {
validateId(user, 'user');
Expand Down Expand Up @@ -80,31 +116,6 @@ function setupProcessMethods() {
}
}

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');
return _umask(octal);
}

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

function execId(id, type, method) {
validateId(id, 'id');
// Result is 0 on success, 1 if credential is unknown.
Expand Down