From 4bbe5394f4309130115d73e8c3ae647d1515316c Mon Sep 17 00:00:00 2001
From: Robert Adam <dev@robert-adam.de>
Date: Fri, 3 Apr 2020 11:20:14 +0200
Subject: [PATCH] src/mumble/ClientUser.cpp: Narrow down the scope of
 QWriteLocker

In ClientUser::remove a QWriteLocker is created on
ClientUser::c_qrwlUsers in order to remove the respective user from
ClientUser::c_qmUsers.
However the lock is not released after that but it is instead held until
the function exits. This causes the lock to be held even when
AudioOutput::removeBuffer is being called. Apart from this being bad
practice, it can also lead to a deadlock (verified for the case in which
a ReadLock on ClientUser::c_qrwlUsers is being requested inside
AudioOutput::mix [after the lock on AudioOutput::qrwlOutputs has been
acquired]. This can happen if one calls e.g. ClientUser::get in that
function. In that case the deadlock can happen if audio is being
received and played while one or more users are disconnecting from the
server).
---
 src/mumble/ClientUser.cpp | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/src/mumble/ClientUser.cpp b/src/mumble/ClientUser.cpp
index 47ffda89e..c5b7141b1 100644
--- a/src/mumble/ClientUser.cpp
+++ b/src/mumble/ClientUser.cpp
@@ -80,21 +80,37 @@ ClientUser *ClientUser::match(const ClientUser *other, bool matchname) {
 }
 
 void ClientUser::remove(unsigned int uiSession) {
-	QWriteLocker lock(&c_qrwlUsers);
-	ClientUser *p = c_qmUsers.take(uiSession);
-	if (p) {
-		if (p->cChannel)
-			p->cChannel->removeUser(p);
+	ClientUser *p;
+	{
+		QWriteLocker lock(&c_qrwlUsers);
+		p = c_qmUsers.take(uiSession);
 
-		AudioOutputPtr ao = g.ao;
-		if (ao)
-			ao->removeBuffer(p);
+		if (p) {
+			if (p->cChannel)
+				p->cChannel->removeUser(p);
 
-		if (p->tsState != Settings::Passive) {
-			QWriteLocker writeLock(&c_qrwlTalking);
-			c_qlTalking.removeAll(p);
+			if (p->tsState != Settings::Passive) {
+				QWriteLocker writeLock(&c_qrwlTalking);
+				c_qlTalking.removeAll(p);
+			}
 		}
 	}
+
+	if (p) {
+		AudioOutputPtr ao = g.ao;
+		if (ao) {
+			// It is safe to call this function and to give the ClientUser pointer
+			// to it even though we don't hold the lock anymore as it will only take
+			// the pointer to use as the key in a HashMap lookup. At no point in the
+			// code triggered by this function call will the ClientUser pointer be
+			// dereferenced.
+			// Furthermore ClientUser objects are deleted in UserModel::removeUser which
+			// calls this very function before doing so. Thus the object shouldn't be
+			// deleted before this function returns anyways.
+			ao->removeBuffer(p);
+		}
+
+	}
 }
 
 void ClientUser::remove(ClientUser *p) {