Merge pull request #4048 from LiterallyVoid/literallyvoid/fix-4047-deadlock

Fix signal safety deadlock

src/main.cpp

@@ -1,3 +1,4 @@
+#include <fcntl.h>
 #include <spdlog/spdlog.h>
 #include <sys/types.h>
 #include <sys/wait.h>
@@ -7,66 +8,115 @@
 #include <mutex>
 
 #include "client.hpp"
+#include "util/SafeSignal.hpp"
 
 std::mutex reap_mtx;
 std::list<pid_t> reap;
-volatile bool reload;
 
-void* signalThread(void* args) {
+static int signal_pipe_write_fd;
-  int err;
+
-  int signum;
+// Write a single signal to `signal_pipe_write_fd`.
-  sigset_t mask;
+// This function is set as a signal handler, so it must be async-signal-safe.
-  sigemptyset(&mask);
+static void writeSignalToPipe(int signum) {
-  sigaddset(&mask, SIGCHLD);
+  ssize_t amt = write(signal_pipe_write_fd, &signum, sizeof(int));
+
+  // There's not much we can safely do inside of a signal handler.
+  // Let's just ignore any errors.
+  (void)amt;
+}
+
+// This initializes `signal_pipe_write_fd`, and sets up signal handlers.
+//
+// This function will run forever, emitting every `SIGUSR1`, `SIGUSR2`,
+// `SIGINT`, `SIGCHLD`, and `SIGRTMIN + 1`...`SIGRTMAX` signal received
+// to `signal_handler`.
+static void catchSignals(waybar::SafeSignal<int>& signal_handler) {
+  int fd[2];
+  pipe(fd);
+
+  int signal_pipe_read_fd = fd[0];
+  signal_pipe_write_fd = fd[1];
+
+  // This pipe should be able to buffer ~thousands of signals. If it fills up,
+  // we'll drop signals instead of blocking.
+
+  // We can't allow the write end to block because we'll be writing to it in a
+  // signal handler, which could interrupt the loop that's reading from it and
+  // deadlock.
+
+  fcntl(signal_pipe_write_fd, F_SETFL, O_NONBLOCK);
+
+  std::signal(SIGUSR1, writeSignalToPipe);
+  std::signal(SIGUSR2, writeSignalToPipe);
+  std::signal(SIGINT, writeSignalToPipe);
+  std::signal(SIGCHLD, writeSignalToPipe);
+
+  for (int sig = SIGRTMIN + 1; sig <= SIGRTMAX; ++sig) {
+    std::signal(sig, writeSignalToPipe);
+  }
 
   while (true) {
-    err = sigwait(&mask, &signum);
+    int signum;
-    if (err != 0) {
+    ssize_t amt = read(signal_pipe_read_fd, &signum, sizeof(int));
-      spdlog::error("sigwait failed: {}", strerror(errno));
+    if (amt < 0) {
+      spdlog::error("read from signal pipe failed with error {}, closing thread", strerror(errno));
+      break;
+    }
+
+    if (amt != sizeof(int)) {
       continue;
     }
 
-    switch (signum) {
+    signal_handler.emit(signum);
-      case SIGCHLD:
-        spdlog::debug("Received SIGCHLD in signalThread");
-        if (!reap.empty()) {
-          reap_mtx.lock();
-          for (auto it = reap.begin(); it != reap.end(); ++it) {
-            if (waitpid(*it, nullptr, WNOHANG) == *it) {
-              spdlog::debug("Reaped child with PID: {}", *it);
-              it = reap.erase(it);
-            }
-          }
-          reap_mtx.unlock();
-        }
-        break;
-      default:
-        spdlog::debug("Received signal with number {}, but not handling", signum);
-        break;
-    }
   }
 }
 
-void startSignalThread() {
+// Must be called on the main thread.
-  int err;
+//
-  sigset_t mask;
+// If this signal should restart or close the bar, this function will write
-  sigemptyset(&mask);
+// `true` or `false`, respectively, into `reload`.
-  sigaddset(&mask, SIGCHLD);
+static void handleSignalMainThread(int signum, bool& reload) {
+  if (signum >= SIGRTMIN + 1 && signum <= SIGRTMAX) {
+    for (auto& bar : waybar::Client::inst()->bars) {
+      bar->handleSignal(signum);
+    }
 
-  // Block SIGCHLD so it can be handled by the signal thread
+    return;
-  // Any threads created by this one (the main thread) should not
-  // modify their signal mask to unblock SIGCHLD
-  err = pthread_sigmask(SIG_BLOCK, &mask, nullptr);
-  if (err != 0) {
-    spdlog::error("pthread_sigmask failed in startSignalThread: {}", strerror(err));
-    exit(1);
   }
 
-  pthread_t thread_id;
+  switch (signum) {
-  err = pthread_create(&thread_id, nullptr, signalThread, nullptr);
+    case SIGUSR1:
-  if (err != 0) {
+      spdlog::debug("Visibility toggled");
-    spdlog::error("pthread_create failed in startSignalThread: {}", strerror(err));
+      for (auto& bar : waybar::Client::inst()->bars) {
-    exit(1);
+        bar->toggle();
+      }
+      break;
+    case SIGUSR2:
+      spdlog::info("Reloading...");
+      reload = true;
+      waybar::Client::inst()->reset();
+      break;
+    case SIGINT:
+      spdlog::info("Quitting.");
+      reload = false;
+      waybar::Client::inst()->reset();
+      break;
+    case SIGCHLD:
+      spdlog::debug("Received SIGCHLD in signalThread");
+      if (!reap.empty()) {
+        reap_mtx.lock();
+        for (auto it = reap.begin(); it != reap.end(); ++it) {
+          if (waitpid(*it, nullptr, WNOHANG) == *it) {
+            spdlog::debug("Reaped child with PID: {}", *it);
+            it = reap.erase(it);
+          }
+        }
+        reap_mtx.unlock();
+      }
+      break;
+    default:
+      spdlog::debug("Received signal with number {}, but not handling", signum);
+      break;
   }
 }
 
@@ -74,32 +124,16 @@ int main(int argc, char* argv[]) {
   try {
     auto* client = waybar::Client::inst();
 
-    std::signal(SIGUSR1, [](int /*signal*/) {
+    bool reload;
-      for (auto& bar : waybar::Client::inst()->bars) {
-        bar->toggle();
-      }
-    });
 
-    std::signal(SIGUSR2, [](int /*signal*/) {
+    waybar::SafeSignal<int> posix_signal_received;
-      spdlog::info("Reloading...");
+    posix_signal_received.connect([&](int signum) { handleSignalMainThread(signum, reload); });
-      reload = true;
-      waybar::Client::inst()->reset();
-    });
 
-    std::signal(SIGINT, [](int /*signal*/) {
+    std::thread signal_thread([&]() { catchSignals(posix_signal_received); });
-      spdlog::info("Quitting.");
-      reload = false;
-      waybar::Client::inst()->reset();
-    });
 
-    for (int sig = SIGRTMIN + 1; sig <= SIGRTMAX; ++sig) {
+    // Every `std::thread` must be joined or detached.
-      std::signal(sig, [](int sig) {
+    // This thread should run forever, so detach it.
-        for (auto& bar : waybar::Client::inst()->bars) {
+    signal_thread.detach();
-          bar->handleSignal(sig);
-        }
-      });
-    }
-    startSignalThread();
 
     auto ret = 0;
     do {