Add bounds check iChanID before array access

[ann0see]

Short description of changes

Guards iChanId to definitely be in bounds. Checking the flow revealed that there is indeed no other bounds check and we may have INVALID_INDEX (=-1) which would yield to an invalid access (found by AI review)

CHANGELOG: Resolve out of bounds access issue

Context: Fixes an issue?

No. Only discussed internally.

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

Status of this Pull Request

Ready for quick !! review. This is a potentially severe bug (as discussed internally)

What is missing until this pull request can be merged?

Review and testing

Checklist

• 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

[ann0see]

the comment was wrong.

[pljones]

It includes the word if. The reason it's not size_t is because size_t is unsigned. As far as I'm aware, the comment is correct.

[ann0see]

Ok. We still need to have an int then until after the check.

[softins]

Looks good to me.

Although as a separate thing, I would prefer to make MathUtils::InRange() inclusive only of the lower bound, and exclusive of the upper bound, so that we could have MAX_NUM_CHANNELS as the upper bound instead of MAX_NUM_CHANNELS - 1, and similarly for the only other use of InRange(), which is in server.cpp

[dingodoppelt]

Looks good to me.

Although as a separate thing, I would prefer to make MathUtils::InRange() inclusive only of the lower bound, and exclusive of the upper bound, so that we could have MAX_NUM_CHANNELS as the upper bound instead of MAX_NUM_CHANNELS - 1, and similarly for the only other use of InRange(), which is in server.cpp

Can we do that as part of this PR?

[ann0see]

No.

[dingodoppelt]

No.

Created #3812

[mcfnord]

Worth merging: iChanID comes off the wire as an unvalidated unsigned byte (EvaluateConClientListMes, 0–255), and iFaderNumber is a stack-local int[150] — so a value in 150–255 is a server-controlled stack OOB write on every client. (Note the real risk is the upper bound; -1 can't arrive over the wire.) L1354 is the only wire-indexed write, so this closes it. The assumption has sat unenforced in-source since 2022. LGTM.

[pljones]

Once #3812 lands on main, this must go next. Then those two commits get ported to release/3_12 on on PR, please.

[ann0see]

Rebased and updated