Re: Potential missing checks

[Mansour Ahmadi]

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 <address@hidden> wrote:

On Mon, 23 Mar 2020 at 22:04, Mansour Ahmadi <address@hidden> 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