Don't hold interruptCallbacks lock during interrupt handling

This changes the representation of the interrupt callback list to
be safe to use during interrupt handling.

Holding a lock while executing arbitrary functions is something to
avoid in general, because of the risk of deadlock.

Such a deadlock occurs in https://github.com/NixOS/nix/issues/3294
where ~CurlDownloader tries to deregister its interrupt callback.

This happens during what seems to be a triggerInterrupt() by the
daemon connection's MonitorFdHup thread. This bit I can not confirm
based on the stack trace though; it's based on reading the code,
so no absolute certainty, but a smoking gun nonetheless.
This commit is contained in:
Robert Hensing 2022-02-06 13:25:56 +01:00
parent 4369771870
commit c3b942e0fc

View file

@ -1561,7 +1561,22 @@ std::pair<unsigned short, unsigned short> getWindowSize()
} }
static Sync<std::list<std::function<void()>>> _interruptCallbacks; /* We keep track of interrupt callbacks using integer tokens, so we can iterate
safely without having to lock the data structure while executing arbitrary
functions.
*/
struct InterruptCallbacks {
typedef int64_t Token;
/* We use unique tokens so that we can't accidentally delete the wrong
handler because of an erroneous double delete. */
Token nextToken = 0;
/* Used as a list, see InterruptCallbacks comment. */
std::map<Token, std::function<void()>> callbacks;
};
static Sync<InterruptCallbacks> _interruptCallbacks;
static void signalHandlerThread(sigset_t set) static void signalHandlerThread(sigset_t set)
{ {
@ -1582,9 +1597,22 @@ void triggerInterrupt()
{ {
_isInterrupted = true; _isInterrupted = true;
{
InterruptCallbacks::Token i = 0;
std::function<void()> callback;
do {
{ {
auto interruptCallbacks(_interruptCallbacks.lock()); auto interruptCallbacks(_interruptCallbacks.lock());
for (auto & callback : *interruptCallbacks) { auto lb = interruptCallbacks->callbacks.lower_bound(i);
if (lb != interruptCallbacks->callbacks.end()) {
callback = lb->second;
i = lb->first + 1;
} else {
callback = nullptr;
}
}
if (callback) {
try { try {
callback(); callback();
} catch (...) { } catch (...) {
@ -1592,6 +1620,8 @@ void triggerInterrupt()
} }
} }
} }
while (callback);
}
} }
static sigset_t savedSignalMask; static sigset_t savedSignalMask;
@ -1694,21 +1724,22 @@ void restoreProcessContext(bool restoreMounts)
/* RAII helper to automatically deregister a callback. */ /* RAII helper to automatically deregister a callback. */
struct InterruptCallbackImpl : InterruptCallback struct InterruptCallbackImpl : InterruptCallback
{ {
std::list<std::function<void()>>::iterator it; InterruptCallbacks::Token token;
~InterruptCallbackImpl() override ~InterruptCallbackImpl() override
{ {
_interruptCallbacks.lock()->erase(it); auto interruptCallbacks(_interruptCallbacks.lock());
interruptCallbacks->callbacks.erase(token);
} }
}; };
std::unique_ptr<InterruptCallback> createInterruptCallback(std::function<void()> callback) std::unique_ptr<InterruptCallback> createInterruptCallback(std::function<void()> callback)
{ {
auto interruptCallbacks(_interruptCallbacks.lock()); auto interruptCallbacks(_interruptCallbacks.lock());
interruptCallbacks->push_back(callback); auto token = interruptCallbacks->nextToken++;
interruptCallbacks->callbacks.emplace(token, callback);
auto res = std::make_unique<InterruptCallbackImpl>(); auto res = std::make_unique<InterruptCallbackImpl>();
res->it = interruptCallbacks->end(); res->token = token;
res->it--;
return std::unique_ptr<InterruptCallback>(res.release()); return std::unique_ptr<InterruptCallback>(res.release());
} }