child_process: validate execFile & fork arguments #4508
Conversation
silverwind
added
the
child_process
|
CI: https://ci.nodejs.org/job/node-test-commit/1603/ If I'm not mistaken, this would be |
Trott
added
the
semver-major
|
LGTM if CI is green. Definitely |
|
@Trott: So I've never used Jenkins and I'm not familiar will all of the various builds for the Node project. Can you help me interpret the results of the build you linked to? Ideally, what are my next steps? |
|
@ChuckLangford: If there are any fails during the test run you want to look at them and check if they are related (what part of the code/tests did you change? Is the fail relevant to this?). We're not quite at a full 'green run' just yet. Walking through the errors:
|
|
@ChuckLangford Unfortunately for Node.js but fortunately for you, the failures in that CI run have nothing to do with your code. We're chipping away at the issues, but it's a long slog.... But it looks like your changes are totally fine as far as Jenkins is concerned. |
|
Thanks for the education @jbergstroem @Trott |
ChuckLangford
force-pushed
the
2681
branch
2 times, most recently
from
71ef8ca
to
1d9bf60
Compare
The argument parsing for execFile and fork are inconsistent. execFile throws on one invalid argument but not others. fork has similar logic but the implementation is very different. This update implements consistency for both functions. Fixes: nodejs#2681
Updated tests caught an unused variable. Removed it.
|
Just updated the code after rebasing my branch to the latest and running updated tests. |
|
Hi, @ChuckLangford! I imagine your rebasing is indicative of a desire to see this land rather than languish. Looks like we have just the one Anyone from CTC want to comment here or tag it |
| @@ -114,6 +109,82 @@ exports.exec = function(command /*, options, callback*/) { | |||
| opts.callback); | |||
| }; | |||
|
|
|||
| function normalizeArgsOptions(allArgs, defaultOption) { | |||
| var pos = 1, arrayPos = -1, objectPos = -1, args; | |||
There was a problem hiding this comment.
I think we're using let and const for new code these days, best switch where appropriate
|
I'm finding the normalizing logic very hard to follow, mainly because it's not clear what the variables are eventually used for. Perhaps a comment or two might help ease the denseness of this code? |
| options = util._extend({}, arguments[1]); | ||
| } | ||
|
|
||
| var opts = normalizeForkArgs(arguments, options); |
There was a problem hiding this comment.
I don't think we need a normalizeForkArgs(), as this code is specific to fork(). If the logic needs to be tightened up, it can be done here. fork() is one of the simpler cases, as we only need to validate an optional array and an optional object.
|
I'm not opposed to improving argument checking, but I think this PR in its current form is too complex for what it actually buys us. |
|
Thanks so much for the feedback @cjihrig and @rvagg. If I understand correctly, you're both finding the code hard to follow and potentially difficult to maintain. This brings up a question I had while working on this code. In general, does the Node project favor code that is verbose but easier to read/understand over code that is shorter but potentially confusing? I'm thinking you've both answered this question but just wanted to confirm. |
That is a loaded question. It depends. I think that, in general, simplicity, readability, and maintainability are valued. However, Node MUST be fast in a lot of situations, and there are plenty of places throughout core where sacrifices are made in the name of performance. The answer you get may vary depending on who answers it as well. |
This. There is no unified answer here and because we aim for general consensus amongst the collaborators who choose to weigh in on a different topic it's always going to be a back-and-forth to find a mid-point that is satisfactory. There are a lot of places in core that I could apply my comment about needing in-line comments, there are far too many obscure areas in core that are difficult to understand. But we don't do bulk-changes for the sake of the changes themselves, so it's one bit at a time. |
|
Thanks for all the feedback everyone. I'm closing this because the changes necessary to make this code more readable will look different enough to warrant a new PR. |
Validate fork/execFile arguments. Fixes: nodejs#2681 Refs: nodejs#4508 PR-URL: nodejs#7399 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Fixes: nodejs#2681 Refs: nodejs#4508 PR-URL: nodejs#7399 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
The argument parsing for execFile and fork are inconsistent. execFile
throws on one invalid argument but not others. fork has similar logic
but the implementation is very different. This update implements
consistency for both functions.
Fixes: #2681
@jasnell