Re: [PATCH v5 1/6] qcow2: Fix dangling pointer after reopen for 'file'

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

06.07.2021 14:23, Kevin Wolf wrote:
> Without an external data file, s->data_file is a second pointer with the
> same value as bs->file. When changing bs->file to a different BdrvChild
> and freeing the old BdrvChild, s->data_file must also be updated,
> otherwise it points to freed memory and causes crashes.
> 
> This problem was caught by iotests case 245.
> 
> Fixes: df2b7086f169239ebad5d150efa29c9bb6d4f820
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Still, some ideas below:

> ---
>   block/qcow2.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ee4530cdbd..cb459ef6a6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -962,6 +962,7 @@ static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
>   }
>   
>   typedef struct Qcow2ReopenState {
> +    bool had_data_file;
>       Qcow2Cache *l2_table_cache;
>       Qcow2Cache *refcount_block_cache;
>       int l2_slice_size; /* Number of entries in a slice of the L2 table */
> @@ -1932,6 +1933,8 @@ static int qcow2_reopen_prepare(BDRVReopenState *state,
>       r = g_new0(Qcow2ReopenState, 1);
>       state->opaque = r;
>   
> +    r->had_data_file = has_data_file(state->bs);
> +

So, during reopen, at some moment s->data_file become invalid. So, we shouldn't rely on it..

Maybe we need

        s->data_file = NULL;

here..

>       ret = qcow2_update_options_prepare(state->bs, r, state->options,
>                                          state->flags, errp);
>       if (ret < 0) {
> @@ -1966,7 +1969,18 @@ fail:
>   
>   static void qcow2_reopen_commit(BDRVReopenState *state)
>   {
> +    BDRVQcow2State *s = state->bs->opaque;
> +    Qcow2ReopenState *r = state->opaque;
> +
>       qcow2_update_options_commit(state->bs, state->opaque);

Worth doing

        assert(r->had_data_file == has_data_file(state->bs));

here, to be double sure?

> +    if (!r->had_data_file && s->data_file != state->bs->file) {
> +        /*
> +         * If s->data_file is just a second pointer to bs->file (which is the
> +         * case without an external data file), it may need to be updated.
> +         */
> +        s->data_file = state->bs->file;
> +        assert(!has_data_file(state->bs));
> +    }
>       g_free(state->opaque);
>   }
>   
> 


-- 
Best regards,
Vladimir