Message ID | 1319036398-14320-4-git-send-email-pbonzini@redhat.com |
---|---|
State | New |
Headers | show |
Am 19.10.2011 16:59, schrieb Paolo Bonzini: > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/vmdk.c | 37 +++++++++++++++---------------------- > 1 files changed, 15 insertions(+), 22 deletions(-) > > diff --git a/block/vmdk.c b/block/vmdk.c > index 21566eb..12b38d2 100644 > --- a/block/vmdk.c > +++ b/block/vmdk.c > @@ -622,20 +622,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, > return -ENOTSUP; > } > s->desc_offset = 0; > - ret = vmdk_parse_extents(buf, bs, bs->file->filename); > - if (ret) { > - vmdk_free_extents(bs); > - return ret; > - } > - > - /* try to open parent images, if exist */ > - ret = vmdk_parent_open(bs); > - if (ret) { > - vmdk_free_extents(bs); > - return ret; > - } > - s->parent_cid = vmdk_read_cid(bs, 1); > - return 0; > + return vmdk_parse_extents(buf, bs, bs->file->filename); > } This code is moved into bdrv_open, but there's another path how this code can be reached: vmdk_parse_extents() -> vmdk_open_sparse() -> vmdk_open_vmdk4() -> vmdk_open_desc_file(). Don't we forget to open the parent file there now? Kevin > > static int vmdk_open(BlockDriverState *bs, int flags) > @@ -645,17 +632,23 @@ static int vmdk_open(BlockDriverState *bs, int flags) > > if (vmdk_open_sparse(bs, bs->file, flags) == 0) { > s->desc_offset = 0x200; > - /* try to open parent images, if exist */ > - ret = vmdk_parent_open(bs); > + } else { > + ret = vmdk_open_desc_file(bs, flags, 0); > if (ret) { > - vmdk_free_extents(bs); > - return ret; > + goto fail; > } > - s->parent_cid = vmdk_read_cid(bs, 1); > - return 0; > - } else { > - return vmdk_open_desc_file(bs, flags, 0); > } > + /* try to open parent images, if exist */ > + ret = vmdk_parent_open(bs); > + if (ret) { > + goto fail; > + } > + s->parent_cid = vmdk_read_cid(bs, 1); > + return ret; > + > +fail: > + vmdk_free_extents(bs); > + return ret; > } > > static int get_whole_cluster(BlockDriverState *bs,
On 10/20/2011 11:28 AM, Kevin Wolf wrote: > This code is moved into bdrv_open, but there's another path how this > code can be reached: > > vmdk_parse_extents() -> vmdk_open_sparse() -> vmdk_open_vmdk4() -> > vmdk_open_desc_file(). > > Don't we forget to open the parent file there now? Let's look at the call chain: vmdk_open: return vmdk_open_desc_file(bs, flags, 0); -> vmdk_open_desc_file: ret = vmdk_parse_extents(buf, bs, bs->file->filename); -> vmdk_parse_extents: p points into vmdk_open_desc_file's buf: ret = sscanf(p, "%10s %" SCNd64 " %10s %511s %" SCNd64, access, §ors, type, fname, &flat_offset); path_combine(extent_path, sizeof(extent_path), desc_file_path, fname); ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags); ret = vmdk_open_sparse(bs, extent_file, bs->open_flags); So here it finds the sub-vmdk, opens it and passes it to vmdk_open_sparse. -> vmdk_open_sparse: return vmdk_open_vmdk4(bs, file, flags); The extent_file is then passed to vmdk_open_vmdk4: -> vmdk_open_vmdk4: ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header)); return vmdk_open_desc_file(bs, flags, header.desc_offset << 9); This reads the header from extent_file, but vmdk_open_desc_file will still read from bs->file. -> vmdk_open_desc_file: s->desc_offset = 0; if (vmdk_parent_open(bs)) { -> vmdk_parent_open: if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) { So s->desc_offset is always zero, and bs->file never changes. So the data that vmdk_parent_open reads comes always from the same place, and anyway there have only one place where it can write it: vmdk_parent_open: pstrcpy(bs->backing_file, end_name - p_name + 1, p_name); So the code is completely bogus in the case of recursive calls to vmdk_open_desc_file with parent images. Maybe it simply cannot happen, and in that case the patched code is less bogus. It is also possible that it can happen, and the bug is in setting s->desc_offset to zero. Even in that case, calling vmdk_parent_open once would be correct and the patched code is less bogus. Paolo
Am 20.10.2011 12:12, schrieb Paolo Bonzini: > On 10/20/2011 11:28 AM, Kevin Wolf wrote: >> This code is moved into bdrv_open, but there's another path how this >> code can be reached: >> >> vmdk_parse_extents() -> vmdk_open_sparse() -> vmdk_open_vmdk4() -> >> vmdk_open_desc_file(). >> >> Don't we forget to open the parent file there now? > > Let's look at the call chain: > > vmdk_open: > return vmdk_open_desc_file(bs, flags, 0); > > -> vmdk_open_desc_file: > ret = vmdk_parse_extents(buf, bs, bs->file->filename); > > -> vmdk_parse_extents: > > p points into vmdk_open_desc_file's buf: > > ret = sscanf(p, "%10s %" SCNd64 " %10s %511s %" SCNd64, > access, §ors, type, fname, &flat_offset); > path_combine(extent_path, sizeof(extent_path), > desc_file_path, fname); > ret = bdrv_file_open(&extent_file, extent_path, bs->open_flags); > ret = vmdk_open_sparse(bs, extent_file, bs->open_flags); > > So here it finds the sub-vmdk, opens it and passes it to vmdk_open_sparse. > > -> vmdk_open_sparse: > return vmdk_open_vmdk4(bs, file, flags); > > The extent_file is then passed to vmdk_open_vmdk4: > > -> vmdk_open_vmdk4: > ret = bdrv_pread(file, sizeof(magic), &header, sizeof(header)); > return vmdk_open_desc_file(bs, flags, header.desc_offset << 9); > > This reads the header from extent_file, but vmdk_open_desc_file will > still read from bs->file. > -> vmdk_open_desc_file: > s->desc_offset = 0; > if (vmdk_parent_open(bs)) { > -> vmdk_parent_open: > if (bdrv_pread(bs->file, s->desc_offset, desc, DESC_SIZE) != DESC_SIZE) { > > So s->desc_offset is always zero, and bs->file never changes. So the data > that vmdk_parent_open reads comes always from the same place, and anyway > there have only one place where it can write it: > > vmdk_parent_open: > pstrcpy(bs->backing_file, end_name - p_name + 1, p_name); > > So the code is completely bogus in the case of recursive calls to > vmdk_open_desc_file with parent images. Maybe it simply cannot happen, > and in that case the patched code is less bogus. > > It is also possible that it can happen, and the bug is in setting > s->desc_offset to zero. Even in that case, calling vmdk_parent_open > once would be correct and the patched code is less bogus. Ok, if it can't happen it seems reasonable and could still qualify as cleanup. Maybe you can add a line or two to the commit message for v2. Kevin
diff --git a/block/vmdk.c b/block/vmdk.c index 21566eb..12b38d2 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -622,20 +622,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, return -ENOTSUP; } s->desc_offset = 0; - ret = vmdk_parse_extents(buf, bs, bs->file->filename); - if (ret) { - vmdk_free_extents(bs); - return ret; - } - - /* try to open parent images, if exist */ - ret = vmdk_parent_open(bs); - if (ret) { - vmdk_free_extents(bs); - return ret; - } - s->parent_cid = vmdk_read_cid(bs, 1); - return 0; + return vmdk_parse_extents(buf, bs, bs->file->filename); } static int vmdk_open(BlockDriverState *bs, int flags) @@ -645,17 +632,23 @@ static int vmdk_open(BlockDriverState *bs, int flags) if (vmdk_open_sparse(bs, bs->file, flags) == 0) { s->desc_offset = 0x200; - /* try to open parent images, if exist */ - ret = vmdk_parent_open(bs); + } else { + ret = vmdk_open_desc_file(bs, flags, 0); if (ret) { - vmdk_free_extents(bs); - return ret; + goto fail; } - s->parent_cid = vmdk_read_cid(bs, 1); - return 0; - } else { - return vmdk_open_desc_file(bs, flags, 0); } + /* try to open parent images, if exist */ + ret = vmdk_parent_open(bs); + if (ret) { + goto fail; + } + s->parent_cid = vmdk_read_cid(bs, 1); + return ret; + +fail: + vmdk_free_extents(bs); + return ret; } static int get_whole_cluster(BlockDriverState *bs,
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block/vmdk.c | 37 +++++++++++++++---------------------- 1 files changed, 15 insertions(+), 22 deletions(-)