child_process: fix handling of incorrect uid/gid in spawn #22574
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
uid/gid must be uint32, which is asserted on a c++ side but wasn't checked on a JS side and therefore resulted in a process crash. Refs: nodejs#22570
lundibundi
added
the
child_process
nodejs-github-bot
added
the
child_process
addaleax
approved these changes
Aug 29, 2018
lundibundi
added
the
author ready
cjihrig
reviewed
Aug 29, 2018
lib/child_process.js
Outdated
| if (options.uid != null && !Number.isInteger(options.uid)) { | ||
| throw new ERR_INVALID_ARG_TYPE('options.uid', 'integer', options.uid); | ||
| if (options.uid != null && | ||
| (!Number.isInteger(options.uid) || !isUint32(options.uid))) { |
There was a problem hiding this comment.
Is the Number.isInteger() check still needed? Also, don't we want int32 checks to match the C++ checks?
There was a problem hiding this comment.
It will look kind of strange (especially if it's an object: {} >>> 1 and {} | 0) but I guess it is redundant.
I actually thought maybe it's worth changing them too, though I don't have enough knowledge of the topic. Linux usage and googling tell me that uid/gid must be non-negative, though maybe some of the systems we support actually support negative uid/gid. Therefore I'd also like some input on this.
In the end, I can always change those to isint32 and a topic of changing them to uint checks may be resolved in a separate PR.
There was a problem hiding this comment.
After some more googling OpenBSD seem to use -2 as internal "nobody" still.
For now, I'll change it to Int32 and we can discuss this later.
starkwang
approved these changes
Aug 29, 2018
lpinca
approved these changes
Aug 29, 2018
cjihrig
approved these changes
Aug 29, 2018
BridgeAR
approved these changes
Aug 29, 2018
trivikr
approved these changes
Aug 29, 2018
Oliyy
approved these changes
Aug 30, 2018
|
Landed in 22789fd |
addaleax
pushed a commit
that referenced
this pull request
Aug 31, 2018
uid/gid must be uint32, which is asserted on a c++ side but wasn't checked on a JS side and therefore resulted in a process crash. Refs: #22570 PR-URL: #22574 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
addaleax
pushed a commit
that referenced
this pull request
Aug 31, 2018
uid/gid must be uint32, which is asserted on a c++ side but wasn't checked on a JS side and therefore resulted in a process crash. Refs: #22570 PR-URL: #22574 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
targos
pushed a commit
that referenced
this pull request
Sep 3, 2018
uid/gid must be uint32, which is asserted on a c++ side but wasn't checked on a JS side and therefore resulted in a process crash. Refs: #22570 PR-URL: #22574 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
targos
pushed a commit
that referenced
this pull request
Sep 6, 2018
uid/gid must be uint32, which is asserted on a c++ side but wasn't checked on a JS side and therefore resulted in a process crash. Refs: #22570 PR-URL: #22574 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Weijia Wang <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
This was referenced Sep 6, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
uid/gid must be int32, which is asserted on a c++ side but wasn't
checked on a JS side and therefore resulted in a process crash.
Refs: #22570
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAlso, I'm not sure about the 'correct type' in TypeError, is 'int32' okay?