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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: David Carlier <[email protected]>
  • Loading branch information
bnoordhuis authored and targos committed Apr 22, 2020
1 parent d3c9a82 commit f223d2c
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 f223d2c

Please sign in to comment.