| Message ID | 20140401214954.GA2902@smaugslair |
|---|---|
| State | New |
| Headers | show |
On 04/01/2014 03:49 PM, Shwetha Mathangi Chandra Choodamani wrote: > This patch fixes the bug in qemu-img info that wouldn't populate the extent type for default formats. > The extent type has now been set where necessary. This is the second version in the series after inputs > from Fam Zheng(famz@redhat.com). > > Signed-off-by: Shwetha Mathangi Chandra Choodamani <saphira.brightscales@gmail.com> > --- > block/vmdk.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 49 insertions(+), 4 deletions(-) > > + bdrv_pread(file, sizeof(magic), buf, size); > + sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64, > + access, §ors, type, fname, &flat_offset); Odd indentation. Furthermore, sscanf() has undefined behavior if the input values overflow the width of the integer variable you are parsing into. While you are not the first client of sscanf in the code base, I strongly recommend that you use something that isn't vulnerable to undefined behavior on untrusted external input, or at the VERY least, check the sscanf() return value as well as put a %n parameter at the end of your format string to ensure that you parsed what you thought you did.
On Tue, 04/01 17:49, Shwetha Mathangi Chandra Choodamani wrote: > This patch fixes the bug in qemu-img info that wouldn't populate the extent type for default formats. > The extent type has now been set where necessary. This is the second version in the series after inputs > from Fam Zheng(famz@redhat.com). > > Signed-off-by: Shwetha Mathangi Chandra Choodamani <saphira.brightscales@gmail.com> > --- > block/vmdk.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 49 insertions(+), 4 deletions(-) > > diff --git a/block/vmdk.c b/block/vmdk.c > index b69988d..d4a37ad 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -499,6 +499,18 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs, > VMDK3Header header; > VmdkExtent *extent; > IIRC VMFS always comes with a description file, so we won't miss the extent type and this part is not necessary. > + char access[11]; > + char type[11]; > + char fname[512]; > + int64_t sectors = 0; > + int64_t flat_offset; > + int64_t size; > + size = bdrv_getlength(file); > + char *buf; > + buf = g_malloc0(size + 1); > + bdrv_pread(file, sizeof(magic), buf, size); > + sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64, > + access, §ors, type, fname, &flat_offset); > ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header)); > if (ret < 0) { > error_setg_errno(errp, -ret, > @@ -515,6 +527,18 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs, > le32_to_cpu(header.granularity), > &extent, > errp); > + while (strcmp(access, "RW")) { > + while (*buf) { > + sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64, > + access, §ors, type, fname, &flat_offset); > + if (*buf == '\n') { > + buf++; > + break; > + } > + buf++; > + } > + } > + extent->type = g_strdup(type); > if (ret < 0) { > return ret; > } > @@ -523,6 +547,7 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs, > /* free extent allocated by vmdk_add_extent */ > vmdk_free_last_extent(bs); > } > + g_free(buf); > return ret; > } > > @@ -566,7 +591,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, > VmdkExtent *extent; > BDRVVmdkState *s = bs->opaque; > int64_t l1_backup_offset = 0; > - > + char access[11]; > + char type[11]; > + char fname[512]; > + int64_t sectors = 0; > + int64_t flat_offset; > ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header)); > if (ret < 0) { > error_setg_errno(errp, -ret, > @@ -586,11 +615,9 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, > return ret; > } > } > - Unnecessary blank line change. > if (!s->create_type) { > s->create_type = g_strdup("monolithicSparse"); > } > - Same here. > if (le64_to_cpu(header.gd_offset) == VMDK4_GD_AT_END) { > /* > * The footer takes precedence over the header, so read it in. The > @@ -694,6 +721,25 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, > g_free(s->create_type); > s->create_type = g_strdup("streamOptimized"); > } > + > + if (!extent->type) { > + uint64_t desc_offset = le64_to_cpu(header.desc_offset); > + char *buf = vmdk_read_desc(file, desc_offset<<9, errp); > + sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64, > + access, §ors, type, fname, &flat_offset); > + while (strcmp(access, "RW")) { > + while (*buf) { > + sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64, > + access, §ors, type, fname, &flat_offset); > + if (*buf == '\n') { > + buf++; > + break; > + } > + buf++; > + } > + } > + } No. Why do you need to read the description? Here we already know the extent type by looking at s->create_type, it would be "monolithicSparse" or "streamOptimized" in the cases that we are trying to fix in this patch, so no need to parse the description text. Just set extent->type according to s->create_type. > + extent->type = g_strdup(type); > extent->has_marker = le32_to_cpu(header.flags) & VMDK4_FLAG_MARKER; > extent->version = le32_to_cpu(header.version); > extent->has_zero_grain = le32_to_cpu(header.flags) & VMDK4_FLAG_ZERO_GRAIN; > @@ -711,7 +757,6 @@ static int vmdk_parse_description(const char *desc, const char *opt_name, > { > char *opt_pos, *opt_end; > const char *end = desc + strlen(desc); > - Blank line change, please drop it. Fam > opt_pos = strstr(desc, opt_name); > if (!opt_pos) { > return VMDK_ERROR; > -- > 1.7.9.5 > >
diff --git a/block/vmdk.c b/block/vmdk.c index b69988d..d4a37ad 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -499,6 +499,18 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs, VMDK3Header header; VmdkExtent *extent; + char access[11]; + char type[11]; + char fname[512]; + int64_t sectors = 0; + int64_t flat_offset; + int64_t size; + size = bdrv_getlength(file); + char *buf; + buf = g_malloc0(size + 1); + bdrv_pread(file, sizeof(magic), buf, size); + sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64, + access, §ors, type, fname, &flat_offset); ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header)); if (ret < 0) { error_setg_errno(errp, -ret, @@ -515,6 +527,18 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs, le32_to_cpu(header.granularity), &extent, errp); + while (strcmp(access, "RW")) { + while (*buf) { + sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64, + access, §ors, type, fname, &flat_offset); + if (*buf == '\n') { + buf++; + break; + } + buf++; + } + } + extent->type = g_strdup(type); if (ret < 0) { return ret; } @@ -523,6 +547,7 @@ static int vmdk_open_vmfs_sparse(BlockDriverState *bs, /* free extent allocated by vmdk_add_extent */ vmdk_free_last_extent(bs); } + g_free(buf); return ret; } @@ -566,7 +591,11 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, VmdkExtent *extent; BDRVVmdkState *s = bs->opaque; int64_t l1_backup_offset = 0; - + char access[11]; + char type[11]; + char fname[512]; + int64_t sectors = 0; + int64_t flat_offset; ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header)); if (ret < 0) { error_setg_errno(errp, -ret, @@ -586,11 +615,9 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, return ret; } } - if (!s->create_type) { s->create_type = g_strdup("monolithicSparse"); } - if (le64_to_cpu(header.gd_offset) == VMDK4_GD_AT_END) { /* * The footer takes precedence over the header, so read it in. The @@ -694,6 +721,25 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, g_free(s->create_type); s->create_type = g_strdup("streamOptimized"); } + + if (!extent->type) { + uint64_t desc_offset = le64_to_cpu(header.desc_offset); + char *buf = vmdk_read_desc(file, desc_offset<<9, errp); + sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64, + access, §ors, type, fname, &flat_offset); + while (strcmp(access, "RW")) { + while (*buf) { + sscanf(buf, "%10s %" SCNd64 " %10s \"%511[^\n\r\"]\" %" SCNd64, + access, §ors, type, fname, &flat_offset); + if (*buf == '\n') { + buf++; + break; + } + buf++; + } + } + } + extent->type = g_strdup(type); extent->has_marker = le32_to_cpu(header.flags) & VMDK4_FLAG_MARKER; extent->version = le32_to_cpu(header.version); extent->has_zero_grain = le32_to_cpu(header.flags) & VMDK4_FLAG_ZERO_GRAIN; @@ -711,7 +757,6 @@ static int vmdk_parse_description(const char *desc, const char *opt_name, { char *opt_pos, *opt_end; const char *end = desc + strlen(desc); - opt_pos = strstr(desc, opt_name); if (!opt_pos) { return VMDK_ERROR;
This patch fixes the bug in qemu-img info that wouldn't populate the extent type for default formats. The extent type has now been set where necessary. This is the second version in the series after inputs from Fam Zheng(famz@redhat.com). Signed-off-by: Shwetha Mathangi Chandra Choodamani <saphira.brightscales@gmail.com> --- block/vmdk.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 4 deletions(-)