diff --git a/source/client/client.cc b/source/client/client.cc index 40ca7db01..8ac63bed9 100644 --- a/source/client/client.cc +++ b/source/client/client.cc @@ -87,19 +87,29 @@ bool Main::run() { OutputCollectorImpl output_collector(time_system, *options_); bool result; { + // The SignalHandler scope covers process->shutdown() as well: the shutdown/drain phase + // is the most likely to hang, and signals must keep being handled there. Requesting + // execution cancellation on an already-finished run is safe: worker cancellation and + // shutdown() serialize on the same lock inside ProcessImpl (encap subprocess + // termination is guarded by the runner's own mutex), and RemoteProcessImpl logs that + // cancellation is unsupported. The SignalHandler restores the previous (typically + // default) dispositions as soon as it consumes the first signal, so any further signal + // terminates the process (escalation) even if the cancellation callback blocks on a + // hung shutdown. auto signal_handler = std::make_unique([&process]() { process->requestExecutionCancellation(); }); result = process->run(output_collector); + auto formatter = output_formatter_factory.create(options_->outputFormat()); + absl::StatusOr formatted_proto = + formatter->formatProto(output_collector.toProto()); + if (!formatted_proto.ok()) { + ENVOY_LOG(error, "An error occurred while formatting proto"); + result = false; + } else { + std::cout << *formatted_proto; + } + process->shutdown(); } - auto formatter = output_formatter_factory.create(options_->outputFormat()); - absl::StatusOr formatted_proto = formatter->formatProto(output_collector.toProto()); - if (!formatted_proto.ok()) { - ENVOY_LOG(error, "An error occurred while formatting proto"); - result = false; - } else { - std::cout << *formatted_proto; - } - process->shutdown(); if (!result) { ENVOY_LOG(error, "An error occurred."); } else { diff --git a/source/common/signal_handler.cc b/source/common/signal_handler.cc index b94477aa2..3bade6fca 100644 --- a/source/common/signal_handler.cc +++ b/source/common/signal_handler.cc @@ -9,7 +9,14 @@ namespace Nighthawk { namespace { std::function signal_handler_delegate; -void signal_handler(int signal) { signal_handler_delegate(signal); } +// The delegate is cleared at the very end of ~SignalHandler(), after the dispositions have +// been restored; the null check is defense-in-depth for the small window where a signal was +// already dispatched into this handler while the destructor is swapping dispositions. +void signal_handler(int signal) { + if (signal_handler_delegate) { + signal_handler_delegate(signal); + } +} } // namespace SignalHandler::SignalHandler(const std::function& signal_callback) { @@ -17,6 +24,15 @@ SignalHandler::SignalHandler(const std::function& signal_callback) { // The shutdown thread will be notified of by our signal handler and take it from there. RELEASE_ASSERT(pipe(pipe_fds_.data()) == 0, "pipe failed"); + signal_handler_delegate = [this](int) { onSignal(); }; + // Save the dispositions we replace so we can restore them after the first handled signal + // (second-signal escalation) and upon destruction. This happens before the shutdown + // thread is spawned so the thread is guaranteed to observe the saved values. + previous_sigterm_handler_ = signal(SIGTERM, signal_handler); + previous_sigint_handler_ = signal(SIGINT, signal_handler); + RELEASE_ASSERT(previous_sigterm_handler_ != SIG_ERR, "signal(SIGTERM) failed"); + RELEASE_ASSERT(previous_sigint_handler_ != SIG_ERR, "signal(SIGINT) failed"); + shutdown_thread_ = std::thread([this, signal_callback]() { int tmp; RELEASE_ASSERT(read(pipe_fds_[0], &tmp, sizeof(int)) >= 0, "read failed"); @@ -24,21 +40,46 @@ SignalHandler::SignalHandler(const std::function& signal_callback) { RELEASE_ASSERT(close(pipe_fds_[1]) == 0, "write side close failed"); pipe_fds_.clear(); if (!destructing_) { + // The first signal is about to be handled: fall back to the previous (typically + // default) dispositions BEFORE invoking the callback, so that any further signal + // gets the OS default action - giving the user a force-quit escalation path. This + // must happen before the callback rather than after it: the callback may itself + // block on the very shutdown sequence the user is trying to escape (e.g. + // requestExecutionCancellation() contends on the lock held across the worker drain + // loop in ProcessImpl::shutdown()), and a restore placed after it would never run + // in exactly that case. Calling signal() from this (non-main) thread is fine: + // POSIX specifies signal()/sigaction() as thread-safe (and async-signal-safe). + signal(SIGTERM, previous_sigterm_handler_); + signal(SIGINT, previous_sigint_handler_); signal_callback(); } }); - - signal_handler_delegate = [this](int) { onSignal(); }; - signal(SIGTERM, signal_handler); - signal(SIGINT, signal_handler); } SignalHandler::~SignalHandler() { + // Ordering rationale: + // 1) Set destructing_ first, so that if the shutdown thread gets woken up below (or by a + // concurrently arriving signal) it will not invoke the callback mid-destruction. + // 2) Restore the previous signal dispositions. From this point on no NEW invocations of + // the file-scope signal_handler() can start, so it becomes safe to tear down the + // state it depends on. (If the shutdown thread already restored them after handling + // a first signal, this re-installs the same values: harmless.) + // 3) Wake the shutdown thread if no signal ever fired (initiateShutdown() writes to the + // pipe; it is a no-op when the thread already consumed a signal and closed the pipe) + // and join it, draining the only thread that runs the callback or touches the pipe. + // 4) Only then clear signal_handler_delegate. Doing this any earlier could leave an + // installed handler pointing at an empty std::function. Inherent (accepted) race: a + // signal already dispatched into signal_handler() on another thread just before (2) + // may still be executing; keeping the delegate alive until after restore+join makes + // that window safe - the delegate only writes to the pipe, a no-op once closed. destructing_ = true; + signal(SIGTERM, previous_sigterm_handler_); + signal(SIGINT, previous_sigint_handler_); initiateShutdown(); if (shutdown_thread_.joinable()) { shutdown_thread_.join(); } + signal_handler_delegate = nullptr; } void SignalHandler::initiateShutdown() { diff --git a/source/common/signal_handler.h b/source/common/signal_handler.h index c69a45869..1ee3c4d22 100644 --- a/source/common/signal_handler.h +++ b/source/common/signal_handler.h @@ -19,7 +19,10 @@ using SignalCallback = std::function; * Utility class for handling TERM and INT signals. Allows wiring up a callback that * should be invoked upon signal reception. This callback implementation does not have to be * signal safe, as a different thread is used to fire it. - * NOTE: Only the first observed signal will result in the callback being invoked. + * NOTE: Only the first observed signal will result in the callback being invoked. After the + * first signal has been handled, the signal dispositions that were active before this + * instance was constructed are restored (typically SIG_DFL), so a second signal gets the + * OS default action (usually process termination) - an escalation path when shutdown hangs. * WARNING: only a single instance should be active at any given time in a process, and * the responsibility for not breaking this rule is not enforced at this time. * @@ -70,6 +73,14 @@ class SignalHandler final : public Envoy::Logger::Loggable pipe_fds_; bool destructing_{false}; + + // The dispositions that were installed for SIGTERM/SIGINT before this instance replaced + // them (as returned by signal()). Restored after the first handled signal (so a second + // signal gets the OS default action) and again in the destructor. Relies on the existing + // single-active-instance assumption (see the class WARNING above); the file-scope + // delegate in signal_handler.cc already imposes it. + void (*previous_sigterm_handler_)(int) = nullptr; + void (*previous_sigint_handler_)(int) = nullptr; }; using SignalHandlerPtr = std::unique_ptr; diff --git a/test/common/signal_handler_test.cc b/test/common/signal_handler_test.cc index 53cfb72c6..a325974e8 100644 --- a/test/common/signal_handler_test.cc +++ b/test/common/signal_handler_test.cc @@ -1,5 +1,9 @@ +#include +#include #include #include +#include +#include #include "source/common/signal_handler.h" @@ -31,5 +35,124 @@ TEST(SignalHandlerTest, DestructDoesNotFireHandler) { EXPECT_FALSE(signal_handled); } +// Regression test: ~SignalHandler() used to leave the file-scope static `signal_handler` +// installed for SIGTERM/SIGINT, whose delegate (the file-scope std::function +// `signal_handler_delegate`) captured a `this` pointer to the DESTROYED SignalHandler +// instance - a dangling delegate; any later signal was undefined behavior, best-case a +// silent no-op. The destructor now restores the dispositions saved at construction time +// (and clears the delegate), so after destruction SIGTERM/SIGINT are back to SIG_DFL. +TEST(SignalHandlerTest, DestructorRestoresDefaultSignalDisposition) { + // Tests in this binary share a process, and earlier tests may already have left the + // stale nighthawk handler installed for these signals. To be independent of test + // ordering, force a known clean state (SIG_DFL) before constructing the handler under + // test. We intentionally do NOT reinstall the captured previous handlers afterwards: + // they may be the stale (dangling-delegate) handler, and SIG_DFL is the safe, + // non-polluting end state - every other test installs its own SignalHandler before + // raising signals. + const auto previous_term_disposition = std::signal(SIGTERM, SIG_DFL); + const auto previous_int_disposition = std::signal(SIGINT, SIG_DFL); + ASSERT_NE(previous_term_disposition, SIG_ERR); + ASSERT_NE(previous_int_disposition, SIG_ERR); + + { + SignalHandler signal_handler([]() {}); + } + + // Query the current dispositions via a std::signal() round-trip: std::signal returns + // the handler that was installed at the time of the call. + const auto post_destruction_term = std::signal(SIGTERM, SIG_DFL); + const auto post_destruction_int = std::signal(SIGINT, SIG_DFL); + EXPECT_EQ(post_destruction_term, SIG_DFL) + << "~SignalHandler() left a stale SIGTERM handler installed"; + EXPECT_EQ(post_destruction_int, SIG_DFL) + << "~SignalHandler() left a stale SIGINT handler installed"; +} + +// Second-signal escalation: after the FIRST signal is handled (the shutdown thread has +// consumed it and run the callback), the SignalHandler restores the signal dispositions +// that were active before it was constructed. A SECOND Ctrl-C therefore gets the OS +// default action - process termination - instead of being silently swallowed, giving a +// user stuck in a hanging shutdown/drain a force-quit escalation path. +// +// We deliberately do NOT raise a real second SIGTERM here: with the escalation in place +// it would kill the test binary. Instead we prove the escalation by observing that the +// installed disposition falls back to SIG_DFL. The restore runs on the shutdown thread +// after the callback returns, so it is asynchronous with respect to the callback firing; +// we poll for it with a bounded deadline. The poll uses sigaction() with a null +// new-action, which is a pure READ of the current disposition - unlike a +// std::signal(SIGTERM, ) round-trip, it cannot race with (and clobber) the +// shutdown thread's restore. +TEST(SignalHandlerTest, SecondSignalFallsBackToDefaultDisposition) { + // Tests in this binary share a process; force a known clean state so the dispositions + // the SignalHandler under test saves (and later restores) are SIG_DFL, independent of + // test ordering. + ASSERT_NE(std::signal(SIGTERM, SIG_DFL), SIG_ERR); + ASSERT_NE(std::signal(SIGINT, SIG_DFL), SIG_ERR); + + std::atomic callback_count{0}; + std::promise first_callback_fired; + + SignalHandler signal_handler([&callback_count, &first_callback_fired]() { + if (++callback_count == 1) { + first_callback_fired.set_value(); + } + }); + + std::raise(SIGTERM); + first_callback_fired.get_future().wait(); + EXPECT_EQ(callback_count, 1); + + // Poll (read-only) until both dispositions are back to SIG_DFL, bounded at 2 seconds. + const auto dispositions_restored = []() { + struct sigaction term_action = {}; + struct sigaction int_action = {}; + if (sigaction(SIGTERM, nullptr, &term_action) != 0 || + sigaction(SIGINT, nullptr, &int_action) != 0) { + return false; + } + return term_action.sa_handler == SIG_DFL && int_action.sa_handler == SIG_DFL; + }; + // There is no injectable time source in SignalHandler; this bounded wall-clock poll is + // inherent to observing an asynchronous disposition change made by its shutdown thread. + const auto deadline = + std::chrono::steady_clock::now() + std::chrono::seconds(2); // NO_CHECK_FORMAT(real_time) + while (!dispositions_restored() && + std::chrono::steady_clock::now() < deadline) { // NO_CHECK_FORMAT(real_time) + std::this_thread::sleep_for(std::chrono::milliseconds(10)); // NO_CHECK_FORMAT(real_time) + } + EXPECT_TRUE(dispositions_restored()) + << "second-signal escalation: SIGTERM/SIGINT dispositions were not restored to " + "SIG_DFL after the first signal was handled, so a second Ctrl-C would still be " + "silently swallowed"; +} + +// Regression death test: guards the dangling-delegate hazard from the process-lifetime +// angle. ~SignalHandler() restores the saved dispositions (typically SIG_DFL), so raising +// SIGTERM after destruction terminates the process. Before that fix, the stale static +// handler swallowed the signal (invoking the dangling delegate - UB that in practice +// wrote to a closed/cleared pipe and returned), the statement completed without dying, +// and this test failed with "failed to die". +// +// The "threadsafe" death test style is required: it re-executes the test binary for +// the child so the child starts single-threaded, which matters because SignalHandler +// spawns a shutdown thread (the default "fast" style just fork()s a process that may +// already have threads, which is unsafe/flaky here). This repo already uses death +// tests elsewhere (e.g. test/rate_limiter_test.cc). +TEST(SignalHandlerDeathTest, RaisingSignalAfterDestructionTerminatesProcess) { + const std::string previous_style = GTEST_FLAG_GET(death_test_style); + GTEST_FLAG_SET(death_test_style, "threadsafe"); + EXPECT_DEATH( + { + { + SignalHandler signal_handler([]() {}); + } + // With dispositions restored to SIG_DFL by the destructor above, this kills + // the child process (default action for SIGTERM). + std::raise(SIGTERM); + }, + ""); + GTEST_FLAG_SET(death_test_style, previous_style); +} + } // namespace } // namespace Nighthawk