Investigate clientChannels[] out of bounds read/crash

[ann0see]

Describe the bug

INVALID_INDEX might not be checked everywhere correctly. AI found out that there could be an out of bounds read possiblity in client.cpp if calling SetRemoteChanGain:

jamulus/src/client.cpp

Lines 1020 to 1031 in 508f1f3

	// allocate and map client-side channel 0
	int iChanID = FindClientChannel ( iServerChanID, true ); // should always return channel 0
	// for headless mode we support to mute our own signal in the personal mix
	// (note that the check for headless is done in the main.cpp and must not
	// be checked here)
	if ( bMuteMeInPersonalMix )
	{
	SetRemoteChanGain ( iChanID, 0, false );
	}

To Reproduce

Not tested. Would probably need some server side trigger with invalid Channel ID.
Probably worth checking on the protocol level for invalid IDs.

Ox Alpha (GLM 5.3-Flash) suggested:

Guard the call site (minimal change):

if ( iChanID != INVALID_INDEX && bMuteMeInPersonalMix ) { ... }

and/or reject invalid IDs in EvaluateClientIDMes; optionally add a defensive Q_ASSERT/range check in SetRemoteChanGain/SetRemoteChanPan mirroring OnControllerInFaderLevel (client.cpp:956). Related latent gap: OnControllerInPanValue (client.cpp:965-975) lacks the bounds check its fader sibling has.

Expected behavior

No crash

AI analysis

• src/protocol.cpp:1046-1063 (EvaluateClientIDMes) — validates only body size (1 byte); the ID value itself (0–255) is passed through unchecked.
• src/client.cpp:1010-1036 (CClient::OnClientIDReceived):

int iChanID = FindClientChannel ( iServerChanID, true ); // should always return channel 0
...
if ( bMuteMeInPersonalMix )
{
    SetRemoteChanGain ( iChanID, 0, false );   // iChanID can be INVALID_INDEX (-1)
}

• src/client.cpp:1833-1885 (FindClientChannel) returns INVALID_INDEX (-1) when iServerChannelID < 0 || >= MAX_NUM_CHANNELS or when all 150 client channel slots are occupied.
• src/client.cpp:506-539 (SetRemoteChanGain): &clientChannels[iId] with no bounds check →
  • timer inactive: OOB write at client.cpp:535 (clientChan->oldGain = clientChan->newGain = fGain;) plus OOB read of iServerChannelID fed to Channel.SetRemoteChanGain() (that callee is range-checked, channel.cpp:302);
  • timer active: OOB write at client.cpp:522, and minGainOrPanId = -1 causes OnTimerRemoteChanGainOrPan (client.cpp:546-548) to iterate from index −1 afterwards.
• clientChannels is a fixed CClientChannel[150] member array (src/client.h:392).

Contrast with the correct pattern used 700 lines earlier: OnMuteStateHasChangedReceived checks if ( iChanID != INVALID_INDEX ) (client.cpp:340-346).

[mcfnord]

🤖 AI: Reproduced end-to-end under AddressSanitizer at 508f1f3d — confirmed. A CLIENT_ID whose byte is ≥ MAX_NUM_CHANNELS drives FindClientChannel to INVALID_INDEX, and SetRemoteChanGain ( iChanID, 0, false ) writes 4 bytes into the CClientChannel slot immediately before the array (client.cpp:535). ASan trace: ParseMessageBody → EvaluateClientIDMes (only the 1-byte size is checked) → OnClientIDReceived → that write.

Two refinements:

• Headless --mutemyown only — the call is gated by bMuteMeInPersonalMix, which the GUI disables. Exposure is a headless --mutemyown client on a hostile or buggy server; a normal server never assigns an ID ≥ MAX_NUM_CHANNELS.
• Silent on a stock build — with the in-object clientChannels[MAX_NUM_CHANNELS] array the same input neither crashes nor trips ASan ([-1] hits adjacent CClient members); it surfaced only once that array was heap-allocated for redzones. So the effect is corruption, not a reliable crash.

The sibling OnControllerInPanValue gap reads latent-only — its only emitters (MIDI mapping, JSON-RPC fader) already bound the channel index.

[ann0see]

Yep. GLM also time me that it's only happening if mutemyown is set. It's definitely not severe and easy to fix.