Fix out of bounds access from uninitialized m_ActiveWeapon in the prediction

[yorushi-code]

Problem

The client segfaults in CCharacter::Unfreeze() (prediction). Reproducible on a tee whose snapshotted weapon is WEAPON_NINJA:

mov    0x408(%rdi),%eax          ; m_Core.m_ActiveWeapon = 0x5105503e (heap garbage)
test   %eax,%eax
js     ...                       ; only the >= 0 guard, no upper bound
shl    $0x4,%rax                 ; * sizeof(CWeaponStat)
cmpb   $0x0,0x418(%rdi,%rax,1)   ; m_aWeapons[i].m_Got -> reads ~20 GB past the object

CCharacterCore::Reset() never initializes m_ActiveWeapon or m_aWeapons, so a freshly constructed prediction character keeps whatever was in the heap until CCharacter::Read() assigns 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 (m_ActiveWeapon is still WEAPON_GUN, so the flag is not set) and for frozen tees on old clients. Such a character never gets a valid m_ActiveWeapon, and the next Unfreeze() indexes m_aWeapons out of bounds.

Note that address watchpoints cannot catch this: nothing writes the garbage, and CGameWorld::CopyWorld re-news the prediction characters every tick.

A second uninitialized read, found with valgrind

While verifying this upstream I ran memcheck on this tree as well. Reset() also reads m_HookedPlayer and m_Id before anything assigns them — SetHookedPlayer(-1) at gamecore.cpp:156 compares against both at :703 and :705, and the owner assigns them after Reset():

$ valgrind --tool=memcheck --track-origins=yes ./testrunner --gtest_filter='UninitProbe.*'

==147760== Conditional jump or move depends on uninitialised value(s)
==147760==    at CCharacterCore::SetHookedPlayer(int) (gamecore.cpp:703)
==147760==    by CCharacterCore::Reset() (gamecore.cpp:156)
==147760==  Uninitialised value was created by a heap allocation
...
==147760== ERROR SUMMARY: 4 errors from 4 contexts

Three of those are m_HookedPlayer / m_Id, the fourth is the m_ActiveWeapon read above. Nothing worse happens today only because m_pWorld is still null at that point and short circuits the branch that would evaluate m_pWorld->m_apCharacters[m_HookedPlayer]. After this PR the same probe reports ERROR SUMMARY: 0 errors from 0 contexts.

Changes

1. Fix uninitialized m_ActiveWeapon crashing the prediction — Reset() sets m_ActiveWeapon = WEAPON_GUN and clears m_aWeapons, the way the server already does by hand in CCharacter::Spawn().
2. Bound-check m_ActiveWeapon before using it as an index — the guards only checked >= 0. Adds CCharacter::HasValidActiveWeapon() and uses it everywhere m_ActiveWeapon indexes m_aWeapons, which also closes an out of bounds write in Read() and avoids CTuningParams::GetWeaponFireDelay()'s dbg_assert_failed("invalid weapon"). -1 still means "no weapon". CHud::RenderCursor() gets the same % NUM_WEAPONS clamp the other cursor sites in that file already use.
3. Give m_Id and m_HookedPlayer a defined initial value — default member initializers, fixing the reads above. Initializing in the declaration rather than inside Reset() keeps Reset()'s detach logic intact for cores that really were hooked to someone.

Upstream status

Worth knowing before you next merge DDNet:

• The prediction-constructor half of this is already fixed upstream by client: More pred character initializations ddnet/ddnet#12547, merged about a week ago. It assigns m_Core.m_ActiveWeapon = -1 in the prediction CCharacter constructor plus an else if(m_Core.m_ActiveWeapon < 0) fallback in Read(). This tree predates that PR, which is why the crash reproduces here and not on current DDNet master.
• Changes 1 and 3 are proposed upstream as Fix uninitialized reads in CCharacterCore::Reset() ddnet/ddnet#12653. Change 2 is in that PR too, offered as optional hardening.

So a future DDNet merge will bring ddnet#12547 in on its own, and ddnet#12653 as well if it lands. Since these commits touch the same lines as the upstream ones, the merge should converge rather than conflict. If you would rather not carry a fork-local patch at all, the minimal alternative is cherry-picking ddnet#12547 — that stops the crash, but leaves Reset() handing out undefined m_ActiveWeapon and the m_HookedPlayer read in place.

Tests

New src/test/gamecore_test.cpp fills a core with garbage, calls Reset(), and asserts m_ActiveWeapon is a valid index and no weapon is marked as got. It fails without change 1 and passes with it. Full suite 339/339. Running in daily play since 2026-08-15.

The memcheck probe is a throwaway, not part of this PR; happy to attach it.

AI was used to draft the patch and this description. I reviewed all of it and verified the change independently — the unit test fails without the fix and passes with it, memcheck goes from 4 errors to 0, and the full suite passes.

[yorushi-code]

For context: upstream fixed part of this differently. ddnet#12547 added m_Core.m_ActiveWeapon = -1 to the prediction CCharacter constructor plus an else if(m_Core.m_ActiveWeapon < 0) fallback in Read(). TClient's base predates that PR, so neither exists here — which is why the crash reproduces on TClient but not on current DDNet master.

That means you have two options: cherry-pick ddnet#12547, or take this, which fixes it in CCharacterCore::Reset() instead so no construction path has to remember to assign the weapon afterwards. I opened the same change upstream as ddnet#12653, since Reset() leaving m_ActiveWeapon uninitialized is still a live out of bounds index there via CHud::RenderCursor().

[yorushi-code]

Updated this PR: added a third commit and rewrote the description, since verifying the change upstream turned up more than I started with.

Short version of what matters for TClient, given you will pull DDNet at some point:

The crash is live here and not upstream. ddnet#12547 landed about a week ago and assigns m_Core.m_ActiveWeapon = -1 in the prediction CCharacter constructor. master_new predates it, so on this tree a WEAPON_NINJA snapshot still leaves m_ActiveWeapon as heap garbage and the next Unfreeze() indexes m_aWeapons with it.

There is a second issue that upstream has too. Reset() calls SetHookedPlayer(-1) at gamecore.cpp:156, which reads m_HookedPlayer and m_Id before anything assigns them. memcheck on this tree reports 4 uninitialized reads, 0 after this PR. It is harmless today only because m_pWorld is still null there and short circuits the branch that would do m_pWorld->m_apCharacters[m_HookedPlayer].

Three options, in the order I would rank them:

1. Take this PR. Fixes both at the initializer, so it stays correct regardless of when you merge DDNet. These are the same edits proposed upstream in Fix uninitialized reads in CCharacterCore::Reset() ddnet/ddnet#12653, so a later merge should converge on the same lines rather than fight them.
2. Cherry-pick client: More pred character initializations ddnet/ddnet#12547 only. Stops the crash with a change you will get anyway on the next merge, but leaves Reset() handing out an undefined m_ActiveWeapon and the m_HookedPlayer read in place.
3. Wait for the next DDNet merge. That fixes the crash whenever it happens, but anyone on a ninjajetpack or frozen on an old client can segfault until then.

If you want this narrower, I can drop commit 2 (the bounds checks and the HUD clamp) and leave just the two initialization commits — say the word.

[yorushi-code]

Correction to what I wrote above, before you spend time on it.

The root cause I gave for this crash is wrong, and I have retracted it upstream in ddnet#12653 (comment here). Short version: m_ActiveWeapon is never undefined at construction. Both CEntity allocators in src/game/alloc.h mem_zero() the whole object before any constructor runs, so Reset() always has defined memory, and memcheck on the real spawn path reports nothing.

Going back to the coredump from the crash on this client: the object is intact — valid vtable, proximity radius 28.0f, sane position — right up to m_ActiveWeapon, which held 0x503d510f. The faulting address is exactly this + 0x418 + 16 * 0x503d510f, and everything in the object past that offset decodes as StrToInts-packed strings. So the field was overwritten with foreign data. Something is corrupting the object; initialising it earlier does not stop that.

What that means for this PR:

• The initialisation commits do not fix the crash. They were aimed at a cause that is not there.
• The bounds checks would have stopped the segfault, but only by turning a corrupted read into a silent no-op, which would have made the real bug harder to find rather than fixing it.

I would rather not have you merge this on a wrong premise, so treat my earlier "take this PR" recommendation as withdrawn. I am going to chase the actual corruption with a sanitizer build and come back with something that stands up. Happy to close this in the meantime if you prefer a clean queue.

[SollyBunny]

If a fix is available upstream, I would not merge a different fix down here

#208