| Message ID | 20170714110048.GC28095@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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;
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() $ 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