mackron / dr_libs

Audio decoding libraries for C/C++, each in a single source file.
Other
1.27k stars 206 forks source link

dr_wav: potential assignment of undefined value, flagged by static Clang analyzer #155

Closed kcgen closed 3 years ago

kcgen commented 3 years ago

2020-11-10_11-36

Full report attached:

clang-analysis-report.zip

mackron commented 3 years ago

I've marked some related items as duplicates, as they all seem to be complaining about that pIn parameter. Looking through this, I'm quite confident this is a false positive.

Note that framesToRead and bytesPerFrame are the relevant variables to pay attention to. Digging in to it, see that framesToRead is assumed not 0:

image

Then see that bytesPerFrame is also assumed not 0:

image

Then drwav_read_raw() is called with the second parameter set to framesToRead * bytesPerFrame. These are both non-0, so therefore shouldn't ever evaluate to 0 unless it hits an overflow case (maybe this is what's happening internally by the Clang analyzer?):

image

However, when you enter drwav_read_raw(), you see Clang assume the second parameter is 0.

image

Note in that block how 0 is being returned. If you keep digging deeper, you see that Clang seems to think the return value was non-0...

image

However, having said all that, I have added an explicit check for the framesToRead * bytesPerFrame result evaluating to 0 in the extremely rare case that there's an overflow or something causing it: 7c446354aa7be1fc1c70acf163b656faf2011f91. This is in the dev branch. If that doesn't resolve it, I'm going to set this one to Will Not Fix.

kcgen commented 3 years ago

@mackron , the fix worked exactly as you expected. That code path when bytes-to-read are 0 is now never taken.

Interestingly, PVS reported that the maximum-size bounds checks will never exceed DR_WAV_MAX_SIZE - and indeed there are some criteria where that will be true (I added a comment in your commit).

2020-11-11_11-07

I can confirm that asserting this limit passes:

https://github.com/dosbox-staging/dosbox-staging/commit/13f3092d2931eb2edc305998bb2ae65ade68c125

kcgen commented 3 years ago

We still have the pile of unused variable warnings for runningPos, but the usual convention of describing deliberately unused variables worked perfectly:

https://github.com/dosbox-staging/dosbox-staging/commit/55fcd22634591edf48d18d183e9ee216659a4a5f

Would you be able to accept that?

Your changes, plus these two other commits now has dr_wav running clean through the latest Clang and PVS analyzers.

mackron commented 3 years ago

Yeah I'm happy to do add that line to silence those warnings. Will do that tonight.

mackron commented 3 years ago

I've added that line here: https://github.com/mackron/dr_libs/commit/e847236a1c59db85acb3ce8ba56508cfe107de6a

kcgen commented 3 years ago

Thanks @mackron! (Closing)