From 29d640f51348ddb16d75d0abcc8a11a3a4286632 Mon Sep 17 00:00:00 2001 From: yorushi-code Date: Wed, 19 Aug 2026 06:23:24 -0400 Subject: [PATCH 1/3] Fix uninitialized m_ActiveWeapon crashing the prediction CCharacterCore::Reset() left m_ActiveWeapon and m_aWeapons untouched, so a freshly constructed prediction character kept whatever garbage was in the heap until CCharacter::Read() assigned a weapon. Read() only assigns it when the snapshotted weapon is not WEAPON_NINJA, and the server snaps WEAPON_NINJA without CHARACTERFLAG_WEAPON_NINJA for ninjajetpack tees and for frozen tees on old clients. Such a tee never got a valid m_ActiveWeapon, and the next CCharacter::Unfreeze() indexed m_aWeapons[m_ActiveWeapon] (the >= 0 check has no upper bound), reading roughly 16 * garbage bytes past the object: mov 0x408(%rdi),%eax ; m_Core.m_ActiveWeapon = 0x5105503e test %eax,%eax js ... ; only the >= 0 guard shl $0x4,%rax ; * sizeof(CWeaponStat) cmpb $0x0,0x418(%rdi,%rax,1) ; m_aWeapons[i].m_Got -> SIGSEGV The server already sets m_ActiveWeapon = WEAPON_GUN right after Reset() in CCharacter::Spawn(), the prediction never did. --- CMakeLists.txt | 1 + src/game/gamecore.cpp | 6 ++++++ src/test/gamecore_test.cpp | 30 ++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+) create mode 100644 src/test/gamecore_test.cpp 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/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/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); + } +} From 466ecd54906cd0ccf6e23a53fdb6d0668d42b5e4 Mon Sep 17 00:00:00 2001 From: yorushi-code Date: Wed, 19 Aug 2026 06:23:35 -0400 Subject: [PATCH 2/3] Bound-check m_ActiveWeapon before using it as an index The existing guards only checked m_ActiveWeapon >= 0, so any value past NUM_WEAPONS - 1 indexed m_aWeapons out of bounds - including an out of bounds write in CCharacter::Read(). CTuningParams::GetWeaponFireDelay() asserts on anything else than a real weapon, and the HUD indexed m_aSpriteWeaponCursors with the predicted weapon without an upper bound either. Add CCharacter::HasValidActiveWeapon() and use it in every place that indexes with m_ActiveWeapon, keeping the "-1 means no weapon" semantics unchanged. --- src/game/client/components/hud.cpp | 2 +- src/game/client/prediction/entities/character.cpp | 14 +++++++------- src/game/client/prediction/entities/character.h | 3 +++ 3 files changed, 11 insertions(+), 8 deletions(-) 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; } From f2ea80cf1c25ea27f2a573408dc21e63a0f994ec Mon Sep 17 00:00:00 2001 From: yorushi-code Date: Wed, 19 Aug 2026 07:44:44 -0400 Subject: [PATCH 3/3] Give m_Id and m_HookedPlayer a defined initial value CCharacterCore::Reset() calls SetHookedPlayer(-1), which reads m_HookedPlayer and m_Id before either has been assigned - the owner assigns them after Reset(), both in CCharacter::Spawn() on the server and in the prediction CCharacter constructor. valgrind memcheck on a default initialized core, current master: Conditional jump or move depends on uninitialised value(s) at CCharacterCore::SetHookedPlayer(int) (gamecore.cpp:711) by CCharacterCore::Reset() (gamecore.cpp:161) Uninitialised value was created by a heap allocation Three such reports, at gamecore.cpp:711 and :713. Nothing worse happens today only because m_pWorld is still null at that point and short circuits the branch that would do m_pWorld->m_apCharacters[m_HookedPlayer]. Initializing in the declaration rather than in Reset() keeps Reset()'s detach logic intact for cores that really were hooked to someone. --- src/game/gamecore.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) 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); };