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é <berra...@redhat.com> > --- > 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