process: better validation for process->pid usage

Previously, we might close a uv_process_t before doing waitpid on the
zombie, leaving it forever undead. Track the state of the child, so that
the user can avoid this.

docs/src/process.rst

@@ -172,7 +172,12 @@ Public members
 
 .. c:member:: int uv_process_t.pid
 
-    The PID of the spawned process. It's set after calling :c:func:`uv_spawn`.
+    The PID of the spawned process if it is alive. It is set after calling
+    :c:func:`uv_spawn` and cleared before calling :c:func:`exit_cb`. The value
+    is unique only while this is non-zero, otherwise, another process handle
+    may have already been reassigned the same number before the callback ran.
+
+    If you call :c:func:`uv_spawn`
 
 .. note::
     The :c:type:`uv_handle_t` members also apply.
@@ -267,6 +272,14 @@ API
     setgid specified, or not having enough memory to allocate for the new
     process.
 
+    Whether the call succeeds of fails, you must call :c:func:`uv_close` before
+    freeing the memory of handle (unlike other init function in libuv).
+
+    .. warning::
+        On unix, if `handle->pid != 0` when you call `uv_close`, you will create a
+        zombie that libuv cannot reap. You are responsible for calling
+        `waitpid` later. This is not relevant on Windows.
+
     .. versionchanged:: 1.24.0 Added `UV_PROCESS_WINDOWS_HIDE_CONSOLE` and
                         `UV_PROCESS_WINDOWS_HIDE_GUI` flags.
 
@@ -278,6 +291,10 @@ API
     Sends the specified signal to the given process handle. Check the documentation
     on :c:ref:`signal` for signal support, specially on Windows.
 
+    If the specified process is already dead, this will not kill a random
+    process. By contrast, `uv_kill` may kill an random process if you use a
+    cached value of :c:func:`uv_process_get_pid`.
+
 .. c:function:: int uv_kill(int pid, int signum)
 
     Sends the specified signal to the given PID. Check the documentation

src/unix/process.c

@@ -145,6 +145,7 @@ void uv__wait_children(uv_loop_t* loop) {
     }
 
     assert(pid == process->pid);
+    process->pid = 0; // pid is no longer valid (or unique)
     process->status = status;
     uv__queue_remove(&process->queue);
     uv__queue_insert_tail(&pending, &process->queue);
@@ -968,6 +969,10 @@ int uv_spawn(uv_loop_t* loop,
              const uv_process_options_t* options) {
 #if defined(__APPLE__) && (TARGET_OS_TV || TARGET_OS_WATCH)
   /* fork is marked __WATCHOS_PROHIBITED __TVOS_PROHIBITED. */
+  uv__handle_init(loop, (uv_handle_t*)process, UV_PROCESS);
+  QUEUE_INIT(&process->queue);
+  process->status = 0;
+  process->pid = 0;
   return UV_ENOSYS;
 #else
   int pipes_storage[8][2];
@@ -991,6 +996,7 @@ int uv_spawn(uv_loop_t* loop,
   uv__handle_init(loop, (uv_handle_t*)process, UV_PROCESS);
   uv__queue_init(&process->queue);
   process->status = 0;
+  process->pid = 0;
 
   stdio_count = options->stdio_count;
   if (stdio_count < 3)
@@ -1095,6 +1101,8 @@ error:
 
 
 int uv_process_kill(uv_process_t* process, int signum) {
+  if (process->pid == 0)
+    return UV_ESRCH;
   return uv_kill(process->pid, signum);
 }
 
@@ -1115,6 +1123,9 @@ int uv_kill(int pid, int signum) {
 
 
 void uv__process_close(uv_process_t* handle) {
+  /* Warning: if handle->pid != 0, the caller is creating a zombie that we
+   * cannot reap. We assume here that it is intentional, and that the user will
+   * be wise and cleanup later. */
   uv__queue_remove(&handle->queue);
   uv__handle_stop(handle);
 #ifdef UV_USE_SIGCHLD

src/win/process.c

@@ -836,10 +836,6 @@ void uv__process_proc_exit(uv_loop_t* loop, uv_process_t* handle) {
     handle->wait_handle = INVALID_HANDLE_VALUE;
   }
 
-  /* Set the handle to inactive: no callbacks will be made after the exit
-   * callback. */
-  uv__handle_stop(handle);
-
   if (GetExitCodeProcess(handle->process_handle, &status)) {
     exit_code = status;
   } else {
@@ -847,6 +843,15 @@ void uv__process_proc_exit(uv_loop_t* loop, uv_process_t* handle) {
     exit_code = uv_translate_sys_error(GetLastError());
   }
 
+  /* Clean-up the process handle eagerly. */
+  CloseHandle(handle->process_handle);
+  handle->process_handle = INVALID_HANDLE_VALUE;
+  handle->pid = 0;
+
+  /* Set the handle to inactive: no callbacks will be made after the exit
+   * callback. */
+  uv__handle_stop(handle);
+
   /* Fire the exit callback. */
   if (handle->exit_cb) {
     handle->exit_cb(handle, exit_code, handle->exit_signal);
@@ -881,7 +886,8 @@ void uv__process_endgame(uv_loop_t* loop, uv_process_t* handle) {
   assert(!(handle->flags & UV_HANDLE_CLOSED));
 
   /* Clean-up the process handle. */
-  CloseHandle(handle->process_handle);
+  if (handle->process_handle != INVALID_HANDLE_VALUE)
+    CloseHandle(handle->process_handle);
 
   uv__handle_close(handle);
 }
@@ -1365,7 +1371,7 @@ int uv_process_kill(uv_process_t* process, int signum) {
   int err;
 
   if (process->process_handle == INVALID_HANDLE_VALUE) {
-    return UV_EINVAL;
+    return UV_ESRCH;
   }
 
   err = uv__kill(process->process_handle, signum);

test/test-spawn.c

@@ -84,8 +84,6 @@ static void fail_cb(uv_process_t* process,
 static void kill_cb(uv_process_t* process,
                     int64_t exit_status,
                     int term_signal) {
-  int err;
-
   printf("exit_cb\n");
   exit_cb_called++;
 #ifdef _WIN32
@@ -106,8 +104,6 @@ static void kill_cb(uv_process_t* process,
   uv_close((uv_handle_t*) process, close_cb);
 
   /*
-   * Sending signum == 0 should check if the
-   * child process is still alive, not kill it.
    * This process should be dead.
    */
   err = uv_kill(process->pid, 0);
@@ -204,13 +200,12 @@ TEST_IMPL(spawn_fails) {
 #ifndef _WIN32
 TEST_IMPL(spawn_fails_check_for_waitpid_cleanup) {
   int r;
-  int status;
-  int err;
 
   init_process_options("", fail_cb);
   options.file = options.args[0] = "program-that-had-better-not-exist";
 
   r = uv_spawn(uv_default_loop(), &process, &options);
+  ASSERT(process.pid == 0);
   ASSERT(r == UV_ENOENT || r == UV_EACCES);
   ASSERT_OK(uv_is_active((uv_handle_t*) &process));
   ASSERT_OK(uv_run(uv_default_loop(), UV_RUN_DEFAULT));