Re: [Qemu-devel] [BUG] 216 Alerts reported by LGTM for QEMU (some might be release critical)

[Kevin Wolf]

Am 14.07.2019 um 15:28 hat Stefan Weil geschrieben:
> Am 13.07.2019 um 21:42 schrieb Paolo Bonzini:
> > On 13/07/19 19:46, Stefan Weil wrote:
> >> LGTM reports 16 errors, 81 warnings and 119 recommendations:
> >> https://lgtm.com/projects/g/qemu/qemu/alerts/?mode=list.
> >>
> >> Some of them are already known (wrong format strings), others look like
> >> real errors:
> >>
> >> - several multiplication results which don't work as they should in
> >> contrib/vhost-user-gpu, block/* (m->nb_clusters * s->cluster_size only
> >> 32 bit!),  target/i386/translate.c and other files

Request sizes are limited to 32 bit in the generic block layer before
they are even passed to the individual block drivers, so most if not all
of these are going to be false positives.

> > m->nb_clusters here is limited by s->l2_slice_size (see for example
> > handle_alloc) so I wouldn't be surprised if this is a false positive.  I
> > couldn't find this particular multiplication in Coverity, but it has
> > about 250 issues marked as intentional or false positive so there's
> > probably a lot of overlap with what LGTM found.
> >
> > Paolo
> 
> From other projects I know that there is a certain overlap between the
> results from Coverity Scan an LGTM, but it is good to have both
> analyzers, and the results from LGTM are typically quite reliable.
> 
> Even if we know that there is no multiplication overflow, the code could
> be modified. Either the assigned value should use the same data type as
> the factors (possible when there is never an overflow, avoids a size
> extension), or the multiplication could use the larger data type by
> adding a type cast to one of the factors (then an overflow cannot
> happen, static code analysers and human reviewers have an easier job,
> but the multiplication costs more time).

But if you look at the code we're talking about, you see that it's
complaining about things where being more explicit would make things
less readable.

For example, if complains about the multiplication in this line:

    s->file_size += n * s->header.cluster_size;

We know that n * s->header.cluster_size fits in 32 bits, but
s->file_size is 64 bits (and has to be 64 bits). Do you really think we
should introduce another uint32_t variable to store the intermediate
result? And if we cast n to uint64_t, not only might the multiplication
cost more time, but also human readers would wonder why the result could
become larger than 32 bits. So a cast would be misleading.


It also complains about this line:

    ret = bdrv_truncate(bs->file, (3 + l1_clusters) * s->cluster_size,
                        PREALLOC_MODE_OFF, &local_err);

Here, we don't even assign the result to a 64 bit variable, but just
pass it to a function which takes a 64 bit parameter. Again, I don't
think introducing additional variables for the intermediate result or
adding casts would be an improvement of the situation.


So I don't think this is a good enough tool to base our code on what it
does and doesn't understand. It would have too much of a negative impact
on our code. We'd rather need a way to mark false positives as such and
move on without changing the code in such cases.

Kevin