Re: [PULL 30/39] block: bdrv_reopen_multiple: refresh permissions on updated graph

[Peter Maydell]

On Fri, 30 Apr 2021 at 11:53, Kevin Wolf <kwolf@redhat.com> wrote:
>
> From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
> Move bdrv_reopen_multiple to new paradigm of permission update:
> first update graph relations, then do refresh the permissions.
>
> We have to modify reopen process in file-posix driver: with new scheme
> we don't have prepared permissions in raw_reopen_prepare(), so we
> should reconfigure fd in raw_check_perm(). Still this seems more native
> and simple anyway.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Message-Id: <20210428151804.439460-31-vsementsov@virtuozzo.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Hi; Coverity thinks this change introduced a resource leak
(CID 1452772):



> @@ -4271,6 +4270,9 @@ int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, 
> Error **errp)
>  {
>      int ret = -1;
>      BlockReopenQueueEntry *bs_entry, *next;
> +    Transaction *tran = tran_new();
> +    g_autoptr(GHashTable) found = NULL;
> +    g_autoptr(GSList) refresh_list = NULL;

Now we allocate a new Transaction at the start of the function...

>
>      assert(bs_queue != NULL);
>

...but in the code between these two hunks there is this:

    QTAILQ_FOREACH(bs_entry, bs_queue, entry) {
        ret = bdrv_flush(bs_entry->state.bs);
        if (ret < 0) {
            error_setg_errno(errp, -ret, "Error flushing drive");
            goto cleanup;
        }
    }

which jumps to 'cleanup' on failure...


> -    if (ret == 0) {
> -        QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
> -            BlockDriverState *bs = bs_entry->state.bs;
> +    ret = 0;
> +    goto cleanup;
>
> -            if (bs->drv->bdrv_reopen_commit_post)
> -                bs->drv->bdrv_reopen_commit_post(&bs_entry->state);
> +abort:
> +    tran_abort(tran);
> +    QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
> +        if (bs_entry->prepared) {
> +            bdrv_reopen_abort(&bs_entry->state);
>          }
> +        qobject_unref(bs_entry->state.explicit_options);
> +        qobject_unref(bs_entry->state.options);
>      }
> +
>  cleanup:
>      QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
> -        if (ret) {
> -            if (bs_entry->prepared) {
> -                bdrv_reopen_abort(&bs_entry->state);
> -            }
> -            qobject_unref(bs_entry->state.explicit_options);
> -            qobject_unref(bs_entry->state.options);
> -        }
> -        if (bs_entry->state.new_backing_bs) {
> -            bdrv_unref(bs_entry->state.new_backing_bs);
> -        }
>          g_free(bs_entry);
>      }
>      g_free(bs_queue);

...and the 'cleanup' label doesn't free the Transaction.


An easy fix would be to move the call to tran_new() down to
below the loop that calls bdrv_flush().

thanks
-- PMM