| Message ID | 20181016132018.5054-1-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration/block-dirty-bitmap: fix Coverity CID1390625 | expand |
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.
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
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.
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.
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
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?
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
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
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).
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)) {
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(+)