Fix out of bounds access from uninitialized m_ActiveWeapon in the prediction

CMakeLists.txt

@@ -3427,6 +3427,7 @@ if((GTEST_FOUND OR DOWNLOAD_GTEST) AND SERVER)
     dbg_test.cpp
     editor_test.cpp
     fs_test.cpp
+    gamecore_test.cpp
     gameworld_test.cpp
     git_revision_test.cpp
     hash_test.cpp

src/game/client/components/hud.cpp

@@ -834,7 +834,7 @@ void CHud::RenderCursor()
 	if(Client()->State() != IClient::STATE_DEMOPLAYBACK && GameClient()->m_Snap.m_pLocalCharacter)
 	{
 		// Render local cursor
-		CurWeapon = maximum(0, GameClient()->m_aClients[GameClient()->m_Snap.m_LocalClientId].m_Predicted.m_ActiveWeapon);
+		CurWeapon = maximum(0, GameClient()->m_aClients[GameClient()->m_Snap.m_LocalClientId].m_Predicted.m_ActiveWeapon % NUM_WEAPONS);
 		TargetPos = GameClient()->m_Controls.m_aTargetPos[g_Config.m_ClDummy];
 	}
 	else

src/game/client/prediction/entities/character.cpp

@@ -53,7 +53,7 @@ void CCharacter::HandleJetpack()
 	if(m_NumInputs < 2)
 		return;
 
-	if(m_Core.m_ActiveWeapon < 0)
+	if(!HasValidActiveWeapon())
 		return;
 
 	vec2 Direction = normalize(vec2(m_LatestInput.m_TargetX, m_LatestInput.m_TargetY));
@@ -283,7 +283,7 @@ void CCharacter::FireWeapon()
 	if(CountInput(m_LatestPrevInput.m_Fire, m_LatestInput.m_Fire).m_Presses)
 		WillFire = true;
 
-	if(FullAuto && (m_LatestInput.m_Fire & 1) && m_Core.m_ActiveWeapon >= 0 && m_Core.m_aWeapons[m_Core.m_ActiveWeapon].m_Ammo)
+	if(FullAuto && (m_LatestInput.m_Fire & 1) && HasValidActiveWeapon() && m_Core.m_aWeapons[m_Core.m_ActiveWeapon].m_Ammo)
 		WillFire = true;
 
 	if(!WillFire)
@@ -301,7 +301,7 @@ void CCharacter::FireWeapon()
 	}
 
 	// check for ammo
-	if(m_Core.m_ActiveWeapon < 0 || !m_Core.m_aWeapons[m_Core.m_ActiveWeapon].m_Ammo || m_FreezeTime)
+	if(!HasValidActiveWeapon() || !m_Core.m_aWeapons[m_Core.m_ActiveWeapon].m_Ammo || m_FreezeTime)
 	{
 		return;
 	}
@@ -491,7 +491,7 @@ void CCharacter::FireWeapon()
 	m_AttackTick = GameWorld()->GameTick(); // NOLINT(clang-analyzer-unix.Malloc)
 
 	// -1 is no weapon, handled here so pain sound still plays when firing in freeze
-	if(!m_ReloadTimer && m_Core.m_ActiveWeapon != -1)
+	if(!m_ReloadTimer && HasValidActiveWeapon())
 	{
 		m_ReloadTimer = GetTuning(GetOverriddenTuneZone())->GetWeaponFireDelay(m_Core.m_ActiveWeapon) * GameWorld()->GameTickSpeed();
 	}
@@ -1210,7 +1210,7 @@ bool CCharacter::Unfreeze()
 {
 	if(m_FreezeTime > 0)
 	{
-		if(m_Core.m_ActiveWeapon >= 0 && !m_Core.m_aWeapons[m_Core.m_ActiveWeapon].m_Got)
+		if(HasValidActiveWeapon() && !m_Core.m_aWeapons[m_Core.m_ActiveWeapon].m_Got)
 			m_Core.m_ActiveWeapon = WEAPON_GUN;
 		m_FreezeTime = 0;
 		m_Core.m_FreezeStart = 0;
@@ -1410,7 +1410,7 @@ void CCharacter::Read(CNetObj_Character *pChar, CNetObj_DDNetCharacter *pExtende
 		// ddnetcharacter is not available, try to get some info from the tunings and the character netobject instead.
 
 		// remove weapons that are unavailable. if the current weapon is ninja just set ammo to zero in case the player is frozen
-		if(m_Core.m_ActiveWeapon >= 0 && pChar->m_Weapon != m_Core.m_ActiveWeapon)
+		if(HasValidActiveWeapon() && pChar->m_Weapon != m_Core.m_ActiveWeapon)
 		{
 			if(pChar->m_Weapon == WEAPON_NINJA)
 			{
@@ -1535,7 +1535,7 @@ void CCharacter::Read(CNetObj_Character *pChar, CNetObj_DDNetCharacter *pExtende
 
 	// in most cases the reload timer can be determined from the last attack tick
 	// (this is only needed for autofire weapons to prevent the predicted reload timer from desyncing)
-	if(IsLocal && m_Core.m_ActiveWeapon != -1 && m_Core.m_ActiveWeapon != WEAPON_HAMMER && !m_Core.m_aWeapons[WEAPON_NINJA].m_Got)
+	if(IsLocal && HasValidActiveWeapon() && m_Core.m_ActiveWeapon != WEAPON_HAMMER && !m_Core.m_aWeapons[WEAPON_NINJA].m_Got)
 	{
 		if(maximum(m_LastTuneZoneTick, m_LastWeaponSwitchTick) + GameWorld()->GameTickSpeed() < GameWorld()->GameTick())
 		{

src/game/client/prediction/entities/character.h

@@ -88,6 +88,9 @@ class CCharacter : public CEntity
 	int GetLastWeapon() const { return m_LastWeapon; }
 	void SetLastWeapon(int LastWeap) { m_LastWeapon = LastWeap; }
 	int GetActiveWeapon() const { return m_Core.m_ActiveWeapon; }
+	// m_ActiveWeapon is -1 when no weapon is active, everything else has to be a
+	// valid index into m_aWeapons before it may be used as one
+	bool HasValidActiveWeapon() const { return m_Core.m_ActiveWeapon >= 0 && m_Core.m_ActiveWeapon < NUM_WEAPONS; }
 	void SetActiveWeapon(int ActiveWeapon);
 	CCharacterCore GetCore() { return m_Core; }
 	void SetCore(const CCharacterCore &Core) { m_Core = Core; }

src/game/gamecore.cpp

@@ -160,6 +160,12 @@ void CCharacterCore::Reset()
 	m_Jumps = 2;
 	m_TriggeredEvents = 0;
 
+	// m_ActiveWeapon is used to index m_aWeapons, leaving it uninitialized causes
+	// out of bounds accesses in the client prediction (CCharacter::Unfreeze() etc.)
+	m_ActiveWeapon = WEAPON_GUN;
+	for(CWeaponStat &Weapon : m_aWeapons)
+		Weapon = CWeaponStat{};
+
 	// DDNet Character
 	m_Solo = false;
 	m_Jetpack = false;

src/game/gamecore.h

@@ -241,7 +241,8 @@ class CCharacterCore
 	void Quantize();
 
 	// DDRace
-	int m_Id;
+	// -1 means no player, assigned by the owner after Reset()
+	int m_Id = -1;
 	bool m_Reset;
 	CCollision *Collision() { return m_pCollision; }
 
@@ -276,7 +277,9 @@ class CCharacterCore
 private:
 	CTeamsCore *m_pTeams;
 	int m_MoveRestrictions;
-	int m_HookedPlayer;
+	// Reset() passes this to SetHookedPlayer(), which reads it and m_Id before
+	// either has been assigned, so both need a defined value from the start
+	int m_HookedPlayer = -1;
 	static bool IsSwitchActiveCb(unsigned char Number, void *pUser);
 };
 

src/test/gamecore_test.cpp

@@ -0,0 +1,30 @@
+#include <game/gamecore.h>
+
+#include <gtest/gtest.h>
+
+// Reset() has to leave the core in a fully initialized state. Both the server and
+// the client prediction index m_aWeapons with m_ActiveWeapon (CCharacter::Unfreeze(),
+// CCharacter::FireWeapon(), ...), so an uninitialized m_ActiveWeapon is an out of
+// bounds access waiting to happen.
+TEST(GameCore, ResetInitializesWeapons)
+{
+	CCharacterCore Core;
+	// values as found in uninitialized memory
+	Core.m_ActiveWeapon = 0x5105503e;
+	for(auto &Weapon : Core.m_aWeapons)
+	{
+		Weapon.m_AmmoRegenStart = 0x5105503e;
+		Weapon.m_Ammo = 0x5105503e;
+		Weapon.m_Ammocost = 0x5105503e;
+		Weapon.m_Got = true;
+	}
+
+	Core.Reset();
+
+	EXPECT_GE(Core.m_ActiveWeapon, 0);
+	EXPECT_LT(Core.m_ActiveWeapon, NUM_WEAPONS);
+	for(const auto &Weapon : Core.m_aWeapons)
+	{
+		EXPECT_FALSE(Weapon.m_Got);
+	}
+}