Skip to content

Commit c9e0d40

Browse files
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 storag.
1 parent ad8563b commit c9e0d40

2 files changed

Lines changed: 67 additions & 10 deletions

File tree

lib/perfdata/perfdatawriterconnection.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ void PerfdataWriterConnection::Disconnect()
6666
return;
6767
}
6868

69-
std::promise<void> promise;
69+
SyncResult<void> ret;
7070

7171
IoEngine::SpawnCoroutine(m_Strand, [&](boost::asio::yield_context yc) {
7272
try {
@@ -90,13 +90,13 @@ void PerfdataWriterConnection::Disconnect()
9090
m_ReconnectTimer.cancel();
9191

9292
Disconnect(std::move(yc));
93-
promise.set_value();
93+
ret.SetValue();
9494
} catch (const std::exception& ex) {
95-
promise.set_exception(std::current_exception());
95+
ret.SetException(std::current_exception());
9696
}
9797
});
9898

99-
promise.get_future().get();
99+
ret.Get();
100100
}
101101

102102
AsioTlsOrTcpStream PerfdataWriterConnection::MakeStream() const

lib/perfdata/perfdatawriterconnection.hpp

Lines changed: 63 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include <boost/beast/http/message.hpp>
1111
#include <boost/beast/http/string_body.hpp>
1212
#include <future>
13+
#include <utility>
1314

1415
namespace icinga {
1516

@@ -21,6 +22,62 @@ class PerfdataWriterConnection final : public Object
2122
static constexpr auto InitialRetryWait = 50ms;
2223
static constexpr auto FinalRetryWait = 32s;
2324

25+
template<typename T>
26+
class SyncResult
27+
{
28+
using ValueType = std::variant<std::monostate, std::conditional_t<std::is_void_v<T>, bool, T>, std::exception_ptr>;
29+
30+
public:
31+
template<typename U, typename V = T, typename = std::enable_if_t<!std::is_void_v<V>>>
32+
void SetValue(U&& v)
33+
{
34+
{
35+
std::lock_guard lock(m_Mutex);
36+
m_Value = std::forward<U>(v);
37+
}
38+
m_Cv.notify_one();
39+
}
40+
41+
template<typename V = T, typename = std::enable_if_t<std::is_void_v<V>>>
42+
void SetValue()
43+
{
44+
{
45+
std::lock_guard lock(m_Mutex);
46+
m_Value = true;
47+
}
48+
m_Cv.notify_one();
49+
}
50+
51+
void SetException(std::exception_ptr ep)
52+
{
53+
{
54+
std::lock_guard lock(m_Mutex);
55+
m_Value = ValueType{ep};
56+
}
57+
m_Cv.notify_one();
58+
}
59+
60+
T Get()
61+
{
62+
std::unique_lock l(m_Mutex);
63+
m_Cv.wait(l, [&] { return !std::holds_alternative<std::monostate>(m_Value); });
64+
if (std::holds_alternative<std::exception_ptr>(m_Value)) {
65+
std::rethrow_exception(std::get<std::exception_ptr>(m_Value));
66+
}
67+
68+
if constexpr (std::is_void_v<T>) {
69+
return;
70+
} else {
71+
return std::move(std::get<T>(m_Value));
72+
}
73+
}
74+
75+
private:
76+
std::mutex m_Mutex;
77+
std::condition_variable m_Cv;
78+
ValueType m_Value;
79+
};
80+
2481
public:
2582
DECLARE_PTR_TYPEDEFS(PerfdataWriterConnection);
2683

@@ -66,7 +123,7 @@ class PerfdataWriterConnection final : public Object
66123
}
67124

68125
using RetType = decltype(WriteMessage(std::declval<Buffer>(), std::declval<boost::asio::yield_context>()));
69-
std::promise<RetType> promise;
126+
SyncResult<RetType> ret;
70127

71128
IoEngine::SpawnCoroutine(m_Strand, [&](boost::asio::yield_context yc) {
72129
while (true) {
@@ -75,16 +132,16 @@ class PerfdataWriterConnection final : public Object
75132

76133
if constexpr (std::is_void_v<RetType>) {
77134
WriteMessage(std::forward<Buffer>(buf), yc);
78-
promise.set_value();
135+
ret.SetValue();
79136
} else {
80-
promise.set_value(WriteMessage(std::forward<Buffer>(buf), yc));
137+
ret.SetValue(WriteMessage(std::forward<Buffer>(buf), yc));
81138
}
82139

83140
m_RetryTimeout = InitialRetryWait;
84141
return;
85142
} catch (const std::exception& ex) {
86143
if (m_Stopped) {
87-
promise.set_exception(std::make_exception_ptr(Stopped{}));
144+
ret.SetException(std::make_exception_ptr(Stopped{}));
88145
return;
89146
}
90147

@@ -98,14 +155,14 @@ class PerfdataWriterConnection final : public Object
98155
try {
99156
BackoffWait(yc);
100157
} catch (const std::exception&) {
101-
promise.set_exception(std::make_exception_ptr(Stopped{}));
158+
ret.SetException(std::make_exception_ptr(Stopped{}));
102159
return;
103160
}
104161
}
105162
}
106163
});
107164

108-
return promise.get_future().get();
165+
return ret.Get();
109166
}
110167

111168
void Disconnect();

0 commit comments

Comments
 (0)