Fix #9388: thread unsafe use of NetworkAdminConsole/IConsolePrint

6 files changed, 79 insertions, 2 deletions

diff --git a/src/debug.cpp b/src/debug.cpp
index 2ac1dcc69..cf0006e31 100644
--- a/src/debug.cpp
+++ b/src/debug.cpp
@@ -14,6 +14,7 @@
 #include "string_func.h"
 #include "fileio_func.h"
 #include "settings_type.h"
+#include <mutex>
 
 #if defined(_WIN32)
 #include "os/windows/win32.h"
@@ -26,6 +27,16 @@ SOCKET _debug_socket = INVALID_SOCKET;
 
 #include "safeguards.h"
 
+/** Element in the queue of debug messages that have to be passed to either NetworkAdminConsole or IConsolePrint.*/
+struct QueuedDebugItem {
+	std::string level;   ///< The used debug level.
+	std::string message; ///< The actual formatted message.
+};
+std::atomic<bool> _debug_remote_console; ///< Whether we need to send data to either NetworkAdminConsole or IConsolePrint.
+std::mutex _debug_remote_console_mutex; ///< Mutex to guard the queue of debug messages for either NetworkAdminConsole or IConsolePrint.
+std::vector<QueuedDebugItem> _debug_remote_console_queue; ///< Queue for debug messages to be passed to NetworkAdminConsole or IConsolePrint.
+std::vector<QueuedDebugItem> _debug_remote_console_queue_spare; ///< Spare queue to swap with _debug_remote_console_queue.
+
 int _debug_driver_level;
 int _debug_grf_level;
 int _debug_map_level;
@@ -107,6 +118,11 @@ void DebugPrint(const char *level, const std::string &message)
 {
 	if (_debug_socket != INVALID_SOCKET) {
 		std::string msg = fmt::format("{}dbg: [{}] {}\n", GetLogPrefix(), level, message);
+
+		/* Prevent sending a message concurrently, as that might cause interleaved messages. */
+		static std::mutex _debug_socket_mutex;
+		std::lock_guard<std::mutex> lock(_debug_socket_mutex);
+
 		/* Sending out an error when this fails would be nice, however... the error
 		 * would have to be send over this failing socket which won't work. */
 		send(_debug_socket, msg.c_str(), (int)msg.size(), 0);
@@ -130,8 +146,11 @@ void DebugPrint(const char *level, const std::string &message)
 		std::string msg = fmt::format("{}dbg: [{}] {}\n", GetLogPrefix(), level, message);
 		fputs(msg.c_str(), stderr);
 
-		NetworkAdminConsole(level, message);
-		if (_settings_client.gui.developer >= 2) IConsolePrint(CC_DEBUG, "dbg: [{}] {}", level, message);
+		if (_debug_remote_console.load()) {
+			/* Only add to the queue when there is at least one consumer of the data. */
+			std::lock_guard<std::mutex> lock(_debug_remote_console_mutex);
+			_debug_remote_console_queue.push_back({ level, message });
+		}
 	}
 }
 
@@ -229,3 +248,47 @@ const char *GetLogPrefix()
 	return _log_prefix;
 }
 
+/**
+ * Send the queued Debug messages to either NetworkAdminConsole or IConsolePrint from the
+ * GameLoop thread to prevent concurrent accesses to both the NetworkAdmin's packet queue
+ * as well as IConsolePrint's buffers.
+ *
+ * This is to be called from the GameLoop thread.
+ */
+void DebugSendRemoteMessages()
+{
+	if (!_debug_remote_console.load()) return;
+
+	{
+		std::lock_guard<std::mutex> lock(_debug_remote_console_mutex);
+		std::swap(_debug_remote_console_queue, _debug_remote_console_queue_spare);
+	}
+
+	for (auto &item : _debug_remote_console_queue_spare) {
+		NetworkAdminConsole(item.level, item.message);
+		if (_settings_client.gui.developer >= 2) IConsolePrint(CC_DEBUG, "dbg: [{}] {}", item.level, item.message);
+	}
+
+	_debug_remote_console_queue_spare.clear();
+}
+
+/**
+ * Reconsider whether we need to send debug messages to either NetworkAdminConsole
+ * or IConsolePrint. The former is when they have enabled console handling whereas
+ * the latter depends on the gui.developer setting's value.
+ *
+ * This is to be called from the GameLoop thread.
+ */
+void DebugReconsiderSendRemoteMessages()
+{
+	bool enable = _settings_client.gui.developer >= 2;
+
+	for (ServerNetworkAdminSocketHandler *as : ServerNetworkAdminSocketHandler::IterateActive()) {
+		if (as->update_frequency[ADMIN_UPDATE_CONSOLE] & ADMIN_FREQUENCY_AUTOMATIC) {
+			enable = true;
+			break;
+		}
+	}
+
+	_debug_remote_console.store(enable);
+}
diff --git a/src/debug.h b/src/debug.h
index 37375fb08..770bc7882 100644
--- a/src/debug.h
+++ b/src/debug.h
@@ -122,4 +122,7 @@ void CDECL ShowInfoF(const char *str, ...) WARN_FORMAT(1, 2);
 
 const char *GetLogPrefix();
 
+void DebugSendRemoteMessages();
+void DebugReconsiderSendRemoteMessages();
+
 #endif /* DEBUG_H */
diff --git a/src/network/network_admin.cpp b/src/network/network_admin.cpp
index 8ccb6361d..811daaced 100644
--- a/src/network/network_admin.cpp
+++ b/src/network/network_admin.cpp
@@ -76,6 +76,11 @@ ServerNetworkAdminSocketHandler::~ServerNetworkAdminSocketHandler()
 	_network_admins_connected--;
 	Debug(net, 3, "[admin] '{}' ({}) has disconnected", this->admin_name, this->admin_version);
 	if (_redirect_console_to_admin == this->index) _redirect_console_to_admin = INVALID_ADMIN_ID;
+
+	if (this->update_frequency[ADMIN_UPDATE_CONSOLE] & ADMIN_FREQUENCY_AUTOMATIC) {
+		this->update_frequency[ADMIN_UPDATE_CONSOLE] = (AdminUpdateFrequency)0;
+		DebugReconsiderSendRemoteMessages();
+	}
 }
 
 /**
@@ -688,6 +693,8 @@ NetworkRecvStatus ServerNetworkAdminSocketHandler::Receive_ADMIN_UPDATE_FREQUENC
 
 	this->update_frequency[type] = freq;
 
+	if (type == ADMIN_UPDATE_CONSOLE) DebugReconsiderSendRemoteMessages();
+
 	return NETWORK_RECV_STATUS_OKAY;
 }
 
diff --git a/src/openttd.cpp b/src/openttd.cpp
index db2bb3bdb..189010aad 100644
--- a/src/openttd.cpp
+++ b/src/openttd.cpp
@@ -1453,6 +1453,8 @@ void GameLoop()
 	/* Check for UDP stuff */
 	if (_network_available) NetworkBackgroundLoop();
 
+	DebugSendRemoteMessages();
+
 	if (_networking && !HasModalProgress()) {
 		/* Multiplayer */
 		NetworkGameLoop();
diff --git a/src/settings.cpp b/src/settings.cpp
index 3d142d9ce..52f30be08 100644
--- a/src/settings.cpp
+++ b/src/settings.cpp
@@ -1242,6 +1242,7 @@ void LoadFromConfig(bool startup)
 		HandleOldDiffCustom(false);
 
 		ValidateSettings();
+		DebugReconsiderSendRemoteMessages();
 
 		/* Display scheduled errors */
 		extern void ScheduleErrorMessage(ErrorList &datas);
diff --git a/src/table/settings/gui_settings.ini b/src/table/settings/gui_settings.ini
index 427402660..f08c84981 100644
--- a/src/table/settings/gui_settings.ini
+++ b/src/table/settings/gui_settings.ini
@@ -753,6 +753,7 @@ def      = 1
 min      = 0
 max      = 2
 cat      = SC_EXPERT
+post_cb  = [](auto) { DebugReconsiderSendRemoteMessages(); }
 
 [SDTC_BOOL]
 var      = gui.newgrf_developer_tools