Backport #3810: bounds check iChanID before array access (release/3_12)

[mcfnord]

🤖 AI: Backports #3810's iChanID bounds check to release/3_12. The check calls MathUtils::InRange, which reached main inside #3741's chat-handling refactor and got its half-open interval in #3812, so at ann0see's request the branch is the original commits cherry-picked with -x, authorship preserved, in main's order: #3741's 562d6c66, 16b1ade0, 4f9a532a, 1e1f54ab; #3812's 6d305cb7, 65513e5b, fe98b1ad; #3810's 562cfd84. Base is release/3_12 at e6a87df6.

One conflict, in 6d305cb7: its server.cpp hunk edits the bool form of SendChatTextToConChannel that #3731 gave the method after #3741. Resolved by keeping 562d6c66's void form with the half-open check. #3731 is not on this branch, so SendChatTextToConChannel has no caller here.

MathUtils in util.h is byte-identical to main; the audiomixerboard.cpp hunk is 562cfd84 unchanged; the broadcast path emits the same IsConnected/CreateChatTextMes sequence as before. clang-format-14 --dry-run --Werror is clean on the four touched files; server and client builds link with no warning in any touched file.

Fixes the same issue as #3810, for release/3_12.

---

🤖 This message was written by AI and reviewed by @mcfnord.

[ann0see]

I don't like this inlining: we should ship the same code as on main.

[mcfnord]

MY LLM WROTE:

@ann0see Fair — pushed 57525a2b to ship the same code as main instead. Rather than inline the check, I backported just MathUtils::InRange (the template method itself, half-open-interval semantics from #3812) into class MathUtils in util.h. release/3_12's MathUtils class already exists, so this is a single self-contained method addition — no dependency on the rest of #3741/#3812's chat refactor. audiomixerboard.cpp's ApplyNewConClientList now calls MathUtils::InRange<int>( iChanID, 0, MAX_NUM_CHANNELS ), byte-for-byte identical to main's 562cfd84.

Verified: clang-format-14 --dry-run --Werror clean, full client build (qmake && make) compiles and links with no errors/warnings.

[pljones]

OK, that's small enough.

[ann0see]

I still prefer the exact same commits cherry picked.

[dingodoppelt]

I'd prefer cherry-picking the original commits. We didn't squash on merge so there are a few which could be squashed only for this backport.

[mcfnord]

MY LLM WROTE:

Force-pushed with actual cherry-picks instead of the hand-written equivalent.

29a13e49 squashes just the src/util.h hunks from the six main commits that shaped MathUtils::InRange into its current form — #3741's 562d6c66 + 4f9a532a, then #3812's 6d305cb7 + 65513e5b + fe98b1ad — applied in that order. I left out their server.cpp/server.h hunks: those belong to #3741's SendChatTextToConChannel/SendChatTextToAllConChannels refactor, which release/3_12 doesn't have and which is out of scope for a bounds-check backport. Squashing was necessary because several of those six commits are one-line follow-ups to each other (comment wording, style) — cherry-picking them individually would have produced intermediate states that don't compile standalone once the chat-refactor hunks are dropped.

f0180425 is git cherry-pick -x 562cfd84 unmodified — same commit, same author, (cherry picked from commit ...) trailer intact.

Net result is identical to the previous push, plus one line I'd missed: the // Returns true if value is in [lower, upper)... comment above InRange, which main has and my earlier hand-typed version didn't. Confirmed byte-for-byte against MathUtils::InRange on main.

Verified: clang-format-14 --dry-run --Werror clean, full client build (qmake && make) compiles and links Jamulus with no errors/warnings.

[ann0see]

Those are still not the same commits as we have on main - due to different authorship. I think you should just git cherrypick the correct ones in the correct order.

[pljones]

I'd prefer cherry-picking the original commits. We didn't squash on merge so there are a few which could be squashed only for this backport.

I failed to find clean ones.

[pljones]

And again.

Please do not set code that has not landed on main to a release branch. Until it's clean and tested on main, it's not eligible for a backport > and that means it can't target a release branch.

(repeating myself unnecessarily...)

[mcfnord]

MY LLM WROTE:

Thanks both. The branch already ships main's code: MathUtils::InRange in util.h is byte-for-byte identical to main, and the audiomixerboard.cpp bounds-check equals #3810's 562cfd84.

The only open question is commit structure. A faithful git cherry-pick -x 562d6c66 onto release/3_12 applies cleanly — but it carries SendChatTextToConChannel/SendChatTextToAllConChannels and their server.cpp/server.h changes with it (6d305cb7 does the same), landing a server chat refactor on a bug-fix branch. That's why this branch squashes only the util.h hunks instead.

So: (a) keep the util.h-only commit — identical to main, no server changes; or (b) cherry-pick the originals and accept the chat refactor on release/3_12. Happy either way — just flagging (b) isn't code-neutral here.

[ann0see]

Not sure. It might be fine to have the chat refactor in.

[ann0see]

@mcfnord I think you should rebase this and add the chat refactor.

Needs re review

Re review

[dingodoppelt]

This looks fine, compiles and works as expected.

[pljones]

Rebase error?

[mcfnord]

🤖 AI: Not a rebase error: the chat refactor is in on purpose. ann0see asked for it, since MathUtils::InRange reached main inside that refactor (#3741) and got its half-open interval in #3812. The branch is eight git cherry-pick -x of the original commits onto release/3_12 at e6a87df6, in main's order: #3741's 562d6c66, 16b1ade0, 4f9a532a, 1e1f54ab; #3812's 6d305cb7, 65513e5b, fe98b1ad; then #3810's 562cfd84. Original authorship and trailers are kept, and all of it was merged on main by July.

One conflict, in 6d305cb7: its server.cpp hunk edits the bool form of SendChatTextToConChannel that #3731 gave the method after #3741. Resolved by keeping 562d6c66's void form with the half-open check. #3731 is not on release/3_12, so the method has no caller here, as on main between #3741 and #3731.

MathUtils in util.h is byte-identical to main, the audiomixerboard.cpp hunk is 562cfd84 unchanged, and the broadcast path emits the same calls as before. The PR description was still the July inline version; it is updated to match.

[pljones]

No, I don't think we can be adding those two methods on the backport branch. Only the template<typename T> static inline bool InRange(..) and CAudioMixerBoard::ApplyNewConClientList change should be here.

[ann0see]

But that's harder to get only the template out - unless it was in separate commits?

[pljones]

It doesn't matter.

The code that needs backporting is clearly defined.

Cherry-picking is good if that's clean. Here, it's bad because it brings in unwanted extra code. So here we do not want to cherry-pick.

Instead it needs a piecemeal backport of the amended lines.

[pljones]

@mcfnord I think you should rebase this and add the chat refactor.

Sorry, I missed this. I disagree. It's better to fix the bug with a clean and clear patch. It's not religion.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: QUIET

Plan: Advanced

Run ID: 0d9453e7-b0dc-497f-8b92-7cc40671f826

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

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