migration/block-dirty-bitmap: fix Coverity CID1390625
PATCH 1/1 migration/block-dirty-bitmap: fix Coverity CID1390625
Theoretically possible that we finish the skipping loop with bs = NULL and the following code will crash trying to dereference it. Fix that.
--- migration/block-dirty-bitmap.c | 4 ++++ 1 file changed, 4 insertions(+)
migration/block-dirty-bitmap.c +4 -0
@@ -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)) {
Signature · 1 line
2.18.0
Quoted 13 lines · 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 <[email protected]>--- migration/block-dirty-bitmap.c | 4 ++++ 1 file changed, 4 insertions(+)migration/block-dirty-bitmap.c +0
-0
migration/block-dirty-bitmap.c static int init_dirty_bitmap_migration(void) · line 297
Quoted 10 lines
migration/block-dirty-bitmap.c +4
-0@@ -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.
Quoted 28 lines · Stefan Hajnoczi
Quoted 13 lines · 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 <[email protected]>--- migration/block-dirty-bitmap.c | 4 ++++ 1 file changed, 4 insertions(+)migration/block-dirty-bitmap.c +0
-0migration/block-dirty-bitmap.c static int init_dirty_bitmap_migration(void) · line 297
Quoted 10 lines
migration/block-dirty-bitmap.c +4
-0@@ -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
Quoted 1 line
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
Quoted 29 lines · Peter Maydell
Quoted 28 lines · Stefan Hajnoczi
Quoted 13 lines · 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 <[email protected]>--- migration/block-dirty-bitmap.c | 4 ++++ 1 file changed, 4 insertions(+)migration/block-dirty-bitmap.c +0
-0migration/block-dirty-bitmap.c static int init_dirty_bitmap_migration(void) · line 297
Quoted 10 lines
migration/block-dirty-bitmap.c +4
-0@@ -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
Quoted 8 lines
Quoted 1 line
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.
Quoted 29 lines · Peter Maydell
Quoted 28 lines · Stefan Hajnoczi
Quoted 13 lines · 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 <[email protected]>--- migration/block-dirty-bitmap.c | 4 ++++ 1 file changed, 4 insertions(+)migration/block-dirty-bitmap.c +0
-0migration/block-dirty-bitmap.c static int init_dirty_bitmap_migration(void) · line 297
Quoted 10 lines
migration/block-dirty-bitmap.c +4
-0@@ -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
Quoted 8 lines
Quoted 1 line
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;
migration/block-dirty-bitmap.c +1 -1
@@ -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.
Quoted 25 lines · John Snow
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;
migration/block-dirty-bitmap.c +1
-1@@ -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_bscan theoretically return NULL and might convince coverity there's a problem. In practice it won't be.
Quoted 5 lines
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
Quoted 44 lines · Peter Maydell
Quoted 25 lines · John Snow
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;
migration/block-dirty-bitmap.c +1
-1@@ -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_bscan theoretically return NULL and might convince coverity there's a problem. In practice it won't be.Quoted 5 lines
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 anassert()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.
Quoted 3 lines
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
Quoted 13 lines · John Snow
Quoted 11 lines · Peter Maydell
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 anassert()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.
Quoted 3 lines
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.
Quoted 2 lines
(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.
Quoted 4 lines
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
Quoted 13 lines · 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 <[email protected]>--- migration/block-dirty-bitmap.c | 4 ++++ 1 file changed, 4 insertions(+)migration/block-dirty-bitmap.c +0
-0
migration/block-dirty-bitmap.c static int init_dirty_bitmap_migration(void) · line 291
Quoted 4 lines
migration/block-dirty-bitmap.c +1
-0@@ -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?
Quoted 17 lines · Stefan Hajnoczi
Quoted 16 lines · 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 <[email protected]>--- migration/block-dirty-bitmap.c | 4 ++++ 1 file changed, 4 insertions(+)migration/block-dirty-bitmap.c +0
-0@@ -288,6 +288,10 @@ static int init_dirty_bitmap_migration(void) bs =backing_bs (bs); }
Quoted 4 lines
migration/block-dirty-bitmap.c static int init_dirty_bitmap_migration(void) · line 291
Quoted 1 line
migration/block-dirty-bitmap.c +1
-0+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).
Signature · 2 lines
Best regards, Vladimir