Skip to content
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.

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

Coverity flagged Out-of-bounds read (CWE-125) #13

Open
kcgen opened this issue Nov 28, 2021 · 3 comments
Open

Coverity flagged Out-of-bounds read (CWE-125) #13

kcgen opened this issue Nov 28, 2021 · 3 comments

Comments

@kcgen
Copy link

kcgen commented Nov 28, 2021

"Incorrect values read from a different memory region will cause incorrect computations. In OPL3_Reset: Out-of-bounds read from a buffer (CWE-125)"

2021-11-28_07-02

2021-11-28_07-03

@kcgen
Copy link
Author

kcgen commented Nov 28, 2021

This was caught in a recent Coverity scan. Happy to help run more and confirm / trial any fixes, as you see fit 👍

@kmar
Copy link

kmar commented Dec 23, 2021

well, coverity obviously doesn't understand that if ((channum % 9) < 2) will only allow max channum of 10 to actually pass the condition so no out of bounds index will ever happen (13 < 18)

this is the problem with static analyzers - they're unreliable and the typical large amount of false positives makes one waste time coding around them rather than doing something productive - which is why I rely solely on sanitizers these days. they're reliable and free

@kcgen
Copy link
Author

kcgen commented Jan 8, 2022

Thanks @kmar,

Indeed - the code as a whole is good.

Coverity's flagged the if (channum % 9) < 3) logic with the "local" flag, which will allow any batch of channums through (even those beyond 18). It hangs together because the for loop's limit prevents pushing larger number through this logic.

The % 9 logic is relative but the channel array size is absolute.

I believe Coverity would be happy either way:

  • make both relative: if the channel array would grow in proportion to this loop bounds (both become relative and scale).
  • make both absolute: convert (channum % 9) < 3) to use absolutes just like the array size:
    if (channum <= 2 || (channum >= 9 && channum <= 11)) or without magic numbers:
    if (is_channel_4op(channum) || is_channel_4op2(channum))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants