child_process: Add nullptr checks after allocations #6256
Conversation
addaleax
added
the
child_process
| int32_t uid_i32 = uid_v->Int32Value(); | ||
| uv_uid_t uid = static_cast<uv_uid_t>(uid_i32); | ||
| // uv_uid_t may be unsigned, so compare with the widest available type | ||
| if (static_cast<intmax_t>(uid_i32) != static_cast<intmax_t>(uid)) { |
There was a problem hiding this comment.
I don't think this logic is entirely correct (but neither is the existing logic.) People sometimes pass e.g. { uid: -2 } but this check would reject that.
(Also, there's a narrowing conversion two lines up when sizeof(int32_t) > sizeof(uv_uid_t). That's mostly academical, though.)
There was a problem hiding this comment.
People sometimes pass e.g.
{ uid: -2 }but this check would reject that.
This check would reject that iff uv_uid_t is unsigned, which is kind of the point here… or am I missing something?
And yes, that the conversion is narrowing is intentional – the goal is to see whether the value is still the “same” after converting to uv_uid_t.
There was a problem hiding this comment.
uv_uid_t is unsigned on most platforms (all supported platforms, actually) but e.g. on OS X, the entry for user nobody looks like this:
$ grep nobody /etc/passwd
nobody:*:-2:-2:Unprivileged User:/var/empty:/usr/bin/false
Passing that works (or should work) because signed-to-unsigned conversion of -2 is UINT_MAX-1 (and has well-defined behavior; the other way around is implementation-defined behavior.)
I remember we've had to rework similar uid/gid checks because they broke working code.
There was a problem hiding this comment.
@bnoordhuis Okay, I’m removing these changes then. Thanks for the explanation!
addaleax
force-pushed
the
process-wrap-cleanup
branch
2 times, most recently
from
071f697
to
4daebc1
Compare
addaleax
changed the title
|
LGTM. Commit log nit: |
Add `CHECK_NE(·, nullptr)` after allocations made when spawning child processes.
addaleax
force-pushed
the
process-wrap-cleanup
branch
from
4daebc1
to
1cf6bf7
Compare
mscdex
added
the
c++
|
LGTM |
|
@addaleax ... this one too? ;-) |
|
Sure. 😄 Landed in 29ca969. :) |
Add `CHECK_NE(·, nullptr)` after allocations made when spawning child processes. PR-URL: #6256 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
|
Thanks! :-) |
Add `CHECK_NE(·, nullptr)` after allocations made when spawning child processes. PR-URL: #6256 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Add `CHECK_NE(·, nullptr)` after allocations made when spawning child processes. PR-URL: #6256 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Add `CHECK_NE(·, nullptr)` after allocations made when spawning child processes. PR-URL: nodejs#6256 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Add `CHECK_NE(·, nullptr)` after allocations made when spawning child processes. PR-URL: #6256 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Add `CHECK_NE(·, nullptr)` after allocations made when spawning child processes. PR-URL: #6256 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Add `CHECK_NE(·, nullptr)` after allocations made when spawning child processes. PR-URL: #6256 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Checklist
Affected core subsystem(s)
child_process
Description of change
Make sure that the out-of-range tests for setting the uid/gid work whenuid_t/gid_tare unsigned types (e.g. Linux) and the supplied value is negative, to the degree to which that is possible.