From c3b942e0fc4777f9033f614b6b1f462c0f8c473e Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Sun, 6 Feb 2022 13:25:56 +0100
Subject: [PATCH 1/2] 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<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)
 {
@@ -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<void()> 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<std::function<void()>>::iterator it;
+    InterruptCallbacks::Token token;
     ~InterruptCallbackImpl() override
     {
-        _interruptCallbacks.lock()->erase(it);
+        auto interruptCallbacks(_interruptCallbacks.lock());
+        interruptCallbacks->callbacks.erase(token);
     }
 };
 
 std::unique_ptr<InterruptCallback> createInterruptCallback(std::function<void()> callback)
 {
     auto interruptCallbacks(_interruptCallbacks.lock());
-    interruptCallbacks->push_back(callback);
+    auto token = interruptCallbacks->nextToken++;
+    interruptCallbacks->callbacks.emplace(token, callback);
 
     auto res = std::make_unique<InterruptCallbackImpl>();
-    res->it = interruptCallbacks->end();
-    res->it--;
+    res->token = token;
 
     return std::unique_ptr<InterruptCallback>(res.release());
 }

From ddb6740e7da2ec0cc6ad71ac7e40f8513a1103c2 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Mon, 21 Feb 2022 15:43:43 +0100
Subject: [PATCH 2/2] triggerInterrupt: Refactor to use break

---
 src/libutil/util.cc | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/src/libutil/util.cc b/src/libutil/util.cc
index 03cbd7765..44463161f 100644
--- a/src/libutil/util.cc
+++ b/src/libutil/util.cc
@@ -1599,28 +1599,24 @@ void triggerInterrupt()
 
     {
         InterruptCallbacks::Token i = 0;
-        std::function<void()> callback;
-        do {
+        while (true) {
+            std::function<void()> callback;
             {
                 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 (lb == interruptCallbacks->callbacks.end())
+                    break;
+
+                callback = lb->second;
+                i = lb->first + 1;
             }
 
-            if (callback) {
-                try {
-                    callback();
-                } catch (...) {
-                    ignoreException();
-                }
+            try {
+                callback();
+            } catch (...) {
+                ignoreException();
             }
         }
-        while (callback);
     }
 }