src: fix spawnSync CHECK when SIGKILL fails

bnoordhuis · targos · commit f223d2c7e4fd · 2020-04-22T17:43:30.000+02:00
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>
diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc
@@ -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));
     }
   }
 

Original file line number	Diff line number	Diff line change
`@@ -607,8 +607,9 @@ void SyncProcessRunner::Kill() {`
`607`	`607`	`if (r < 0 && r != UV_ESRCH) {`
`608`	`608`	`SetError(r);`
`609`	`609`
`610`		`- r = uv_process_kill(&uv_process_, SIGKILL);`
`611`		`- CHECK(r >= 0 \|\| r == UV_ESRCH);`
	`610`	`+ // Deliberately ignore the return value, we might not have`
	`611`	`+ // sufficient privileges to signal the child process.`
	`612`	`+ USE(uv_process_kill(&uv_process_, SIGKILL));`
`612`	`613`	`}`
`613`	`614`	`}`
`614`	`615`