Yes, I can confirm the presence of this bug and this is a valid fix. May I ask where is this kind of vmdk from? Because regularly we see extents in identical size.
--- Best regards! Fam Zheng On Fri, Nov 9, 2012 at 3:05 AM, Gerhard Wiesinger <li...@wiesinger.com> wrote: > Fixed a MAJOR BUG in VMDK files on file boundaries on reads > and ALSO ON WRITES WHICH MIGHT CORRUPT THE IMAGE AND DATA!!!!!! > > Triggered for example with the following VMDK file (partly listed): > # Extent description > RW 4193792 FLAT "XP-W1-f001.vmdk" 0 > RW 2097664 FLAT "XP-W1-f002.vmdk" 0 > RW 4193792 FLAT "XP-W1-f003.vmdk" 0 > RW 512 FLAT "XP-W1-f004.vmdk" 0 > RW 4193792 FLAT "XP-W1-f005.vmdk" 0 > RW 2097664 FLAT "XP-W1-f006.vmdk" 0 > RW 4193792 FLAT "XP-W1-f007.vmdk" 0 > RW 512 FLAT "XP-W1-f008.vmdk" 0 > > Patch includes: > 1.) Patch fixes wrong calculation on extent boundaries. Especially it fixes > the relativeness of the sector number to the current extent. > 2.) Added debug code to block.c and to block/vmdk.c to verify correctness > 3.) Also optimized code which avoids multiplication and uses shifts. > > Verfied correctness with: > 1.) Converted either with Virtualbox to VDI and then with qemu-img and then > with qemu-img only > VBoxManage clonehd --format vdi /VM/XP-W/new/XP-W1.vmdk > ~/.VirtualBox/Harddisks/XP-W1-new-test.vdi > ./qemu-img convert -O raw ~/.VirtualBox/Harddisks/XP-W1-new-test.vdi > /root/QEMU/VM-XP-W1/XP-W1-via-VBOX.img > md5sum /root/QEMU/VM-XP-W/XP-W1-direct.img > md5sum /root/QEMU/VM-XP-W/XP-W1-via-VBOX.img > => same MD5 hash > 2.) Verified debug log files > 3.) Run Windows XP successfully > 4.) chkdsk run successfully without any errors > > Signed-off-by: Gerhard Wiesinger <li...@wiesinger.com> > --- > block.c | 50 +++++++++++++++++++++++ > block/vmdk.c | 129 > ++++++++++++++++++++++++++++++++++++++++++++++++++++++----- > 2 files changed, 170 insertions(+), 9 deletions(-) > > diff --git a/block.c b/block.c > index da1fdca..69259f2 100644 > --- a/block.c > +++ b/block.c > @@ -49,6 +49,12 @@ > #include <windows.h> > #endif > > +#if 0 > + #define DEBUG_BLOCK > +#endif > + > +#define DEBUG_BLOCK_PREFIX "BLOCK: " > + > #define NOT_DONE 0x7fffffff /* used while emulated sync operation in > progress */ > > typedef enum { > @@ -789,6 +795,18 @@ int bdrv_open(BlockDriverState *bs, const char > *filename, int flags, > int ret; > char tmp_filename[PATH_MAX]; > > +#ifdef DEBUG_BLOCK > + const char *format = "(nil)"; > + if (drv) { > + format = drv->format_name; > + } > + > + printf(DEBUG_BLOCK > + "bdrv_open: filename=%s, BlockDriver=%p, format=%s\n", > + filename, drv, format > + ); > +#endif > + > if (flags & BDRV_O_SNAPSHOT) { > BlockDriverState *bs1; > int64_t total_size; > @@ -2004,6 +2022,22 @@ static int bdrv_rw_co(BlockDriverState *bs, int64_t > sector_num, uint8_t *buf, > int bdrv_read(BlockDriverState *bs, int64_t sector_num, > uint8_t *buf, int nb_sectors) > { > +#ifdef DEBUG_BLOCK > + BlockDriver *drv = bs->drv; > + const char *format_name = "(nil)"; > + if (drv) { > + if (drv->format_name) { > + format_name = drv->format_name; > + } > + } > + > + printf(DEBUG_BLOCK > + "bdrv_read: driver=%s, filename=%s, sector_num=%" PRId64 > + ", nb_sectors=%i\n", > + format_name, bs->filename, sector_num, nb_sectors > + ); > +#endif > + > return bdrv_rw_co(bs, sector_num, buf, nb_sectors, false); > } > > @@ -2060,6 +2094,22 @@ static void set_dirty_bitmap(BlockDriverState *bs, > int64_t sector_num, > int bdrv_write(BlockDriverState *bs, int64_t sector_num, > const uint8_t *buf, int nb_sectors) > { > +#ifdef DEBUG_BLOCK > + BlockDriver *drv = bs->drv; > + const char *format_name = "(nil)"; > + if (drv) { > + if (drv->format_name) { > + format_name = drv->format_name; > + } > + } > + > + printf(DEBUG_BLOCK > + "bdrv_write: driver=%s, filename=%s, sector_num=%" PRId64 > + ", nb_sectors=%i\n", > + format_name, bs->filename, sector_num, nb_sectors > + ); > +#endif > + > return bdrv_rw_co(bs, sector_num, (uint8_t *)buf, nb_sectors, true); > } > > diff --git a/block/vmdk.c b/block/vmdk.c > index 1a80e5a..92ab92c 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -29,6 +29,15 @@ > #include "migration.h" > #include <zlib.h> > > +#if 0 > + #define DEBUG_VMDK > +#endif > + > +#define DEBUG_VMDK_PREFIX "VMDK: " > +#define DEBUG_VMDK_SEPARATOR \ > + "########################################" \ > + "########################################" > + > #define VMDK3_MAGIC (('C' << 24) | ('O' << 16) | ('W' << 8) | 'D') > #define VMDK4_MAGIC (('K' << 24) | ('D' << 16) | ('M' << 8) | 'V') > #define VMDK4_COMPRESSION_DEFLATE 1 > @@ -377,6 +386,16 @@ static VmdkExtent *vmdk_add_extent(BlockDriverState > *bs, > VmdkExtent *extent; > BDRVVmdkState *s = bs->opaque; > > +#ifdef DEBUG_VMDK > + printf(DEBUG_VMDK_PREFIX > + "vmdk_add_extent: flat=%i, sectors=%" PRId64 > + ", l1_backup_offset=%" PRId64 > + ", l1_size=%u, l2_size=%i, cluster_sectors=%u\n", > + (int)flat, sectors, l1_backup_offset, > + l1_size, l2_size, cluster_sectors > + ); > +#endif > + > s->extents = g_realloc(s->extents, > (s->num_extents + 1) * sizeof(VmdkExtent)); > extent = &s->extents[s->num_extents]; > @@ -674,6 +693,14 @@ static int vmdk_parse_extents(const char *desc, > BlockDriverState *bs, > return ret; > } > > +#ifdef DEBUG_VMDK > + printf(DEBUG_VMDK_PREFIX > + "valid extent found: access=%s, sectors=%" PRId64 > + ", type=%s, filename=%s, flat_offset=%" PRId64 "\n", > + access, sectors, type, fname, flat_offset > + ); > +#endif > + > /* save to extents array */ > if (!strcmp(type, "FLAT")) { > /* FLAT extent */ > @@ -704,6 +731,45 @@ next_line: > return 0; > } > > +#ifdef DEBUG_VMDK > +static void vmdk_debug_print_one_extent(VmdkExtent *extent) > +{ > + printf(DEBUG_VMDK_PREFIX DEBUG_VMDK_SEPARATOR "\n"); > + printf(DEBUG_VMDK_PREFIX "filename%s\n", extent->file->filename); > + printf(DEBUG_VMDK_PREFIX "flat=%i\n", (int)extent->flat); > + printf(DEBUG_VMDK_PREFIX "compressed=%i\n", (int)extent->compressed); > + printf(DEBUG_VMDK_PREFIX "has_marker=%i\n", (int)extent->has_marker); > + printf(DEBUG_VMDK_PREFIX "sectors=%" PRId64 "\n", extent->sectors); > + printf(DEBUG_VMDK_PREFIX "end_sector=%" PRId64 "\n", > extent->end_sector); > + printf(DEBUG_VMDK_PREFIX "flat_start_offset=%" PRId64 "\n", > + extent->flat_start_offset); > + printf(DEBUG_VMDK_PREFIX "l1_table_offsett=%" PRId64 "\n", > + extent->l1_table_offset); > + printf(DEBUG_VMDK_PREFIX "l1_backup_table_offsett=%" > + PRId64 "\n", extent->l1_backup_table_offset); > + printf(DEBUG_VMDK_PREFIX "l1_size=%u\n", extent->l1_size); > + printf(DEBUG_VMDK_PREFIX "l1_entry_sectors=%u\n", > extent->l1_entry_sectors); > + printf(DEBUG_VMDK_PREFIX "l2_size=%u\n", extent->l2_size); > + printf(DEBUG_VMDK_PREFIX "cluster_sectors=%u\n", > extent->cluster_sectors); > + fflush(stdout); > +} > + > +static void vmdk_debug_print_extents(BlockDriverState *bs) > +{ > + BDRVVmdkState *s = bs->opaque; > + VmdkExtent *extent = &s->extents[0]; > + > + printf(DEBUG_VMDK_PREFIX DEBUG_VMDK_SEPARATOR "\n"); > + printf(DEBUG_VMDK_PREFIX "total_sectors=%" PRId64 "\n", > bs->total_sectors); > + printf(DEBUG_VMDK_PREFIX "num_extents=%u\n", s->num_extents); > + > + while (extent < &s->extents[s->num_extents]) { > + vmdk_debug_print_one_extent(extent); > + extent++; > + } > +} > +#endif > + > static int vmdk_open_desc_file(BlockDriverState *bs, int flags, > int64_t desc_offset) > { > @@ -728,7 +794,13 @@ static int vmdk_open_desc_file(BlockDriverState *bs, > int flags, > return -ENOTSUP; > } > s->desc_offset = 0; > - return vmdk_parse_extents(buf, bs, bs->file->filename); > + ret = vmdk_parse_extents(buf, bs, bs->file->filename); > + > +#ifdef DEBUG_VMDK > + vmdk_debug_print_extents(bs); > +#endif > + > + return ret; > } > > static int vmdk_open(BlockDriverState *bs, int flags) > @@ -774,6 +846,14 @@ static int get_whole_cluster(BlockDriverState *bs, > /* 128 sectors * 512 bytes each = grain size 64KB */ > uint8_t whole_grain[extent->cluster_sectors * 512]; > > +#ifdef DEBUG_VMDK > + printf(DEBUG_VMDK_PREFIX > + "vmdk_get_whole_cluster: filename=%s, cluster_offset=%" PRId64 > + ", offset=%" PRId64 "\n", > + extent->file->filename, cluster_offset, offset > + ); > +#endif > + > /* we will be here if it's first write on non-exist grain(cluster). > * try to read from parent image, if exist */ > if (bs->backing_hd) { > @@ -1032,18 +1112,35 @@ static int vmdk_read_extent(VmdkExtent *extent, > int64_t cluster_offset, > VmdkGrainMarker *marker; > uLongf buf_len; > > +#ifdef DEBUG_VMDK > + printf(DEBUG_VMDK_PREFIX > + "vmdk_read_extent: filename=%s, cluster_offset=%" PRId64 > + ", offset_in_cluster=%" PRId64 ", nb_sectors=%i\n", > + extent->file->filename, cluster_offset, > + offset_in_cluster, nb_sectors > + ); > +#endif > > if (!extent->compressed) { > + > +#ifdef DEBUG_VMDK > + printf(DEBUG_VMDK_PREFIX > + "vmdk_read_extent:bdrv_pread filename=%s, offset=%" PRId64 > + ", length=%i\n", > + extent->file->filename, offset_in_cluster, nb_sectors << 9 > + ); > +#endif > + > ret = bdrv_pread(extent->file, > cluster_offset + offset_in_cluster, > - buf, nb_sectors * 512); > - if (ret == nb_sectors * 512) { > + buf, nb_sectors << 9); > + if (ret == nb_sectors << 9) { > return 0; > } else { > return -EIO; > } > } > - cluster_bytes = extent->cluster_sectors * 512; > + cluster_bytes = extent->cluster_sectors << 9; > /* Read two clusters in case GrainMarker + compressed data > one > cluster */ > buf_bytes = cluster_bytes * 2; > cluster_buf = g_malloc(buf_bytes); > @@ -1092,9 +1189,18 @@ static int vmdk_read(BlockDriverState *bs, int64_t > sector_num, > BDRVVmdkState *s = bs->opaque; > int ret; > uint64_t n, index_in_cluster; > + uint64_t extent_begin_sector, extent_relative_sector_num; > VmdkExtent *extent = NULL; > uint64_t cluster_offset; > > +#ifdef DEBUG_VMDK > + printf(DEBUG_VMDK_PREFIX > + "vmdk_read: sectornum=%" PRId64 > + ", nb_sectors=%i\n", > + sector_num, nb_sectors > + ); > +#endif > + > while (nb_sectors > 0) { > extent = find_extent(s, sector_num, extent); > if (!extent) { > @@ -1103,7 +1209,9 @@ static int vmdk_read(BlockDriverState *bs, int64_t > sector_num, > ret = get_cluster_offset( > bs, extent, NULL, > sector_num << 9, 0, &cluster_offset); > - index_in_cluster = sector_num % extent->cluster_sectors; > + extent_begin_sector = extent->end_sector - extent->sectors; > + extent_relative_sector_num = sector_num - extent_begin_sector; > + index_in_cluster = extent_relative_sector_num % > extent->cluster_sectors; > n = extent->cluster_sectors - index_in_cluster; > if (n > nb_sectors) { > n = nb_sectors; > @@ -1119,11 +1227,11 @@ static int vmdk_read(BlockDriverState *bs, int64_t > sector_num, > return ret; > } > } else { > - memset(buf, 0, 512 * n); > + memset(buf, 0, n << 9); > } > } else { > ret = vmdk_read_extent(extent, > - cluster_offset, index_in_cluster * 512, > + cluster_offset, index_in_cluster << 9, > buf, n); > if (ret) { > return ret; > @@ -1131,7 +1239,7 @@ static int vmdk_read(BlockDriverState *bs, int64_t > sector_num, > } > nb_sectors -= n; > sector_num += n; > - buf += n * 512; > + buf += n << 9; > } > return 0; > } > @@ -1154,6 +1262,7 @@ static int vmdk_write(BlockDriverState *bs, int64_t > sector_num, > VmdkExtent *extent = NULL; > int n, ret; > int64_t index_in_cluster; > + uint64_t extent_begin_sector, extent_relative_sector_num; > uint64_t cluster_offset; > VmdkMetaData m_data; > > @@ -1196,7 +1305,9 @@ static int vmdk_write(BlockDriverState *bs, int64_t > sector_num, > if (ret) { > return -EINVAL; > } > - index_in_cluster = sector_num % extent->cluster_sectors; > + extent_begin_sector = extent->end_sector - extent->sectors; > + extent_relative_sector_num = sector_num - extent_begin_sector; > + index_in_cluster = extent_relative_sector_num % > extent->cluster_sectors; > n = extent->cluster_sectors - index_in_cluster; > if (n > nb_sectors) { > n = nb_sectors; > -- > 1.7.11.7 >