Fix orderly helper appsec shutdown

The sent SIGUSR1 was just terminating the process.
DataDog · Sep 17, 2024 · 6fea9c9 · 6fea9c9
1 parent 17b3c7a
commit 6fea9c9
Show file tree

Hide file tree

Showing 5 changed files with 44 additions and 14 deletions.
diff --git a/appsec/src/helper/main.cpp b/appsec/src/helper/main.cpp
@@ -10,14 +10,15 @@
 #include "config.hpp"
 #include "runner.hpp"
 #include "subscriber/waf.hpp"
+#include <csignal>
+#include <cstdlib>
 #include <spdlog/common.h>
 #include <spdlog/sinks/basic_file_sink.h>
 #include <spdlog/sinks/stdout_sinks.h>
 #include <spdlog/spdlog.h>
 #include <string_view>
 
 extern "C" {
-#include <csignal>
 #include <fcntl.h>
 #include <pthread.h>
 #include <sys/file.h>
@@ -101,6 +102,15 @@ int appsec_helper_main_impl()
         return 1;
     }
 
+    // block SIGUSR1 (only used to interrupt the runner)
+    sigset_t mask;
+    sigemptyset(&mask);
+    sigaddset(&mask, SIGUSR1);
+    if (auto err = pthread_sigmask(SIG_BLOCK, &mask, nullptr)) {
+        SPDLOG_ERROR("Failed to block SIGUSR1: error number {}", err);
+        return 1;
+    }
+
     auto runner = std::make_unique<dds::runner>(config, interrupted);
     SPDLOG_INFO("starting runner on new thread");
     std::thread thr{[runner = std::move(runner)]() {
@@ -149,8 +159,13 @@ appsec_helper_shutdown() noexcept
             return 0;
         }
         if (std::chrono::steady_clock::now() >= deadline) {
-            SPDLOG_WARN("Could not finish AppSec helper before deadline");
-            return 1;
+            // we need to call exit() to avoid a segfault in the still running
+            // helper threads after the helper shared library is unloaded by
+            // trampoline.c
+            SPDLOG_WARN("Could not finish AppSec helper before deadline. "
+                        "Calling exit().");
+            std::exit(EXIT_FAILURE); // NOLINT
+            __builtin_unreachable();
         }
         std::this_thread::sleep_for(std::chrono::milliseconds{10}); // NOLINT
     }

diff --git a/appsec/src/helper/network/acceptor.cpp b/appsec/src/helper/network/acceptor.cpp
@@ -80,13 +80,10 @@ socket::ptr acceptor::accept()
     struct sockaddr_un addr {};
     socklen_t len = sizeof(addr);
 
-    int s;
-    do {
-        // NOLINTNEXTLINE
-        s = ::accept(sock_, reinterpret_cast<struct sockaddr *>(&addr), &len);
-    } while (s == -1 && errno == EINTR);
+    // NOLINTNEXTLINE
+    int s = ::accept(sock_, reinterpret_cast<struct sockaddr *>(&addr), &len);
     if (s == -1) {
-        if (errno == EAGAIN) {
+        if (errno == EINTR || errno == EAGAIN) {
             throw dds::timeout_error();
         }
 

diff --git a/appsec/src/helper/runner.cpp b/appsec/src/helper/runner.cpp
@@ -7,6 +7,7 @@
 
 #include "client.hpp"
 #include "subscriber/waf.hpp"
+#include <csignal>
 #include <cstdio>
 #include <spdlog/spdlog.h>
 #include <stdexcept>
@@ -32,6 +33,26 @@ network::base_acceptor::ptr acceptor_from_config(const config::config &cfg)
 
     return std::make_unique<network::local::acceptor>(sock_path);
 }
+
+void unblock_sigusr1()
+{
+    // the signal handler need not do anything (just interrupt accept())
+    struct sigaction alarmer = {};
+    alarmer.sa_handler = [](int) {};
+    if (sigaction(SIGUSR1, &alarmer, nullptr) < 0) {
+        throw std::runtime_error{
+            "Failed to set SIGUSR1 handler: errno " + std::to_string(errno)};
+    }
+
+    sigset_t mask;
+    sigemptyset(&mask);
+    sigaddset(&mask, SIGUSR1);
+    if (pthread_sigmask(SIG_UNBLOCK, &mask, nullptr) != 0) {
+        throw std::runtime_error{
+            "Failed to unblock SIGUSR1: errno " + std::to_string(errno)};
+    }
+}
+
 } // namespace
 
 runner::runner(const config::config &cfg, std::atomic<bool> &interrupted)
@@ -54,8 +75,8 @@ runner::runner(const config::config &cfg,
 void runner::run()
 {
     try {
-        auto last_not_idle = std::chrono::steady_clock::now();
         SPDLOG_INFO("Runner running");
+        unblock_sigusr1();
         while (!interrupted()) {
             network::base_socket::ptr socket;
             try {
@@ -80,8 +101,6 @@ void runner::run()
 
             worker_pool_.launch(
                 [c](worker::queue_consumer &q) mutable { c->run(q); });
-
-            last_not_idle = std::chrono::steady_clock::now();
         }
     } catch (const std::exception &e) {
         SPDLOG_ERROR("exception: {}", e.what());

diff --git a/appsec/src/helper/runner.hpp b/appsec/src/helper/runner.hpp
@@ -30,7 +30,7 @@ class runner {
 
     void run() noexcept(false);
 
-    bool interrupted() const
+    [[nodiscard]] bool interrupted() const
     {
         return interrupted_.load(std::memory_order_acquire);
     }

diff --git a/...sts/integration/src/test/groovy/com/datadog/appsec/php/integration/Apache2FpmTests.groovy b/...sts/integration/src/test/groovy/com/datadog/appsec/php/integration/Apache2FpmTests.groovy
@@ -12,7 +12,6 @@ import org.testcontainers.junit.jupiter.Testcontainers
 import java.net.http.HttpResponse
 
 import static com.datadog.appsec.php.integration.TestParams.getPhpVersion
-import static com.datadog.appsec.php.integration.TestParams.getTracerVersion
 import static com.datadog.appsec.php.integration.TestParams.getVariant
 import static org.testcontainers.containers.Container.ExecResult