| Message ID | 20181116184324.8093-1-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration/block-dirty-bitmap: Silence coverity CID 1390625 | expand |
16.11.2018 21:43, John Snow wrote: > Coverity warns that backing_bs() could give us a NULL pointer, which > we then use without checking that it isn't. > > In our loop condition, we check bs && bs->drv as a point of habit, but > by nature of the block graph, we cannot have null bs pointers here. > > This loop skips only implicit nodes, which always have children, so > this loop should never encounter a null value. You mean, always have backing (not file for ex.)? Should we at least add a comment near "bool implicit;" that the node must have backing.. Do we have filters, using 'file' child instead of backing, will we want to auto insert them, and therefore mark them with implicit=true? And one more thing: So, it's looks like a wrong way to search for all block-nodes, instead of looping through backing chain to the first not-implicit bds, we must recursively explore the whole block graph, to find _all_ the bitmaps. > > Tighten the loop condition to coax Coverity into dropping > its false positive. > > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> > --- > migration/block-dirty-bitmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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); > } > >
On Fri, 16 Nov 2018 at 18:43, John Snow <jsnow@redhat.com> wrote: > > Coverity warns that backing_bs() could give us a NULL pointer, which > we then use without checking that it isn't. > > In our loop condition, we check bs && bs->drv as a point of habit, but > by nature of the block graph, we cannot have null bs pointers here. > > This loop skips only implicit nodes, which always have children, so > this loop should never encounter a null value. > > Tighten the loop condition to coax Coverity into dropping > its false positive. > > Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> > Signed-off-by: John Snow <jsnow@redhat.com> I've just noticed that Coverity is still warning about this case -- did this patch get forgotten, or is it obsoleted by a different approach? (This is now one of just 5 coverity issues outstanding.) > --- > migration/block-dirty-bitmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > 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); > } thanks -- PMM
On 11/20/18 10:15 AM, Vladimir Sementsov-Ogievskiy wrote: > 16.11.2018 21:43, John Snow wrote: >> Coverity warns that backing_bs() could give us a NULL pointer, which >> we then use without checking that it isn't. >> >> In our loop condition, we check bs && bs->drv as a point of habit, but >> by nature of the block graph, we cannot have null bs pointers here. >> >> This loop skips only implicit nodes, which always have children, so >> this loop should never encounter a null value. > I let this drop again :) > You mean, always have backing (not file for ex.)? Should we at least add a comment > near "bool implicit;" that the node must have backing.. > > Do we have filters, using 'file' child instead of backing, will we want to auto insert them, and therefore mark them with implicit=true? > I actually have no idea. I guess this is the sort of thing we actually really want a dedicated kind of API for. "Find first non-filter" seems like a common use case that we'd want. [But maybe I'll avoid this problem.] > And one more thing: > So, it's looks like a wrong way to search for all block-nodes, instead of looping through backing chain to the first not-implicit bds, we must recursively explore the whole block graph, to find _all_ the bitmaps. > Looking at this again after not having done so for so long -- I guess that bdrv_first/bdrv_next only iterate over *top level* BDSes and not any children thereof. You're right, even the method here isn't quite correct. We want to find ALL nodes, wherever they are. query_named_block_nodes uses an implementation in block.c to accomplish this because the API is not public.... or, it wasn't, but it looks like we have bdrv_next_all_states now, and we could use this to just find ALL of the bdrv nodes. Ehm.... let me send something a little more RFC-caliber that should address your concern (as well as Peter's) here. --js
On 4/30/19 6:08 PM, John Snow wrote: > > > On 11/20/18 10:15 AM, Vladimir Sementsov-Ogievskiy wrote: >> 16.11.2018 21:43, John Snow wrote: >>> Coverity warns that backing_bs() could give us a NULL pointer, which >>> we then use without checking that it isn't. >>> >>> In our loop condition, we check bs && bs->drv as a point of habit, but >>> by nature of the block graph, we cannot have null bs pointers here. >>> >>> This loop skips only implicit nodes, which always have children, so >>> this loop should never encounter a null value. >> > > I let this drop again :) > >> You mean, always have backing (not file for ex.)? Should we at least add a comment >> near "bool implicit;" that the node must have backing.. >> >> Do we have filters, using 'file' child instead of backing, will we want to auto insert them, and therefore mark them with implicit=true? >> > > I actually have no idea. I guess this is the sort of thing we actually > really want a dedicated kind of API for. "Find first non-filter" seems > like a common use case that we'd want. > > [But maybe I'll avoid this problem.] Max has already tried to tackle that problem: https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg01713.html > >> And one more thing: >> So, it's looks like a wrong way to search for all block-nodes, instead of looping through backing chain to the first not-implicit bds, we must recursively explore the whole block graph, to find _all_ the bitmaps. >> > > Looking at this again after not having done so for so long -- I guess > that bdrv_first/bdrv_next only iterate over *top level* BDSes and not > any children thereof. You're right, even the method here isn't quite > correct. We want to find ALL nodes, wherever they are. > > query_named_block_nodes uses an implementation in block.c to accomplish > this because the API is not public.... or, it wasn't, but it looks like > we have bdrv_next_all_states now, and we could use this to just find ALL > of the bdrv nodes. > > Ehm.... let me send something a little more RFC-caliber that should > address your concern (as well as Peter's) here. Max's series also tries to improve how we visit nodes when determining which bitmaps to find.
On 5/1/19 11:24 AM, Eric Blake wrote: > On 4/30/19 6:08 PM, John Snow wrote: >> >> >> On 11/20/18 10:15 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 16.11.2018 21:43, John Snow wrote: >>>> Coverity warns that backing_bs() could give us a NULL pointer, which >>>> we then use without checking that it isn't. >>>> >>>> In our loop condition, we check bs && bs->drv as a point of habit, but >>>> by nature of the block graph, we cannot have null bs pointers here. >>>> >>>> This loop skips only implicit nodes, which always have children, so >>>> this loop should never encounter a null value. >>> >> >> I let this drop again :) >> >>> You mean, always have backing (not file for ex.)? Should we at least add a comment >>> near "bool implicit;" that the node must have backing.. >>> >>> Do we have filters, using 'file' child instead of backing, will we want to auto insert them, and therefore mark them with implicit=true? >>> >> >> I actually have no idea. I guess this is the sort of thing we actually >> really want a dedicated kind of API for. "Find first non-filter" seems >> like a common use case that we'd want. >> >> [But maybe I'll avoid this problem.] > > Max has already tried to tackle that problem: > https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg01713.html > OK, that's great! >> >>> And one more thing: >>> So, it's looks like a wrong way to search for all block-nodes, instead of looping through backing chain to the first not-implicit bds, we must recursively explore the whole block graph, to find _all_ the bitmaps. >>> >> >> Looking at this again after not having done so for so long -- I guess >> that bdrv_first/bdrv_next only iterate over *top level* BDSes and not >> any children thereof. You're right, even the method here isn't quite >> correct. We want to find ALL nodes, wherever they are. >> >> query_named_block_nodes uses an implementation in block.c to accomplish >> this because the API is not public.... or, it wasn't, but it looks like >> we have bdrv_next_all_states now, and we could use this to just find ALL >> of the bdrv nodes. >> >> Ehm.... let me send something a little more RFC-caliber that should >> address your concern (as well as Peter's) here. > > Max's series also tries to improve how we visit nodes when determining > which bitmaps to find. > 2/11 adds the "skip filters" helper I mentioned wanting, but the idea of visiting *every* node for bitmap migration is not addressed in that series. Therefore, I believe these series conflict, but that this one takes precedence for this particular issue. --js
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); }
Coverity warns that backing_bs() could give us a NULL pointer, which we then use without checking that it isn't. In our loop condition, we check bs && bs->drv as a point of habit, but by nature of the block graph, we cannot have null bs pointers here. This loop skips only implicit nodes, which always have children, so this loop should never encounter a null value. Tighten the loop condition to coax Coverity into dropping its false positive. Suggested-by: Stefan Hajnoczi <stefanha@redhat.com> Signed-off-by: John Snow <jsnow@redhat.com> --- migration/block-dirty-bitmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)