[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Potential Null dereference
From: |
Kevin Wolf |
Subject: |
Re: Potential Null dereference |
Date: |
Tue, 24 Mar 2020 13:58:24 +0100 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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