Fixes #3161: "Multiplication result converted to larger type" in client.cpp

[pljones]

Short description of changes

Intended to resolve CodeQL messages "Multiplication result converted to larger type".

CHANGELOG: Client: (Refactor) Prevent multiplication result converting to larger type

Context: Fixes an issue?

Fixes -3161

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

No.

Status of this Pull Request

Looks like it fixes the failed checks:
https://github.com/jamulussoftware/jamulus/security/code-scanning?query=pr%3A3162

(Before: https://github.com/jamulussoftware/jamulus/security/code-scanning?query=branch%3Amain)

What is missing until this pull request can be merged?

Code review by a human. Probably, smoke testing would be a good idea, too :)

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

AUTOBUILD: Please build all targets

[ann0see]

We should calculate an upper bound of these and check if that's valid.

[pljones]

i < iSndCrdFrameSizeFactor which is a small number (usually 1, 2 or 4).
iNumAudioChannels is 1 or 2.
SYSTEM_FRAME_SIZE_SAMPLES is 64 and iOPUSFrameSizeSamples is either that or twice that.

So the result should be < 16K.

[softins]

Exactly, which illustrates why the CodeQL warning is silly in a lot of contexts, including this one.

I have mentioned an alternative way to fix it in the comments on #3161, and have what I think is an even better way just compiling and about to push to a branch.

[pljones]

We're more likely to see buffer overruns -- but we don't.

vecZeros and vecsStereoSndCrd are 2 * Sound.Init ( iPrefMonoFrameSize ), so likely to be < 16K as well.

[pljones]

I think iSndCrdFrameSizeFactor is used to ensure the buffer overruns don't happen, in fact - calculated from the Sound.Init result.

[softins]

The CodeQL warnings we are addressing are nothing to do with buffer overruns. Just a perceived arithmetic overflow that will never happen with the values we are using.

[pljones]

With i, iNumAudioChannels and iOPUSFrameSizeSamples as int, however, MAX_INT * MAX_INT * MAX_INT would be out of bounds for the index type.

[ann0see]

@softins @pgScorpio I think you both have more C++ experience here.
I'd like to hear your review - please focus ONLY on this code. Design flaws are part of other issues.

[ann0see]

How would we test this? With a high number of channels all muted?

[softins]

I don't think this fixes it. The multiplication is still done at the smaller size, before casting the result to the larger size, which is already implicit in the [] operator.

To do it by casting, I think you only cast i, which makes the multiplication be done all at size_t:

...[(static_cast<size_t>(i)) * iNumAudioChannels * iOPUSFrameSizeSamples]
...

[pgScorpio]

Multiplication will be done in the type of the first operant. Nevertheless if the result is assigned to a smaller type there should still be a check to handle any overflows.

[softins]

How would we test this? With a high number of channels all muted?

We wouldn't. It's purely a theoretical overflow that is just a nuisance in CodeQL. It wouldn't actually be reached until millions of channels. The sole purpose of this change is to silence the CodeQL warnings. And it makes the code slightly less efficient to do so.