Potential missing checks

Potential missing checks

[Mansour Ahmadi]

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

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:

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

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

Thanks,
Mansour

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

Re: Potential missing checks

[Peter Maydell]

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

Re: Potential missing checks

[Mansour Ahmadi]

[-- 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 --]

Re: Potential missing checks

[Peter Maydell]

On Tue, 24 Mar 2020 at 20:39, Mansour Ahmadi <mansourweb@gmail.com> wrote:
>
> 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.

The thing is, you're making us do all the work here. That's
not very useful to us. It's doubly unuseful when there's
a strong chance that when we do do the work of looking
at the code it turns out that there's no problem.

"I did some static analysis, and I looked at the
results, and I dug through the QEMU code, and it
does seem to me that this could well be a bug" is
definitely useful. "I did some static analysis using
only analysis techniques that have an pretty
low false positive rate, and here is a selection of
the results" is also useful. But "I just ran the
code through an analyser that produces lots of
false positives and then I didn't do any further
human examination of the results" is of much less
utility to the project, I'm afraid.

> 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, this is in the disassembler. The difference is
just whether we print out a register+offset memory
reference where the offset happens to be zero
as "[reg, #0]" or just "[reg]", and the no-special-case-0
is for encodings which are always pc-relative.
So even if it was a missing check the results are
entirely harmless, since anybody reading the disassembly
will understand the #0 fine.

Secondly, this code is imported from binutils,
so we usually don't worry too much about fixing
up minor bugs in it.

Finally, I went and checked the Arm specs, and for
the kinds of PC-relative load/store that the second
example is handling the specified disassembly format
does mandate the "pc, #0" (whereas the other example
is correctly skipping it for 0-immediates because
in those insns the offset is optional in disassembly).

So the code is correct as it stands.

thanks
-- PMM

Re: Potential missing checks

[Mansour Ahmadi]

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

Thanks for the explanation.


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

> On Tue, 24 Mar 2020 at 20:39, Mansour Ahmadi <mansourweb@gmail.com> wrote:
> >
> > 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.
>
> The thing is, you're making us do all the work here. That's
> not very useful to us. It's doubly unuseful when there's
> a strong chance that when we do do the work of looking
> at the code it turns out that there's no problem.
>
> "I did some static analysis, and I looked at the
> results, and I dug through the QEMU code, and it
> does seem to me that this could well be a bug" is
> definitely useful. "I did some static analysis using
> only analysis techniques that have an pretty
> low false positive rate, and here is a selection of
> the results" is also useful. But "I just ran the
> code through an analyser that produces lots of
> false positives and then I didn't do any further
> human examination of the results" is of much less
> utility to the project, I'm afraid.
>
> > 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, this is in the disassembler. The difference is
> just whether we print out a register+offset memory
> reference where the offset happens to be zero
> as "[reg, #0]" or just "[reg]", and the no-special-case-0
> is for encodings which are always pc-relative.
> So even if it was a missing check the results are
> entirely harmless, since anybody reading the disassembly
> will understand the #0 fine.
>
> Secondly, this code is imported from binutils,
> so we usually don't worry too much about fixing
> up minor bugs in it.
>
> Finally, I went and checked the Arm specs, and for
> the kinds of PC-relative load/store that the second
> example is handling the specified disassembly format
> does mandate the "pc, #0" (whereas the other example
> is correctly skipping it for 0-immediates because
> in those insns the offset is optional in disassembly).
>
> So the code is correct as it stands.
>
> thanks
> -- PMM
>

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