Re: [Qemu-block] [PATCH] migration/block-dirty-bitmap: fix Coverity CID1390625

[John Snow]

On 11/16/18 5:04 AM, Peter Maydell wrote:
> On 16 November 2018 at 03:28, John Snow <address@hidden> wrote:
>> I looked again. I think Vladimir's patch will shut up Coverity for sure,
>> feel free to apply it if you want this out of your hair.
>>
>> Stefan suggests the following, however;
>>
>>
>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>> index 5e90f44c2f..00c068fda3 100644
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -284,7 +284,7 @@ static int init_dirty_bitmap_migration(void)
>>          const char *drive_name = bdrv_get_device_or_node_name(bs);
>>
>>          /* skip automatically inserted nodes */
>> -        while (bs && bs->drv && bs->implicit) {
>> +        while (bs->drv && bs->implicit) {
>>              bs = backing_bs(bs);
>>          }
>>
>>
>> that by removing the assumption that bs could ever be null here (it
>> shouldn't), that we'll coax coverity into not warning anymore. I don't
>> know if that will work, because backing_bs can theoretically return NULL
>> and might convince coverity there's a problem. In practice it won't be.
>>
>> I don't know how to check this to see if Stefan's suggestion is appropriate.
>>
>> For such a small, trivial issue though, just merge this and be done with
>> it, in my opinion. If you want to take this fix directly as a "build
>> fix" I wouldn't object.
> 
> Personally I think the main benefit of this sort of Coverity
> warning is that it nudges you to work through and figure out
> whether something really can be NULL or not. Stefan's fix
> will satisfy Coverity, because what it's complaining about
> is that the code in one place assumes a pointer can't be NULL
> but in another place does have handling for NULL: it is the
> inconsistency that triggers the warning. If backing_bs()
> can't return NULL (ie if you would be happy in theory to put
> an assert() in after the call) then I think Stefan's fix is
> better. If it can return NULL ever then Vladimir's fix is
> what you want.
> 
> thanks
> -- PMM
> 

I understand the point of Coverity.

I really don't think it can, but I don't actually know how to verify it
or to convince Coverity of the same. Stefan's suggestion seems most
appropriate if it actually does calm Coverity down.

The loop above, there, just iterates down a chain in a graph, looking
for the first node that satisfies a certain criteria -- that it's not an
implicit node. These are reserved for intermediary filter nodes that
should ALWAYS have a child. That is their intended design.

(Is it mechanically possible to violate this? That's very hard to audit
and promise for you. There are always bugs lurking somewhere else.)

I think the only two places where we create such nodes are in:

block/commit.c:        commit_top_bs->implicit = true;
block/mirror.c:        mirror_top_bs->implicit = true;


in commit, it's inserted explicitly above `top`.
in mirror, we use bdrv_append(mirror_top_bs, bs, ...) to put the
implicit node above the target.

In both cases, the loop *cannot* end at a node without a backing file.

Now, this is not to say there might not be a bug somewhere else when we
do graph manipulation that we might accidentally end up with such an
errant configuration ...

Stefan's suggestion is probably most appropriate, *if* it actually
shushes Coverity. I'll send you a small patch and you can find out? I
don't want to task offload but I also genuinely don't know if that will
hint to coverity strongly enough that this is a false positive.

--js