cpp/openttd-patchpack/source Changeset - r25942:fb8ed4e7ecae

Changeset - r25942:fb8ed4e7ecae

Parent rev.

Child rev.

[Not reviewed]

master

0 6 0

Rubidium - 3 years ago 2021-07-23 20:36:17
rubidium@openttd.org

Fix #9388: thread unsafe use of NetworkAdminConsole/IConsolePrint

6 files changed with 79 insertions and 2 deletions:

src/debug.cpp

src/debug.h

src/network/network_admin.cpp

src/openttd.cpp

src/settings.cpp

src/table/settings/gui_settings.ini

0 comments (0 inline, 0 general)

src/debug.cpp

➞

Show inline comments

@@ @@ -5,36 +5,47 @@ @@
  * See the GNU General Public License for more details. You should have received a copy of the GNU General Public License along with OpenTTD. If not, see <http://www.gnu.org/licenses/>.
  */
 /** @file debug.cpp Handling of printing debug messages. */
 #include "stdafx.h"
 #include <stdarg.h>
 #include "console_func.h"
 #include "debug.h"
 #include "string_func.h"
 #include "fileio_func.h"
 #include "settings_type.h"
 #include <mutex>
 #if defined(_WIN32)
 #include "os/windows/win32.h"
 #endif
 #include "walltime_func.h"
 #include "network/network_admin.h"
 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;
 int _debug_misc_level;
 int _debug_net_level;
 int _debug_sprite_level;
 int _debug_oldloader_level;
 int _debug_npf_level;
 int _debug_yapf_level;
 int _debug_freetype_level;
 int _debug_script_level;
 int _debug_sl_level;
@@ @@ -98,49 +109,57 @@ char *DumpDebugFacilityNames(char *buf, @@
 	return buf;
+}
 /**
  * Internal function for outputting the debug line.
  * @param level Debug category.
  * @param message The message to output.
  */
 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);
 		return;
+	}
 	if (strcmp(level, "desync") == 0) {
 		static FILE *f = FioFOpenFile("commands-out.log", "wb", AUTOSAVE_DIR);
 		if (f == nullptr) return;
 		fprintf(f, "%s%s\n", GetLogPrefix(), message.c_str());
 		fflush(f);
 #ifdef RANDOM_DEBUG
 	} else if (strcmp(level, "random") == 0) {
 		static FILE *f = FioFOpenFile("random-out.log", "wb", AUTOSAVE_DIR);
 		if (f == nullptr) return;
 		fprintf(f, "%s\n", message.c_str());
 		fflush(f);
 #endif
 	} else {
 		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 });
+		}
+	}
+}
 /**
  * Set debugging levels by parsing the text in \a s.
  * For setting individual levels a string like \c "net=3,grf=6" should be used.
  * If the string starts with a number, the number is used as global debugging level.
  * @param s Text describing the wanted debugging levels.
  */
 void SetDebugString(const char *s)
+{
 	int v;
@@ @@ -220,12 +239,56 @@ const char *GetDebugString() @@
  */
 const char *GetLogPrefix()
+{
 	static char _log_prefix[24];
 	if (_settings_client.gui.show_date_in_logs) {
 		LocalTime::Format(_log_prefix, lastof(_log_prefix), "[%Y-%m-%d %H:%M:%S] ");
 	} else {
 		*_log_prefix = '\0';
+	}
 	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);
+}

src/debug.h

➞

Show inline comments

@@ @@ -113,13 +113,16 @@ const char *GetDebugString(); @@
 		Debug(misc, 0, "[{}] {} us [avg: {:.1f} us]", str, _sum_, _sum_/(double)_i_);\
 		_i_ = 0;\
 		_sum_ = 0;\
 	}\
+}
 void ShowInfo(const char *str);
 void CDECL ShowInfoF(const char *str, ...) WARN_FORMAT(1, 2);
 const char *GetLogPrefix();
 void DebugSendRemoteMessages();
 void DebugReconsiderSendRemoteMessages();
 #endif /* DEBUG_H */

src/network/network_admin.cpp

➞

Show inline comments

@@ @@ -67,24 +67,29 @@ ServerNetworkAdminSocketHandler::ServerN @@
 	this->status = ADMIN_STATUS_INACTIVE;
 	this->connect_time = std::chrono::steady_clock::now();
+}
 /**
  * Clear everything related to this admin.
  */
 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();
+	}
+}
 /**
  * Whether a connection is allowed or not at this moment.
  * @return Whether the connection is allowed.
  */
 /* static */ bool ServerNetworkAdminSocketHandler::AllowConnection()
+{
 	bool accept = !_settings_client.network.admin_password.empty() && _network_admins_connected < MAX_ADMINS;
 	/* We can't go over the MAX_ADMINS limit here. However, if we accept
 	 * the connection, there has to be space in the pool. */
 	static_assert(NetworkAdminSocketPool::MAX_SIZE == MAX_ADMINS);
@@ @@ -679,24 +684,26 @@ NetworkRecvStatus ServerNetworkAdminSock @@
 	AdminUpdateType type = (AdminUpdateType)p->Recv_uint16();
 	AdminUpdateFrequency freq = (AdminUpdateFrequency)p->Recv_uint16();
 	if (type >= ADMIN_UPDATE_END || (_admin_update_type_frequencies[type] & freq) != freq) {
 		/* The server does not know of this UpdateType. */
 		Debug(net, 1, "[admin] Not supported update frequency {} ({}) from '{}' ({})", type, freq, this->admin_name, this->admin_version);
 		return this->SendError(NETWORK_ERROR_ILLEGAL_PACKET);
+	}
 	this->update_frequency[type] = freq;
 	if (type == ADMIN_UPDATE_CONSOLE) DebugReconsiderSendRemoteMessages();
 	return NETWORK_RECV_STATUS_OKAY;
+}
 NetworkRecvStatus ServerNetworkAdminSocketHandler::Receive_ADMIN_POLL(Packet *p)
+{
 	if (this->status == ADMIN_STATUS_INACTIVE) return this->SendError(NETWORK_ERROR_NOT_EXPECTED);
 	AdminUpdateType type = (AdminUpdateType)p->Recv_uint8();
 	uint32 d1 = p->Recv_uint32();
 	switch (type) {
 		case ADMIN_UPDATE_DATE:

src/openttd.cpp

➞

Show inline comments

@@ @@ -1444,24 +1444,26 @@ void GameLoop() @@
 	/* switch game mode? */
 	if (_switch_mode != SM_NONE && !HasModalProgress()) {
 		SwitchToMode(_switch_mode);
 		_switch_mode = SM_NONE;
+	}
 	IncreaseSpriteLRU();
 	/* Check for UDP stuff */
 	if (_network_available) NetworkBackgroundLoop();
 	DebugSendRemoteMessages();
 	if (_networking && !HasModalProgress()) {
 		/* Multiplayer */
 		NetworkGameLoop();
 	} else {
 		if (_network_reconnect > 0 && --_network_reconnect == 0) {
 			/* This means that we want to reconnect to the last host
 			 * We do this here, because it means that the network is really closed */
 			NetworkClientConnectGame(_settings_client.network.last_joined, COMPANY_SPECTATOR);
+		}
 		/* Singleplayer */
 		StateGameLoop();
+	}

src/settings.cpp

➞

Show inline comments

@@ @@ -1233,24 +1233,25 @@ void LoadFromConfig(bool startup) @@
+		}
 		_grfconfig_newgame = GRFLoadConfig(generic_ini, "newgrf", false);
 		_grfconfig_static  = GRFLoadConfig(generic_ini, "newgrf-static", true);
 		AILoadConfig(generic_ini, "ai_players");
 		GameLoadConfig(generic_ini, "game_scripts");
 		PrepareOldDiffCustom();
 		IniLoadSettings(generic_ini, _old_gameopt_settings, "gameopt", &_settings_newgame, false);
 		HandleOldDiffCustom(false);
 		ValidateSettings();
 		DebugReconsiderSendRemoteMessages();
 		/* Display scheduled errors */
 		extern void ScheduleErrorMessage(ErrorList &datas);
 		ScheduleErrorMessage(_settings_error_list);
 		if (FindWindowById(WC_ERRMSG, 0) == nullptr) ShowFirstError();
+	}
+}
 /** Save the values to the configuration file */
 void SaveToConfig()
+{
 	ConfigIniFile generic_ini(_config_file);

src/table/settings/gui_settings.ini

➞

Show inline comments

@@ @@ -744,24 +744,25 @@ flags = SF_NOT_IN_SAVE | SF_NO_NETWOR @@
 def      = 0
 min      = 0
 max      = 2
 [SDTC_VAR]
 var      = gui.developer
 type     = SLE_UINT8
 flags    = SF_NOT_IN_SAVE | SF_NO_NETWORK_SYNC
 def      = 1
 min      = 0
 max      = 2
 cat      = SC_EXPERT
 post_cb  = [](auto) { DebugReconsiderSendRemoteMessages(); }
 [SDTC_BOOL]
 var      = gui.newgrf_developer_tools
 flags    = SF_NOT_IN_SAVE | SF_NO_NETWORK_SYNC
 def      = false
 post_cb  = InvalidateNewGRFChangeWindows
 cat      = SC_EXPERT
 [SDTC_BOOL]
 var      = gui.ai_developer_tools
 flags    = SF_NOT_IN_SAVE | SF_NO_NETWORK_SYNC
 def      = false

0 comments (0 inline, 0 general)