FIX(server): Missing lock in Ice-call setACL

The implementation for the Ice setACL function did not take a write lock
even though it writes to objects that are owned by the main thread
(thereby violating the synchronization scheme in the server code).

Due to it deleting Group objects this is strongly suspected to have
caused a Segfault crash of the server when the voice thread would check
whether a user has permission in a channel while the ACL for that is not
cached. The Segfault then occurs in Group::isMember when dereferencing
the Group pointer (which got deleted in the Ice function).

1 files changed, 39 insertions, 35 deletions

diff --git a/src/murmur/MurmurIce.cpp b/src/murmur/MurmurIce.cpp
index 7f03b3ef5..464f75efd 100644
--- a/src/murmur/MurmurIce.cpp
+++ b/src/murmur/MurmurIce.cpp
@@ -1443,47 +1443,51 @@ static void impl_Server_setACL(const ::Murmur::AMD_Server_setACLPtr cb, int serv
 	NEED_SERVER;
 	NEED_CHANNEL;
 
-	::Group *g;
-	ChanACL *acl;
-
-	QHash< QString, QSet< int > > hOldTemp;
-	foreach (g, channel->qhGroups) {
-		hOldTemp.insert(g->qsName, g->qsTemporary);
-		delete g;
-	}
-	foreach (acl, channel->qlACL)
-		delete acl;
+	{
+		QWriteLocker locker(&server->qrwlVoiceThread);
 
-	channel->qhGroups.clear();
-	channel->qlACL.clear();
+		::Group *g;
+		ChanACL *acl;
 
-	channel->bInheritACL = inherit;
-	foreach (const ::Murmur::Group &gi, groups) {
-		QString name    = u8(gi.name);
-		g               = new ::Group(channel, name);
-		g->bInherit     = gi.inherit;
-		g->bInheritable = gi.inheritable;
+		QHash< QString, QSet< int > > hOldTemp;
+		foreach (g, channel->qhGroups) {
+			hOldTemp.insert(g->qsName, g->qsTemporary);
+			delete g;
+		}
+		foreach (acl, channel->qlACL)
+			delete acl;
+
+		channel->qhGroups.clear();
+		channel->qlACL.clear();
+
+		channel->bInheritACL = inherit;
+		foreach (const ::Murmur::Group &gi, groups) {
+			QString name    = u8(gi.name);
+			g               = new ::Group(channel, name);
+			g->bInherit     = gi.inherit;
+			g->bInheritable = gi.inheritable;
 #if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)
-		QVector< int > addVec(gi.add.begin(), gi.add.end());
-		QVector< int > removeVec(gi.remove.begin(), gi.remove.end());
+			QVector< int > addVec(gi.add.begin(), gi.add.end());
+			QVector< int > removeVec(gi.remove.begin(), gi.remove.end());
 
-		g->qsAdd    = QSet< int >(addVec.begin(), addVec.end());
-		g->qsRemove = QSet< int >(removeVec.begin(), removeVec.end());
+			g->qsAdd    = QSet< int >(addVec.begin(), addVec.end());
+			g->qsRemove = QSet< int >(removeVec.begin(), removeVec.end());
 #else
-		// Qt 5.14 prefers to use the new range-based constructor for vectors and sets
-		g->qsAdd    = QVector< int >::fromStdVector(gi.add).toList().toSet();
-		g->qsRemove = QVector< int >::fromStdVector(gi.remove).toList().toSet();
+			// Qt 5.14 prefers to use the new range-based constructor for vectors and sets
+			g->qsAdd    = QVector< int >::fromStdVector(gi.add).toList().toSet();
+			g->qsRemove = QVector< int >::fromStdVector(gi.remove).toList().toSet();
 #endif
-		g->qsTemporary = hOldTemp.value(name);
-	}
-	foreach (const ::Murmur::ACL &ai, acls) {
-		acl             = new ChanACL(channel);
-		acl->bApplyHere = ai.applyHere;
-		acl->bApplySubs = ai.applySubs;
-		acl->iUserId    = ai.userid;
-		acl->qsGroup    = u8(ai.group);
-		acl->pDeny      = static_cast< ChanACL::Permissions >(ai.deny) & ChanACL::All;
-		acl->pAllow     = static_cast< ChanACL::Permissions >(ai.allow) & ChanACL::All;
+			g->qsTemporary = hOldTemp.value(name);
+		}
+		foreach (const ::Murmur::ACL &ai, acls) {
+			acl             = new ChanACL(channel);
+			acl->bApplyHere = ai.applyHere;
+			acl->bApplySubs = ai.applySubs;
+			acl->iUserId    = ai.userid;
+			acl->qsGroup    = u8(ai.group);
+			acl->pDeny      = static_cast< ChanACL::Permissions >(ai.deny) & ChanACL::All;
+			acl->pAllow     = static_cast< ChanACL::Permissions >(ai.allow) & ChanACL::All;
+		}
 	}
 
 	server->clearACLCache();