[Qemu-devel] qcow2 not cleaning up during image create failure

[Qemu-devel] qcow2 not cleaning up during image create failure

[Daniel P. Berrange]

I've just been looking at the qcow2 image creation code, and found that
if any method in qcow2_create2() returns an error, then we'll report that,
but leave the newly created image file on disk in some partially initialized
state. A user may unwittingly use this file later with undefined behaviour.
This is particularly bad if we fail to setup encryption, because the user
is left with a file with no encryption enabled.

So I'm wondering how is the best way to clean up after failure ?

Naively I would like to just unlink(filename), but IIUC, filename is
not guaranteed to refer to a local file, and AFAIK, there is not
bdrv_delete() method todo this portably.

If we can't delete a file (because its a block device or network
volume), then we must at least blank out the just-written qcow2
header with zeros.

Ideas / suggestions.

You can demo this easily, by adding  ret = -EINVAL; immediately
after the first blk_pwrite() call in qcow2_create2()

diff --git a/block/qcow2.c b/block/qcow2.c
index 75d3e3c731..205c924f6d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2778,6 +2778,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
 
     ret = blk_pwrite(blk, 0, header, cluster_size, 0);
     g_free(header);
+    ret = -EINVAL;
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not write qcow2 header");
         goto out;


$ qemu-img  create -f qcow2 eek.img 1g
Formatting 'eek.img', fmt=qcow2 size=1073741824 cluster_size=65536 lazy_refcounts=off refcount_bits=16
qemu-img: eek.img: Could not write qcow2 header: Invalid argument

$ qemu-img info eek.img 
image: eek.img
file format: qcow2
virtual size: 0 (0 bytes)
disk size: 64K
cluster_size: 65536
Format specific information:
    compat: 1.1
    lazy refcounts: false
    refcount bits: 16
    corrupt: false


I imagine this may well affect other disk format drivers too.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] qcow2 not cleaning up during image create failure

[Fam Zheng]

On Fri, 07/14 12:00, Daniel P. Berrange wrote:
> I've just been looking at the qcow2 image creation code, and found that
> if any method in qcow2_create2() returns an error, then we'll report that,
> but leave the newly created image file on disk in some partially initialized
> state. A user may unwittingly use this file later with undefined behaviour.
> This is particularly bad if we fail to setup encryption, because the user
> is left with a file with no encryption enabled.
> 
> So I'm wondering how is the best way to clean up after failure ?
> 
> Naively I would like to just unlink(filename), but IIUC, filename is
> not guaranteed to refer to a local file, and AFAIK, there is not
> bdrv_delete() method todo this portably.
> 
> If we can't delete a file (because its a block device or network
> volume), then we must at least blank out the just-written qcow2
> header with zeros.
> 
> Ideas / suggestions.

Or just write the header as the last step?

Fam

Re: [Qemu-devel] qcow2 not cleaning up during image create failure

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1386 bytes --]

On 07/14/2017 06:57 AM, Fam Zheng wrote:
> On Fri, 07/14 12:00, Daniel P. Berrange wrote:
>> I've just been looking at the qcow2 image creation code, and found that
>> if any method in qcow2_create2() returns an error, then we'll report that,
>> but leave the newly created image file on disk in some partially initialized
>> state. A user may unwittingly use this file later with undefined behaviour.
>> This is particularly bad if we fail to setup encryption, because the user
>> is left with a file with no encryption enabled.
>>
>> So I'm wondering how is the best way to clean up after failure ?
>>
>> Naively I would like to just unlink(filename), but IIUC, filename is
>> not guaranteed to refer to a local file, and AFAIK, there is not
>> bdrv_delete() method todo this portably.
>>
>> If we can't delete a file (because its a block device or network
>> volume), then we must at least blank out the just-written qcow2
>> header with zeros.
>>
>> Ideas / suggestions.
> 
> Or just write the header as the last step?

Or even keep the current steps where we write portions of the header
early, but ensure that all the early writes do NOT set the magic number,
and only the final write to the file puts QFI\xfb into place.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] qcow2 not cleaning up during image create failure

[Daniel P. Berrange]

On Fri, Jul 14, 2017 at 07:57:43PM +0800, Fam Zheng wrote:
> On Fri, 07/14 12:00, Daniel P. Berrange wrote:
> > I've just been looking at the qcow2 image creation code, and found that
> > if any method in qcow2_create2() returns an error, then we'll report that,
> > but leave the newly created image file on disk in some partially initialized
> > state. A user may unwittingly use this file later with undefined behaviour.
> > This is particularly bad if we fail to setup encryption, because the user
> > is left with a file with no encryption enabled.
> > 
> > So I'm wondering how is the best way to clean up after failure ?
> > 
> > Naively I would like to just unlink(filename), but IIUC, filename is
> > not guaranteed to refer to a local file, and AFAIK, there is not
> > bdrv_delete() method todo this portably.
> > 
> > If we can't delete a file (because its a block device or network
> > volume), then we must at least blank out the just-written qcow2
> > header with zeros.
> > 
> > Ideas / suggestions.
> 
> Or just write the header as the last step?

qcow2 has a multi-step creation process - it writes a minimal header,
then opens the file and writes some more metadata.  So delaying write
of the header is impractical with that approach.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|