From 9a45279c69aaa7f94336e275e720880edd08798f Mon Sep 17 00:00:00 2001 From: Johannes Schmidt Date: Thu, 2 Apr 2026 13:32:43 +0200 Subject: [PATCH 1/5] Read until end of file in response reading test-cases --- test/perfdata-perfdatatargetfixture.hpp | 11 ++++++++++- test/perfdata-perfdatawriterconnection.cpp | 4 ++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/test/perfdata-perfdatatargetfixture.hpp b/test/perfdata-perfdatatargetfixture.hpp index bac1c504d..8c2a381bc 100644 --- a/test/perfdata-perfdatatargetfixture.hpp +++ b/test/perfdata-perfdatatargetfixture.hpp @@ -71,11 +71,20 @@ public: BOOST_REQUIRE(stream->next_layer().IsVerifyOK()); } - void Shutdown() + void Shutdown(bool wait = false) { BOOST_REQUIRE(std::holds_alternative::Ptr>(m_Stream)); auto& stream = std::get::Ptr>(m_Stream); try { + if (wait) { + std::array buf{}; + boost::asio::mutable_buffer readBuf (buf.data(), buf.size()); + boost::system::error_code ec; + + do { + stream->read_some(readBuf, ec); + } while (!ec); + } stream->next_layer().shutdown(); } catch (const std::exception& ex) { if (const auto* se = dynamic_cast(&ex); diff --git a/test/perfdata-perfdatawriterconnection.cpp b/test/perfdata-perfdatawriterconnection.cpp index 16ed299a9..0f2435198 100644 --- a/test/perfdata-perfdatawriterconnection.cpp +++ b/test/perfdata-perfdatawriterconnection.cpp @@ -207,7 +207,7 @@ BOOST_AUTO_TEST_CASE(stuck_reading_response) requestReadPromise.set_value(); // Do not send a response but react to the shutdown to be polite. shutdownPromise.get_future().get(); - Shutdown(); + Shutdown(true); }}; TestThread timeoutThread{[&]() { @@ -315,7 +315,7 @@ BOOST_AUTO_TEST_CASE(http_send_retry) SendResponse(); - Shutdown(); + Shutdown(true); }}; boost::beast::http::request request{boost::beast::http::verb::post, "foo", 10}; From f3beafb46b1b3eadf3b6297e150538a498ed76df Mon Sep 17 00:00:00 2001 From: Johannes Schmidt Date: Wed, 22 Apr 2026 11:38:29 +0200 Subject: [PATCH 2/5] Fix a race-condition when perfdata writer is stuck in handshake The issue occurs when ::Connect in `EnsureConnected()` returns after `Disconnect()` has already set `m_Stopped` to true. By adding a check and throwing an exception before entering `async_handshake()` the behavior should now always be consistent. --- lib/perfdata/perfdatawriterconnection.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/perfdata/perfdatawriterconnection.cpp b/lib/perfdata/perfdatawriterconnection.cpp index 46000c28f..e8a56e899 100644 --- a/lib/perfdata/perfdatawriterconnection.cpp +++ b/lib/perfdata/perfdatawriterconnection.cpp @@ -62,7 +62,7 @@ bool PerfdataWriterConnection::IsStopped() const void PerfdataWriterConnection::Disconnect() { - if (m_Stopped.exchange(true, std::memory_order_relaxed)) { + if (m_Stopped.exchange(true)) { return; } @@ -133,6 +133,10 @@ void PerfdataWriterConnection::EnsureConnected(const boost::asio::yield_context& ::Connect(stream->lowest_layer(), m_Host, m_Port, yc); if constexpr (std::is_same_v, Shared::Ptr>) { + if (m_Stopped) { + BOOST_THROW_EXCEPTION(Stopped{}); + } + using type = boost::asio::ssl::stream_base::handshake_type; stream->next_layer().async_handshake(type::client, yc); From 7fc7e83358083bf93b9e8afce84949c924e45e88 Mon Sep 17 00:00:00 2001 From: Johannes Schmidt Date: Wed, 22 Apr 2026 11:42:31 +0200 Subject: [PATCH 3/5] Fix ineffective cancel() when stuck in perfdata writer handshake --- lib/perfdata/perfdatawriterconnection.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/perfdata/perfdatawriterconnection.cpp b/lib/perfdata/perfdatawriterconnection.cpp index e8a56e899..c8cc80e9d 100644 --- a/lib/perfdata/perfdatawriterconnection.cpp +++ b/lib/perfdata/perfdatawriterconnection.cpp @@ -76,9 +76,13 @@ void PerfdataWriterConnection::Disconnect() * completion. */ std::visit( - [](const auto& stream) { + [&](const auto& stream) { if (stream->lowest_layer().is_open()) { - stream->lowest_layer().cancel(); + if (m_Connected) { + stream->lowest_layer().cancel(); + } else { + stream->lowest_layer().close(); + } } }, m_Stream @@ -160,7 +164,7 @@ void PerfdataWriterConnection::EnsureConnected(const boost::asio::yield_context& void PerfdataWriterConnection::Disconnect(boost::asio::yield_context yc) { - if (!m_Connected.exchange(false, std::memory_order_relaxed)) { + if (!m_Connected.exchange(false)) { return; } From bdd4aae489620440de1f17b734f37a7de0aeb4ab Mon Sep 17 00:00:00 2001 From: Johannes Schmidt Date: Tue, 26 May 2026 12:04:26 +0200 Subject: [PATCH 4/5] Ignore errors for `close()` and TCP-`shutdown()` This makes the TCP-path also ignore all errors/exceptions, same as the TLS-path's `GracefulDisconnect()`. --- lib/perfdata/perfdatawriterconnection.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/perfdata/perfdatawriterconnection.cpp b/lib/perfdata/perfdatawriterconnection.cpp index c8cc80e9d..4b0838d0a 100644 --- a/lib/perfdata/perfdatawriterconnection.cpp +++ b/lib/perfdata/perfdatawriterconnection.cpp @@ -173,8 +173,9 @@ void PerfdataWriterConnection::Disconnect(boost::asio::yield_context yc) if constexpr (std::is_same_v, Shared::Ptr>) { stream->GracefulDisconnect(m_Strand, yc); } else { - stream->lowest_layer().shutdown(boost::asio::socket_base::shutdown_both); - stream->lowest_layer().close(); + boost::system::error_code ec; + stream->lowest_layer().shutdown(boost::asio::socket_base::shutdown_both, ec); + stream->lowest_layer().close(ec); } }, m_Stream From 9a4dd4689ca32f39d5424ffc061c445e1dfd3fb8 Mon Sep 17 00:00:00 2001 From: Johannes Schmidt Date: Wed, 29 Apr 2026 12:30:28 +0200 Subject: [PATCH 5/5] Fix `PerfdataWriterConnection` segfaults on non-X86 architectures The issue is that std::promise internally also used thread local storage, in a call to `std::call_once` in `std::promise::set_value()`. The theory is that since all paths in `Send()` run this `std::call_once` routine and from then on, then Coroutine function looks like a normal function, the compiler inlined `set_value()` and moved the common parts of it to a common location for all paths before the suspension point in WriteMessage(yc). When finally the coroutine is resumes, it is likely that that happens under a different thread, which still has `__once_callable` in `std::call_once` set as `nullptr`, leading to the segmentation fault. The fix is to not use std::promise across coroutine suspension points and instead reimplement the functionality we required from it in a small helper class `SyncResult` that does not require any thread local storage. --- lib/perfdata/perfdatawriterconnection.cpp | 8 +-- lib/perfdata/perfdatawriterconnection.hpp | 63 ++++++++++++++++++++--- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/lib/perfdata/perfdatawriterconnection.cpp b/lib/perfdata/perfdatawriterconnection.cpp index 4b0838d0a..8b50f061a 100644 --- a/lib/perfdata/perfdatawriterconnection.cpp +++ b/lib/perfdata/perfdatawriterconnection.cpp @@ -66,7 +66,7 @@ void PerfdataWriterConnection::Disconnect() return; } - std::promise promise; + SyncResult ret; IoEngine::SpawnCoroutine(m_Strand, [&](boost::asio::yield_context yc) { try { @@ -90,13 +90,13 @@ void PerfdataWriterConnection::Disconnect() m_ReconnectTimer.cancel(); Disconnect(std::move(yc)); - promise.set_value(); + ret.SetValue(); } catch (const std::exception& ex) { - promise.set_exception(std::current_exception()); + ret.SetException(std::current_exception()); } }); - promise.get_future().get(); + ret.Get(); } AsioTlsOrTcpStream PerfdataWriterConnection::MakeStream() const diff --git a/lib/perfdata/perfdatawriterconnection.hpp b/lib/perfdata/perfdatawriterconnection.hpp index 729878a29..9f91f037e 100644 --- a/lib/perfdata/perfdatawriterconnection.hpp +++ b/lib/perfdata/perfdatawriterconnection.hpp @@ -10,6 +10,7 @@ #include #include #include +#include namespace icinga { @@ -21,6 +22,56 @@ class PerfdataWriterConnection final : public Object static constexpr auto InitialRetryWait = 50ms; static constexpr auto FinalRetryWait = 32s; + template + class SyncResult + { + using ValueType = std::variant, bool, T>, std::exception_ptr>; + + public: + template>> + void SetValue(U&& v) + { + std::lock_guard lock(m_Mutex); + m_Value = std::forward(v); + m_Cv.notify_one(); + } + + template>> + void SetValue() + { + std::lock_guard lock(m_Mutex); + m_Value = true; + m_Cv.notify_one(); + } + + void SetException(std::exception_ptr ep) + { + std::lock_guard lock(m_Mutex); + m_Value = ValueType{ep}; + m_Cv.notify_one(); + } + + T Get() + { + std::unique_lock l(m_Mutex); + m_Cv.wait(l, [&] { return !std::holds_alternative(m_Value); }); + if (std::holds_alternative(m_Value)) { + std::rethrow_exception(std::get(m_Value)); + } + + if constexpr (std::is_void_v) { + return; + } else { + return std::move(std::get(m_Value)); + } + } + + private: + std::mutex m_Mutex; + std::condition_variable m_Cv; + ValueType m_Value; + }; + public: DECLARE_PTR_TYPEDEFS(PerfdataWriterConnection); @@ -66,7 +117,7 @@ public: } using RetType = decltype(WriteMessage(std::declval(), std::declval())); - std::promise promise; + SyncResult ret; IoEngine::SpawnCoroutine(m_Strand, [&](boost::asio::yield_context yc) { while (true) { @@ -75,16 +126,16 @@ public: if constexpr (std::is_void_v) { WriteMessage(std::forward(buf), yc); - promise.set_value(); + ret.SetValue(); } else { - promise.set_value(WriteMessage(std::forward(buf), yc)); + ret.SetValue(WriteMessage(std::forward(buf), yc)); } m_RetryTimeout = InitialRetryWait; return; } catch (const std::exception& ex) { if (m_Stopped) { - promise.set_exception(std::make_exception_ptr(Stopped{})); + ret.SetException(std::make_exception_ptr(Stopped{})); return; } @@ -98,14 +149,14 @@ public: try { BackoffWait(yc); } catch (const std::exception&) { - promise.set_exception(std::make_exception_ptr(Stopped{})); + ret.SetException(std::make_exception_ptr(Stopped{})); return; } } } }); - return promise.get_future().get(); + return ret.Get(); } void Disconnect();