diff --git a/CMakeLists.txt b/CMakeLists.txt index 40818900b83..8ed3b2e81d9 100644 --- a/CMakeLists.txt +++ b/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 diff --git a/src/game/client/components/hud.cpp b/src/game/client/components/hud.cpp index 4c9dad87da5..858b73cadc1 100644 --- a/src/game/client/components/hud.cpp +++ b/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 diff --git a/src/game/client/prediction/entities/character.cpp b/src/game/client/prediction/entities/character.cpp index dee551dc925..4ccefd4648b 100644 --- a/src/game/client/prediction/entities/character.cpp +++ b/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()) { diff --git a/src/game/client/prediction/entities/character.h b/src/game/client/prediction/entities/character.h index 3cdddbc64ae..6116ffbbc03 100644 --- a/src/game/client/prediction/entities/character.h +++ b/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; } diff --git a/src/game/gamecore.cpp b/src/game/gamecore.cpp index e29fe2b3e69..ce994c3e910 100644 --- a/src/game/gamecore.cpp +++ b/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; diff --git a/src/game/gamecore.h b/src/game/gamecore.h index 07eb261e957..eac562e4bf0 100644 --- a/src/game/gamecore.h +++ b/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); }; diff --git a/src/test/gamecore_test.cpp b/src/test/gamecore_test.cpp new file mode 100644 index 00000000000..c11d357a720 --- /dev/null +++ b/src/test/gamecore_test.cpp @@ -0,0 +1,30 @@ +#include + +#include + +// 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); + } +}