Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pljones wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1257,15 +1257,15 @@ void CClient::ProcessAudioDataIntern ( CVector<int16_t>& vecsStereoSndCrd )
if ( bMuteOutStream )
{
iUnused = opus_custom_encode ( CurOpusEncoder,
&vecZeros[i * iNumAudioChannels * iOPUSFrameSizeSamples],
&vecZeros[static_cast<size_t> ( i * iNumAudioChannels * iOPUSFrameSizeSamples )],
Copy link
Copy Markdown
Member

@ann0see ann0see Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

@pljones pljones Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@softins softins Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

@pljones pljones Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

@pljones pljones Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@softins softins Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

@pljones pljones Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@softins softins Aug 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
...

Copy link
Copy Markdown
Contributor

@pgScorpio pgScorpio Aug 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

iOPUSFrameSizeSamples,
&vecCeltData[0],
iCeltNumCodedBytes );
}
else
{
iUnused = opus_custom_encode ( CurOpusEncoder,
&vecsStereoSndCrd[i * iNumAudioChannels * iOPUSFrameSizeSamples],
&vecsStereoSndCrd[static_cast<size_t> ( i * iNumAudioChannels * iOPUSFrameSizeSamples )],
iOPUSFrameSizeSamples,
&vecCeltData[0],
iCeltNumCodedBytes );
Expand Down Expand Up @@ -1311,7 +1311,7 @@ void CClient::ProcessAudioDataIntern ( CVector<int16_t>& vecsStereoSndCrd )
iUnused = opus_custom_decode ( CurOpusDecoder,
pCurCodedData,
iCeltNumCodedBytes,
&vecsStereoSndCrd[i * iNumAudioChannels * iOPUSFrameSizeSamples],
&vecsStereoSndCrd[static_cast<size_t> ( i * iNumAudioChannels * iOPUSFrameSizeSamples )],
iOPUSFrameSizeSamples );
}
}
Expand Down