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

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

[Vladimir Sementsov-Ogievskiy]

Theoretically possible that we finish the skipping loop with bs = NULL
and the following code will crash trying to dereference it. Fix that.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 migration/block-dirty-bitmap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 477826330c..6de808f95f 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -288,6 +288,10 @@ static int init_dirty_bitmap_migration(void)
             bs = backing_bs(bs);
         }
 
+        if (!bs || bs->implicit) {
+            continue;
+        }
+
         for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
              bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
         {
-- 
2.18.0

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

[Stefan Hajnoczi]

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

On Tue, Oct 16, 2018 at 04:20:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Theoretically possible that we finish the skipping loop with bs = NULL
> and the following code will crash trying to dereference it. Fix that.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  migration/block-dirty-bitmap.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 477826330c..6de808f95f 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -288,6 +288,10 @@ static int init_dirty_bitmap_migration(void)
>              bs = backing_bs(bs);
>          }
>  
> +        if (!bs || bs->implicit) {
> +            continue;
> +        }
> +
>          for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
>               bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
>          {

Previous discussion:
http://qemu.11.n7.nabble.com/PATCH-migration-Appease-coverity-skip-empty-block-trees-td582504.html

I've CCed John so he can take a look.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Peter Maydell]

On 17 October 2018 at 10:51, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Oct 16, 2018 at 04:20:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Theoretically possible that we finish the skipping loop with bs = NULL
>> and the following code will crash trying to dereference it. Fix that.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  migration/block-dirty-bitmap.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>> index 477826330c..6de808f95f 100644
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -288,6 +288,10 @@ static int init_dirty_bitmap_migration(void)
>>              bs = backing_bs(bs);
>>          }
>>
>> +        if (!bs || bs->implicit) {
>> +            continue;
>> +        }
>> +
>>          for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
>>               bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
>>          {
>
> Previous discussion:
> http://qemu.11.n7.nabble.com/PATCH-migration-Appease-coverity-skip-empty-block-trees-td582504.html
>
> I've CCed John so he can take a look.

So have you block-layer folks figured out how you want to address
this Coverity issue yet?

thanks
-- PMM

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

[John Snow]

On 11/15/18 6:48 AM, Peter Maydell wrote:
> On 17 October 2018 at 10:51, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Tue, Oct 16, 2018 at 04:20:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> Theoretically possible that we finish the skipping loop with bs = NULL
>>> and the following code will crash trying to dereference it. Fix that.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>  migration/block-dirty-bitmap.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>>> index 477826330c..6de808f95f 100644
>>> --- a/migration/block-dirty-bitmap.c
>>> +++ b/migration/block-dirty-bitmap.c
>>> @@ -288,6 +288,10 @@ static int init_dirty_bitmap_migration(void)
>>>              bs = backing_bs(bs);
>>>          }
>>>
>>> +        if (!bs || bs->implicit) {
>>> +            continue;
>>> +        }
>>> +
>>>          for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
>>>               bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
>>>          {
>>
>> Previous discussion:
>> http://qemu.11.n7.nabble.com/PATCH-migration-Appease-coverity-skip-empty-block-trees-td582504.html
>>
>> I've CCed John so he can take a look.
> 
> So have you block-layer folks figured out how you want to address
> this Coverity issue yet?
> 
> thanks
> -- PMM
> 

I'm sorry, Peter.

I've let this one slip through the cracks many times and I thought it
had been handled.

I'll look now.

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

[John Snow]

On 11/15/18 6:48 AM, Peter Maydell wrote:
> On 17 October 2018 at 10:51, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Tue, Oct 16, 2018 at 04:20:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> Theoretically possible that we finish the skipping loop with bs = NULL
>>> and the following code will crash trying to dereference it. Fix that.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>  migration/block-dirty-bitmap.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>>> index 477826330c..6de808f95f 100644
>>> --- a/migration/block-dirty-bitmap.c
>>> +++ b/migration/block-dirty-bitmap.c
>>> @@ -288,6 +288,10 @@ static int init_dirty_bitmap_migration(void)
>>>              bs = backing_bs(bs);
>>>          }
>>>
>>> +        if (!bs || bs->implicit) {
>>> +            continue;
>>> +        }
>>> +
>>>          for (bitmap = bdrv_dirty_bitmap_next(bs, NULL); bitmap;
>>>               bitmap = bdrv_dirty_bitmap_next(bs, bitmap))
>>>          {
>>
>> Previous discussion:
>> http://qemu.11.n7.nabble.com/PATCH-migration-Appease-coverity-skip-empty-block-trees-td582504.html
>>
>> I've CCed John so he can take a look.
> 
> So have you block-layer folks figured out how you want to address
> this Coverity issue yet?
> 
> thanks
> -- PMM
> 

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.

I'm sorry for the fuss.

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

[Peter Maydell]

On 16 November 2018 at 03:28, John Snow <jsnow@redhat.com> 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

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

[Stefan Hajnoczi]

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

On Tue, Oct 16, 2018 at 04:20:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Theoretically possible that we finish the skipping loop with bs = NULL
> and the following code will crash trying to dereference it. Fix that.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  migration/block-dirty-bitmap.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 477826330c..6de808f95f 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -288,6 +288,10 @@ static int init_dirty_bitmap_migration(void)
>              bs = backing_bs(bs);
>          }
>  
> +        if (!bs || bs->implicit) {

Why is it necessary to check bs->implicit?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [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 <jsnow@redhat.com> 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

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

[Peter Maydell]

On 16 November 2018 at 16:16, John Snow <jsnow@redhat.com> wrote:
> On 11/16/18 5:04 AM, Peter Maydell wrote:
>> 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.

> 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.

I think it will quieten Coverity; if it doesn't, then at that point
we can happily mark the issue a false-positive. But as I say what
Coverity is really identifying here is "you checked whether
this pointer was NULL, but then there's a code path forward
to a place where you used it without checking" -- so removing
the unnecessary check will make it happy.

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

If you believe by design that it can't be NULL, then we're OK:
if it ever turns out that it isn't, then we will get a nice
easy-to-debug SEGV when we dereference the NULL pointer,
and we can find out then what bug resulted in our design
assumption being broken.

> 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.

Coverity runs only on git master, so fixes have to go into
master to be tested, unfortunately.

thanks
-- PMM

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

[Vladimir Sementsov-Ogievskiy]

16.11.2018 17:29, Stefan Hajnoczi wrote:
> On Tue, Oct 16, 2018 at 04:20:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Theoretically possible that we finish the skipping loop with bs = NULL
>> and the following code will crash trying to dereference it. Fix that.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   migration/block-dirty-bitmap.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
>> index 477826330c..6de808f95f 100644
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -288,6 +288,10 @@ static int init_dirty_bitmap_migration(void)
>>               bs = backing_bs(bs);
>>           }
>>   
>> +        if (!bs || bs->implicit) {
> 
> Why is it necessary to check bs->implicit?
> 


to be sure, that it is not implicit, as previous loop may finish with implicit = true and bs!= NULL, if drv == NULL. (hmm, may be we want to check drv too?)

same thing is in bdrv_query_info, without any checks, and in bdrv_block_device_info with assert(bs).


-- 
Best regards,
Vladimir