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.mayd...@linaro.org> wrote: > On Mon, 23 Mar 2020 at 22:04, Mansour Ahmadi <manso...@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 >
