From 05970178b285fa798d1905b792a3b9a4c8c33771 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Wed, 28 Aug 2024 23:04:53 +0200 Subject: [PATCH] Fix map downloading overriding more recent map change messages Following sequence of events was possible: 1. HTTP map download for map X is started (e.g. client initially connects to server). 2. Map change message for map Y arrives while downloading (e.g. vote for another map has passed on the server). 3. Client already has map Y and loads it directly without starting or resetting the map download. 4. HTTP map download task for map X finishes. 5. Map X is loaded, overriding the map Y that should be loaded. This is one cause for #2804. This could potentially also happen with the map download from the game-server, if the final map data chunk for a downloaded map is received directly after another map change message for a map which does not need to be downloaded, though this seems more unlikely. This is fixed by resetting the current map download immediately after receiving a valid map change message instead of only when starting a new map download. The relevant code to reset the map download is moved to `CClient::ResetMapDownload` to avoid duplicate code and ensure that the temporary map download file is always deleted and closed. --- src/engine/client/client.cpp | 74 ++++++++++++++---------------------- 1 file changed, 29 insertions(+), 45 deletions(-) diff --git a/src/engine/client/client.cpp b/src/engine/client/client.cpp index 06b399eae..f6aa93b71 100644 --- a/src/engine/client/client.cpp +++ b/src/engine/client/client.cpp @@ -247,11 +247,7 @@ void CClient::SendReady(int Conn) void CClient::SendMapRequest() { - if(m_MapdownloadFileTemp) - { - io_close(m_MapdownloadFileTemp); - Storage()->RemoveFile(m_aMapdownloadFilenameTemp, IStorage::TYPE_SAVE); - } + dbg_assert(!m_MapdownloadFileTemp, "Map download already in progress"); m_MapdownloadFileTemp = Storage()->OpenFile(m_aMapdownloadFilenameTemp, IOFLAG_WRITE, IStorage::TYPE_SAVE); if(IsSixup()) { @@ -656,6 +652,7 @@ void CClient::DisconnectWithReason(const char *pReason) m_aRconAuthed[0] = 0; mem_zero(m_aRconUsername, sizeof(m_aRconUsername)); mem_zero(m_aRconPassword, sizeof(m_aRconPassword)); + m_MapDetailsPresent = false; m_ServerSentCapabilities = false; m_UseTempRconCommands = 0; m_ExpectedRconCommands = -1; @@ -671,22 +668,10 @@ void CClient::DisconnectWithReason(const char *pReason) m_CurrentServerCurrentPingTime = -1; m_CurrentServerNextPingTime = -1; - // disable all downloads - m_MapdownloadChunk = 0; - if(m_pMapdownloadTask) - m_pMapdownloadTask->Abort(); - if(m_MapdownloadFileTemp) - { - io_close(m_MapdownloadFileTemp); - Storage()->RemoveFile(m_aMapdownloadFilenameTemp, IStorage::TYPE_SAVE); - } - m_MapdownloadFileTemp = 0; - m_MapdownloadSha256Present = false; - m_MapdownloadSha256 = SHA256_ZEROED; - m_MapdownloadCrc = 0; - m_MapdownloadTotalsize = -1; - m_MapdownloadAmount = 0; - m_MapDetailsPresent = false; + ResetMapDownload(); + m_aMapdownloadFilename[0] = '\0'; + m_aMapdownloadFilenameTemp[0] = '\0'; + m_aMapdownloadName[0] = '\0'; // clear the current server info mem_zero(&m_CurrentServerInfo, sizeof(m_CurrentServerInfo)); @@ -1543,6 +1528,8 @@ void CClient::ProcessServerPacket(CNetChunk *pPacket, int Conn, bool Dummy) DummyDisconnect(0); } + ResetMapDownload(); + SHA256_DIGEST *pMapSha256 = nullptr; const char *pMapUrl = nullptr; if(MapDetailsWerePresent && str_comp(m_aMapDetailsName, pMap) == 0 && m_MapDetailsCrc == MapCrc) @@ -1559,12 +1546,6 @@ void CClient::ProcessServerPacket(CNetChunk *pPacket, int Conn, bool Dummy) } else { - if(m_MapdownloadFileTemp) - { - io_close(m_MapdownloadFileTemp); - Storage()->RemoveFile(m_aMapdownloadFilenameTemp, IStorage::TYPE_SAVE); - } - // start map download FormatMapDownloadFilename(pMap, pMapSha256, MapCrc, false, m_aMapdownloadFilename, sizeof(m_aMapdownloadFilename)); FormatMapDownloadFilename(pMap, pMapSha256, MapCrc, true, m_aMapdownloadFilenameTemp, sizeof(m_aMapdownloadFilenameTemp)); @@ -1573,16 +1554,11 @@ void CClient::ProcessServerPacket(CNetChunk *pPacket, int Conn, bool Dummy) str_format(aBuf, sizeof(aBuf), "starting to download map to '%s'", m_aMapdownloadFilenameTemp); m_pConsole->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "client/network", aBuf); - m_MapdownloadChunk = 0; str_copy(m_aMapdownloadName, pMap); - m_MapdownloadSha256Present = (bool)pMapSha256; m_MapdownloadSha256 = pMapSha256 ? *pMapSha256 : SHA256_ZEROED; m_MapdownloadCrc = MapCrc; m_MapdownloadTotalsize = MapSize; - m_MapdownloadAmount = 0; - - ResetMapDownload(); if(pMapSha256) { @@ -2225,9 +2201,25 @@ void CClient::ResetMapDownload() if(m_pMapdownloadTask) { m_pMapdownloadTask->Abort(); - m_pMapdownloadTask = NULL; + m_pMapdownloadTask = nullptr; } - m_MapdownloadFileTemp = 0; + + if(m_MapdownloadFileTemp) + { + io_close(m_MapdownloadFileTemp); + m_MapdownloadFileTemp = 0; + } + + if(Storage()->FileExists(m_aMapdownloadFilenameTemp, IStorage::TYPE_SAVE)) + { + Storage()->RemoveFile(m_aMapdownloadFilenameTemp, IStorage::TYPE_SAVE); + } + + m_MapdownloadChunk = 0; + m_MapdownloadSha256Present = false; + m_MapdownloadSha256 = SHA256_ZEROED; + m_MapdownloadCrc = 0; + m_MapdownloadTotalsize = -1; m_MapdownloadAmount = 0; } @@ -2235,9 +2227,8 @@ void CClient::FinishMapDownload() { m_pConsole->Print(IConsole::OUTPUT_LEVEL_ADDINFO, "client/network", "download complete, loading map"); - int Prev = m_MapdownloadTotalsize; - m_MapdownloadTotalsize = -1; - SHA256_DIGEST *pSha256 = m_MapdownloadSha256Present ? &m_MapdownloadSha256 : 0; + const int PrevMapdownloadTotalsize = m_MapdownloadTotalsize; + SHA256_DIGEST *pSha256 = m_MapdownloadSha256Present ? &m_MapdownloadSha256 : nullptr; bool FileSuccess = true; if(Storage()->FileExists(m_aMapdownloadFilename, IStorage::TYPE_SAVE)) @@ -2252,7 +2243,6 @@ void CClient::FinishMapDownload() return; } - // load map const char *pError = LoadMap(m_aMapdownloadName, m_aMapdownloadFilename, pSha256, m_MapdownloadCrc); if(!pError) { @@ -2263,17 +2253,11 @@ void CClient::FinishMapDownload() else if(m_pMapdownloadTask) // fallback { ResetMapDownload(); - m_MapdownloadTotalsize = Prev; + m_MapdownloadTotalsize = PrevMapdownloadTotalsize; SendMapRequest(); } else { - if(m_MapdownloadFileTemp) - { - io_close(m_MapdownloadFileTemp); - m_MapdownloadFileTemp = 0; - Storage()->RemoveFile(m_aMapdownloadFilenameTemp, IStorage::TYPE_SAVE); - } ResetMapDownload(); DisconnectWithReason(pError); }