| Message ID | 1309224777-31024-10-git-send-email-famcool@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng <famcool@gmail.com> wrote: > +/* find an option value out of descriptor file */ > +static int vmdk_parse_description(const char *desc, const char *opt_name, > + char *buf, int buf_size) > +{ > + char *opt_pos, *opt_end; > + const char *end = desc + strlen(desc); > + > + opt_pos = strstr(desc, opt_name); > + if (!opt_pos) { > + return -1; > + } > + /* Skip "=\"" following opt_name */ > + opt_pos += strlen(opt_name) + 2; > + if (opt_pos >= end) { > + return -1; > + } This can produce false positives because strstr(desc, opt_name) will match anything that contains the opt_name string. Also we don't verify that "=\"" follow the opt_name. Luckily we only use this for "createType" which will hopefully never cause false positives. > + opt_end = opt_pos; > + while (opt_end < end && *opt_end != '"') { > + opt_end++; > + } > + if (opt_end == end || buf_size < opt_end - opt_pos + 1) { > + return -1; > + } > + pstrcpy(buf, opt_end - opt_pos + 1, opt_pos); > + return 0; > +} > + > +static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, > + const char *desc_file_path) > +{ > + int ret = 0; > + int r; > + char access[11]; > + char type[11]; > + char fname[512]; > + const char *p = desc; > + int64_t sectors = 0; > + int64_t flat_offset; > + > + while (*p) { > + if (strncmp(p, "RW", strlen("RW"))) { > + goto next_line; > + } This check is duplicated below and can be removed. > + /* parse extent line: > + * RW [size in sectors] FLAT "file-name.vmdk" OFFSET > + * or > + * RW [size in sectors] SPARSE "file-name.vmdk" > + */ > + flat_offset = -1; > + sscanf(p, "%10s %lld %10s %512s", > + access, §ors, type, fname); Please check the sscanf(3) return value to ensure the format string matched. The value of access[], type[], fname[], sectors will be unchanged at the point where sscanf(3) fails so your checks will not work. Declared as char fname[512] but using "%512s" format string. The format string needs to be 511 to leave space for the NUL terminator. > + if (!strcmp(type, "FLAT")) { > + sscanf(p, "%10s %lld %10s %512s %lld", > + access, §ors, type, fname, &flat_offset); > + if (flat_offset == -1) { > + return -EINVAL; > + } > + } > + > + /* trim the quotation marks around */ > + if (fname[0] == '"') { > + memmove(fname, fname + 1, strlen(fname) + 1); This copies 1 byte too many, just strlen(fname) will do. > + if (fname[strlen(fname) - 1] == '"') { > + fname[strlen(fname) - 1] = '\0'; > + } > + } If starts as fname[] = {'"', '\0'} then this if statement checks fname[-1] == '"'! > + if (!(strlen(access) && sectors && strlen(type) && strlen(fname))) { > + goto next_line; > + } These can be replaced by checking the sscanf(3) return value above. Validating sectors is still a good idea though. > + if (strcmp(type, "FLAT") && strcmp(type, "SPARSE")) { > + goto next_line; > + } > + if (strcmp(access, "RW")) { > + goto next_line; > + } > + ret++; This variable is unused. > + /* save to extents array */ > + if (!strcmp(type, "FLAT")) { > + /* FLAT extent */ > + char extent_path[PATH_MAX]; > + BlockDriverState *extent_file; > + BlockDriver *drv; > + VmdkExtent *extent; > + > + extent_file = bdrv_new(""); > + drv = bdrv_find_format("file"); > + if (!drv) { bdrv_delete(extent_file); > + return -EINVAL; > + } > + path_combine(extent_path, sizeof(extent_path), > + desc_file_path, fname); > + r = bdrv_open(extent_file, extent_path, > + BDRV_O_RDWR | BDRV_O_NO_BACKING, drv); We should honor the bs->open_flags. Otherwise cache=none|writeback|writethrough|unsafe is ignored on the actual extent files. > + if (r) { bdrv_delete(extent_file); > + return -EINVAL; > + } > + extent = vmdk_add_extent(bs, extent_file, true, sectors, > + 0, 0, 0, 0, sectors); > + extent->flat_start_offset = flat_offset; > + } else { > + /* SPARSE extent, not supported for now */ > + fprintf(stderr, > + "VMDK: Not supported extent type \"%s\""".\n", type); > + return -ENOTSUP; > + } > +next_line: > + /* move to next line */ > + while (*p && *p != '\n') { > + p++; > + } > + p++; > + } > + return 0; > +} > + > +static int vmdk_open_desc_file(BlockDriverState *bs, int flags) > +{ > + int ret; > + char buf[2048]; > + char ct[128]; > + BDRVVmdkState *s = bs->opaque; > + > + ret = bdrv_pread(bs->file, 0, buf, sizeof(buf)); > + if (ret < 0) { > + return ret; > + } > + buf[2047] = '\0'; > + if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) { > + return -EINVAL; > + } > + if (strcmp(ct, "monolithicFlat")) { > + fprintf(stderr, > + "VMDK: Not supported image type \"%s\""".\n", ct); > + return -ENOTSUP; > + } > + s->desc_offset = 0; > + ret = vmdk_parse_extents(buf, bs, bs->file->filename); > + if (ret) { > + return ret; > + } > + > + /* try to open parent images, if exist */ > + if (vmdk_parent_open(bs)) { > + qemu_free(s->extents); This duplicates extent freeing code from vmdk_close(). Please reuse that (maybe by moving it into a vmdk_free_extents() function), it also frees l1_table, l2_cache, and l1_backup_table. Stefan
On Wed, Jun 29, 2011 at 11:57 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng <famcool@gmail.com> wrote: >> +/* find an option value out of descriptor file */ >> +static int vmdk_parse_description(const char *desc, const char *opt_name, >> + char *buf, int buf_size) >> +{ >> + char *opt_pos, *opt_end; >> + const char *end = desc + strlen(desc); >> + >> + opt_pos = strstr(desc, opt_name); >> + if (!opt_pos) { >> + return -1; >> + } >> + /* Skip "=\"" following opt_name */ >> + opt_pos += strlen(opt_name) + 2; >> + if (opt_pos >= end) { >> + return -1; >> + } > > This can produce false positives because strstr(desc, opt_name) will > match anything that contains the opt_name string. Also we don't > verify that "=\"" follow the opt_name. Luckily we only use this for > "createType" which will hopefully never cause false positives. > >> + opt_end = opt_pos; >> + while (opt_end < end && *opt_end != '"') { >> + opt_end++; >> + } >> + if (opt_end == end || buf_size < opt_end - opt_pos + 1) { >> + return -1; >> + } >> + pstrcpy(buf, opt_end - opt_pos + 1, opt_pos); >> + return 0; >> +} >> + >> +static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, >> + const char *desc_file_path) >> +{ >> + int ret = 0; >> + int r; >> + char access[11]; >> + char type[11]; >> + char fname[512]; >> + const char *p = desc; >> + int64_t sectors = 0; >> + int64_t flat_offset; >> + >> + while (*p) { >> + if (strncmp(p, "RW", strlen("RW"))) { >> + goto next_line; >> + } > > This check is duplicated below and can be removed. > >> + /* parse extent line: >> + * RW [size in sectors] FLAT "file-name.vmdk" OFFSET >> + * or >> + * RW [size in sectors] SPARSE "file-name.vmdk" >> + */ >> + flat_offset = -1; >> + sscanf(p, "%10s %lld %10s %512s", >> + access, §ors, type, fname); > > Please check the sscanf(3) return value to ensure the format string > matched. The value of access[], type[], fname[], sectors will be > unchanged at the point where sscanf(3) fails so your checks will not > work. > > Declared as char fname[512] but using "%512s" format string. The > format string needs to be 511 to leave space for the NUL terminator. > >> + if (!strcmp(type, "FLAT")) { >> + sscanf(p, "%10s %lld %10s %512s %lld", >> + access, §ors, type, fname, &flat_offset); >> + if (flat_offset == -1) { >> + return -EINVAL; >> + } >> + } >> + >> + /* trim the quotation marks around */ >> + if (fname[0] == '"') { >> + memmove(fname, fname + 1, strlen(fname) + 1); > > This copies 1 byte too many, just strlen(fname) will do. I meant to copy the NULL terminator too. > >> + if (fname[strlen(fname) - 1] == '"') { >> + fname[strlen(fname) - 1] = '\0'; >> + } >> + } > > If starts as fname[] = {'"', '\0'} then this if statement checks > fname[-1] == '"'! > >> + if (!(strlen(access) && sectors && strlen(type) && strlen(fname))) { >> + goto next_line; >> + } > > These can be replaced by checking the sscanf(3) return value above. > Validating sectors is still a good idea though. > >> + if (strcmp(type, "FLAT") && strcmp(type, "SPARSE")) { >> + goto next_line; >> + } >> + if (strcmp(access, "RW")) { >> + goto next_line; >> + } >> + ret++; > > This variable is unused. > >> + /* save to extents array */ >> + if (!strcmp(type, "FLAT")) { >> + /* FLAT extent */ >> + char extent_path[PATH_MAX]; >> + BlockDriverState *extent_file; >> + BlockDriver *drv; >> + VmdkExtent *extent; >> + >> + extent_file = bdrv_new(""); >> + drv = bdrv_find_format("file"); >> + if (!drv) { > > bdrv_delete(extent_file); > >> + return -EINVAL; >> + } >> + path_combine(extent_path, sizeof(extent_path), >> + desc_file_path, fname); >> + r = bdrv_open(extent_file, extent_path, >> + BDRV_O_RDWR | BDRV_O_NO_BACKING, drv); > > We should honor the bs->open_flags. Otherwise > cache=none|writeback|writethrough|unsafe is ignored on the actual > extent files. > >> + if (r) { > > bdrv_delete(extent_file); > >> + return -EINVAL; >> + } >> + extent = vmdk_add_extent(bs, extent_file, true, sectors, >> + 0, 0, 0, 0, sectors); >> + extent->flat_start_offset = flat_offset; >> + } else { >> + /* SPARSE extent, not supported for now */ >> + fprintf(stderr, >> + "VMDK: Not supported extent type \"%s\""".\n", type); >> + return -ENOTSUP; >> + } >> +next_line: >> + /* move to next line */ >> + while (*p && *p != '\n') { >> + p++; >> + } >> + p++; >> + } >> + return 0; >> +} >> + >> +static int vmdk_open_desc_file(BlockDriverState *bs, int flags) >> +{ >> + int ret; >> + char buf[2048]; >> + char ct[128]; >> + BDRVVmdkState *s = bs->opaque; >> + >> + ret = bdrv_pread(bs->file, 0, buf, sizeof(buf)); >> + if (ret < 0) { >> + return ret; >> + } >> + buf[2047] = '\0'; >> + if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) { >> + return -EINVAL; >> + } >> + if (strcmp(ct, "monolithicFlat")) { >> + fprintf(stderr, >> + "VMDK: Not supported image type \"%s\""".\n", ct); >> + return -ENOTSUP; >> + } >> + s->desc_offset = 0; >> + ret = vmdk_parse_extents(buf, bs, bs->file->filename); >> + if (ret) { >> + return ret; >> + } >> + >> + /* try to open parent images, if exist */ >> + if (vmdk_parent_open(bs)) { >> + qemu_free(s->extents); > > This duplicates extent freeing code from vmdk_close(). Please reuse > that (maybe by moving it into a vmdk_free_extents() function), it also > frees l1_table, l2_cache, and l1_backup_table. > > Stefan >
On Thu, Jun 30, 2011 at 2:57 AM, Fam Zheng <famcool@gmail.com> wrote: > On Wed, Jun 29, 2011 at 11:57 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: >> On Tue, Jun 28, 2011 at 2:32 AM, Fam Zheng <famcool@gmail.com> wrote: >>> + /* trim the quotation marks around */ >>> + if (fname[0] == '"') { >>> + memmove(fname, fname + 1, strlen(fname) + 1); >> >> This copies 1 byte too many, just strlen(fname) will do. > I meant to copy the NULL terminator too. Yes. The problem is the copying starts at fname + 1 but strlen(3) starts at fname. So there is already an extra byte. The + 1 adds an additional byte after the NUL. Stefan
diff --git a/block/vmdk.c b/block/vmdk.c index c84ea90..8083c31 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -65,6 +65,7 @@ typedef struct VmdkExtent { bool flat; int64_t sectors; int64_t end_sector; + int64_t flat_start_offset; int64_t l1_table_offset; int64_t l1_backup_table_offset; uint32_t *l1_table; @@ -390,9 +391,10 @@ fail: static int vmdk_parent_open(BlockDriverState *bs) { char *p_name; - char desc[DESC_SIZE]; + char desc[DESC_SIZE + 1]; BDRVVmdkState *s = bs->opaque; + desc[DESC_SIZE] = '\0'; if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) { return -1; } @@ -588,6 +590,157 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, int flags) return ret; } +/* find an option value out of descriptor file */ +static int vmdk_parse_description(const char *desc, const char *opt_name, + char *buf, int buf_size) +{ + char *opt_pos, *opt_end; + const char *end = desc + strlen(desc); + + opt_pos = strstr(desc, opt_name); + if (!opt_pos) { + return -1; + } + /* Skip "=\"" following opt_name */ + opt_pos += strlen(opt_name) + 2; + if (opt_pos >= end) { + return -1; + } + opt_end = opt_pos; + while (opt_end < end && *opt_end != '"') { + opt_end++; + } + if (opt_end == end || buf_size < opt_end - opt_pos + 1) { + return -1; + } + pstrcpy(buf, opt_end - opt_pos + 1, opt_pos); + return 0; +} + +static int vmdk_parse_extents(const char *desc, BlockDriverState *bs, + const char *desc_file_path) +{ + int ret = 0; + int r; + char access[11]; + char type[11]; + char fname[512]; + const char *p = desc; + int64_t sectors = 0; + int64_t flat_offset; + + while (*p) { + if (strncmp(p, "RW", strlen("RW"))) { + goto next_line; + } + /* parse extent line: + * RW [size in sectors] FLAT "file-name.vmdk" OFFSET + * or + * RW [size in sectors] SPARSE "file-name.vmdk" + */ + flat_offset = -1; + sscanf(p, "%10s %lld %10s %512s", + access, §ors, type, fname); + if (!strcmp(type, "FLAT")) { + sscanf(p, "%10s %lld %10s %512s %lld", + access, §ors, type, fname, &flat_offset); + if (flat_offset == -1) { + return -EINVAL; + } + } + + /* trim the quotation marks around */ + if (fname[0] == '"') { + memmove(fname, fname + 1, strlen(fname) + 1); + if (fname[strlen(fname) - 1] == '"') { + fname[strlen(fname) - 1] = '\0'; + } + } + if (!(strlen(access) && sectors && strlen(type) && strlen(fname))) { + goto next_line; + } + if (strcmp(type, "FLAT") && strcmp(type, "SPARSE")) { + goto next_line; + } + if (strcmp(access, "RW")) { + goto next_line; + } + ret++; + + /* save to extents array */ + if (!strcmp(type, "FLAT")) { + /* FLAT extent */ + char extent_path[PATH_MAX]; + BlockDriverState *extent_file; + BlockDriver *drv; + VmdkExtent *extent; + + extent_file = bdrv_new(""); + drv = bdrv_find_format("file"); + if (!drv) { + return -EINVAL; + } + path_combine(extent_path, sizeof(extent_path), + desc_file_path, fname); + r = bdrv_open(extent_file, extent_path, + BDRV_O_RDWR | BDRV_O_NO_BACKING, drv); + if (r) { + return -EINVAL; + } + extent = vmdk_add_extent(bs, extent_file, true, sectors, + 0, 0, 0, 0, sectors); + extent->flat_start_offset = flat_offset; + } else { + /* SPARSE extent, not supported for now */ + fprintf(stderr, + "VMDK: Not supported extent type \"%s\""".\n", type); + return -ENOTSUP; + } +next_line: + /* move to next line */ + while (*p && *p != '\n') { + p++; + } + p++; + } + return 0; +} + +static int vmdk_open_desc_file(BlockDriverState *bs, int flags) +{ + int ret; + char buf[2048]; + char ct[128]; + BDRVVmdkState *s = bs->opaque; + + ret = bdrv_pread(bs->file, 0, buf, sizeof(buf)); + if (ret < 0) { + return ret; + } + buf[2047] = '\0'; + if (vmdk_parse_description(buf, "createType", ct, sizeof(ct))) { + return -EINVAL; + } + if (strcmp(ct, "monolithicFlat")) { + fprintf(stderr, + "VMDK: Not supported image type \"%s\""".\n", ct); + return -ENOTSUP; + } + s->desc_offset = 0; + ret = vmdk_parse_extents(buf, bs, bs->file->filename); + if (ret) { + return ret; + } + + /* try to open parent images, if exist */ + if (vmdk_parent_open(bs)) { + qemu_free(s->extents); + return -EINVAL; + } + s->parent_cid = vmdk_read_cid(bs, 1); + return 0; +} + static int vmdk_open(BlockDriverState *bs, int flags) { uint32_t magic; @@ -602,7 +755,7 @@ static int vmdk_open(BlockDriverState *bs, int flags) } else if (magic == VMDK4_MAGIC) { return vmdk_open_vmdk4(bs, flags); } else { - return -EINVAL; + return vmdk_open_desc_file(bs, flags); } } @@ -683,7 +836,7 @@ static int get_cluster_offset(BlockDriverState *bs, if (m_data) m_data->valid = 0; if (extent->flat) { - *cluster_offset = 0; + *cluster_offset = extent->flat_start_offset; return 0; } @@ -836,16 +989,20 @@ static int vmdk_read(BlockDriverState *bs, int64_t sector_num, /* if not allocated, try to read from parent image, if exist */ if (bs->backing_hd) { if (!vmdk_is_cid_valid(bs)) - return -1; + return -EINVAL; ret = bdrv_read(bs->backing_hd, sector_num, buf, n); if (ret < 0) - return -1; + return ret; } else { memset(buf, 0, 512 * n); } } else { - if(bdrv_pread(bs->file, cluster_offset + index_in_cluster * 512, buf, n * 512) != n * 512) - return -1; + ret = bdrv_pread(extent->file, + cluster_offset + index_in_cluster * 512, + buf, n * 512); + if (ret != n * 512) { + return ret; + } } nb_sectors -= n; sector_num += n; @@ -869,7 +1026,7 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num, "(VMDK) Wrong offset: sector_num=0x%" PRIx64 " total_sectors=0x%" PRIx64 "\n", sector_num, bs->total_sectors); - return -1; + return -EIO; } while (nb_sectors > 0) { @@ -892,16 +1049,18 @@ static int vmdk_write(BlockDriverState *bs, int64_t sector_num, n = nb_sectors; } - if (bdrv_pwrite(bs->file, + ret = bdrv_pwrite(extent->file, cluster_offset + index_in_cluster * 512, - buf, n * 512) - != n * 512) { - return -1; + buf, + n * 512); + if (ret != n * 512) { + ret = ret < 0 ? ret : 0; + return ret; } if (m_data.valid) { /* update L2 tables */ if (vmdk_L2update(extent, &m_data) == -1) { - return -1; + return -EIO; } } nb_sectors -= n;
Parse vmdk decriptor file and open mono flat image. Read/write the flat extent. Signed-off-by: Fam Zheng <famcool@gmail.com> --- block/vmdk.c | 185 +++++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 files changed, 172 insertions(+), 13 deletions(-)