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.
Robyt3 · Nov 22, 2024 · adeaa41 · adeaa41
1 parent 3dab25b
commit adeaa41
Show file tree

Hide file tree

Showing 5 changed files with 14 additions and 16 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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