Re: Potential Null dereference

[Kevin Wolf]

Am 24.03.2020 um 13:37 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 24.03.2020 14:59, Vladimir Sementsov-Ogievskiy wrote:
> > Aha, new crashes! Let's look at them.
> > 
> > 41 and 155 failed with crash, 141 without but I see "+{"error": {"class": 
> > "GenericError", "desc": "Block device drv0 is in use"}}" in its output.
> > 
> > Hmm, but all crashes are because of assert(QTAILQ_EMPTY(&all_bdrv_states)); 
> > in bdrv_close_all..
> > 
> > So, we have a problem, but another one..
> 
> Investigate it a bit more.
> 
> Accordingly to comment, bdrv_attach_child unrefs child bs even on failure, so 
> let's do
> 
> --- a/block.c
> +++ b/block.c
> @@ -2768,6 +2768,7 @@ void bdrv_set_backing_hd_ex(BlockDriverState *bs, 
> BlockDriverState *backing_hd,
> 
>      if (bdrv_attach_child_fail) {
>          bs->backing = NULL;
> +        bdrv_unref(backing_hd);
>          error_setg(errp, "Unpredicted failure :)");
>      } else {
>          bs->backing = bdrv_attach_child(bs, backing_hd, "backing", 
> &child_backing,
> 
> 
> - then, all three tests fails, but without crashes. OK.
> 
> The only interesting thing is that, it seems that bdrv_attach_child doesn't 
> unref child on all failure paths.
> 
> It calls bdrv_root_attach_child..
> 
> which doesn't unref child on the path
> 
> if (bdrv_get_aio_context(child_bs) != ctx) {
>   ...
>    if (ret < 0) {
>          error_propagate(errp, local_err);
>          g_free(child);
>          bdrv_abort_perm_update(child_bs);
>          return NULL;
>    }
> }
> 
> or am I wrong?

I think you're right, we need a bdrv_unref() there. The function comment
makes it clear that bdrv_root_attach_child() is supposed to consume the
reference both in success and error cases.

Kevin