Skip to content

Commit

Permalink
src: fix spawnSync CHECK when SIGKILL fails
Browse files Browse the repository at this point in the history
We might not have sufficient privileges to signal the child process
so don't make assumptions about the return value of `uv_process_kill()`.

Example:

    node -e 'child_process.spawnSync("sudo", ["ls"], { maxBuffer: 1 })'

No test because:

1. The test needs to run as root (can't invoke sudo), and

2. The parent needs to drop privileges but can't, because
   then the child process won't have sufficient privileges.

Fixes: #31747

PR-URL: #31768
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
  • Loading branch information
bnoordhuis authored and addaleax committed Mar 11, 2020
1 parent 0b49e2f commit d09e1da
Showing 1 changed file with 3 additions and 2 deletions.
5 changes: 3 additions & 2 deletions src/spawn_sync.cc
Original file line number Diff line number Diff line change
Expand Up @@ -607,8 +607,9 @@ void SyncProcessRunner::Kill() {
if (r < 0 && r != UV_ESRCH) {
SetError(r);

r = uv_process_kill(&uv_process_, SIGKILL);
CHECK(r >= 0 || r == UV_ESRCH);
// Deliberately ignore the return value, we might not have
// sufficient privileges to signal the child process.
USE(uv_process_kill(&uv_process_, SIGKILL));
}
}

Expand Down

0 comments on commit d09e1da

Please sign in to comment.