Re: [PATCH 1/2] block: Fix Transaction leak in bdrv_root_attach_child()

[Vladimir Sementsov-Ogievskiy]

03.05.2021 14:49, Max Reitz wrote:
> On 03.05.21 13:05, Kevin Wolf wrote:
> > The error path needs to call tran_finalize(), too.
> >
> > Fixes: CID 1452773
> > Fixes: 548a74c0dbc858edd1a7ee3045b5f2fe710bd8b1
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   block.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index 874c22c43e..5c0ced6238 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2918,13 +2918,14 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState 
> > *child_bs,
> >                                      child_role, perm, shared_perm, opaque,
> >                                      &child, tran, errp);
> >       if (ret < 0) {
> > -        bdrv_unref(child_bs);
> > -        return NULL;
> > +        assert(child == NULL);
> > +        goto out;
> >       }
> >       ret = bdrv_refresh_perms(child_bs, errp);
> > -    tran_finalize(tran, ret);
> > +out:
> > +    tran_finalize(tran, ret);
> >       bdrv_unref(child_bs);
> >       return child;
>
> Looks OK, so:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> However, the function’s description says that it will return NULL on error.  
> But if bdrv_refresh_perms() fails, it will still return a non-NULL child.  Is 
> that right?

No, it's reset to NULL on transaction abort, so code is correct. It's not obvious, and 
I've added a comment and assertion in my version of this fix "[PATCH 1/6] block: fix 
leak of tran in bdrv_root_attach_child"

--
Best regards,
Vladimir