From 7f4ab752b2d24c0ac907c677b95a64ce5d71b55f Mon Sep 17 00:00:00 2001 From: Kawe Mazidjatari <48657826+Mauler125@users.noreply.github.com> Date: Sun, 10 Mar 2024 02:13:15 +0100 Subject: [PATCH] Engine: server bulk ban check bug fixes If the code was ran in the main thread, it would never fire the actual check logic. This patch changed the code so it would always fire from the start of the frame. Also fixed a memory leak that occurred when passing in an external banned list to SV_CheckClientsForBan (previously, 'SV_CheckForBan'); the code always allocated a new banned list, but only freed it if no external banned list was provided. This patch changed the code so it only allocates it if no external one is provided. --- r5dev/engine/server/server.cpp | 5 +- r5dev/engine/server/sv_main.cpp | 94 +++++++++++++++------------------ r5dev/engine/server/sv_main.h | 6 +-- 3 files changed, 51 insertions(+), 54 deletions(-) diff --git a/r5dev/engine/server/server.cpp b/r5dev/engine/server/server.cpp index 3befd0ba..23866ba4 100644 --- a/r5dev/engine/server/server.cpp +++ b/r5dev/engine/server/server.cpp @@ -184,7 +184,10 @@ CClient* CServer::ConnectClient(CServer* pServer, user_creds_s* pChallenge) { if (!pClient->GetNetChan()->GetRemoteAddress().IsLoopback()) { - std::thread th(SV_IsClientBanned, pClient, string(pszAddresBuffer), nNucleusID, string(pszPersonaName), nPort); + const string addressBufferCopy(pszAddresBuffer); + const string personaNameCopy(pszPersonaName); + + std::thread th(SV_CheckForBanAndDisconnect, pClient, addressBufferCopy, nNucleusID, personaNameCopy, nPort); th.detach(); } } diff --git a/r5dev/engine/server/sv_main.cpp b/r5dev/engine/server/sv_main.cpp index f4a8df16..07c5a631 100644 --- a/r5dev/engine/server/sv_main.cpp +++ b/r5dev/engine/server/sv_main.cpp @@ -18,78 +18,71 @@ //----------------------------------------------------------------------------- // Purpose: checks if particular client is banned on the comp server //----------------------------------------------------------------------------- -void SV_IsClientBanned(CClient* pClient, const string& svIPAddr, +void SV_CheckForBanAndDisconnect(CClient* const pClient, const string& svIPAddr, const NucleusID_t nNucleusID, const string& svPersonaName, const int nPort) { Assert(pClient != nullptr); string svError; - bool bCompBanned = g_MasterServer.CheckForBan(svIPAddr, nNucleusID, svPersonaName, svError); + const bool bCompBanned = g_MasterServer.CheckForBan(svIPAddr, nNucleusID, svPersonaName, svError); if (bCompBanned) { - if (!ThreadInMainThread()) - { - g_TaskQueue.Dispatch([pClient, svError, svIPAddr, nNucleusID, nPort] + g_TaskQueue.Dispatch([pClient, svError, svIPAddr, nNucleusID, nPort] + { + // Make sure client isn't already disconnected, + // and that if there is a valid netchannel, that + // it hasn't been taken by a different client by + // the time this task is getting executed. + const CNetChan* const pChan = pClient->GetNetChan(); + if (pChan && pClient->GetNucleusID() == nNucleusID) { - // Make sure client isn't already disconnected, - // and that if there is a valid netchannel, that - // it hasn't been taken by a different client by - // the time this task is getting executed. - CNetChan* pChan = pClient->GetNetChan(); - if (pChan && pClient->GetNucleusID() == nNucleusID) - { - int nUserID = pClient->GetUserID(); + const int nUserID = pClient->GetUserID(); - pClient->Disconnect(Reputation_t::REP_MARK_BAD, svError.c_str()); - Warning(eDLL_T::SERVER, "Removed client '[%s]:%i' from slot #%i ('%llu' is banned globally!)\n", - svIPAddr.c_str(), nPort, nUserID, nNucleusID); - } - }, 0); - } + pClient->Disconnect(Reputation_t::REP_MARK_BAD, svError.c_str()); + Warning(eDLL_T::SERVER, "Removed client '[%s]:%i' from slot #%i ('%llu' is banned globally!)\n", + svIPAddr.c_str(), nPort, nUserID, nNucleusID); + } + }, 0); } } //----------------------------------------------------------------------------- // Purpose: checks if particular client is banned on the master server //----------------------------------------------------------------------------- -void SV_ProcessBulkCheck(const CBanSystem::BannedList_t* pBannedVec, const bool bDelete) +void SV_ProcessBulkCheck(const CBanSystem::BannedList_t* const pBannedVec) { - CBanSystem::BannedList_t* outBannedVec = new CBanSystem::BannedList_t(); + CBanSystem::BannedList_t* const outBannedVec = new CBanSystem::BannedList_t(); g_MasterServer.GetBannedList(*pBannedVec, *outBannedVec); - // Caller wants to destroy the vector. - if (bDelete) - { - delete pBannedVec; - } - - if (!ThreadInMainThread()) - { - g_TaskQueue.Dispatch([outBannedVec] - { - SV_CheckForBan(outBannedVec, true); - }, 0); - } + g_TaskQueue.Dispatch([outBannedVec] + { + SV_CheckClientsForBan(outBannedVec); + delete outBannedVec; + }, 0); } //----------------------------------------------------------------------------- // Purpose: creates a snapshot of the currently connected clients // Input : *pBannedVec - if passed, will check for bans and kick the clients -// bDelete - if set, will delete the passed in vector //----------------------------------------------------------------------------- -void SV_CheckForBan(const CBanSystem::BannedList_t* pBannedVec /*= nullptr*/, const bool bDelete /*= false*/) +void SV_CheckClientsForBan(const CBanSystem::BannedList_t* const pBannedVec /*= nullptr*/) { Assert(ThreadInMainThread()); - CBanSystem::BannedList_t* bannedVec = new CBanSystem::BannedList_t; + + CBanSystem::BannedList_t* bannedVec = !pBannedVec + ? new CBanSystem::BannedList_t + : nullptr; for (int c = 0; c < g_ServerGlobalVariables->m_nMaxClients; c++) // Loop through all possible client instances. { - CClient* pClient = g_pServer->GetClient(c); + CClient* const pClient = g_pServer->GetClient(c); + if (!pClient) continue; - CNetChan* pNetChan = pClient->GetNetChan(); + const CNetChan* const pNetChan = pClient->GetNetChan(); + if (!pNetChan) continue; @@ -99,13 +92,13 @@ void SV_CheckForBan(const CBanSystem::BannedList_t* pBannedVec /*= nullptr*/, co if (pNetChan->GetRemoteAddress().IsLoopback()) continue; - const char* szIPAddr = pNetChan->GetAddress(true); + const char* const szIPAddr = pNetChan->GetAddress(true); const NucleusID_t nNucleusID = pClient->GetNucleusID(); // If no banned list was provided, build one with all clients // on the server. This will be used for bulk checking so live // bans could be performed, as this function is called periodically. - if (!pBannedVec) + if (bannedVec) bannedVec->AddToTail(CBanSystem::Banned_t(szIPAddr, nNucleusID)); else { @@ -128,19 +121,20 @@ void SV_CheckForBan(const CBanSystem::BannedList_t* pBannedVec /*= nullptr*/, co } } - // Caller wants to destroy the vector. - if (bDelete && pBannedVec) + if (bannedVec && !bannedVec->IsEmpty()) { - delete pBannedVec; - } + std::thread bulkCheck([bannedVec]() + { + SV_ProcessBulkCheck(bannedVec); + delete bannedVec; + }); - if (!pBannedVec && !bannedVec->IsEmpty()) - { - std::thread(&SV_ProcessBulkCheck, bannedVec, true).detach(); + bulkCheck.detach(); } - else + else if (bannedVec) { delete bannedVec; + bannedVec = nullptr; } } @@ -169,7 +163,7 @@ bool SV_ActivateServer() return v_SV_ActivateServer(); } -void SV_BroadcastVoiceData(CClient* cl, int nBytes, char* data) +void SV_BroadcastVoiceData(CClient* const cl, const int nBytes, char* const data) { if (!sv_voiceenable->GetBool()) return; diff --git a/r5dev/engine/server/sv_main.h b/r5dev/engine/server/sv_main.h index 3200e6a5..1c2e75d2 100644 --- a/r5dev/engine/server/sv_main.h +++ b/r5dev/engine/server/sv_main.h @@ -28,9 +28,9 @@ inline bool IsDedicated() void SV_InitGameDLL(); void SV_ShutdownGameDLL(); bool SV_ActivateServer(); -void SV_BroadcastVoiceData(CClient* cl, int nBytes, char* data); -void SV_IsClientBanned(CClient* pClient, const string& svIPAddr, const NucleusID_t nNucleusID, const string& svPersonaName, const int nPort); -void SV_CheckForBan(const CBanSystem::BannedList_t* pBannedVec = nullptr, const bool bDelete = false); +void SV_BroadcastVoiceData(CClient* const cl, const int nBytes, char* const data); +void SV_CheckForBanAndDisconnect(CClient* const pClient, const string& svIPAddr, const NucleusID_t nNucleusID, const string& svPersonaName, const int nPort); +void SV_CheckClientsForBan(const CBanSystem::BannedList_t* const pBannedVec = nullptr); /////////////////////////////////////////////////////////////////////////////// ///////////////////////////////////////////////////////////////////////////////