process: better validation for process->pid usage (#3539)

Previously, the user might unknowingly close a uv_process_t before
doing waitpid on the zombie, leaving it forever undead. Track the state
of the child, so that the application wrapper can avoid this by calling
uv_process_kill and checking for UV_ESRCH error.

docs/src/process.rst

@@ -172,7 +172,10 @@ 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. It is set after calling :c:func:`uv_spawn`
+    and retains the value even after the process exits. The value is only
+    unique while the process is alive; after exit, another process may be
+    reassigned the same PID.
 
 .. note::
     The :c:type:`uv_handle_t` members also apply.
@@ -259,14 +262,20 @@ API
     Initializes the process handle and starts the process. If the process is
     successfully spawned, this function will return 0. Otherwise, the
     negative error code corresponding to the reason it couldn't spawn is
-    returned. Note that either way you must eventually call :c:func:`uv_close`
-    to close the handle again.
+    returned. Note that either way-success or failure--you must eventually call
+    :c:func:`uv_close` to close the handle again before freeing the memory of
+    the handle, unlike other the other init functions in libuv.
 
     Possible reasons for failing to spawn would include (but not be limited to)
     the file to execute not existing, not having permissions to use the setuid or
     setgid specified, or not having enough memory to allocate for the new
     process.
 
+    .. warning::
+        On unix, if the process has not yet exited 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 +287,11 @@ 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 different
+    process which happened to reuse the same pid. By contrast, `uv_kill` may
+    kill an arbitrary other 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->flags |= UV_HANDLE_ESRCH; /* 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->flags & UV_HANDLE_ESRCH)
+    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 UV_HANDLE_ESRCH is not set, 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/uv-common.h

@@ -137,6 +137,7 @@ enum {
   UV_HANDLE_POLL_SLOW                   = 0x01000000,
 
   /* Only used by uv_process_t handles. */
+  UV_HANDLE_ESRCH                       = 0x01000000,
   UV_HANDLE_REAP                        = 0x10000000
 };
 

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->flags |= UV_HANDLE_ESRCH;
+
+  /* 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);
 }
@@ -1364,8 +1370,8 @@ static int uv__kill(HANDLE process_handle, int signum) {
 int uv_process_kill(uv_process_t* process, int signum) {
   int err;
 
-  if (process->process_handle == INVALID_HANDLE_VALUE) {
-    return UV_EINVAL;
+  if (process->flags & UV_HANDLE_ESRCH) {
+    return UV_ESRCH;
   }
 
   err = uv__kill(process->process_handle, signum);