| Message ID | e56fc50abedd9a112e0683342c8eafda063cd2f9.1456935548.git.jcody@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02.03.2016 17:24, Jeff Cody wrote: > The function qemu_strtoul() reads 'unsigned long' sized data, > which is larger than uint32_t on 64-bit machines. > > Even though the snap_id field in the header is 32-bits, we must > accomodate the full size in qemu_strtoul(). > > This patch also adds more meaningful error handling to the > qemu_strtoul() call, and subsequent results. > > Reported-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/sheepdog.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/block/sheepdog.c b/block/sheepdog.c > index 8739acc..87f0027 100644 > --- a/block/sheepdog.c > +++ b/block/sheepdog.c > @@ -2543,7 +2543,7 @@ static int sd_snapshot_delete(BlockDriverState *bs, > const char *name, > Error **errp) > { > - uint32_t snap_id = 0; > + unsigned long snap_id = 0; > char snap_tag[SD_MAX_VDI_TAG_LEN]; > Error *local_err = NULL; > int fd, ret; > @@ -2565,12 +2565,15 @@ static int sd_snapshot_delete(BlockDriverState *bs, > memset(buf, 0, sizeof(buf)); > memset(snap_tag, 0, sizeof(snap_tag)); > pstrcpy(buf, SD_MAX_VDI_LEN, s->name); > - if (qemu_strtoul(snapshot_id, NULL, 10, (unsigned long *)&snap_id)) { > - return -1; > + ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id); > + if (ret || snap_id > UINT32_MAX) { > + error_setg(errp, "Invalid snapshot ID: %s", > + snapshot_id ? snapshot_id : "<null>"); > + return -EINVAL; > } > > if (snap_id) { > - hdr.snapid = snap_id; > + hdr.snapid = (uint32_t) snap_id; BTW, not so sure why you are doing an explicit cast to uint32_t here but not in the call to find_vdi_name() below. But I'll spare you a v4 :-) Reviewed-by: Max Reitz <mreitz@redhat.com> > } else { > pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id); > pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag); >
On 03/02/2016 09:24 AM, Jeff Cody wrote: > The function qemu_strtoul() reads 'unsigned long' sized data, > which is larger than uint32_t on 64-bit machines. > > Even though the snap_id field in the header is 32-bits, we must > accomodate the full size in qemu_strtoul(). s/accomodate/accommodate/ Someday, it might be nice to write a qemu_strtoi() and qemu_strtoui() that provide a guaranteed 32-bit result, rather than a platform-dependent width (we already have qemu_strto[u]ll() for 64-bit results). It's a bit tricky, since C doesn't provide a native parsing to int, and the case for overflow detection is different on 32-bit machines than on 64-bit machines (libvirt took a couple of tries before getting something that worked correctly), but would probably be worth it in the long run. But that idea doesn't have to hold up this patch.
On 02.03.2016 17:24, Jeff Cody wrote: > The function qemu_strtoul() reads 'unsigned long' sized data, > which is larger than uint32_t on 64-bit machines. > > Even though the snap_id field in the header is 32-bits, we must > accomodate the full size in qemu_strtoul(). > > This patch also adds more meaningful error handling to the > qemu_strtoul() call, and subsequent results. > > Reported-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/sheepdog.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) Another problem with this function is that it doesn't always set errp on error. Actually, this patch introduces the first instance where it does. qemu-img will not print an error if errp is not set; it actually ignores bdrv_snapshot_delete_by_id_or_name()'s return value. So this is a real issue that should be fixed as well. Max
On Wed, Mar 02, 2016 at 05:32:11PM +0100, Max Reitz wrote: > On 02.03.2016 17:24, Jeff Cody wrote: > > The function qemu_strtoul() reads 'unsigned long' sized data, > > which is larger than uint32_t on 64-bit machines. > > > > Even though the snap_id field in the header is 32-bits, we must > > accomodate the full size in qemu_strtoul(). > > > > This patch also adds more meaningful error handling to the > > qemu_strtoul() call, and subsequent results. > > > > Reported-by: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > --- > > block/sheepdog.c | 11 +++++++---- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > Another problem with this function is that it doesn't always set errp on > error. Actually, this patch introduces the first instance where it does. > > qemu-img will not print an error if errp is not set; it actually ignores > bdrv_snapshot_delete_by_id_or_name()'s return value. So this is a real > issue that should be fixed as well. > I'll (or perhaps one of the sheepdog maintainers?) will handle that in subsequent patch(es).
On Wed, Mar 02, 2016 at 09:30:45AM -0700, Eric Blake wrote: > On 03/02/2016 09:24 AM, Jeff Cody wrote: > > The function qemu_strtoul() reads 'unsigned long' sized data, > > which is larger than uint32_t on 64-bit machines. > > > > Even though the snap_id field in the header is 32-bits, we must > > accomodate the full size in qemu_strtoul(). > > s/accomodate/accommodate/ Thanks, I'll fix that up when applying this patch to my block branch. > > Someday, it might be nice to write a qemu_strtoi() and qemu_strtoui() > that provide a guaranteed 32-bit result, rather than a > platform-dependent width (we already have qemu_strto[u]ll() for 64-bit > results). It's a bit tricky, since C doesn't provide a native parsing > to int, and the case for overflow detection is different on 32-bit > machines than on 64-bit machines (libvirt took a couple of tries before > getting something that worked correctly), but would probably be worth it > in the long run. > > But that idea doesn't have to hold up this patch. > > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
On Thu, Mar 3, 2016 at 1:24 AM, Jeff Cody <jcody@redhat.com> wrote: > The function qemu_strtoul() reads 'unsigned long' sized data, > which is larger than uint32_t on 64-bit machines. > > Even though the snap_id field in the header is 32-bits, we must > accomodate the full size in qemu_strtoul(). > > This patch also adds more meaningful error handling to the > qemu_strtoul() call, and subsequent results. > > Reported-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/sheepdog.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > Thanks a lot for your fix! Reviewed-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> Thanks, Hitoshi > > diff --git a/block/sheepdog.c b/block/sheepdog.c > index 8739acc..87f0027 100644 > --- a/block/sheepdog.c > +++ b/block/sheepdog.c > @@ -2543,7 +2543,7 @@ static int sd_snapshot_delete(BlockDriverState *bs, > const char *name, > Error **errp) > { > - uint32_t snap_id = 0; > + unsigned long snap_id = 0; > char snap_tag[SD_MAX_VDI_TAG_LEN]; > Error *local_err = NULL; > int fd, ret; > @@ -2565,12 +2565,15 @@ static int sd_snapshot_delete(BlockDriverState *bs, > memset(buf, 0, sizeof(buf)); > memset(snap_tag, 0, sizeof(snap_tag)); > pstrcpy(buf, SD_MAX_VDI_LEN, s->name); > - if (qemu_strtoul(snapshot_id, NULL, 10, (unsigned long *)&snap_id)) { > - return -1; > + ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id); > + if (ret || snap_id > UINT32_MAX) { > + error_setg(errp, "Invalid snapshot ID: %s", > + snapshot_id ? snapshot_id : "<null>"); > + return -EINVAL; > } > > if (snap_id) { > - hdr.snapid = snap_id; > + hdr.snapid = (uint32_t) snap_id; > } else { > pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id); > pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag); > -- > 1.9.3 > > >
On Thu, Mar 3, 2016 at 1:36 AM, Jeff Cody <jcody@redhat.com> wrote: > On Wed, Mar 02, 2016 at 05:32:11PM +0100, Max Reitz wrote: > > On 02.03.2016 17:24, Jeff Cody wrote: > > > The function qemu_strtoul() reads 'unsigned long' sized data, > > > which is larger than uint32_t on 64-bit machines. > > > > > > Even though the snap_id field in the header is 32-bits, we must > > > accomodate the full size in qemu_strtoul(). > > > > > > This patch also adds more meaningful error handling to the > > > qemu_strtoul() call, and subsequent results. > > > > > > Reported-by: Paolo Bonzini <pbonzini@redhat.com> > > > Signed-off-by: Jeff Cody <jcody@redhat.com> > > > --- > > > block/sheepdog.c | 11 +++++++---- > > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > Another problem with this function is that it doesn't always set errp on > > error. Actually, this patch introduces the first instance where it does. > > > > qemu-img will not print an error if errp is not set; it actually ignores > > bdrv_snapshot_delete_by_id_or_name()'s return value. So this is a real > > issue that should be fixed as well. > > > > I'll (or perhaps one of the sheepdog maintainers?) will handle that in > subsequent patch(es). > Thanks for your pointing. I didn't notice the problem when I posted the patch. I'll fix it later. Thanks, Hitoshi
On Wed, Mar 02, 2016 at 11:24:42AM -0500, Jeff Cody wrote: > The function qemu_strtoul() reads 'unsigned long' sized data, > which is larger than uint32_t on 64-bit machines. > > Even though the snap_id field in the header is 32-bits, we must > accomodate the full size in qemu_strtoul(). > > This patch also adds more meaningful error handling to the > qemu_strtoul() call, and subsequent results. > > Reported-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/sheepdog.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/block/sheepdog.c b/block/sheepdog.c > index 8739acc..87f0027 100644 > --- a/block/sheepdog.c > +++ b/block/sheepdog.c > @@ -2543,7 +2543,7 @@ static int sd_snapshot_delete(BlockDriverState *bs, > const char *name, > Error **errp) > { > - uint32_t snap_id = 0; > + unsigned long snap_id = 0; > char snap_tag[SD_MAX_VDI_TAG_LEN]; > Error *local_err = NULL; > int fd, ret; > @@ -2565,12 +2565,15 @@ static int sd_snapshot_delete(BlockDriverState *bs, > memset(buf, 0, sizeof(buf)); > memset(snap_tag, 0, sizeof(snap_tag)); > pstrcpy(buf, SD_MAX_VDI_LEN, s->name); > - if (qemu_strtoul(snapshot_id, NULL, 10, (unsigned long *)&snap_id)) { > - return -1; > + ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id); > + if (ret || snap_id > UINT32_MAX) { > + error_setg(errp, "Invalid snapshot ID: %s", > + snapshot_id ? snapshot_id : "<null>"); > + return -EINVAL; > } > > if (snap_id) { > - hdr.snapid = snap_id; > + hdr.snapid = (uint32_t) snap_id; > } else { > pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id); > pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag); > -- > 1.9.3 > Thanks, Applied to my block branch: git://github.com/codyprime/qemu-kvm-jtc.git block -Jeff
diff --git a/block/sheepdog.c b/block/sheepdog.c index 8739acc..87f0027 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -2543,7 +2543,7 @@ static int sd_snapshot_delete(BlockDriverState *bs, const char *name, Error **errp) { - uint32_t snap_id = 0; + unsigned long snap_id = 0; char snap_tag[SD_MAX_VDI_TAG_LEN]; Error *local_err = NULL; int fd, ret; @@ -2565,12 +2565,15 @@ static int sd_snapshot_delete(BlockDriverState *bs, memset(buf, 0, sizeof(buf)); memset(snap_tag, 0, sizeof(snap_tag)); pstrcpy(buf, SD_MAX_VDI_LEN, s->name); - if (qemu_strtoul(snapshot_id, NULL, 10, (unsigned long *)&snap_id)) { - return -1; + ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id); + if (ret || snap_id > UINT32_MAX) { + error_setg(errp, "Invalid snapshot ID: %s", + snapshot_id ? snapshot_id : "<null>"); + return -EINVAL; } if (snap_id) { - hdr.snapid = snap_id; + hdr.snapid = (uint32_t) snap_id; } else { pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id); pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag);
The function qemu_strtoul() reads 'unsigned long' sized data, which is larger than uint32_t on 64-bit machines. Even though the snap_id field in the header is 32-bits, we must accomodate the full size in qemu_strtoul(). This patch also adds more meaningful error handling to the qemu_strtoul() call, and subsequent results. Reported-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Jeff Cody <jcody@redhat.com> --- block/sheepdog.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)