diff mbox

[v3,1/1] block/sheepdog: fix argument passed to qemu_strtoul()

Message ID e56fc50abedd9a112e0683342c8eafda063cd2f9.1456935548.git.jcody@redhat.com
State New
Headers show

Commit Message

Jeff Cody March 2, 2016, 4:24 p.m. UTC 
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(-)

Comments

Max Reitz March 2, 2016, 4:28 p.m. UTC | #1 
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);
>
Eric Blake March 2, 2016, 4:30 p.m. UTC | #2 
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.
Max Reitz March 2, 2016, 4:32 p.m. UTC | #3 
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
Jeff Cody March 2, 2016, 4:36 p.m. UTC | #4 
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).
Jeff Cody March 2, 2016, 4:36 p.m. UTC | #5 
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
>
Hitoshi Mitake March 3, 2016, 4:06 a.m. UTC | #6 
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
>
>
>
Hitoshi Mitake March 3, 2016, 4:07 a.m. UTC | #7 
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
Jeff Cody March 10, 2016, 2:35 a.m. UTC | #8 
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 mbox

Patch

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);