From a0a0950a06aeef388bbe55abaa2d0dc386c5dbe6 Mon Sep 17 00:00:00 2001
From: Keno Fischer <keno@juliacomputing.com>
Date: Tue, 26 Sep 2023 17:47:44 +0000
Subject: [PATCH] Fix race condition in sandbox

The current sandbox code has a race condition. If the child exits
before the sigwait signal mask is setup, we will hang forever waiting
for a signal. This has been observed in practice in Yggdrasil.
I don't know that the sigwait here is supposed to do - plain
waitpid should be fine, so try removing it to see if everything
works.

If the sigwait does turn out to be necessary, we should be able
to fix this by moving the signal mask setup to above the fork instead.
---
 deps/userns_sandbox.c | 55 +++++++++++++++++++++++--------------------
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/deps/userns_sandbox.c b/deps/userns_sandbox.c
index be30a6a..ef0878b 100644
--- a/deps/userns_sandbox.c
+++ b/deps/userns_sandbox.c
@@ -259,35 +259,38 @@ static int sandbox_main(const char * root_dir, const char * new_cd, int sandbox_
 
   // Let's perform normal init functions, handling signals from orphaned
   // children, etc
-  sigset_t waitset;
-  sigemptyset(&waitset);
-  sigaddset(&waitset, SIGCHLD);
-  sigprocmask(SIG_BLOCK, &waitset, NULL);
   for (;;) {
-    int sig;
-    sigwait(&waitset, &sig);
-
-    pid_t reaped_pid;
-    while ((reaped_pid = waitpid(-1, &status, 0)) != -1) {
-      if (reaped_pid == child_pid) {
-        // If it was the main pid that exited, we're going to exit too.
-        // If we died of a signal, return that signal + 256, which we will
-        // notice on the other end as a signal.
-        if (WIFSIGNALED(status)) {
-          uint32_t reported_exit_code = 256 + WTERMSIG(status);
-          check(sizeof(uint32_t) == write(parent_pipe[1], &reported_exit_code, sizeof(uint32_t)));
-          return 0;
-        }
-        if (WIFEXITED(status)) {
-          // Normal exits get reported in a more straightforward fashion
-          uint32_t reported_exit_code = WEXITSTATUS(status);
-          check(sizeof(uint32_t) == write(parent_pipe[1], &reported_exit_code, sizeof(uint32_t)));
-          return 0;
-        }
-
-        // Unsure what's going on here, but it isn't good.
+    pid_t reaped_pid = waitpid(-1, &status, 0);
+    if (reaped_pid == -1) {
+      if (errno == ECHILD) {
+        // Somehow we missed the reaping of our child - this shouldn't happen,
+        // but let's make sure we don't hang forever if it somehow does.
+        check(-2);
+      }
+      if (errno != EINTR) {
+        // Some other unexpected error
         check(-1);
       }
+      continue;
+    }
+    if (reaped_pid == child_pid) {
+      // If it was the main pid that exited, we're going to exit too.
+      // If we died of a signal, return that signal + 256, which we will
+      // notice on the other end as a signal.
+      if (WIFSIGNALED(status)) {
+        uint32_t reported_exit_code = 256 + WTERMSIG(status);
+        check(sizeof(uint32_t) == write(parent_pipe[1], &reported_exit_code, sizeof(uint32_t)));
+        return 0;
+      }
+      if (WIFEXITED(status)) {
+        // Normal exits get reported in a more straightforward fashion
+        uint32_t reported_exit_code = WEXITSTATUS(status);
+        check(sizeof(uint32_t) == write(parent_pipe[1], &reported_exit_code, sizeof(uint32_t)));
+        return 0;
+      }
+
+      // Unsure what's going on here, but it isn't good.
+      check(-1);
     }
   }
 }