From adeaa41b56cb9ad93bc4d9c8438285d54a6a5b09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20M=C3=BCller?= Date: Fri, 22 Nov 2024 16:40:50 +0100 Subject: [PATCH] Fix Teehistorian server crash with MSVC due to uninitialized memory The variable `CServer::CClient::m_Authed` and the function `IServer::GetAuthedState` may only be used when the client slot is not empty, as the authed state is uninitialized before a client slot has been used for the first time. When compiling the server with MSVC the variable is not zero-initialized like with other compilers currently. This was causing the server compiled with MSVC to crash on start when Teehistorian is enabled, as empty clients were incorrectly considered as authenticated for recording the initial auth state to the Teehistorian file. For these uninitialized clients, the `IServer::GetAuthName` function was returning `nullptr` which was then causing the `CPacker::AddString` function to crash when trying to pack the `nullptr` string. The separate `IServer::ClientAuthed` function is replaced with `IServer::GetAuthedState` function. --- src/engine/server.h | 1 - src/engine/server/server.cpp | 13 ++++--------- src/engine/server/server.h | 1 - src/game/server/gamecontext.cpp | 13 +++++++++---- src/game/server/player.cpp | 2 +- 5 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/engine/server.h b/src/engine/server.h index 42e2463194e..f6d950e5f29 100644 --- a/src/engine/server.h +++ b/src/engine/server.h @@ -58,7 +58,6 @@ class IServer : public IInterface virtual int ClientCountry(int ClientId) const = 0; virtual bool ClientSlotEmpty(int ClientId) const = 0; virtual bool ClientIngame(int ClientId) const = 0; - virtual bool ClientAuthed(int ClientId) const = 0; virtual bool GetClientInfo(int ClientId, CClientInfo *pInfo) const = 0; virtual void SetClientDDNetVersion(int ClientId, int DDNetVersion) = 0; virtual void GetClientAddr(int ClientId, char *pAddrStr, int Size) const = 0; diff --git a/src/engine/server/server.cpp b/src/engine/server/server.cpp index 76da5914f14..af3d59c5819 100644 --- a/src/engine/server/server.cpp +++ b/src/engine/server/server.cpp @@ -563,16 +563,16 @@ void CServer::SetRconCid(int ClientId) int CServer::GetAuthedState(int ClientId) const { + dbg_assert(ClientId >= 0 && ClientId < MAX_CLIENTS, "ClientId is not valid"); + dbg_assert(m_aClients[ClientId].m_State != CServer::CClient::STATE_EMPTY, "Client slot is empty"); return m_aClients[ClientId].m_Authed; } const char *CServer::GetAuthName(int ClientId) const { + dbg_assert(ClientId >= 0 && ClientId < MAX_CLIENTS, "ClientId is not valid"); int Key = m_aClients[ClientId].m_AuthKey; - if(Key == -1) - { - return 0; - } + dbg_assert(Key != -1, "Client not authed"); return m_AuthManager.KeyIdent(Key); } @@ -659,11 +659,6 @@ bool CServer::ClientIngame(int ClientId) const return ClientId >= 0 && ClientId < MAX_CLIENTS && m_aClients[ClientId].m_State == CServer::CClient::STATE_INGAME; } -bool CServer::ClientAuthed(int ClientId) const -{ - return ClientId >= 0 && ClientId < MAX_CLIENTS && m_aClients[ClientId].m_Authed; -} - int CServer::Port() const { return m_NetServer.Address().port; diff --git a/src/engine/server/server.h b/src/engine/server/server.h index ec4950716fd..9e5b2c2e48f 100644 --- a/src/engine/server/server.h +++ b/src/engine/server/server.h @@ -304,7 +304,6 @@ class CServer : public IServer int ClientCountry(int ClientId) const override; bool ClientSlotEmpty(int ClientId) const override; bool ClientIngame(int ClientId) const override; - bool ClientAuthed(int ClientId) const override; int Port() const override; int MaxClients() const override; int ClientCount() const override; diff --git a/src/game/server/gamecontext.cpp b/src/game/server/gamecontext.cpp index 092a19dc939..b957f82f09e 100644 --- a/src/game/server/gamecontext.cpp +++ b/src/game/server/gamecontext.cpp @@ -4014,11 +4014,16 @@ void CGameContext::OnInit(const void *pPersistentData) for(int i = 0; i < MAX_CLIENTS; i++) { - int Level = Server()->GetAuthedState(i); - if(Level) + if(Server()->ClientSlotEmpty(i)) { - m_TeeHistorian.RecordAuthInitial(i, Level, Server()->GetAuthName(i)); + continue; + } + const int Level = Server()->GetAuthedState(i); + if(Level == AUTHED_NO) + { + continue; } + m_TeeHistorian.RecordAuthInitial(i, Level, Server()->GetAuthName(i)); } } @@ -4462,7 +4467,7 @@ void CGameContext::OnSetAuthed(int ClientId, int Level) } if(m_TeeHistorianActive) { - if(Level) + if(Level != AUTHED_NO) { m_TeeHistorian.RecordAuthLogin(ClientId, Level, Server()->GetAuthName(ClientId)); } diff --git a/src/game/server/player.cpp b/src/game/server/player.cpp index f4c54044923..12767386b48 100644 --- a/src/game/server/player.cpp +++ b/src/game/server/player.cpp @@ -378,7 +378,7 @@ void CPlayer::Snap(int SnappingClient) pPlayerInfo->m_PlayerFlags = PlayerFlags_SixToSeven(m_PlayerFlags); if(SnappingClientVersion >= VERSION_DDRACE && (m_PlayerFlags & PLAYERFLAG_AIM)) pPlayerInfo->m_PlayerFlags |= protocol7::PLAYERFLAG_AIM; - if(Server()->ClientAuthed(m_ClientId)) + if(Server()->GetAuthedState(m_ClientId) != AUTHED_NO) pPlayerInfo->m_PlayerFlags |= protocol7::PLAYERFLAG_ADMIN; // Times are in milliseconds for 0.7