btrfs-progs: initial scan-build results

Hi,

With btrfs-progs needing & getting some more love I decided to run today's master through clang's very awesome static analyzer [1] to see what a more complete data flow analysis than gcc's -Wall yields. The results can be found at [2] and are somewhat reason for concern. =:)

Please note that even though all messages are typically "real" in the sense that they _could_ happen, it does not mean that they do during normal operation, since some codepaths might just be dynamic/rare. That being said, quite a few warnings seemed sufficiently serious to me that I decided to post this. For example there's IMHO no sane way zero-sized allocations make any sense.

IMHO most dead stores are seemingly the easiest to fix (just remove the statement?), but some of them might actually be missing upstream error handling - indicative of something more serious.

Dave, any suggestions on how best to proceed? Any preferences or would another branch be more interesting? I tried to track devel but that gets rebased frequently (or I'm doing something wrong).

Btw running scan-build is easy: get clang, './configure' as usual and 'scan-build make -jX' will create the report in /tmp/scan-build-<time>.

Let me know if this is helpful.

cheers,
Holger

[1] http://clang-analyzer.llvm.org/scan-build.html
[2] http://hoho.duckdns.org/~holger/btrfs-progs/scan-build-2016-03-01-130244-29106-1/

2016-03-01 13:44 GMT+01:00 Holger Hoffstätte
<[email protected]>:

All zero-sized allocations are false positives, except the btrfs-image.c. This can be fixed by placing the num_threads at the top instead of after calloc().

I checked a few, those were false positives.

Hi,

thanks for taking a look. I hadn't actually delved in too deeply yet.

Well..I stepped into metadump_init() and num_pthreads is definitely 0; setting it to 1 before continuing starts & coordinates with a new thread, as expected. What the analyser complains about, and what I also didn't know until just now because I stopped remembering C standard details shortly after approx. BSD 4.2 ;-) is the second half of the following bit from calloc(1):

If nmemb or size is 0, then calloc() returns either NULL, or a unique pointer value that can later be successfully passed to free()

So this works implicitly and is OK since it still checks for NULL and num_pthreads==0 later on.

Well..good thing we talked about it. :-)

cheers
Holger

Right, I've now also reviewed them all and they are all guarded by other conditions or plain wrong - very likely because this particular warning itself turns out to be a bug in llvm:
https://llvm.org/bugs/show_bug.cgi?id=15708

!"§$%

-h

Thanks for running the scan. I've played with the static scanner in the past and my evaluation found like 5% of issues worth fixing, but more or less low or moderate importance. The real value I see is that one has to review some part of code and this usually leads to fixes.

The quality of the reports is IMHO questionable, just look at the path of the reported problems, counted by branches and some important code points. I've simply skipped anything that was more than like 20. Keeping the whole context in head was hard and sometims there were "cannot happen" conditions that were not visible to the checker.

I'm submitting progs to coverity regularly, the reports are totally different from clang checker, and usually worth fixing. Also tracking errors is a bit more friendly as it has the web ui to classify them. A resubmission will mark off the fixed errors. Processing new batch of errors means going through like 2-10 new errors, and it's usually on code that I've seen recently.

Overall, I think it's good to try different checkers from time to time, as the quality supposedly increases.