child_process: fix handling of incorrect uid/gid in spawn - #22574
child_process: fix handling of incorrect uid/gid in spawn#22574lundibundi wants to merge 2 commits into
Conversation
Sorry, something went wrong.
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 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/717/pipeline |
Sorry, something went wrong.
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
New CI: https://ci.nodejs.org/job/node-test-pull-request/16845/ Resume: https://ci.nodejs.org/job/node-test-pull-request/16850/ One more: https://ci.nodejs.org/job/node-test-pull-request/16865/ |
Sorry, something went wrong.
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 <anna@addaleax.net> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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 <anna@addaleax.net> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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 <anna@addaleax.net> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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 <anna@addaleax.net> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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
Also, I'm not sure about the 'correct type' in TypeError, is 'int32' okay?