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

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

[Stefan Weil]

Hi,

LGTM reports 16 errors, 81 warnings and 119 recommendations: 
https://lgtm.com/projects/g/qemu/qemu/alerts/?mode=list.

Some of them are already know (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

- potential buffer overflows in gdbstub.c and other files

I am afraid that the overflows in the block code are release critical, 
maybe that in target/i386/translate.c and other errors, too.

About half of the alerts are issues which can be fixed later.

Regards

Stefan

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

[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 know (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

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

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

[Stefan Weil]

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

Stefan

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

[Peter Maydell]

On Sat, 13 Jul 2019 at 18:46, Stefan Weil <sw@weilnetz.de> wrote:
> LGTM reports 16 errors, 81 warnings and 119 recommendations:
> https://lgtm.com/projects/g/qemu/qemu/alerts/?mode=list.

I had a look at some of these before, but mostly I came
to the conclusion that it wasn't worth trying to put the
effort into keeping up with the site because they didn't
seem to provide any useful way to mark things as false
positives. Coverity has its flaws but at least you can do
that kind of thing in its UI (it runs at about a 33% fp
rate, I think.) "Analyzer thinks this multiply can overflow
but in fact it's not possible" is quite a common false
positive cause...

Anyway, if you want to fish out specific issues, analyse
whether they're false positive or real, and report them
to the mailing list as followups to the patches which
introduced the issue, that's probably the best way for
us to make use of this analyzer. (That is essentially
what I do for coverity.)

thanks
-- PMM

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

[Stefan Weil]

Am 14.07.2019 um 19:30 schrieb Peter Maydell:
[...]
> "Analyzer thinks this multiply can overflow
> but in fact it's not possible" is quite a common false
> positive cause...


The analysers don't complain because a multiply can overflow.

They complain because the code indicates that a larger result is
expected, for example uint64_t = uint32_t * uint32_t. They would not
complain for the same multiplication if it were assigned to a uint32_t.

So there is a simple solution to write the code in a way which avoids
false positives...

Stefan

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

[Markus Armbruster]

Stefan Weil <sw@weilnetz.de> writes:

> Am 14.07.2019 um 19:30 schrieb Peter Maydell:
> [...]
>> "Analyzer thinks this multiply can overflow
>> but in fact it's not possible" is quite a common false
>> positive cause...
>
>
> The analysers don't complain because a multiply can overflow.
>
> They complain because the code indicates that a larger result is
> expected, for example uint64_t = uint32_t * uint32_t. They would not
> complain for the same multiplication if it were assigned to a uint32_t.

I agree this is an anti-pattern.

> So there is a simple solution to write the code in a way which avoids
> false positives...

You wrote elsewhere in this thread:

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

Makes sense to me.

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

[Richard Henderson]

On 7/14/19 5:30 PM, Peter Maydell wrote:
> I had a look at some of these before, but mostly I came
> to the conclusion that it wasn't worth trying to put the
> effort into keeping up with the site because they didn't
> seem to provide any useful way to mark things as false
> positives. Coverity has its flaws but at least you can do
> that kind of thing in its UI (it runs at about a 33% fp
> rate, I think.)

Yes, LGTM wants you to modify the source code with

  /* lgtm [cpp/some-warning-code] */

and on the same line as the reported problem.  Which is mildly annoying in that
you're definitely committing to LGTM in the long term.  Also for any
non-trivial bit of code, it will almost certainly run over 80 columns.


r~

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