Reject out-of-range channel ID in client ID message by JohnZ622 · Pull Request #3970 · jamulussoftware/jamulus

JohnZ622 · 2026-10-09T21:04:12Z

Short description of changes
EvaluateClientIDMes accepted any 1-byte value (0-255). An ID >= MAX_NUM_CHANNELS made FindClientChannel return INVALID_INDEX, and with --mutemyown the client then called SetRemoteChanGain(-1), writing outside clientChannels[].

The behavior now is to drop the message if the ID is out of range, and assert the index in SetRemoteChanGain.

CHANGELOG:
Fixed: CClient::OnClientIDReceived could read array OOB if ID exceeds MAX_NUM_CHANNELS

Context: Fixes an issue?
Fixes: Issue #3926

Does this change need documentation? What needs to be documented and how?
No.

Status of this Pull Request
Ready to merge.

What is missing until this pull request can be merged?
Ready to merge

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

EvaluateClientIDMes accepted any 1-byte value (0-255). An ID >= MAX_NUM_CHANNELS made FindClientChannel return INVALID_INDEX, and with --mutemyown the client then called SetRemoteChanGain(-1), writing outside clientChannels[]. Drop the message if the ID is out of range, and assert the index in SetRemoteChanGain. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

coderabbitai · 2026-10-09T21:04:31Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI
Review profile: QUIET
Plan: Advanced
Run ID: ebf3a40f-eb5a-4270-927e-50a413da8e04

📥 Commits

Reviewing files that changed from the base of the PR and between bad66c0 and 1fa1f5e.

📒 Files selected for processing (2)

src/client.cpp
src/protocol.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Walkthrough

The client gain update now asserts that its channel ID is within the valid range before accessing the channel array. The protocol message evaluator rejects IDs outside that range before emitting ClientIDReceived.

Changes

Client ID validation

Layer / File(s)	Summary
Enforce channel ID bounds `src/client.cpp`, `src/protocol.cpp`	`SetRemoteChanGain` asserts that the channel ID is within range before array access. `EvaluateClientIDMes` returns an error for IDs greater than or equal to `MAX_NUM_CHANNELS`.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix

Suggested reviewers: softins

Merge Risk: ⚪ Minimal · up to 1fa1f

The protocol rejects out-of-range client IDs before dispatch, and the inspected callers provide bounded indices. No actionable merge-blocking risk introduced by this change remains.

Pre-merge checks | 4 | 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly identifies the main change: rejecting client ID messages with out-of-range channel IDs.
Description check		The description includes the required sections, explains the out-of-bounds issue, references Issue `#3926`, states documentation needs, reports status, and provides checklist information. One optional c…

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

pljones · 2026-10-10T10:16:13Z

+
    QMutexLocker locker ( &MutexGainOrPan );

    CClientChannel* clientChan = &clientChannels[iId];


So Q_ASSERT compiles to nothing in released code. This line, then, potentially returns a pointer to ... whatever "address" lies at iId * index size from clientChannels?

The point being, the moment the reader sees "Q_ASSERT" without a comment, they have to work out for themselves what the author intended and the provenance of the data.

Either the protocol handler code -- the exposed point of injection -- is sufficient, or it's not. This feels like visual clutter.

but what if some future user of SetRemoteChanGain forgets to validate the range of iId?

or maybe this function should return an true/false. i.e. return false when iId violates range.

but what if some future user of SetRemoteChanGain forgets to validate the range of iId?

Q_ASSERT doesn't do anything in release code. There is no code generated. It is not a safety net.

or maybe this function should return an true/false. i.e. return false when iId violates range.

Maybe it should just be a lambda on the connection from the protocol signal - which is validated. Unless it's exposed to other callers, in which case, yes, it should validate the parameter as it doesn't necessarily know what the caller is.

The return true/false is only any good when you know the caller will understand it. So in this case, I'd prefer an error being thrown -- it's a "This should never happen" case. (Effectively an "assert" is saying "you should never violate this"; Q_ASSERT does that in debug builds and that's fine if we had test coverage which we knew would catch every case. But we don't.)

@JohnZ622 as background: https://stackoverflow.com/a/2174550

🤖 AI: Every client protocol message runs through one queued slot whose only exception sink is the CGenErr catch around exec(), so a thrown guard here exits the client: built at 4c0dbf23, the proposed throw turns a server's out-of-range CLIENT_ID into exit ( 1 ) where a return drops it.

Maybe it should just be a lambda on the connection from the protocol signal - which is validated.

What do you mean a lambda on the connection from the protocol signal?

The return true/false is only any good when you know the caller will understand it. So in this case, I'd prefer an error being thrown

Should I throw CGenErr?

pljones · 2026-10-10T10:16:29Z

    {
        // just update the new value for sending later;
        // will compare with oldGain when the timer fires
        clientChan->newGain = fGain;


Write to some unknown location.

pljones · 2026-10-10T10:16:58Z

        // will compare with oldGain when the timer fires
        clientChan->newGain = fGain;

        // update range of channel IDs to check in the timer


Store some potentially invalid data as if it were reliable.

pljones reviewed Oct 10, 2026

View reviewed changes

ann0see added this to the Release 4.0.0 milestone Oct 11, 2026

ann0see added bug Something isn't working AI AI generated or potentially AI generated labels Oct 11, 2026

github-project-automation Bot added this to Tracking Oct 11, 2026

github-project-automation Bot moved this to Triage in Tracking Oct 11, 2026

Navigation Menu

Repository navigation

Reject out-of-range channel ID in client ID message - #3970

Reject out-of-range channel ID in client ID message#3970
JohnZ622 wants to merge 1 commit into
jamulussoftware:mainfrom
JohnZ622:fix/client-id-bounds-check

JohnZ622 commented Oct 9, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 9, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

pljones Oct 10, 2026

Uh oh!

pljones Oct 10, 2026 •

edited

Loading

Uh oh!

JohnZ622 Oct 11, 2026

Uh oh!

JohnZ622 Oct 11, 2026

Uh oh!

pljones Oct 11, 2026 •

edited

Loading

Uh oh!

ann0see Oct 11, 2026

Uh oh!

mcfnord Oct 13, 2026

Uh oh!

JohnZ622 Oct 13, 2026

Uh oh!

pljones Oct 10, 2026

Uh oh!

pljones Oct 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		QMutexLocker locker ( &MutexGainOrPan );

		CClientChannel* clientChan = &clientChannels[iId];

Conversation

JohnZ622 commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Oct 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pljones Oct 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pljones Oct 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JohnZ622 commented Oct 9, 2026 •

edited

Loading

coderabbitai Bot commented Oct 9, 2026 •

edited

Loading

pljones Oct 10, 2026 •

edited

Loading

pljones Oct 11, 2026 •

edited

Loading