btrfs-progs: initial scan-build results

btrfs-progs: initial scan-build results

[Holger Hoffstätte]

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/

Re: btrfs-progs: initial scan-build results

[Alexander Fougner]

2016-03-01 13:44 GMT+01:00 Holger Hoffstätte
<holger.hoffstaette@googlemail.com>:
> 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.
>

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

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

I checked a few, those were false positives.

> 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/
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: btrfs-progs: initial scan-build results

[Holger Hoffstätte]

Hi,

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

On 03/01/16 14:41, Alexander Fougner wrote:
> 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().

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

Re: btrfs-progs: initial scan-build results

[Holger Hoffstätte]

On 03/01/16 16:45, Holger Hoffstätte wrote:
> On 03/01/16 14:41, Alexander Fougner wrote:
>> All zero-sized allocations are false positives, except the

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

Re: btrfs-progs: initial scan-build results

[David Sterba]

On Tue, Mar 01, 2016 at 05:39:29PM +0100, Holger Hoffstätte wrote:
> On 03/01/16 16:45, Holger Hoffstätte wrote:
> > On 03/01/16 14:41, Alexander Fougner wrote:
> >> All zero-sized allocations are false positives, except the
> 
> 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

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.