Skip to content

child_process: normalize fork function arguments #20882

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
shobhitchittora wants to merge 5 commits into from
Closed

child_process: normalize fork function arguments #20882

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a0d6b15
child_process: adds normalization for fork function arguments
shobhitchittora May 22, 2018
7a31575
child_process: handles options === null
shobhitchittora May 22, 2018
e1199c2
child_process: fixing indentation + refactored options check for null.
shobhitchittora May 24, 2018
37d6019
child_process: removes unrelated change ( due to lint auto fix )
shobhitchittora May 24, 2018
529480e
child_process: inlines arg normalization in the fork function itself.
shobhitchittora May 25, 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
31 changes: 19 additions & 12 deletions lib/child_process.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,22 +59,29 @@ function stdioStringToArray(option) {
}

exports.fork = function fork(modulePath /* , args, options */) {

// Get options and args arguments.
var execArgv;
var options = {};
var args = [];
var pos = 1;
if (pos < arguments.length && Array.isArray(arguments[pos])) {
args = arguments[pos++];
}

if (pos < arguments.length && arguments[pos] != null) {
if (typeof arguments[pos] !== 'object') {
throw new ERR_INVALID_ARG_VALUE(`arguments[${pos}]`, arguments[pos]);
}
var args = arguments[1];
var options = arguments[2];

@BridgeAR BridgeAR May 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Accessing arguments without checking the length can lead to OOB accesses. That is not that bad anymore but that requires quite some work for the compiler and it still slows things down. Therefore this way of doing it is not really good.

@shobhitchittora shobhitchittora May 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

would this be better -

var pos = 1;

var args = pos < arguments.length ? arguments[pos++] : [];
var options = pos < arguments.length ? arguments[pos++] : {};

@BridgeAR BridgeAR May 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Likely. Just try to make sure each code path is not hit more than absolutely necessary.


if (Array.isArray(args)) {
args = args.slice(0);

@BridgeAR BridgeAR May 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the slice needed? Please add a test case for it and remove it if there is no need for it.

@shobhitchittora shobhitchittora May 25, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. But for consistency I took inspiration from current implementation at -

node/lib/child_process.js

Line 403 in 468ab45

args = args.slice(0);

@BridgeAR BridgeAR May 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That is not a strong argument. Please just verify this by adding a test as suggested.

} else if (options !== undefined &&
(args === undefined ||
args === null ||
typeof args !== 'object')

@BridgeAR BridgeAR May 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If args is not an object and it is not undefined or null, it should actually throw an error. Just ignoring the argument does not help anyone.

@shobhitchittora shobhitchittora May 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adding a

throw new ERR_INVALID_ARG_VALUE('arg', 'object', options);

for the code you mentioned breaks this line in test -

node/test/parallel/test-child-process-spawn-typeerror.js

Line 180 in 468ab45

fork(empty, u, o);

node/test/parallel/test-child-process-spawn-typeerror.js

Line 182 in 468ab45

fork(empty, n, n);

node/test/parallel/test-child-process-spawn-typeerror.js

Line 183 in 468ab45

fork(empty, n, o);

Should we update this test and add assert.thorws for these cases.

@BridgeAR BridgeAR May 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, the test cases would continue to work but the code has to be rewritten further.

) {
args = [];
} else {
options = args;
args = [];
}

options = util._extend({}, arguments[pos++]);
if (options === undefined || options === null) {
options = {};
} else if (typeof options !== 'object') {
throw new ERR_INVALID_ARG_VALUE('options', 'object', options);
}

// Prepare arguments for fork:
Expand Down