From 3613ce59ecb3c520dd65d1b9ea842afac02167e9 Mon Sep 17 00:00:00 2001
From: Julian Oes <julian@oes.ch>
Date: Mon, 14 May 2018 14:21:05 -0400
Subject: [PATCH 1/4] timeout_handler: add test that causes segfault

It turns out that we trigger a segfault if we remove the next timer in
the list during a callback.

This test adds a segfault for the unit test that needs to be fixed.
---
 core/timeout_handler_test.cpp | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/core/timeout_handler_test.cpp b/core/timeout_handler_test.cpp
index bd31f3aa76..8571109b19 100644
--- a/core/timeout_handler_test.cpp
+++ b/core/timeout_handler_test.cpp
@@ -148,3 +148,23 @@ TEST(TimeoutHandler, TimeoutRemovedDuringCallback)
     th.run_once();
     EXPECT_TRUE(timeout_happened);
 }
+
+TEST(TimeoutHandler, NextTimeoutRemovedDuringCallback)
+{
+    Time time {};
+    TimeoutHandler th(time);
+
+    void *cookie1 = nullptr;
+    void *cookie2 = nullptr;
+
+    th.add([&th, &cookie2]() {
+        // This is evil but can potentially happen. We remove the other timer while
+        // being called. This triggers that the iterator is invalid and causes a segfault.
+        th.remove(cookie2);
+    }, 0.5, &cookie1);
+
+    th.add([]() {}, 0.5, &cookie2);
+
+    time.sleep_for(std::chrono::milliseconds(1000));
+    th.run_once();
+}

From 1090deea49ecc349f8de6f1d07e2ee8878fb5005 Mon Sep 17 00:00:00 2001
From: Julian Oes <julian@oes.ch>
Date: Mon, 14 May 2018 14:08:04 -0400
Subject: [PATCH 2/4] core: drop out of the loop after erase

We should not continue the loop once some entry has been removed from
the map.
---
 core/timeout_handler.cpp | 8 ++++++++
 core/timeout_handler.h   | 1 +
 2 files changed, 9 insertions(+)

diff --git a/core/timeout_handler.cpp b/core/timeout_handler.cpp
index 4588913d1b..7f8740b295 100644
--- a/core/timeout_handler.cpp
+++ b/core/timeout_handler.cpp
@@ -48,6 +48,7 @@ void TimeoutHandler::remove(const void *cookie)
     auto it = _timeouts.find(const_cast<void *>(cookie));
     if (it != _timeouts.end()) {
         _timeouts.erase(const_cast<void *>(cookie));
+        _iterator_invalidated = true;
     }
 }
 
@@ -80,6 +81,13 @@ void TimeoutHandler::run_once()
         } else {
             ++it;
         }
+
+        // We leave the loop if anyone has messed with this while we called the callback.
+        // FIXME: there should be a nicer way to do this.
+        if (_iterator_invalidated) {
+            _iterator_invalidated = false;
+            break;
+        }
     }
     _timeouts_mutex.unlock();
 }
diff --git a/core/timeout_handler.h b/core/timeout_handler.h
index a43e3e8783..66008f6f5a 100644
--- a/core/timeout_handler.h
+++ b/core/timeout_handler.h
@@ -35,6 +35,7 @@ class TimeoutHandler
 
     std::map<void *, std::shared_ptr<Timeout>> _timeouts {};
     std::mutex _timeouts_mutex {};
+    bool _iterator_invalidated {false};
 
     Time &_time;
 };

From f33ba8fb932259a792b3e0dc9e5a7b08c4f220d1 Mon Sep 17 00:00:00 2001
From: Julian Oes <julian@oes.ch>
Date: Mon, 14 May 2018 14:24:25 -0400
Subject: [PATCH 3/4] timeout_handler: start over instead of breaking

When the map has been invalidated we should not bail out completely
because some other timers might be due in this iteration. Instead we
should start over and work  our way through them again.
---
 core/timeout_handler.cpp | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/core/timeout_handler.cpp b/core/timeout_handler.cpp
index 7f8740b295..2e78648269 100644
--- a/core/timeout_handler.cpp
+++ b/core/timeout_handler.cpp
@@ -82,11 +82,10 @@ void TimeoutHandler::run_once()
             ++it;
         }
 
-        // We leave the loop if anyone has messed with this while we called the callback.
-        // FIXME: there should be a nicer way to do this.
+        // We start over if anyone has messed with this while we called the callback.
         if (_iterator_invalidated) {
             _iterator_invalidated = false;
-            break;
+            it = _timeouts.begin();
         }
     }
     _timeouts_mutex.unlock();

From 1b65e4b0c2bcaa80ac7809a55c5df249c703eab3 Mon Sep 17 00:00:00 2001
From: Julian Oes <julian@oes.ch>
Date: Mon, 14 May 2018 14:26:49 -0400
Subject: [PATCH 4/4] timeout_handler: fix comment of test

---
 core/timeout_handler_test.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/core/timeout_handler_test.cpp b/core/timeout_handler_test.cpp
index 8571109b19..82a93e0786 100644
--- a/core/timeout_handler_test.cpp
+++ b/core/timeout_handler_test.cpp
@@ -159,7 +159,8 @@ TEST(TimeoutHandler, NextTimeoutRemovedDuringCallback)
 
     th.add([&th, &cookie2]() {
         // This is evil but can potentially happen. We remove the other timer while
-        // being called. This triggers that the iterator is invalid and causes a segfault.
+        // being called. This triggers that the iterator is invalid and causes a segfault
+        // if not handled properly.
         th.remove(cookie2);
     }, 0.5, &cookie1);