From a58aa21934250ac0c7272975e8c78e2d157068be Mon Sep 17 00:00:00 2001
From: Dawid Drozd <drozddawid@gmail.com>
Date: Thu, 26 Mar 2020 11:53:05 +0100
Subject: [PATCH] Fix implicit move that could lead to UB

In some scenarios we could end up with UB. I added simple example in test case where we add class
instance to vector and as we know it may move its elements when resizing.

Maybe we could allow to move and just unlisten previous listener but this would be very annoying as
user needs to remember what would happen in every case. It is better to let user decide and force
him to think about it.
---
 lib/src/dexode/eventbus/Listener.hpp          | 51 +++++++-------
 .../dexode/eventbus/test/SuiteListener.cpp    | 70 ++++++++++++++++++-
 2 files changed, 95 insertions(+), 26 deletions(-)

diff --git a/lib/src/dexode/eventbus/Listener.hpp b/lib/src/dexode/eventbus/Listener.hpp
index ac646cf..0fb92d7 100644
--- a/lib/src/dexode/eventbus/Listener.hpp
+++ b/lib/src/dexode/eventbus/Listener.hpp
@@ -29,12 +29,9 @@ public:
 	}
 
 	Listener(const Listener& other) = delete;
-
-	Listener(Listener&& other) noexcept
-		: _id(other._id) // we don't have to reset listener ID as _bus is moved and we won't call
-						 // unlistenAll
-		, _bus(std::move(other._bus))
-	{}
+	// To see why move is disabled search for tag: FORBID_MOVE_LISTENER in tests
+	// Long story short, what if we capture 'this' in lambda during registering listener in ctor
+	Listener(Listener&& other) = delete;
 
 	~Listener()
 	{
@@ -45,24 +42,8 @@ public:
 	}
 
 	Listener& operator=(const Listener& other) = delete;
-
-	Listener& operator=(Listener&& other) noexcept
-	{
-		if(this == &other)
-		{
-			return *this;
-		}
-
-		if(_bus != nullptr)
-		{
-			unlistenAll(); // remove previous
-		}
-		// we don't have reset listener ID as bus is moved and we won't call unlistenAll
-		_id = other._id;
-		_bus = std::move(other._bus);
-
-		return *this;
-	}
+	// To see why move is disabled search for tag: FORBID_MOVE_LISTENER in tests
+	Listener& operator=(Listener&& other) = delete;
 
 	template <class Event, typename _ = void>
 	constexpr void listen(std::function<void(const Event&)>&& callback)
@@ -127,6 +108,28 @@ public:
 		internal::ListenerAttorney<Bus>::unlisten(*_bus, _id, internal::event_id<Event>());
 	}
 
+	// We want more explicit move so user knows what is going on
+	void transfer(Listener&& from)
+	{
+		if(this == &from)
+		{
+			throw std::runtime_error("Self transfer not allowed");
+		}
+
+		if(_bus != nullptr)
+		{
+			unlistenAll(); // remove previous
+		}
+		// we don't have to reset listener ID as bus is moved and we won't call unlistenAll
+		_id = from._id;
+		_bus = std::move(from._bus);
+	}
+
+	const std::shared_ptr<Bus>& getBus() const
+	{
+		return _bus;
+	}
+
 private:
 	std::uint32_t _id = 0;
 	std::shared_ptr<Bus> _bus = nullptr;
diff --git a/test/unit/src/dexode/eventbus/test/SuiteListener.cpp b/test/unit/src/dexode/eventbus/test/SuiteListener.cpp
index 973ba17..deb7afa 100644
--- a/test/unit/src/dexode/eventbus/test/SuiteListener.cpp
+++ b/test/unit/src/dexode/eventbus/test/SuiteListener.cpp
@@ -1,3 +1,5 @@
+#include <vector>
+
 #include <catch2/catch.hpp>
 
 #include "dexode/EventBus.hpp"
@@ -46,7 +48,7 @@ TEST_CASE("Should unlisten all events When listener instance is overriden", "[Ev
 	REQUIRE(bus.process() == 1);
 	REQUIRE(callCount == 1);
 
-	listener = Listener{};
+	listener.transfer(Listener{});
 
 	bus.postpone(event::Value{2});
 	REQUIRE(bus.process() == 1);
@@ -90,7 +92,7 @@ TEST_CASE("Should keep listeners When listener is moved", "[EventBus][Listener]"
 		REQUIRE(bus->process() == 1);
 		REQUIRE(callCount == 1);
 
-		transferOne = std::move(listener);
+		transferOne.transfer(std::move(listener));
 	}
 
 	bus->postpone(event::Value{3});
@@ -231,4 +233,68 @@ TEST_CASE("Should compile", "[EventBus][Listener]")
 	}
 }
 
+class TestClazz
+{
+public:
+	static int counter;
+
+	TestClazz(int id, const std::shared_ptr<EventBus>& bus)
+		: _id{id}
+		, _listener{bus}
+	{
+		registerListener();
+	}
+
+	TestClazz(TestClazz&& other)
+		: _id{other._id}
+		, _listener{other._listener.getBus()}
+	{
+		// We need to register again
+		registerListener();
+	}
+
+	~TestClazz()
+	{
+		_id = 0;
+	}
+
+private:
+	int _id = 0;
+	EventBus::Listener _listener;
+
+	void registerListener()
+	{
+		_listener.listen([this](const event::Value& event) {
+			if(_id == 1)
+			{
+				++counter;
+			}
+		});
+	}
+};
+
+int TestClazz::counter = 0;
+
+TEST_CASE("Should not allow for mistake with move ctor", "[EventBus][Listener]")
+{
+	/**
+	 * Test case TAG: FORBID_MOVE_LISTENER
+	 *
+	 * This case is little bit complicated.
+	 * We can't move EventBus::Listener as it capture 'this' in ctor so whenever we would use it it
+	 * would lead to UB.
+	 */
+	std::shared_ptr<EventBus> bus = std::make_shared<EventBus>();
+
+	std::vector<TestClazz> vector;
+	vector.emplace_back(1, bus);
+	vector.emplace_back(2, bus);
+	vector.emplace_back(3, bus);
+
+	bus->postpone(event::Value{100});
+	bus->process();
+
+	REQUIRE(TestClazz::counter == 1);
+}
+
 } // namespace dexode::eventbus::test