Re: [Qemu-devel] [PATCH 2/2] qcow2: mark image as corrupt if failing during create

[Max Reitz]

On 19.02.19 13:50, Daniel P. Berrangé wrote:
> During creation we write a minimal qcow2 header and then update it with
> extra features. If the updating fails for some reason we might still be
> left with a valid qcow2 image that will be mistakenly used for I/O. We
> cannot delete the image, since we don't know if we created the
> underlying storage or not. Thus we mark the header as corrupt to
> prevents its later usage.
> 
> Signed-off-by: Daniel P. Berrangé <address@hidden>
> ---
>  block/qcow2.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index ecc577175f..338513e652 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3104,6 +3104,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
> Error **errp)
>  
>      ret = 0;
>  out:
> +    if (ret < 0) {
> +        qcow2_mark_corrupt(blk_bs(blk));

First, blk_bs(blk) may be the qcow2 BDS, or it may the protocol BDS here
(it is the latter before the first blk_new_open() call).  Calling
qcow2_mark_corrupt() unconditionally may mean an invalid access to
bs->opaque (which is not going to be of type BDRVQcow2State if the BDS
is not a qcow2 BDS).

Second, blk may be NULL (at various points, e.g. after blk_new_open()
failed).  Then this would yield a segfault in in blk_bs().

Third, blk_bs(blk) may be NULL (if blk_insert_bs() failed).  Then this
would yield a segfault in qcow2_mark_corrupt().

On a minor note, it is rather probably that blk_new_open() fails.  In
that case, there is currently no way to mark the image corrupt.  Would
it be useful and possible to have a function to mark a qcow2 image
corrupt without relying on qcow2 features, i.e. by writing directly to
the protocol layer (which is always @bs)?  This would be unsafe to use
as long as the protocol layer is opened by the qcow2 driver in some
other node, but we could invoke this function safely after @blk has been
freed.


Or maybe Eric's suggestion really is for the best, i.e. mark the image
corrupt from the start and then clean that after we're all done.  You
don't need a new flag for that, we already have BDRV_O_CHECK.

Max

> +    }
>      blk_unref(blk);
>      bdrv_unref(bs);
>      return ret;
>

signature.asc
Description: OpenPGP digital signature