From c3b942e0fc4777f9033f614b6b1f462c0f8c473e Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 6 Feb 2022 13:25:56 +0100 Subject: [PATCH] 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. --- src/libutil/util.cc | 55 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 12 deletions(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index cd359cfee..03cbd7765 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -1561,7 +1561,22 @@ std::pair getWindowSize() } -static Sync>> _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> callbacks; +}; + +static Sync _interruptCallbacks; static void signalHandlerThread(sigset_t set) { @@ -1583,14 +1598,29 @@ void triggerInterrupt() _isInterrupted = true; { - auto interruptCallbacks(_interruptCallbacks.lock()); - for (auto & callback : *interruptCallbacks) { - try { - callback(); - } catch (...) { - ignoreException(); + InterruptCallbacks::Token i = 0; + std::function callback; + do { + { + auto interruptCallbacks(_interruptCallbacks.lock()); + 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 { + callback(); + } catch (...) { + ignoreException(); + } } } + while (callback); } } @@ -1694,21 +1724,22 @@ void restoreProcessContext(bool restoreMounts) /* RAII helper to automatically deregister a callback. */ struct InterruptCallbackImpl : InterruptCallback { - std::list>::iterator it; + InterruptCallbacks::Token token; ~InterruptCallbackImpl() override { - _interruptCallbacks.lock()->erase(it); + auto interruptCallbacks(_interruptCallbacks.lock()); + interruptCallbacks->callbacks.erase(token); } }; std::unique_ptr createInterruptCallback(std::function callback) { auto interruptCallbacks(_interruptCallbacks.lock()); - interruptCallbacks->push_back(callback); + auto token = interruptCallbacks->nextToken++; + interruptCallbacks->callbacks.emplace(token, callback); auto res = std::make_unique(); - res->it = interruptCallbacks->end(); - res->it--; + res->token = token; return std::unique_ptr(res.release()); }