dr_wav 0.13.2: clang flagged issues

[kcgen]

The recent commit caused Clang to flag 12 new issues:

Open the following index.html and sort by file to see the dr_wav.h block of new issue:

dr_wav-0.13.2-clang-issues.tar.gz

Also happy to open separate tickets, if it's easier to track.

[mackron]

The dead assignments should be fixed now in the dev branch. The others are more complicated. I'm not quite sure what's going on there. I'm hesitant to call these a false positive, but I've rearranged the code just a little bit to see if maybe it might silence those errors. I feel like these particular errors would be more useful with a sample file so I can inspect the data in a debugger.

[kcgen]

Here's the updated analysis; and good news - 3 issues have been solved as you were expecting.

(Link is to a zip artifact that will download)

https://github.com/dosbox-staging/dosbox-staging/suites/3969999617/artifacts/99801415

Unfortunately the static analyzer doesn't use sample inputs (otherwise I would gladly send it/them along).

[mackron]

I've pushed another change to the dev branch with some potential fixes for the remaining warnings. If this doesn't work, I'm not sure what else I can do to fix these. I've added some validation to ensure the number of samples cannot exceed the temp buffer which should act as a guard. Are you able to give that another run?

[kcgen]

Thanks for continuing to try to solve these @mackron ! The update takes care of another couple issues. I've attached the updated report:

clang-analysis-report.zip

Do the dr_libs have unit tests that can be compiled with gcc or clang? This would let me run the analyzer again just those test binaries to get an isolated analysis.

[mackron]

Thanks for that. Looks like it didn't fix anything. Very annoying. I have no idea what's going on with these ones.

These are the tests I use. They have some dependencies for the purpose of correctness benchmarking, however: https://github.com/mackron/dr_libs/tree/master/tests

[mackron]

I can think of one easy way to silence this error, and that is to clear the relevant buffer to 0 so that it's not treated as garbage, but I don't want to 1) mask a potential underlying bug; and 2) incur the inefficiency of clearing the buffer.

[kcgen]

Is the sampleData array (or pIn) uninitialized, and the analyser's found the first pass where it's being read in this uninitialized state?

Is it possible to initialize the buffer to zero just one time during setup? (Assuming a static buffer?)

Another thought: if we know or rely on the uninitialized buffer value(s) being zero and not garbage, then we can assert that state (say, for the [0] element) and not expend the cycles to clear that memory (even once).

[mackron]

This line will fill sampleData at the start of the loop before anything touches it:

drwav_uint64 framesRead = drwav_read_pcm_frames(pWav, framesToReadThisIteration, sampleData);

I thought maybe the return value (framesRead) is different to what is actually written to sampleData, but the implementation of drwav_read_pcm_frames() looks OK to my eye.

Putting sampleData into static space is not an option because it'll break when multiple drwav instances are running across multiple threads. Not only that, I don't want the added memory usage.

I'm thinking if this is a false positive. Take the Clang report on line 6288 (drwav_s32_to_s16). Go back to item 21 (Calling 'drwav_read_pcm_frames'). Item 40 says "Assuming 'framesRead' is not equal to 0". Now, move forward through the chain starting from 21, and go into drwav_read_raw() and observe item 36 which is the return 0; line. If you follow that back, it should be impossible for framesRead to be anything other than 0, right? Or am I missing something? If drwav_read_raw() is returning 0 (it returns the number of bytes read), how is it that drwav_read_pcm_frames() is returning anything other than 0?

I wonder if it's getting tripped up by the division in drwav_read_pcm_frames_le() in the return statement?

[kcgen]

I'm thinking if this is a false positive. Take the Clang report on line 6288 (drwav_s32_to_s16). Go back to item 21 (Calling 'drwav_read_pcm_frames'). Item 40 says "Assuming 'framesRead' is not equal to 0". Now, move forward through the chain starting from 21, and go into drwav_read_raw() and observe item 36 which is the return 0; line. If you follow that back, it should be impossible for framesRead to be anything other than 0, right?

Yes - this is also where I got confused.

go into drwav_read_raw() and observe item 36 which is the return 0; line.

Indeed; when we get back up to the while loop, the next check will detect the zero-value and break out of the while loop, so I'm not sure how execution can get into the latter portion where the 'junk' data is assigned.

[kcgen]

Just for interest sake, I zero-initialized the buffers and adjacent counters, which surprisingly fixed that class of warnings.

This at least indicates that Clang (thinks?) some form of uninitialized read is happening; but this is unlikely given your analysis through the logic. That said, zero-initializing buffers is a best practice -- I'm not sure if could become a toggle-switch at the top (default to no/false.. but projects who prefer this cold enable it?).

I looked into suppressing them, but I need to inject statements into the code itself (https://clang-analyzer.llvm.org/faq.html#suppress_issue).

I followed up with another minor commit took care of the remaining three.

Here's the clang-analysis-report.zip

[mackron]

The dead assignment errors were fixed earlier when you first posted this issue.

I've pushed to the dev branch a change to initialize the temp buffers to 0.

[kcgen]

That's got them all; not a single dr_lib is flagged now :+1:

clang-analysis-report.zip

Thank you @mackron !

[mackron]

Thanks for confirming that for me. This has been released.