From f2acfb50f43965a41cc8a3c3b94c6bf27b8a5cc7 Mon Sep 17 00:00:00 2001
From: Robert Adam <dev@robert-adam.de>
Date: Mon, 17 Aug 2020 18:07:22 +0200
Subject: [PATCH] FIX(client): Racecondition deleting shortcuts

Server-specific shortcuts are read into memory in
MainWindow::msgServerSync and are saved into the DB again in
MainWindow::serverDisconnected.
If however the disconnect function runs before the server
synchronization has finished, the shortcut list in memory is empty and
by writing that into the DB all existing server-specific shortcuts are
lost (overwritten).

This commit adds checks before saving the shortcuts (and
ChannelListeners) so that saving only occurs if the synchronization has
happened before and thus the shortcuts have been loaded in the first
place.
---
 src/mumble/MainWindow.cpp    | 20 +++++++++++++++-----
 src/mumble/Messages.cpp      |  2 ++
 src/mumble/ServerHandler.cpp | 20 ++++++++++++++++++++
 src/mumble/ServerHandler.h   | 14 ++++++++++++++
 4 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/src/mumble/MainWindow.cpp b/src/mumble/MainWindow.cpp
index 52ffeaef7..b264dba14 100644
--- a/src/mumble/MainWindow.cpp
+++ b/src/mumble/MainWindow.cpp
@@ -3093,9 +3093,12 @@ void MainWindow::serverConnected() {
 }
 
 void MainWindow::serverDisconnected(QAbstractSocket::SocketError err, QString reason) {
-	// Note that the saving of the ChannelListeners has to be done, before resetting g.uiSession
-	// Save ChannelListeners
-	ChannelListener::saveToDB();
+	if (g.sh->hasSynchronized()) {
+		// Note that the saving of the ChannelListeners has to be done, before resetting g.uiSession
+		// Save ChannelListeners
+		ChannelListener::saveToDB();
+	}
+
 	// clear ChannelListener
 	ChannelListener::clear();
 
@@ -3117,8 +3120,15 @@ void MainWindow::serverDisconnected(QAbstractSocket::SocketError err, QString re
 	QString uname, pw, host;
 	unsigned short port;
 	g.sh->getConnectionInfo(host, port, uname, pw);
-	if (g.db->setShortcuts(g.sh->qbaDigest, g.s.qlShortcuts))
-		GlobalShortcutEngine::engine->bNeedRemap = true;
+
+	if (g.sh->hasSynchronized()) {
+		// Only save server-specific shortcuts if the client and server have been synchronized before as only then
+		// did the client actually load them from the DB. If we store them without having loaded them, we will effectively
+		// clear the server-specific shortcuts for this server.
+		if (g.db->setShortcuts(g.sh->qbaDigest, g.s.qlShortcuts)) {
+			GlobalShortcutEngine::engine->bNeedRemap = true;
+		}
+	}
 
 	if (aclEdit) {
 		aclEdit->reject();
diff --git a/src/mumble/Messages.cpp b/src/mumble/Messages.cpp
index a0f2ba190..f789711fb 100644
--- a/src/mumble/Messages.cpp
+++ b/src/mumble/Messages.cpp
@@ -200,6 +200,8 @@ void MainWindow::msgServerSync(const MumbleProto::ServerSync &msg) {
 		}
 	});
 
+	g.sh->setServerSynchronized(true);
+
 	emit serverSynchronized();
 }
 
diff --git a/src/mumble/ServerHandler.cpp b/src/mumble/ServerHandler.cpp
index a75c5cc97..bb81fc832 100644
--- a/src/mumble/ServerHandler.cpp
+++ b/src/mumble/ServerHandler.cpp
@@ -309,6 +309,20 @@ void ServerHandler::sendProtoMessage(const ::google::protobuf::Message &msg, uns
 	}
 }
 
+bool ServerHandler::isConnected() const {
+	// If the digest isn't empty, then we are currently connected to a server (the digest being a hash
+	// of the server's certificate)
+	return !qbaDigest.isEmpty();
+}
+
+bool ServerHandler::hasSynchronized() const {
+	return serverSynchronized;
+}
+
+void ServerHandler::setServerSynchronized(bool synchronized) {
+	serverSynchronized = synchronized;
+}
+
 void ServerHandler::hostnameResolved() {
 	ServerResolver *sr = qobject_cast<ServerResolver *>(QObject::sender());
 	QList<ServerResolverRecord> records = sr->records();
@@ -374,6 +388,11 @@ void ServerHandler::run() {
 			ConnectionPtr connection(new Connection(this, qtsSock));
 			cConnection = connection;
 
+			// Technically it isn't necessary to reset this flag here since a ServerHandler will not be used
+			// for multiple connections in a row but just in case that at some point it will, we'll reset the
+			// flag here.
+			serverSynchronized = false;
+
 			qlErrors.clear();
 			qscCert.clear();
 
@@ -657,6 +676,7 @@ void ServerHandler::serverConnectionClosed(QAbstractSocket::SocketError err, con
 	}
 
 	emit disconnected(err, reason);
+
 	exit(0);
 }
 
diff --git a/src/mumble/ServerHandler.h b/src/mumble/ServerHandler.h
index acc0b26d6..feb50fc27 100644
--- a/src/mumble/ServerHandler.h
+++ b/src/mumble/ServerHandler.h
@@ -70,6 +70,10 @@ class ServerHandler : public QThread {
 		bool bUdp;
 		bool bStrong;
 
+		/// Flag indicating whether the server we are currently connected to has
+		/// finished synchronizing already.
+		bool serverSynchronized = false;
+
 #ifdef Q_OS_WIN
 		HANDLE hQoS;
 		DWORD dwFlowUDP;
@@ -112,6 +116,16 @@ class ServerHandler : public QThread {
 		void sendProtoMessage(const ::google::protobuf::Message &msg, unsigned int msgType);
 		void sendMessage(const char *data, int len, bool force = false);
 
+		/// @returns Whether this handler is currently connected to a server.
+		bool isConnected() const;
+
+		/// @returns Whether the server this handler is currently connected to, has finished
+		/// 	synchronizing yet.
+		bool hasSynchronized() const;
+
+		/// @param synchronized Whether the server has finished synchronization
+		void setServerSynchronized(bool synchronized);
+
 #define MUMBLE_MH_MSG(x) void sendMessage(const MumbleProto:: x &msg) { sendProtoMessage(msg, MessageHandler:: x); }
 		MUMBLE_MH_ALL
 #undef MUMBLE_MH_MSG