On Thu, Aug 07, 2014 at 07:03:12PM +0400, Denis V. Lunev wrote:
> On 07/08/14 18:39, Jeff Cody wrote:
> >On Mon, Jul 28, 2014 at 08:23:55PM +0400, Denis V. Lunev wrote:
> >>Parallels has released in the recent updates of Parallels Server 5/6
> >>new addition to his image format. Images with signature WithouFreSpacExt
> >>have offsets in the catalog coded not as offsets in sectors (multiple
> >>of 512 bytes) but offsets coded in blocks (i.e. header->tracks * 512)
> >>
> >>In this case all 64 bits of header->nb_sectors are used for image size.
> >>
> >>This patch implements support of this for qemu-img and also adds specific
> >>check for an incorrect image. Images with block size greater than
> >>INT_MAX/513 are not supported. The biggest available Parallels image
> >>cluster size in the field is 1 Mb. Thus this limit will not hurt
> >>anyone.
> >>
> >>Signed-off-by: Denis V. Lunev <d...@openvz.org>
> >>CC: Jeff Cody <jc...@redhat.com>
> >>CC: Kevin Wolf <kw...@redhat.com>
> >>CC: Stefan Hajnoczi <stefa...@redhat.com>
> >>---
> >> block/parallels.c | 25 ++++++++++++++++++++-----
> >> 1 file changed, 20 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/block/parallels.c b/block/parallels.c
> >>index 466705e..4414a9d 100644
> >>--- a/block/parallels.c
> >>+++ b/block/parallels.c
> >>@@ -30,6 +30,7 @@
> >> /**************************************************************/
> >> #define HEADER_MAGIC "WithoutFreeSpace"
> >>+#define HEADER_MAGIC2 "WithouFreSpacExt"
> >> #define HEADER_VERSION 2
> >> #define HEADER_SIZE 64
> >>@@ -54,6 +55,8 @@ typedef struct BDRVParallelsState {
> >> unsigned int catalog_size;
> >> unsigned int tracks;
> >>+
> >>+ unsigned int off_multiplier;
> >> } BDRVParallelsState;
> >> static int parallels_probe(const uint8_t *buf, int buf_size, const char
> >> *filename)
> >>@@ -63,7 +66,8 @@ static int parallels_probe(const uint8_t *buf, int
> >>buf_size, const char *filenam
> >> if (buf_size < HEADER_SIZE)
> >> return 0;
> >>- if (!memcmp(ph->magic, HEADER_MAGIC, 16) &&
> >>+ if ((!memcmp(ph->magic, HEADER_MAGIC, 16) ||
> >>+ !memcmp(ph->magic, HEADER_MAGIC2, 16)) &&
> >> (le32_to_cpu(ph->version) == HEADER_VERSION))
> >> return 100;
> >>@@ -85,21 +89,31 @@ static int parallels_open(BlockDriverState *bs, QDict
> >>*options, int flags,
> >> goto fail;
> >> }
> >>+ bs->total_sectors = le64_to_cpu(ph.nb_sectors);
> >>+
> >> if (le32_to_cpu(ph.version) != HEADER_VERSION) {
> >> goto fail_format;
> >> }
> >>- if (memcmp(ph.magic, HEADER_MAGIC, 16)) {
> >>+ if (!memcmp(ph.magic, HEADER_MAGIC, 16)) {
> >>+ s->off_multiplier = 1;
> >>+ bs->total_sectors = 0xffffffff & bs->total_sectors;
> >>+ } else if (!memcmp(ph.magic, HEADER_MAGIC2, 16)) {
> >>+ s->off_multiplier = le32_to_cpu(ph.tracks);
> >>+ } else {
> >> goto fail_format;
> >> }
> >>- bs->total_sectors = 0xffffffff & le64_to_cpu(ph.nb_sectors);
> >>-
> >> s->tracks = le32_to_cpu(ph.tracks);
> >> if (s->tracks == 0) {
> >> error_setg(errp, "Invalid image: Zero sectors per track");
> >> ret = -EINVAL;
> >> goto fail;
> >> }
> >>+ if (s->tracks > INT32_MAX/513) {
> >>+ error_setg(errp, "Invalid image: Too big cluster");
> >>+ ret = -EFBIG;
> >>+ goto fail;
> >>+ }
> >> s->catalog_size = le32_to_cpu(ph.catalog_entries);
> >> if (s->catalog_size > INT_MAX / 4) {
> >>@@ -139,7 +153,8 @@ static int64_t seek_to_sector(BlockDriverState *bs,
> >>int64_t sector_num)
> >> /* not allocated */
> >> if ((index > s->catalog_size) || (s->catalog_bitmap[index] == 0))
> >> return -1;
> >>- return (uint64_t)(s->catalog_bitmap[index] + offset) * 512;
> >>+ return
> >>+ ((uint64_t)s->catalog_bitmap[index] * s->off_multiplier + offset)
> >>* 512;
> >This still does a cast to uint_64_t, instead of int64_t; not sure it
> >really matters in practice, as we should be safe now from exceeding an
> >int64_t value in the end result.
>
> this is safe due to above check for s->tracks > INT32_MAX/513
> Actually, original code has exactly the same cast and the situation
> is exactly the same before the patch (uint32_t value * 1) and after
> the patch (uint32_t * (something < INT32_MAX/513))
>
> Though I can change the cast to int64_t, I do not see much difference.
> Should I do this?
>
Right, as I said in practice it should now be safe from exceeding an
int64_t (due to the bounds check on s->tracks). I think it is worth
changing if someone else requests a respin for another reason, but
probably not to do a respin for this on its own.
Another question - do you have any sample images? If they compress
well (bzip2 does a good job with most blank images) it would be nice
to have a couple of parallels images (e.g. one "WithouFreSpacExt" and
one "WithoutFreeSpace") in the tests sample_images directory. If you
can provide both types of images, I'll amend test 076 to include them.
I can go ahead and give my R-b for this patch:
Reviewed-by: Jeff Cody <jc...@redhat.com>
> >> }
> >> static int parallels_read(BlockDriverState *bs, int64_t sector_num,
> >>--
> >>1.9.1
> >>
> >>
>
>
