From 1e9f9790aedfe0ba594be422be7b6129e25f1e45 Mon Sep 17 00:00:00 2001 From: Ted John Date: Tue, 6 Jun 2017 20:05:41 +0100 Subject: [PATCH] Fix #5496: Attempting to log into servers results in crash Caused by the user not having the required 'official' objects. This was because the network was being closed during a network update. Disposed memory would then be accessed later in the update loop. To fix this, a lock has been added to Close() so that it can be deferred to the end of Update(). This isn't particularly nice, but the whole of network will need redesigning to fix this properly for all potentical scenarios where Close() can be called. --- src/openrct2/network/network.cpp | 21 +++++++++++++++++++ src/openrct2/network/network.h | 2 ++ src/openrct2/object.h | 4 ++-- src/openrct2/object/ObjectRepository.cpp | 26 ++++++++++++++++-------- src/openrct2/rct1/S4Importer.cpp | 2 +- 5 files changed, 44 insertions(+), 11 deletions(-) diff --git a/src/openrct2/network/network.cpp b/src/openrct2/network/network.cpp index 94ec42b6f6..29578cd9ec 100644 --- a/src/openrct2/network/network.cpp +++ b/src/openrct2/network/network.cpp @@ -151,6 +151,15 @@ void Network::Close() // which may no longer be valid on Linux and would cause a segfault. return; } + + // HACK Because Close() is closed all over the place, it sometimes gets called inside an Update + // call. This then causes disposed data to be accessed. Therefore, save closing until the + // end of the update loop. + if (_closeLock) { + _requireClose = true; + return; + } + if (mode == NETWORK_MODE_CLIENT) { delete server_connection->Socket; server_connection->Socket = nullptr; @@ -178,6 +187,8 @@ void Network::Close() CloseChatLog(); gfx_invalidate_screen(); + + _requireClose = false; } bool Network::BeginClient(const char* host, uint16 port) @@ -362,6 +373,8 @@ uint8 Network::GetPlayerID() void Network::Update() { + _closeLock = true; + switch (GetMode()) { case NETWORK_MODE_SERVER: UpdateServer(); @@ -370,6 +383,12 @@ void Network::Update() UpdateClient(); break; } + + // If the Close() was called during the update, close it for real + _closeLock = false; + if (_requireClose) { + Close(); + } } void Network::UpdateServer() @@ -405,6 +424,8 @@ void Network::UpdateServer() void Network::UpdateClient() { + assert(server_connection != nullptr); + switch(status){ case NETWORK_STATUS_CONNECTING: { diff --git a/src/openrct2/network/network.h b/src/openrct2/network/network.h index bb70bfd599..f2ea55f2cb 100644 --- a/src/openrct2/network/network.h +++ b/src/openrct2/network/network.h @@ -195,6 +195,8 @@ private: sint32 mode = NETWORK_MODE_NONE; sint32 status = NETWORK_STATUS_NONE; + bool _closeLock = false; + bool _requireClose = false; bool wsa_initialized = false; ITcpSocket * listening_socket = nullptr; uint16 listening_port = 0; diff --git a/src/openrct2/object.h b/src/openrct2/object.h index eaa00e9617..3a563d3650 100644 --- a/src/openrct2/object.h +++ b/src/openrct2/object.h @@ -114,8 +114,8 @@ sint32 object_calculate_checksum(const rct_object_entry * entry, const void * da sint32 find_object_in_entry_group(const rct_object_entry* entry, uint8* entry_type, uint8* entry_index); void object_create_identifier_name(char* string_buffer, size_t size, const rct_object_entry* object); -rct_object_entry *object_list_find_by_name(const char *name); -rct_object_entry *object_list_find(rct_object_entry *entry); +const rct_object_entry * object_list_find_by_name(const char *name); +const rct_object_entry * object_list_find(rct_object_entry *entry); void object_entry_get_name(utf8 * buffer, size_t bufferSize, const rct_object_entry * entry); void object_entry_get_name_fixed(utf8 * buffer, size_t bufferSize, const rct_object_entry * entry); diff --git a/src/openrct2/object/ObjectRepository.cpp b/src/openrct2/object/ObjectRepository.cpp index a2361c9327..5ba2ddb533 100644 --- a/src/openrct2/object/ObjectRepository.cpp +++ b/src/openrct2/object/ObjectRepository.cpp @@ -717,18 +717,28 @@ bool IsObjectCustom(const ObjectRepositoryItem * object) extern "C" { - rct_object_entry * object_list_find(rct_object_entry * entry) + const rct_object_entry * object_list_find(rct_object_entry * entry) { - IObjectRepository * objRepo = GetObjectRepository(); - const ObjectRepositoryItem * item = objRepo->FindObject(entry); - return (rct_object_entry *)&item->ObjectEntry; + const rct_object_entry * result = nullptr; + auto objRepo = GetObjectRepository(); + auto item = objRepo->FindObject(entry); + if (item != nullptr) + { + result = &item->ObjectEntry; + } + return result; } - rct_object_entry * object_list_find_by_name(const char * name) + const rct_object_entry * object_list_find_by_name(const char * name) { - IObjectRepository * objRepo = GetObjectRepository(); - const ObjectRepositoryItem * item = objRepo->FindObject(name); - return (rct_object_entry *)&item->ObjectEntry; + const rct_object_entry * result = nullptr; + auto objRepo = GetObjectRepository(); + auto item = objRepo->FindObject(name); + if (item != nullptr) + { + result = &item->ObjectEntry; + } + return result; } void object_list_load() diff --git a/src/openrct2/rct1/S4Importer.cpp b/src/openrct2/rct1/S4Importer.cpp index 411110d2c0..8ef3bbc2ee 100644 --- a/src/openrct2/rct1/S4Importer.cpp +++ b/src/openrct2/rct1/S4Importer.cpp @@ -468,7 +468,7 @@ private: std::vector objects = RCT1::GetSceneryObjects(sceneryTheme); for (const char * objectName : objects) { - rct_object_entry * foundEntry = object_list_find_by_name(objectName); + const rct_object_entry * foundEntry = object_list_find_by_name(objectName); if (foundEntry != nullptr) { uint8 objectType = foundEntry->flags & 0x0F;