jamulussoftware / jamulussoftware/jamulus
Investigate clientChannels[] out of bounds read/crash
- Dominant language
- C
- Stars
- 1.1k
- Forks
- 248
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 9
Description
**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:
https://github.com/jamulussoftware/jamulus/blob/508f1f3d3ea41b0da76da128ee5d3f3f1b2fafc7/src/client.cpp#L1020-L1031
**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):
```cpp
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`):
```cpp
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`).
Contributor guide
Assessment
This issue has not been assessed yet.