From d580ee30f1b388667568b0a30710e365939ca994 Mon Sep 17 00:00:00 2001
From: Romain Vimont <rom@rom1v.com>
Date: Sun, 3 Jan 2021 17:06:08 +0100
Subject: [PATCH] Separate process wait and close

On Linux, waitpid() both waits for the process to terminate and reaps it
(closes its handle). On Windows, these actions are separated into
WaitForSingleObject() and CloseHandle().

Expose these actions separately, so that it is possible to send a signal
to a process while waiting for its termination without race condition.

This allows to wait for server termination normally, but kill the
process without race condition if it is not terminated after some delay.
---
 app/src/server.c           |  8 ++++----
 app/src/sys/unix/process.c | 31 ++++++++++++++++++++++++++-----
 app/src/sys/win/process.c  | 26 +++++++++++++++++++++++---
 app/src/util/process.h     | 12 +++++++++++-
 4 files changed, 64 insertions(+), 13 deletions(-)

diff --git a/app/src/server.c b/app/src/server.c
index 1697a000..cb2a24db 100644
--- a/app/src/server.c
+++ b/app/src/server.c
@@ -392,7 +392,7 @@ server_init(struct server *server) {
 static int
 run_wait_server(void *data) {
     struct server *server = data;
-    process_wait(server->process, NULL); // ignore exit code
+    process_wait_noclose(server->process, NULL); // ignore exit code
 
     mutex_lock(server->mutex);
     server->process_terminated = true;
@@ -550,14 +550,14 @@ server_stop(struct server *server) {
     // On some devices, closing the sockets is not sufficient to wake up the
     // blocking calls while the device is asleep.
     if (r == SDL_MUTEX_TIMEDOUT) {
-        // FIXME There is a race condition here: there is a small chance that
-        // the process is already terminated, and the PID assigned to a new
-        // process.
+        // The process is terminated, but not reaped (closed) yet, so its PID
+        // is still valid.
         LOGW("Killing the server...");
         process_terminate(server->process);
     }
 
     SDL_WaitThread(server->wait_server_thread, NULL);
+    process_close(server->process);
 }
 
 void
diff --git a/app/src/sys/unix/process.c b/app/src/sys/unix/process.c
index 3f95ce7f..00a8d81c 100644
--- a/app/src/sys/unix/process.c
+++ b/app/src/sys/unix/process.c
@@ -134,15 +134,21 @@ process_terminate(pid_t pid) {
     return kill(pid, SIGTERM) != -1;
 }
 
-bool
-process_wait(pid_t pid, int *exit_code) {
-    int status;
+static bool
+process_wait_internal(pid_t pid, int *exit_code, bool close) {
     int code;
-    if (waitpid(pid, &status, 0) == -1 || !WIFEXITED(status)) {
+    int options = WEXITED;
+    if (!close) {
+        options |= WNOWAIT;
+    }
+
+    siginfo_t info;
+    int r = waitid(P_PID, pid, &info, options);
+    if (r == -1 || info.si_code != CLD_EXITED) {
         // could not wait, or exited unexpectedly, probably by a signal
         code = -1;
     } else {
-        code = WEXITSTATUS(status);
+        code = info.si_status;
     }
     if (exit_code) {
         *exit_code = code;
@@ -150,6 +156,21 @@ process_wait(pid_t pid, int *exit_code) {
     return !code;
 }
 
+bool
+process_wait(pid_t pid, int *exit_code) {
+    return process_wait_internal(pid, exit_code, true);
+}
+
+bool
+process_wait_noclose(pid_t pid, int *exit_code) {
+    return process_wait_internal(pid, exit_code, false);
+}
+
+void
+process_close(pid_t pid) {
+    process_wait_internal(pid, NULL, true);
+}
+
 char *
 get_executable_path(void) {
 // <https://stackoverflow.com/a/1024937/1987178>
diff --git a/app/src/sys/win/process.c b/app/src/sys/win/process.c
index c30e751b..f2a86b22 100644
--- a/app/src/sys/win/process.c
+++ b/app/src/sys/win/process.c
@@ -1,5 +1,6 @@
 #include "util/process.h"
 
+#include <assert.h>
 #include <sys/stat.h>
 
 #include "config.h"
@@ -59,8 +60,8 @@ process_terminate(HANDLE handle) {
     return TerminateProcess(handle, 1);
 }
 
-bool
-process_wait(HANDLE handle, DWORD *exit_code) {
+static bool
+process_wait_internal(HANDLE handle, DWORD *exit_code, bool close) {
     DWORD code;
     if (WaitForSingleObject(handle, INFINITE) != WAIT_OBJECT_0
             || !GetExitCodeProcess(handle, &code)) {
@@ -70,10 +71,29 @@ process_wait(HANDLE handle, DWORD *exit_code) {
     if (exit_code) {
         *exit_code = code;
     }
-    CloseHandle(handle);
+    if (close) {
+        CloseHandle(handle);
+    }
     return !code;
 }
 
+bool
+process_wait(HANDLE handle, DWORD *exit_code) {
+    return process_wait_internal(handle, exit_code, true);
+}
+
+bool
+process_wait_noclose(HANDLE handle, DWORD *exit_code) {
+    return process_wait_internal(handle, exit_code, false);
+}
+
+void
+process_close(HANDLE handle) {
+    bool closed = CloseHandle(handle);
+    assert(closed);
+    (void) closed;
+}
+
 char *
 get_executable_path(void) {
     HMODULE hModule = GetModuleHandleW(NULL);
diff --git a/app/src/util/process.h b/app/src/util/process.h
index 58328a3e..ad2bc0d0 100644
--- a/app/src/util/process.h
+++ b/app/src/util/process.h
@@ -46,10 +46,20 @@ process_execute(const char *const argv[], process_t *process);
 bool
 process_terminate(process_t pid);
 
-// wait and close the process
+// wait and close the process (like waitpid())
 bool
 process_wait(process_t pid, exit_code_t *exit_code);
 
+// wait (but does not close) the process (waitid() with WNOWAIT)
+bool
+process_wait_noclose(process_t pid, exit_code_t *exit_code);
+
+// close the process
+//
+// Semantically, process_wait = process_wait_noclose + process_close.
+void
+process_close(process_t pid);
+
 // convenience function to wait for a successful process execution
 // automatically log process errors with the provided process name
 bool