[Qemu-devel] [PATCH v2 1/1] block/sheepdog: fix argument passed to qemu_strtoul()

[Qemu-devel] [PATCH v2 1/1] block/sheepdog: fix argument passed to qemu_strtoul()

[Jeff Cody]

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 | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 8739acc..418f6ba 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,22 @@ 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) {
+        error_setg_errno(errp, -ret, "Invalid snapshot ID: %s",
+                         snapshot_id ? snapshot_id : "<null>");
+        return ret;
+    }
+
+    if (snap_id > UINT32_MAX) {
+        error_setg_errno(errp, EINVAL, "Snapshot ID numeric value %" PRId64
+                         " exceeds Sheepdog maximum of %" PRId32, snap_id,
+                          UINT32_MAX);
+        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

Re: [Qemu-devel] [PATCH v2 1/1] block/sheepdog: fix argument passed to qemu_strtoul()

[Jeff Cody]

On Wed, Mar 02, 2016 at 11:09:29AM -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 | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 

Ideally, one of the other Sheepdog maintainers (Hitoshi Mitake or Liu
Yuan) could run a test against this patch, as I don't have an easy way
of testing it.

Thanks,
Jeff

Re: [Qemu-devel] [PATCH v2 1/1] block/sheepdog: fix argument passed to qemu_strtoul()

[Paolo Bonzini]

On 02/03/2016 17:09, Jeff Cody wrote:
> +    ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id);
> +    if (ret) {
> +        error_setg_errno(errp, -ret, "Invalid snapshot ID: %s",
> +                         snapshot_id ? snapshot_id : "<null>");
> +        return ret;
> +    }
> +
> +    if (snap_id > UINT32_MAX) {
> +        error_setg_errno(errp, EINVAL, "Snapshot ID numeric value %" PRId64
> +                         " exceeds Sheepdog maximum of %" PRId32, snap_id,
> +                          UINT32_MAX);
> +        return -EINVAL;
>      }

I think including the errno produces a worse error message ("Invalid
snapshot ID: foo: Invalid argument" or something like that), and also
the error should be the same for an id of 10^10 (within uint64_t bounds)
or 10^30 (outside the bounds).  So you could just use

    if (ret < 0 || snap_id > UINT32_MAX) {
        error_setg(errp, "Invalid snapshot ID: %s",
                   snapshot_id ? snapshot_id : "<null>");
        return -EINVAL;
    }

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v2 1/1] block/sheepdog: fix argument passed to qemu_strtoul()

[Kevin Wolf]

Am 02.03.2016 um 17:12 hat Paolo Bonzini geschrieben:
> 
> 
> On 02/03/2016 17:09, Jeff Cody wrote:
> > +    ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id);
> > +    if (ret) {
> > +        error_setg_errno(errp, -ret, "Invalid snapshot ID: %s",
> > +                         snapshot_id ? snapshot_id : "<null>");
> > +        return ret;
> > +    }
> > +
> > +    if (snap_id > UINT32_MAX) {
> > +        error_setg_errno(errp, EINVAL, "Snapshot ID numeric value %" PRId64
> > +                         " exceeds Sheepdog maximum of %" PRId32, snap_id,
> > +                          UINT32_MAX);
> > +        return -EINVAL;
> >      }
> 
> I think including the errno produces a worse error message ("Invalid
> snapshot ID: foo: Invalid argument" or something like that)

Yes. Rule of thumb: If you call error_setg_errno() with a constant
errno, there's something wrong.

Kevin

> , and also
> the error should be the same for an id of 10^10 (within uint64_t bounds)
> or 10^30 (outside the bounds).  So you could just use
> 
>     if (ret < 0 || snap_id > UINT32_MAX) {
>         error_setg(errp, "Invalid snapshot ID: %s",
>                    snapshot_id ? snapshot_id : "<null>");
>         return -EINVAL;
>     }
> 
> Thanks,
> 
> Paolo

Re: [Qemu-devel] [PATCH v2 1/1] block/sheepdog: fix argument passed to qemu_strtoul()

[Jeff Cody]

On Wed, Mar 02, 2016 at 05:12:57PM +0100, Paolo Bonzini wrote:
> 
> 
> On 02/03/2016 17:09, Jeff Cody wrote:
> > +    ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id);
> > +    if (ret) {
> > +        error_setg_errno(errp, -ret, "Invalid snapshot ID: %s",
> > +                         snapshot_id ? snapshot_id : "<null>");
> > +        return ret;
> > +    }
> > +
> > +    if (snap_id > UINT32_MAX) {
> > +        error_setg_errno(errp, EINVAL, "Snapshot ID numeric value %" PRId64
> > +                         " exceeds Sheepdog maximum of %" PRId32, snap_id,
> > +                          UINT32_MAX);
> > +        return -EINVAL;
> >      }
> 
> I think including the errno produces a worse error message ("Invalid
> snapshot ID: foo: Invalid argument" or something like that), and also

Hey, if the user enters invalid input, they deserve a cryptic error
message! Oh, alright - sent a v3 :-)

> the error should be the same for an id of 10^10 (within uint64_t bounds)
> or 10^30 (outside the bounds).  So you could just use
> 
>     if (ret < 0 || snap_id > UINT32_MAX) {
>         error_setg(errp, "Invalid snapshot ID: %s",
>                    snapshot_id ? snapshot_id : "<null>");
>         return -EINVAL;
>     }
> 
> Thanks,
> 
> Paolo