| Message ID | 20170126101827.22378-5-berrange@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01/26/2017 04:18 AM, Daniel P. Berrange wrote: > The qcow driver refuses to open images which are less than > 2 bytes in size, but will happily create such images. Add > a check in the create path to avoid this discrepancy. I agree that we have the 2-byte limit: static int qcow_open(BlockDriverState *bs, QDict *options, int flags, ... if (header.size <= 1) { error_setg(errp, "Image size is too small (must be at least 2 bytes)"); ret = -EINVAL; goto fail; } But why 2 bytes? That's a weird limit from history. Oh well, no one should be creating many new qcow images when we have qcow2, so I see no problem with your patch as-is. > > Reviewed-by: Max Reitz <mreitz@redhat.com> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com> > --- > block/qcow.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > +++ b/block/qcow.c > @@ -799,6 +799,12 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp) > /* Read out options */ > total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), > BDRV_SECTOR_SIZE); > + if (total_size <= 1) { It will never be 1 because of the ROUND_UP (it will either be 0 or a sector size), but your code is defensive to match the open check. Reviewed-by: Eric Blake <eblake@redhat.com> > + error_setg(errp, "Image size is too small, cannot be zero length"); > + ret = -EINVAL; > + goto cleanup; > + } > + > backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE); > if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) { > flags |= BLOCK_FLAG_ENCRYPT; >
On Wed 08 Feb 2017 08:29:27 PM CET, Eric Blake wrote: >> The qcow driver refuses to open images which are less than 2 bytes in >> size, but will happily create such images. Add a check in the create >> path to avoid this discrepancy. > > I agree that we have the 2-byte limit: [...] > But why 2 bytes? That's a weird limit from history. I also don't see what's the point, considering that in qcow_open() bs->total_sectors = header.size / 512; So anything smaller than 512 is an empty image in practice. Maybe it's worth increasing the lower limit in qcow_open(), and/or rejecting to open images with a size that is not multiple of 512. But that would be for a different patch. This one is fine as it is. I think that the condition can be simply (total_size == 0) because it can never have a value between 0 and BDRV_SECTOR_SIZE, but either way Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
diff --git a/block/qcow.c b/block/qcow.c index 7540f43..101c973 100644 --- a/block/qcow.c +++ b/block/qcow.c @@ -799,6 +799,12 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp) /* Read out options */ total_size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0), BDRV_SECTOR_SIZE); + if (total_size <= 1) { + error_setg(errp, "Image size is too small, cannot be zero length"); + ret = -EINVAL; + goto cleanup; + } + backing_file = qemu_opt_get_del(opts, BLOCK_OPT_BACKING_FILE); if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) { flags |= BLOCK_FLAG_ENCRYPT;