From 85850d5a63ab942e9b6112e9131bf7e54e48c559 Mon Sep 17 00:00:00 2001
From: Robert Adam <dev@robert-adam.de>
Date: Sun, 22 Nov 2020 18:29:53 +0100
Subject: [PATCH] FIX(client): Crash due to race-condition in TalkingUI

The race condition was that the channel of a user could be modified
and before it gets updated in the TalkingUI as well, the talking state
of that user changes. Then inside the talkingStateChanged function the
code would assume that the TalkingUIUser is in the container representing
the current channel of that user but in fact in the TalkingUI this user
is still in its own channel, causing the user-search to return nullptr
which then yields a SegFault as soon as the returned pointer gets
dereferenced.
---
 src/mumble/TalkingUI.cpp | 27 ++++++++++++++-------------
 src/mumble/TalkingUI.h   |  4 +++-
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/src/mumble/TalkingUI.cpp b/src/mumble/TalkingUI.cpp
index 5331ccea8..e852939ab 100644
--- a/src/mumble/TalkingUI.cpp
+++ b/src/mumble/TalkingUI.cpp
@@ -360,7 +360,7 @@ void TalkingUI::addChannel(const Channel *channel) {
 	}
 }
 
-void TalkingUI::addUser(const ClientUser *user) {
+TalkingUIUser *TalkingUI::findOrAddUser(const ClientUser *user) {
 	// In a first step, it has to be made sure that the user's channel
 	// exists in this UI.
 	addChannel(user->cChannel);
@@ -395,11 +395,12 @@ void TalkingUI::addUser(const ClientUser *user) {
 		std::unique_ptr< TalkingUIContainer > &channelContainer =
 			m_containers[findContainer(user->cChannel->iId, ContainerType::CHANNEL)];
 		if (!channelContainer) {
-			qCritical("TalkingUI::addUser requesting unknown channel!");
-			return;
+			qCritical("TalkingUI::findOrAddUser requesting unknown channel!");
+			return nullptr;
 		}
 
 		std::unique_ptr< TalkingUIUser > userEntry = std::make_unique< TalkingUIUser >(*user);
+		TalkingUIUser *newUserEntry = userEntry.get();
 
 		// * 1000 as the setting is in seconds whereas the timer expects milliseconds
 		userEntry->setLifeTime(g.s.iTalkingUI_SilentUserLifeTime * 1000);
@@ -420,6 +421,10 @@ void TalkingUI::addUser(const ClientUser *user) {
 		channelContainer->addEntry(std::move(userEntry));
 
 		sortContainers();
+
+		return newUserEntry;
+	} else {
+		return oldUserEntry;
 	}
 }
 
@@ -602,15 +607,11 @@ void TalkingUI::on_talkingStateChanged() {
 		return;
 	}
 
-	addUser(user);
+	TalkingUIUser *userEntry = findOrAddUser(user);
 
-	// addUser puts the user in its current channel, so we can fetch that and know that it'll contain the user
-	std::unique_ptr< TalkingUIContainer > &channel =
-		m_containers[findContainer(user->cChannel->iId, ContainerType::CHANNEL)];
-
-	TalkingUIUser *userEntry = static_cast< TalkingUIUser * >(channel->get(user->uiSession, EntryType::USER));
-
-	userEntry->setTalkingState(user->tsState);
+	if (userEntry) {
+		userEntry->setTalkingState(user->tsState);
+	}
 
 	updateUI();
 }
@@ -673,7 +674,7 @@ void TalkingUI::on_serverSynchronized() {
 		// can't count on it to change its talking state right after it has connected to
 		// a server, we have to add it manually.
 		ClientUser *self = ClientUser::get(g.uiSession);
-		addUser(self);
+		findOrAddUser(self);
 	}
 }
 
@@ -763,7 +764,7 @@ void TalkingUI::on_settingsChanged() {
 	} else {
 		if (self && g.s.bTalkingUI_LocalUserStaysVisible) {
 			// Add the local user as it is requested to be displayed
-			addUser(self);
+			findOrAddUser(self);
 		}
 	}
 
diff --git a/src/mumble/TalkingUI.h b/src/mumble/TalkingUI.h
index 21414567e..8ceeca435 100644
--- a/src/mumble/TalkingUI.h
+++ b/src/mumble/TalkingUI.h
@@ -73,7 +73,9 @@ private:
 	/// Adds an UI entry for the given User, if none exists yet.
 	///
 	/// @param channel A pointer to the user that shall be added
-	void addUser(const ClientUser *user);
+	/// @returns The pointer to the respective user entry in the TalkingUI
+	/// (may be nullptr in case of an error)
+	TalkingUIUser *findOrAddUser(const ClientUser *user);
 	/// Moves the given user into the given channel
 	///
 	/// @paam userSession The session ID of the user