* Potential missing checks @ 2020-03-23 22:03 Mansour Ahmadi 2020-03-24 9:24 ` Peter Maydell 0 siblings, 1 reply; 5+ messages in thread From: Mansour Ahmadi @ 2020-03-23 22:03 UTC (permalink / raw) To: qemu-devel [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Potential missing checks 2020-03-23 22:03 Potential missing checks Mansour Ahmadi @ 2020-03-24 9:24 ` Peter Maydell 2020-03-24 20:39 ` Mansour Ahmadi 0 siblings, 1 reply; 5+ messages in thread From: Peter Maydell @ 2020-03-24 9:24 UTC (permalink / raw) To: Mansour Ahmadi; +Cc: QEMU Developers 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Potential missing checks 2020-03-24 9:24 ` Peter Maydell @ 2020-03-24 20:39 ` Mansour Ahmadi 2020-03-24 21:17 ` Peter Maydell 0 siblings, 1 reply; 5+ messages in thread From: Mansour Ahmadi @ 2020-03-24 20:39 UTC (permalink / raw) To: Peter Maydell; +Cc: QEMU Developers [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Potential missing checks 2020-03-24 20:39 ` Mansour Ahmadi @ 2020-03-24 21:17 ` Peter Maydell 2020-03-24 21:34 ` Mansour Ahmadi 0 siblings, 1 reply; 5+ messages in thread From: Peter Maydell @ 2020-03-24 21:17 UTC (permalink / raw) To: Mansour Ahmadi; +Cc: QEMU Developers 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: Potential missing checks 2020-03-24 21:17 ` Peter Maydell @ 2020-03-24 21:34 ` Mansour Ahmadi 0 siblings, 0 replies; 5+ messages in thread From: Mansour Ahmadi @ 2020-03-24 21:34 UTC (permalink / raw) To: Peter Maydell; +Cc: QEMU Developers [-- 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 --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-03-24 23:08 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-23 22:03 Potential missing checks Mansour Ahmadi 2020-03-24 9:24 ` Peter Maydell 2020-03-24 20:39 ` Mansour Ahmadi 2020-03-24 21:17 ` Peter Maydell 2020-03-24 21:34 ` Mansour Ahmadi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).