Skip to content

Commit 4b771e8

Browse files
smessmersmessmer
committed
Before, the SignalCatcherRegistry just used the std::atomic pointer to remove SignalCatcherImpls, which could get the wrong SignalCatcherImpl if a SignalCatcher registered multiple SignalCatcherImpls (since they all had the same std::atomic pointer). This wasn't an issue in practice since all SignalCatcherImpls are deregistered at the same time, so it got all of them, but it still wasn't how the code was intended to work. Now, SignalCatcherRegistry uses the SignalCatcherImpl pointer, which is the actual intention.
1 parent 6f07ebe commit 4b771e8

1 file changed

Lines changed: 23 additions & 16 deletions

File tree

‎src/cpp-utils/process/SignalCatcher.cpp‎

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -129,13 +129,13 @@ class SignalHandlerRunningRAII final {
129129

130130
class SignalCatcherRegistry final {
131131
public:
132-
void add(int signal, std::atomic<bool>* signal_occurred_flag) {
132+
void add(int signal, details::SignalCatcherImpl* signal_occurred_flag) {
133133
_catchers.write([&] (auto& catchers) {
134134
catchers.emplace_back(signal, signal_occurred_flag);
135135
});
136136
}
137137

138-
void remove(std::atomic<bool>* catcher) {
138+
void remove(details::SignalCatcherImpl* catcher) {
139139
_catchers.write([&] (auto& catchers) {
140140
auto found = std::find_if(catchers.rbegin(), catchers.rend(), [catcher] (const auto& entry) {return entry.second == catcher;});
141141
ASSERT(found != catchers.rend(), "Signal handler not found");
@@ -147,7 +147,7 @@ class SignalCatcherRegistry final {
147147
ASSERT(0 == _catchers.read([] (auto& catchers) {return catchers.size();}), "Leftover signal catchers that weren't destroyed");
148148
}
149149

150-
std::atomic<bool>* find(int signal) {
150+
details::SignalCatcherImpl* find(int signal) {
151151
// this is called in a signal handler and must be mutex-free.
152152
return _catchers.read([&](auto& catchers) {
153153
auto found = std::find_if(catchers.rbegin(), catchers.rend(), [signal](const auto& entry) {return entry.first == signal; });
@@ -165,23 +165,14 @@ class SignalCatcherRegistry final {
165165
SignalCatcherRegistry() = default;
166166

167167
// using LeftRight datastructure because we need mutex-free reads. Signal handlers can't use mutexes.
168-
LeftRight<vector<pair<int, std::atomic<bool>*>>> _catchers;
168+
LeftRight<vector<pair<int, details::SignalCatcherImpl*>>> _catchers;
169169

170170
DISALLOW_COPY_AND_ASSIGN(SignalCatcherRegistry);
171171
};
172172

173-
void got_signal(int signal) {
174-
#if defined(_MSC_VER)
175-
// Only needed on Windows, Linux does this by default. See comment on SignalHandlerRunningRAII class.
176-
SignalHandlerRunningRAII disable_signal_processing_while_handler_running_and_reset_handler_afterwards(signal);
177-
#endif
178-
std::atomic<bool>* catcher = SignalCatcherRegistry::singleton().find(signal);
179-
*catcher = true;
180-
}
181-
182173
class SignalCatcherRegisterer final {
183174
public:
184-
SignalCatcherRegisterer(int signal, std::atomic<bool>* catcher)
175+
SignalCatcherRegisterer(int signal, details::SignalCatcherImpl* catcher)
185176
: _catcher(catcher) {
186177
SignalCatcherRegistry::singleton().add(signal, _catcher);
187178
}
@@ -191,7 +182,7 @@ class SignalCatcherRegisterer final {
191182
}
192183

193184
private:
194-
std::atomic<bool>* _catcher;
185+
details::SignalCatcherImpl* _catcher;
195186

196187
DISALLOW_COPY_AND_ASSIGN(SignalCatcherRegisterer);
197188
};
@@ -203,7 +194,8 @@ namespace details {
203194
class SignalCatcherImpl final {
204195
public:
205196
SignalCatcherImpl(int signal, std::atomic<bool>* signal_occurred_flag)
206-
: _registerer(signal, signal_occurred_flag)
197+
: _signal_occurred_flag(signal_occurred_flag)
198+
, _registerer(signal, this)
207199
, _handler(signal) {
208200
// note: the order of the members ensures that:
209201
// - when registering the signal handler fails, the registerer will be destroyed, unregistering the signal_occurred_flag,
@@ -212,7 +204,12 @@ class SignalCatcherImpl final {
212204
// Allow only the set of signals that is supported on all platforms, see for Windows: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/signal?view=vs-2017
213205
ASSERT(signal == SIGABRT || signal == SIGFPE || signal == SIGILL || signal == SIGINT || signal == SIGSEGV || signal == SIGTERM, "Unknown signal");
214206
}
207+
208+
void setSignalOccurred() {
209+
*_signal_occurred_flag = true;
210+
}
215211
private:
212+
std::atomic<bool>* _signal_occurred_flag;
216213
SignalCatcherRegisterer _registerer;
217214
SignalHandlerRAII _handler;
218215

@@ -221,6 +218,16 @@ class SignalCatcherImpl final {
221218

222219
}
223220

221+
namespace {
222+
void got_signal(int signal) {
223+
#if defined(_MSC_VER)
224+
// Only needed on Windows, Linux does this by default. See comment on SignalHandlerRunningRAII class.
225+
SignalHandlerRunningRAII disable_signal_processing_while_handler_running_and_reset_handler_afterwards(signal);
226+
#endif
227+
SignalCatcherRegistry::singleton().find(signal)->setSignalOccurred();
228+
}
229+
}
230+
224231
SignalCatcher::SignalCatcher(std::initializer_list<int> signals)
225232
: _signal_occurred(false)
226233
, _impls() {

0 commit comments

Comments
 (0)