Re: Potential missing checks

[Mansour Ahmadi <mansourweb@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2537 bytes --]

Thank you for looking into this, Peter. I agree that static analysis has
false positives; that's why I called them potential. Basically, they are
found based on code similarity so I might be wrong and I need a second
opinion from QEMU developers. I appreciate your effort.

For the first case, I noticed a check on offset (if (offset)) before
negating it and passing to stream function here.
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/disas/arm.c#L1748

Similar scenario happened here WITHOUT the check:
https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/disas/arm.c#L2731-L2733

So I wonder whether a check on offset is really missed.

Thank you!
Mansour



On Tue, Mar 24, 2020 at 5:24 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Mon, 23 Mar 2020 at 22:04, Mansour Ahmadi <ManSoSec@gmail.com> wrote:
> >
> > Hi QEMU developers,
> >
> > I noticed the following two potential missing checks by static analysis
> and detecting inconsistencies on the source code of QEMU. here is the
> result:
>
> Hi. Can you provide more details of your analysis, please? "Maybe
> there's an issue
> at this line" is not terribly helpful, especially if one has to follow
> a bunch of URLs
> to even find out which code is being discussed. All static analysers are
> prone
> to false positives, and so the value is in analysing the possible issues,
> not
> in simply dumping raw output with no details onto the mailing list.
>
> > 1)
> > Missing check on offset:
> >
> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/disas/arm.c#L2728-L2733
> >
> > While it is checked here:
> >
> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/disas/arm.c#L1748-L1752
>
> What in particular do you think should be being checked that is not?
>
> > 2)
> > Missing check on bmds->dirty_bitmap:
> >
> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/migration/block.c#L377-L378
> >
> > While it is checked here:
> >
> https://github.com/qemu/qemu/blob/c532b954d96f96d361ca31308f75f1b95bd4df76/migration/block.c#L363-L365
>
> This one looks correct to me -- the second case is the error handling
> path for "failure halfway through creating the list of dirty bitmaps",
> and so it must handle "this one wasn't created yet". The first
> case will only run on data structures where set_dirty_tracking()
> succeeded, and so we know that there can't be any NULL pointers.
> Why do you think it is incorrect?
>
> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 5747 bytes --]