Merge pull request #330 from jolavillette/fix/rsevents-unregister-inflight-uaf

RsEventsService: make unregisterEventsHandler() a barrier (fix shutdown UAF)

Author: csoler

src/services/rseventsservice.cc

@@ -166,18 +166,33 @@ std::error_condition RsEventsService::registerEventsHandler(
 std::error_condition RsEventsService::unregisterEventsHandler(
         RsEventsHandlerId_t hId )
 {
-	RS_STACK_MUTEX(mHandlerMapMtx);
+	std::error_condition retval = RsEventsErrorNum::INVALID_HANDLER_ID;
 
-	for(uint32_t i=0; i<mHandlerMaps.size(); ++i)
 	{
-		auto it = mHandlerMaps[i].find(hId);
-		if(it != mHandlerMaps[i].end())
+		RS_STACK_MUTEX(mHandlerMapMtx);
+
+		for(uint32_t i=0; i<mHandlerMaps.size(); ++i)
 		{
-			mHandlerMaps[i].erase(it);
-			return std::error_condition();
+			auto it = mHandlerMaps[i].find(hId);
+			if(it != mHandlerMaps[i].end())
+			{
+				mHandlerMaps[i].erase(it);
+				retval = std::error_condition();
+				break;
+			}
 		}
 	}
-	return RsEventsErrorNum::INVALID_HANDLER_ID;
+
+	/* At this point no *future* dispatch can pick up this handler. But an
+	 * ongoing handleEvent() may still hold a copy of it in flight (callbacks are
+	 * run outside mHandlerMapMtx). Fence on mDispatchMtx so that, once we return,
+	 * the handler is guaranteed not to be executing either: callers that
+	 * unregister from their destructor (most GUI widgets) can then be destroyed
+	 * safely. Recursive mutex => when called from within a callback on the
+	 * dispatching thread this is a cheap no-op instead of a self-deadlock. */
+	{ std::lock_guard<std::recursive_mutex> dispatchFence(mDispatchMtx); }
+
+	return retval;
 }
 
 void RsEventsService::threadTick()
@@ -223,12 +238,21 @@ void RsEventsService::handleEvent(std::shared_ptr<const RsEvent> event)
 		return;
 	}
 
+	/* Hold mDispatchMtx across the whole dispatch so unregisterEventsHandler()
+	 * can fence on it and guarantee a handler is not running once it returns
+	 * (see mDispatchMtx doc). Recursive: a callback re-entering on this same
+	 * thread (self-unregister or synchronous sendEvent) does not deadlock.
+	 * Safe against GUI teardown because handlers only post asynchronously (Qt
+	 * QueuedConnection) and never block waiting on the thread that unregisters,
+	 * so there is no lock-order cycle. */
+	std::lock_guard<std::recursive_mutex> dispatchLock(mDispatchMtx);
+
 	std::list<std::function<void(std::shared_ptr<const RsEvent>)> > callbacks;
 	{
 		RS_STACK_MUTEX(mHandlerMapMtx);
-		/* It is important to NOT call the callback under mutex protection to
-		 * allow callbacks to send other events or unregister themselves,
-		 * which would otherwise deadlock. */
+		/* It is important to NOT call the callback under mHandlerMapMtx
+		 * protection to allow callbacks to send other events or unregister
+		 * themselves, which would otherwise deadlock. */
 
 		// Call all clients that registered a callback for this event type
 		for(auto& cbit: mHandlerMaps[static_cast<uint32_t>(event->mType)])

src/services/rseventsservice.h

@@ -25,6 +25,7 @@
 #include <cstdint>
 #include <deque>
 #include <array>
+#include <mutex>
 
 #include "retroshare/rsevents.h"
 #include "util/rsthreads.h"
@@ -69,6 +70,20 @@ class RsEventsService :
 	std::error_condition isEventInvalid(std::shared_ptr<const RsEvent> event);
 
 	RsMutex mHandlerMapMtx;
+
+	/** Held by handleEvent() for the whole duration of the callbacks dispatch
+	 * loop, so that unregisterEventsHandler() can act as a barrier: after it
+	 * returns, the removed handler is guaranteed to be neither running nor about
+	 * to start. Without this, unregister only removes the handler from the map,
+	 * but handleEvent() runs callbacks on a *copy* taken outside mHandlerMapMtx
+	 * (on purpose, to let callbacks re-enter), so a callback whose owner is
+	 * being destroyed on another thread could still fire against a dangling
+	 * object -> use-after-free (typically a SIGSEGV in qobject_cast<QThread*>
+	 * inside RsQThreadUtils::postToObject at shutdown). Recursive so that a
+	 * callback re-entering (self-unregister or synchronous sendEvent) on the
+	 * dispatching thread does not deadlock. */
+	std::recursive_mutex mDispatchMtx;
+
 	RsEventsHandlerId_t mLastHandlerId;
 
 	/** Storage for event handlers, keep 10 extra types for plugins that might