Nuked-OPL3 icon indicating copy to clipboard operation
Nuked-OPL3 copied to clipboard

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

Open kcgen opened this issue 3 years ago • 3 comments

"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 avatar Nov 28 '21 15:11 kcgen

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

kcgen avatar Nov 28 '21 15:11 kcgen

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

kmar avatar Dec 23 '21 08:12 kmar

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))

kcgen avatar Jan 08 '22 21:01 kcgen