| Message ID | 1359025358-5725-4-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
Kevin Wolf <kwolf@redhat.com> writes: > Return -errno instead of -1 on errors. While touching the > code, fix a memory leak. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/vpc.c | 36 +++++++++++++++++++++++++----------- > 1 files changed, 25 insertions(+), 11 deletions(-) > > diff --git a/block/vpc.c b/block/vpc.c > index 7948609..9d2b177 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -163,24 +163,29 @@ static int vpc_open(BlockDriverState *bs, int flags) > struct vhd_dyndisk_header* dyndisk_header; > uint8_t buf[HEADER_SIZE]; > uint32_t checksum; > - int err = -1; > int disk_type = VHD_DYNAMIC; > + int ret; > > - if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE) > + ret = bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE); > + if (ret < 0 ) { > goto fail; > + } > > footer = (struct vhd_footer*) s->footer_buf; > if (strncmp(footer->creator, "conectix", 8)) { > int64_t offset = bdrv_getlength(bs->file); > if (offset < HEADER_SIZE) { > + ret = offset; What if 0 <= bdrv_getlength() < HEADER_SIZE? For what it's worth, in other places, we simply bdrv_read() without checking "got enough" first. As far as I can tell, bdrv_read() returns -EIO when it hits EOF prematurely. > goto fail; > } > /* If a fixed disk, the footer is found only at the end of the file */ > - if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE) > - != HEADER_SIZE) { > + ret = bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, > + HEADER_SIZE); > + if (ret < 0) { > goto fail; > } > if (strncmp(footer->creator, "conectix", 8)) { > + ret = -EMEDIUMTYPE; I figure this makes your series depends on Stefan Weil's "block: Fix error report for wrong file format". I'd probably order it the other way, and return -EINVAL here, then change it along with the others in "block: Use error code EMEDIUMTYPE for wrong format in some block drivers". Matter of taste. > goto fail; > } > disk_type = VHD_FIXED; } checksum = be32_to_cpu(footer->checksum); footer->checksum = 0; if (vpc_checksum(s->footer_buf, HEADER_SIZE) != checksum) fprintf(stderr, "block-vpc: The header checksum of '%s' is " "incorrect.\n", bs->filename); I wonder whether this should fail the open. Outside the scope of this patch. > @@ -203,19 +208,21 @@ static int vpc_open(BlockDriverState *bs, int flags) > > /* Allow a maximum disk size of approximately 2 TB */ > if (bs->total_sectors >= 65535LL * 255 * 255) { > - err = -EFBIG; > + ret = -EFBIG; > goto fail; > } > > if (disk_type == VHD_DYNAMIC) { > - if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, > - HEADER_SIZE) != HEADER_SIZE) { > + ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, > + HEADER_SIZE); > + if (ret < 0) { > goto fail; > } > > dyndisk_header = (struct vhd_dyndisk_header *) buf; > > if (strncmp(dyndisk_header->magic, "cxsparse", 8)) { > + ret = -EINVAL; If you keep -EMEDIUMTYPE above, should this be -EMEDIUMTYPE, too? > goto fail; > } > > @@ -226,8 +233,10 @@ static int vpc_open(BlockDriverState *bs, int flags) > s->pagetable = g_malloc(s->max_table_entries * 4); > > s->bat_offset = be64_to_cpu(dyndisk_header->table_offset); > - if (bdrv_pread(bs->file, s->bat_offset, s->pagetable, > - s->max_table_entries * 4) != s->max_table_entries * 4) { > + > + ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, > + s->max_table_entries * 4); > + if (ret < 0) { > goto fail; > } > > @@ -265,8 +274,13 @@ static int vpc_open(BlockDriverState *bs, int flags) > migrate_add_blocker(s->migration_blocker); > > return 0; > - fail: > - return err; > + > +fail: > + g_free(s->pagetable); > +#ifdef CACHE > + g_free(s->pageentry_u8); > +#endif > + return ret; > } > > static int vpc_reopen_prepare(BDRVReopenState *state,
Am 24.01.2013 14:09, schrieb Markus Armbruster: > Kevin Wolf <kwolf@redhat.com> writes: > >> Return -errno instead of -1 on errors. While touching the >> code, fix a memory leak. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> block/vpc.c | 36 +++++++++++++++++++++++++----------- >> 1 files changed, 25 insertions(+), 11 deletions(-) >> >> diff --git a/block/vpc.c b/block/vpc.c >> index 7948609..9d2b177 100644 >> --- a/block/vpc.c >> +++ b/block/vpc.c >> @@ -163,24 +163,29 @@ static int vpc_open(BlockDriverState *bs, int flags) >> struct vhd_dyndisk_header* dyndisk_header; >> uint8_t buf[HEADER_SIZE]; >> uint32_t checksum; >> - int err = -1; >> int disk_type = VHD_DYNAMIC; >> + int ret; >> >> - if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE) >> + ret = bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE); >> + if (ret < 0 ) { >> goto fail; >> + } >> >> footer = (struct vhd_footer*) s->footer_buf; >> if (strncmp(footer->creator, "conectix", 8)) { >> int64_t offset = bdrv_getlength(bs->file); >> if (offset < HEADER_SIZE) { >> + ret = offset; > > What if 0 <= bdrv_getlength() < HEADER_SIZE? Then offset - HEADER_SIZE becomes negative. Not sure what this means, I need to check. offset is signed and the offset parameter of bdrv_pread() is int64_t as well. In there, sector_num will become negative as well and is passed as int64_t to bdrv_read, bdrv_rw_co, bdrv_rw_co_entry, bdrv_co_do_readv, bdrv_check_request. It then is multiplied by BDRV_SECTOR_SIZE and passed to bdrv_check_byte_request. bs->file->growable = 1 because it is opened with bdrv_file_open(), therefore bdrv_check_byte_request doesn't complain. The negative sector number is then passed to the block driver, which can do with it whatever it likes. Just some examples: * raw-posix with aio=threads will store it in RawPosixAIOData.aio_offset and later pass it to preadv/pread. This should return -EINVAL. * raw-win32 stores it in Offset and OffsetHigh in an OVERLAPPED struct, which both seem to be a DWORD. If I should guess, that's a uint32_t, so we get a position far after EOF. No idea what happens, maybe we're lucky and ReadFile() errors out. * nbd places it in struct nbd_request.from, which is uint64_t. Pretty large request, but hopefully the server will do something reasonable with it. So I wouldn't bet that bdrv_pread() handles negative offset correctly (let alone consistently) in all cases. We should probably fix this, but certainly not in this patch. I'm strongly for leaving the check in for now. > For what it's worth, in other places, we simply bdrv_read() without > checking "got enough" first. As far as I can tell, bdrv_read() returns > -EIO when it hits EOF prematurely. You're thinking of a different case here. But now that you brought it up, let me mention that bs->growable means that at least raw-posix reads zeros after EOF instead of failing. > checksum = be32_to_cpu(footer->checksum); > footer->checksum = 0; > if (vpc_checksum(s->footer_buf, HEADER_SIZE) != checksum) > fprintf(stderr, "block-vpc: The header checksum of '%s' is " > "incorrect.\n", bs->filename); > > I wonder whether this should fail the open. Outside the scope of this > patch. I think there was a reason why not, but I can't remember the details. >> @@ -203,19 +208,21 @@ static int vpc_open(BlockDriverState *bs, int flags) >> >> /* Allow a maximum disk size of approximately 2 TB */ >> if (bs->total_sectors >= 65535LL * 255 * 255) { >> - err = -EFBIG; >> + ret = -EFBIG; >> goto fail; >> } >> >> if (disk_type == VHD_DYNAMIC) { >> - if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, >> - HEADER_SIZE) != HEADER_SIZE) { >> + ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, >> + HEADER_SIZE); >> + if (ret < 0) { >> goto fail; >> } >> >> dyndisk_header = (struct vhd_dyndisk_header *) buf; >> >> if (strncmp(dyndisk_header->magic, "cxsparse", 8)) { >> + ret = -EINVAL; > > If you keep -EMEDIUMTYPE above, should this be -EMEDIUMTYPE, too? Good question, I wondered the same when writing the patch. I decided to stay consistent with vpc_probe() which detects an image as VHD image if the first magic is right. Then this case means that it's still a VHD image, but an invalid one. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 24.01.2013 14:09, schrieb Markus Armbruster: >> Kevin Wolf <kwolf@redhat.com> writes: >> >>> Return -errno instead of -1 on errors. While touching the >>> code, fix a memory leak. >>> >>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>> --- >>> block/vpc.c | 36 +++++++++++++++++++++++++----------- >>> 1 files changed, 25 insertions(+), 11 deletions(-) >>> >>> diff --git a/block/vpc.c b/block/vpc.c >>> index 7948609..9d2b177 100644 >>> --- a/block/vpc.c >>> +++ b/block/vpc.c >>> @@ -163,24 +163,29 @@ static int vpc_open(BlockDriverState *bs, int flags) >>> struct vhd_dyndisk_header* dyndisk_header; >>> uint8_t buf[HEADER_SIZE]; >>> uint32_t checksum; >>> - int err = -1; >>> int disk_type = VHD_DYNAMIC; >>> + int ret; >>> >>> - if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE) >>> + ret = bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE); >>> + if (ret < 0 ) { >>> goto fail; >>> + } >>> >>> footer = (struct vhd_footer*) s->footer_buf; >>> if (strncmp(footer->creator, "conectix", 8)) { >>> int64_t offset = bdrv_getlength(bs->file); >>> if (offset < HEADER_SIZE) { >>> + ret = offset; goto fail; } /* If a fixed disk, the footer is found only at the end of the file */ - if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE) - != HEADER_SIZE) { + ret = bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, + HEADER_SIZE); + if (ret < 0) { goto fail; } >> >> What if 0 <= bdrv_getlength() < HEADER_SIZE? > > Then offset - HEADER_SIZE becomes negative. Not sure what this means, I > need to check. > > offset is signed and the offset parameter of bdrv_pread() is int64_t as > well. In there, sector_num will become negative as well and is passed as > int64_t to bdrv_read, bdrv_rw_co, bdrv_rw_co_entry, bdrv_co_do_readv, > bdrv_check_request. It then is multiplied by BDRV_SECTOR_SIZE and passed > to bdrv_check_byte_request. > > bs->file->growable = 1 because it is opened with bdrv_file_open(), > therefore bdrv_check_byte_request doesn't complain. > > The negative sector number is then passed to the block driver, which can > do with it whatever it likes. Just some examples: > > * raw-posix with aio=threads will store it in RawPosixAIOData.aio_offset > and later pass it to preadv/pread. This should return -EINVAL. > > * raw-win32 stores it in Offset and OffsetHigh in an OVERLAPPED struct, > which both seem to be a DWORD. If I should guess, that's a uint32_t, > so we get a position far after EOF. No idea what happens, maybe we're > lucky and ReadFile() errors out. > > * nbd places it in struct nbd_request.from, which is uint64_t. Pretty > large request, but hopefully the server will do something reasonable > with it. > > So I wouldn't bet that bdrv_pread() handles negative offset correctly > (let alone consistently) in all cases. We should probably fix this, but > certainly not in this patch. > > I'm strongly for leaving the check in for now. Sounds like you're thinking about what happens in the bdrv_pread() a few lines down. Not reached, because we goto fail, and return a non-negative number. That's what worries me. >> For what it's worth, in other places, we simply bdrv_read() without >> checking "got enough" first. As far as I can tell, bdrv_read() returns >> -EIO when it hits EOF prematurely. > > You're thinking of a different case here. But now that you brought it > up, let me mention that bs->growable means that at least raw-posix reads > zeros after EOF instead of failing. Aha. Didn't realize that growable applies to reads as well. [...]
Am 25.01.2013 14:37, schrieb Markus Armbruster: > Kevin Wolf <kwolf@redhat.com> writes: > >> Am 24.01.2013 14:09, schrieb Markus Armbruster: >>> Kevin Wolf <kwolf@redhat.com> writes: >>> >>>> Return -errno instead of -1 on errors. While touching the >>>> code, fix a memory leak. >>>> >>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>>> --- >>>> block/vpc.c | 36 +++++++++++++++++++++++++----------- >>>> 1 files changed, 25 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/block/vpc.c b/block/vpc.c >>>> index 7948609..9d2b177 100644 >>>> --- a/block/vpc.c >>>> +++ b/block/vpc.c >>>> @@ -163,24 +163,29 @@ static int vpc_open(BlockDriverState *bs, int flags) >>>> struct vhd_dyndisk_header* dyndisk_header; >>>> uint8_t buf[HEADER_SIZE]; >>>> uint32_t checksum; >>>> - int err = -1; >>>> int disk_type = VHD_DYNAMIC; >>>> + int ret; >>>> >>>> - if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE) >>>> + ret = bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE); >>>> + if (ret < 0 ) { >>>> goto fail; >>>> + } >>>> >>>> footer = (struct vhd_footer*) s->footer_buf; >>>> if (strncmp(footer->creator, "conectix", 8)) { >>>> int64_t offset = bdrv_getlength(bs->file); >>>> if (offset < HEADER_SIZE) { >>>> + ret = offset; > goto fail; > } > /* If a fixed disk, the footer is found only at the end of the file */ > - if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE) > - != HEADER_SIZE) { > + ret = bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, > + HEADER_SIZE); > + if (ret < 0) { > goto fail; > } >>> >>> What if 0 <= bdrv_getlength() < HEADER_SIZE? >> >> Then offset - HEADER_SIZE becomes negative. Not sure what this means, I >> need to check [...] >> I'm strongly for leaving the check in for now. > > Sounds like you're thinking about what happens in the bdrv_pread() a few > lines down. Not reached, because we goto fail, and return a > non-negative number. That's what worries me. Oh, I completely missed that. I thought you were asking what happened if we dropped the check like you suggested. You're right, this needs separate checks for < 0 and < HEADER_SIZE, the latter probably returning -EINVAL. Kevin
diff --git a/block/vpc.c b/block/vpc.c index 7948609..9d2b177 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -163,24 +163,29 @@ static int vpc_open(BlockDriverState *bs, int flags) struct vhd_dyndisk_header* dyndisk_header; uint8_t buf[HEADER_SIZE]; uint32_t checksum; - int err = -1; int disk_type = VHD_DYNAMIC; + int ret; - if (bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE) != HEADER_SIZE) + ret = bdrv_pread(bs->file, 0, s->footer_buf, HEADER_SIZE); + if (ret < 0 ) { goto fail; + } footer = (struct vhd_footer*) s->footer_buf; if (strncmp(footer->creator, "conectix", 8)) { int64_t offset = bdrv_getlength(bs->file); if (offset < HEADER_SIZE) { + ret = offset; goto fail; } /* If a fixed disk, the footer is found only at the end of the file */ - if (bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, HEADER_SIZE) - != HEADER_SIZE) { + ret = bdrv_pread(bs->file, offset-HEADER_SIZE, s->footer_buf, + HEADER_SIZE); + if (ret < 0) { goto fail; } if (strncmp(footer->creator, "conectix", 8)) { + ret = -EMEDIUMTYPE; goto fail; } disk_type = VHD_FIXED; @@ -203,19 +208,21 @@ static int vpc_open(BlockDriverState *bs, int flags) /* Allow a maximum disk size of approximately 2 TB */ if (bs->total_sectors >= 65535LL * 255 * 255) { - err = -EFBIG; + ret = -EFBIG; goto fail; } if (disk_type == VHD_DYNAMIC) { - if (bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, - HEADER_SIZE) != HEADER_SIZE) { + ret = bdrv_pread(bs->file, be64_to_cpu(footer->data_offset), buf, + HEADER_SIZE); + if (ret < 0) { goto fail; } dyndisk_header = (struct vhd_dyndisk_header *) buf; if (strncmp(dyndisk_header->magic, "cxsparse", 8)) { + ret = -EINVAL; goto fail; } @@ -226,8 +233,10 @@ static int vpc_open(BlockDriverState *bs, int flags) s->pagetable = g_malloc(s->max_table_entries * 4); s->bat_offset = be64_to_cpu(dyndisk_header->table_offset); - if (bdrv_pread(bs->file, s->bat_offset, s->pagetable, - s->max_table_entries * 4) != s->max_table_entries * 4) { + + ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, + s->max_table_entries * 4); + if (ret < 0) { goto fail; } @@ -265,8 +274,13 @@ static int vpc_open(BlockDriverState *bs, int flags) migrate_add_blocker(s->migration_blocker); return 0; - fail: - return err; + +fail: + g_free(s->pagetable); +#ifdef CACHE + g_free(s->pageentry_u8); +#endif + return ret; } static int vpc_reopen_prepare(BDRVReopenState *state,
Return -errno instead of -1 on errors. While touching the code, fix a memory leak. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/vpc.c | 36 +++++++++++++++++++++++++----------- 1 files changed, 25 insertions(+), 11 deletions(-)