diff options
Diffstat (limited to 'classification_output/04/other/23300761')
| -rw-r--r-- | classification_output/04/other/23300761 | 321 |
1 files changed, 0 insertions, 321 deletions
diff --git a/classification_output/04/other/23300761 b/classification_output/04/other/23300761 deleted file mode 100644 index e546903ce..000000000 --- a/classification_output/04/other/23300761 +++ /dev/null @@ -1,321 +0,0 @@ -other: 0.963 -assembly: 0.958 -semantic: 0.950 -device: 0.932 -boot: 0.929 -instruction: 0.929 -socket: 0.927 -vnc: 0.926 -graphic: 0.924 -KVM: 0.897 -network: 0.879 -mistranslation: 0.770 - -[Qemu-devel] [BUG] 216 Alerts reported by LGTM for QEMU (some might be release critical) - -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 - -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 - -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 - -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 - -On Sat, 13 Jul 2019 at 18:46, Stefan Weil <address@hidden> 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 - -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 - -Stefan Weil <address@hidden> 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. - -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~ - |