jamulussoftware / jamulussoftware/jamulus

Investigate clientChannels[] out of bounds read/crash

Open
#3,926 2 comments 0 reactions 1 assignee Claimed by @ann0see View on GitHub
AI bug good first issue
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.