All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3 0/3] qemu-io: clean up cvtnum usage
@ 2015-11-04  0:17 John Snow
  2015-11-04  0:17 ` [Qemu-devel] [PATCH v3 1/3] qemu-io: fix cvtnum lval types John Snow
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: John Snow @ 2015-11-04  0:17 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, qemu-devel, mreitz

cvtnum returns an int64_t, not an int, so correct the lvalue types
wherever it is used. While we're at it, make the error messages more
meaningful and hopefully less confusing.

v3:
 - pulled a lot of loose yarn, now missing my sweater
   (Updated patch 1 even further, reported-by Kevin)

v2:
 - Squashed NSIG error-checking from patch 3 into patch 1
 - Reported-by credits for Max and Reviewed-by from Eric added

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch qemu-io-tidy
https://github.com/jnsnow/qemu/tree/qemu-io-tidy

This version is tagged qemu-io-tidy-v3:
https://github.com/jnsnow/qemu/releases/tag/qemu-io-tidy-v3

John Snow (3):
  qemu-io: fix cvtnum lval types
  qemu-io: Check for trailing chars
  qemu-io: Correct error messages

 qemu-io-cmds.c | 150 ++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 95 insertions(+), 55 deletions(-)

-- 
2.4.3

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH v3 1/3] qemu-io: fix cvtnum lval types
  2015-11-04  0:17 [Qemu-devel] [PATCH v3 0/3] qemu-io: clean up cvtnum usage John Snow
@ 2015-11-04  0:17 ` John Snow
  2015-11-04 10:35   ` Kevin Wolf
  2015-11-04  0:17 ` [Qemu-devel] [PATCH v3 2/3] qemu-io: Check for trailing chars John Snow
  2015-11-04  0:17 ` [Qemu-devel] [PATCH v3 3/3] qemu-io: Correct error messages John Snow
  2 siblings, 1 reply; 5+ messages in thread
From: John Snow @ 2015-11-04  0:17 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, qemu-devel, mreitz

cvtnum() returns int64_t: we should not be storing this
result inside of an int.

In a few cases, we need an extra sprinkling of error handling
where we expect to pass this number on towards a function that
expects something smaller than int64_t.

Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 qemu-io-cmds.c | 88 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 53 insertions(+), 35 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 6e5d1e4..f04c1db 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -294,7 +294,7 @@ static void qemu_io_free(void *p)
     qemu_vfree(p);
 }
 
-static void dump_buffer(const void *buffer, int64_t offset, int len)
+static void dump_buffer(const void *buffer, int64_t offset, int64_t len)
 {
     int i, j;
     const uint8_t *p;
@@ -319,7 +319,7 @@ static void dump_buffer(const void *buffer, int64_t offset, int len)
 }
 
 static void print_report(const char *op, struct timeval *t, int64_t offset,
-                         int count, int total, int cnt, int Cflag)
+                         int64_t count, int64_t total, int cnt, int Cflag)
 {
     char s1[64], s2[64], ts[64];
 
@@ -327,12 +327,12 @@ static void print_report(const char *op, struct timeval *t, int64_t offset,
     if (!Cflag) {
         cvtstr((double)total, s1, sizeof(s1));
         cvtstr(tdiv((double)total, *t), s2, sizeof(s2));
-        printf("%s %d/%d bytes at offset %" PRId64 "\n",
+        printf("%s %"PRId64"/%"PRId64" bytes at offset %" PRId64 "\n",
                op, total, count, offset);
         printf("%s, %d ops; %s (%s/sec and %.4f ops/sec)\n",
                s1, cnt, ts, s2, tdiv((double)cnt, *t));
     } else {/* bytes,ops,time,bytes/sec,ops/sec */
-        printf("%d,%d,%s,%.3f,%.3f\n",
+        printf("%"PRId64",%d,%s,%.3f,%.3f\n",
             total, cnt, ts,
             tdiv((double)total, *t),
             tdiv((double)cnt, *t));
@@ -393,8 +393,8 @@ fail:
     return buf;
 }
 
-static int do_read(BlockBackend *blk, char *buf, int64_t offset, int count,
-                   int *total)
+static int do_read(BlockBackend *blk, char *buf, int64_t offset, int64_t count,
+                   int64_t *total)
 {
     int ret;
 
@@ -406,8 +406,8 @@ static int do_read(BlockBackend *blk, char *buf, int64_t offset, int count,
     return 1;
 }
 
-static int do_write(BlockBackend *blk, char *buf, int64_t offset, int count,
-                    int *total)
+static int do_write(BlockBackend *blk, char *buf, int64_t offset, int64_t count,
+                    int64_t *total)
 {
     int ret;
 
@@ -419,8 +419,8 @@ static int do_write(BlockBackend *blk, char *buf, int64_t offset, int count,
     return 1;
 }
 
-static int do_pread(BlockBackend *blk, char *buf, int64_t offset, int count,
-                    int *total)
+static int do_pread(BlockBackend *blk, char *buf, int64_t offset,
+                    int64_t count, int64_t *total)
 {
     *total = blk_pread(blk, offset, (uint8_t *)buf, count);
     if (*total < 0) {
@@ -429,8 +429,8 @@ static int do_pread(BlockBackend *blk, char *buf, int64_t offset, int count,
     return 1;
 }
 
-static int do_pwrite(BlockBackend *blk, char *buf, int64_t offset, int count,
-                     int *total)
+static int do_pwrite(BlockBackend *blk, char *buf, int64_t offset,
+                     int64_t count, int64_t *total)
 {
     *total = blk_pwrite(blk, offset, (uint8_t *)buf, count);
     if (*total < 0) {
@@ -442,8 +442,8 @@ static int do_pwrite(BlockBackend *blk, char *buf, int64_t offset, int count,
 typedef struct {
     BlockBackend *blk;
     int64_t offset;
-    int count;
-    int *total;
+    int64_t count;
+    int64_t *total;
     int ret;
     bool done;
 } CoWriteZeroes;
@@ -463,8 +463,8 @@ static void coroutine_fn co_write_zeroes_entry(void *opaque)
     *data->total = data->count;
 }
 
-static int do_co_write_zeroes(BlockBackend *blk, int64_t offset, int count,
-                              int *total)
+static int do_co_write_zeroes(BlockBackend *blk, int64_t offset, int64_t count,
+                              int64_t *total)
 {
     Coroutine *co;
     CoWriteZeroes data = {
@@ -488,7 +488,7 @@ static int do_co_write_zeroes(BlockBackend *blk, int64_t offset, int count,
 }
 
 static int do_write_compressed(BlockBackend *blk, char *buf, int64_t offset,
-                               int count, int *total)
+                               int64_t count, int64_t *total)
 {
     int ret;
 
@@ -501,7 +501,7 @@ static int do_write_compressed(BlockBackend *blk, char *buf, int64_t offset,
 }
 
 static int do_load_vmstate(BlockBackend *blk, char *buf, int64_t offset,
-                           int count, int *total)
+                           int64_t count, int64_t *total)
 {
     *total = blk_load_vmstate(blk, (uint8_t *)buf, offset, count);
     if (*total < 0) {
@@ -511,7 +511,7 @@ static int do_load_vmstate(BlockBackend *blk, char *buf, int64_t offset,
 }
 
 static int do_save_vmstate(BlockBackend *blk, char *buf, int64_t offset,
-                           int count, int *total)
+                           int64_t count, int64_t *total)
 {
     *total = blk_save_vmstate(blk, (uint8_t *)buf, offset, count);
     if (*total < 0) {
@@ -642,10 +642,11 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
     int c, cnt;
     char *buf;
     int64_t offset;
-    int count;
+    int64_t count;
     /* Some compilers get confused and warn if this is not initialized.  */
-    int total = 0;
-    int pattern = 0, pattern_offset = 0, pattern_count = 0;
+    int64_t total = 0;
+    int pattern = 0;
+    int64_t pattern_offset = 0, pattern_count = 0;
 
     while ((c = getopt(argc, argv, "bCl:pP:qs:v")) != -1) {
         switch (c) {
@@ -712,6 +713,9 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
     if (count < 0) {
         printf("non-numeric length argument -- %s\n", argv[optind]);
         return 0;
+    } else if (count > SIZE_MAX) {
+        printf("length cannot exceed %zu, given %s\n", SIZE_MAX, argv[optind]);
+        return 0;
     }
 
     if (!Pflag && (lflag || sflag)) {
@@ -734,7 +738,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
             return 0;
         }
         if (count & 0x1ff) {
-            printf("count %d is not sector aligned\n",
+            printf("count %"PRId64" is not sector aligned\n",
                    count);
             return 0;
         }
@@ -762,7 +766,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
         memset(cmp_buf, pattern, pattern_count);
         if (memcmp(buf + pattern_offset, cmp_buf, pattern_count)) {
             printf("Pattern verification failed at offset %"
-                   PRId64 ", %d bytes\n",
+                   PRId64 ", %"PRId64" bytes\n",
                    offset + pattern_offset, pattern_count);
         }
         g_free(cmp_buf);
@@ -957,9 +961,9 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
     int c, cnt;
     char *buf = NULL;
     int64_t offset;
-    int count;
+    int64_t count;
     /* Some compilers get confused and warn if this is not initialized.  */
-    int total = 0;
+    int64_t total = 0;
     int pattern = 0xcd;
 
     while ((c = getopt(argc, argv, "bcCpP:qz")) != -1) {
@@ -1019,6 +1023,9 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
     if (count < 0) {
         printf("non-numeric length argument -- %s\n", argv[optind]);
         return 0;
+    } else if (count > SIZE_MAX) {
+        printf("length cannot exceed %zu, given %s\n", SIZE_MAX, argv[optind]);
+        return 0;
     }
 
     if (!pflag) {
@@ -1029,7 +1036,7 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
         }
 
         if (count & 0x1ff) {
-            printf("count %d is not sector aligned\n",
+            printf("count %"PRId64" is not sector aligned\n",
                    count);
             return 0;
         }
@@ -1777,8 +1784,7 @@ static int discard_f(BlockBackend *blk, int argc, char **argv)
     struct timeval t1, t2;
     int Cflag = 0, qflag = 0;
     int c, ret;
-    int64_t offset;
-    int count;
+    int64_t offset, count;
 
     while ((c = getopt(argc, argv, "Cq")) != -1) {
         switch (c) {
@@ -1808,6 +1814,11 @@ static int discard_f(BlockBackend *blk, int argc, char **argv)
     if (count < 0) {
         printf("non-numeric length argument -- %s\n", argv[optind]);
         return 0;
+    } else if (count >> BDRV_SECTOR_BITS > INT_MAX) {
+        printf("length cannot exceed %"PRIu64", given %s\n",
+               (uint64_t)INT_MAX << BDRV_SECTOR_BITS,
+               argv[optind]);
+        return 0;
     }
 
     gettimeofday(&t1, NULL);
@@ -1833,11 +1844,10 @@ out:
 static int alloc_f(BlockBackend *blk, int argc, char **argv)
 {
     BlockDriverState *bs = blk_bs(blk);
-    int64_t offset, sector_num;
-    int nb_sectors, remaining;
+    int64_t offset, sector_num, nb_sectors, remaining;
     char s1[64];
-    int num, sum_alloc;
-    int ret;
+    int num, ret;
+    int64_t sum_alloc;
 
     offset = cvtnum(argv[1]);
     if (offset < 0) {
@@ -1854,6 +1864,10 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
         if (nb_sectors < 0) {
             printf("non-numeric length argument -- %s\n", argv[2]);
             return 0;
+        } else if (nb_sectors > INT_MAX) {
+            printf("length argument cannot exceed %d, given %s\n",
+                   INT_MAX, argv[2]);
+            return 0;
         }
     } else {
         nb_sectors = 1;
@@ -1881,7 +1895,7 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
 
     cvtstr(offset, s1, sizeof(s1));
 
-    printf("%d/%d sectors allocated at offset %s\n",
+    printf("%"PRId64"/%"PRId64" sectors allocated at offset %s\n",
            sum_alloc, nb_sectors, s1);
     return 0;
 }
@@ -2191,10 +2205,14 @@ static const cmdinfo_t sigraise_cmd = {
 
 static int sigraise_f(BlockBackend *blk, int argc, char **argv)
 {
-    int sig = cvtnum(argv[1]);
+    int64_t sig = cvtnum(argv[1]);
     if (sig < 0) {
         printf("non-numeric signal number argument -- %s\n", argv[1]);
         return 0;
+    } else if (sig > NSIG) {
+        printf("signal argument '%s' is too large to be a valid signal\n",
+               argv[1]);
+        return 0;
     }
 
     /* Using raise() to kill this process does not necessarily flush all open
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH v3 2/3] qemu-io: Check for trailing chars
  2015-11-04  0:17 [Qemu-devel] [PATCH v3 0/3] qemu-io: clean up cvtnum usage John Snow
  2015-11-04  0:17 ` [Qemu-devel] [PATCH v3 1/3] qemu-io: fix cvtnum lval types John Snow
@ 2015-11-04  0:17 ` John Snow
  2015-11-04  0:17 ` [Qemu-devel] [PATCH v3 3/3] qemu-io: Correct error messages John Snow
  2 siblings, 0 replies; 5+ messages in thread
From: John Snow @ 2015-11-04  0:17 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, qemu-devel, mreitz

Make sure there's not trailing garbage, e.g.
"64k-whatever-i-want-here"

Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io-cmds.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index f04c1db..2c38ae7 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -136,7 +136,14 @@ static char **breakline(char *input, int *count)
 static int64_t cvtnum(const char *s)
 {
     char *end;
-    return qemu_strtosz_suffix(s, &end, QEMU_STRTOSZ_DEFSUFFIX_B);
+    int64_t ret;
+
+    ret = qemu_strtosz_suffix(s, &end, QEMU_STRTOSZ_DEFSUFFIX_B);
+    if (*end != '\0') {
+        /* Detritus at the end of the string */
+        return -EINVAL;
+    }
+    return ret;
 }
 
 #define EXABYTES(x)     ((long long)(x) << 60)
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH v3 3/3] qemu-io: Correct error messages
  2015-11-04  0:17 [Qemu-devel] [PATCH v3 0/3] qemu-io: clean up cvtnum usage John Snow
  2015-11-04  0:17 ` [Qemu-devel] [PATCH v3 1/3] qemu-io: fix cvtnum lval types John Snow
  2015-11-04  0:17 ` [Qemu-devel] [PATCH v3 2/3] qemu-io: Check for trailing chars John Snow
@ 2015-11-04  0:17 ` John Snow
  2 siblings, 0 replies; 5+ messages in thread
From: John Snow @ 2015-11-04  0:17 UTC (permalink / raw)
  To: qemu-block; +Cc: kwolf, John Snow, qemu-devel, mreitz

Reported-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io-cmds.c | 53 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 2c38ae7..29091f7 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -146,6 +146,21 @@ static int64_t cvtnum(const char *s)
     return ret;
 }
 
+static void print_cvtnum_err(int64_t rc, const char *arg)
+{
+    switch (rc) {
+    case -EINVAL:
+        printf("Parsing error: non-numeric argument,"
+               " or extraneous/unrecognized suffix -- %s\n", arg);
+        break;
+    case -ERANGE:
+        printf("Parsing error: argument too large -- %s\n", arg);
+        break;
+    default:
+        printf("Parsing error: %s\n", arg);
+    }
+}
+
 #define EXABYTES(x)     ((long long)(x) << 60)
 #define PETABYTES(x)    ((long long)(x) << 50)
 #define TERABYTES(x)    ((long long)(x) << 40)
@@ -366,13 +381,13 @@ create_iovec(BlockBackend *blk, QEMUIOVector *qiov, char **argv, int nr_iov,
 
         len = cvtnum(arg);
         if (len < 0) {
-            printf("non-numeric length argument -- %s\n", arg);
+            print_cvtnum_err(len, arg);
             goto fail;
         }
 
         /* should be SIZE_T_MAX, but that doesn't exist */
         if (len > INT_MAX) {
-            printf("too large length argument -- %s\n", arg);
+            printf("Argument '%s' exceeds maximum size %d\n", arg, INT_MAX);
             goto fail;
         }
 
@@ -667,7 +682,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
             lflag = 1;
             pattern_count = cvtnum(optarg);
             if (pattern_count < 0) {
-                printf("non-numeric length argument -- %s\n", optarg);
+                print_cvtnum_err(pattern_count, optarg);
                 return 0;
             }
             break;
@@ -688,7 +703,7 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
             sflag = 1;
             pattern_offset = cvtnum(optarg);
             if (pattern_offset < 0) {
-                printf("non-numeric length argument -- %s\n", optarg);
+                print_cvtnum_err(pattern_offset, optarg);
                 return 0;
             }
             break;
@@ -711,14 +726,14 @@ static int read_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
 
     optind++;
     count = cvtnum(argv[optind]);
     if (count < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(count, argv[optind]);
         return 0;
     } else if (count > SIZE_MAX) {
         printf("length cannot exceed %zu, given %s\n", SIZE_MAX, argv[optind]);
@@ -872,7 +887,7 @@ static int readv_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
     optind++;
@@ -1021,14 +1036,14 @@ static int write_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
 
     optind++;
     count = cvtnum(argv[optind]);
     if (count < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(count, argv[optind]);
         return 0;
     } else if (count > SIZE_MAX) {
         printf("length cannot exceed %zu, given %s\n", SIZE_MAX, argv[optind]);
@@ -1156,7 +1171,7 @@ static int writev_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
     optind++;
@@ -1283,7 +1298,7 @@ static int multiwrite_f(BlockBackend *blk, int argc, char **argv)
         /* Read the offset of the request */
         offset = cvtnum(argv[optind]);
         if (offset < 0) {
-            printf("non-numeric offset argument -- %s\n", argv[optind]);
+            print_cvtnum_err(offset, argv[optind]);
             goto out;
         }
         optind++;
@@ -1510,7 +1525,7 @@ static int aio_read_f(BlockBackend *blk, int argc, char **argv)
 
     ctx->offset = cvtnum(argv[optind]);
     if (ctx->offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(ctx->offset, argv[optind]);
         g_free(ctx);
         return 0;
     }
@@ -1605,7 +1620,7 @@ static int aio_write_f(BlockBackend *blk, int argc, char **argv)
 
     ctx->offset = cvtnum(argv[optind]);
     if (ctx->offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(ctx->offset, argv[optind]);
         g_free(ctx);
         return 0;
     }
@@ -1665,7 +1680,7 @@ static int truncate_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[1]);
     if (offset < 0) {
-        printf("non-numeric truncate argument -- %s\n", argv[1]);
+        print_cvtnum_err(offset, argv[1]);
         return 0;
     }
 
@@ -1812,14 +1827,14 @@ static int discard_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[optind]);
     if (offset < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(offset, argv[optind]);
         return 0;
     }
 
     optind++;
     count = cvtnum(argv[optind]);
     if (count < 0) {
-        printf("non-numeric length argument -- %s\n", argv[optind]);
+        print_cvtnum_err(count, argv[optind]);
         return 0;
     } else if (count >> BDRV_SECTOR_BITS > INT_MAX) {
         printf("length cannot exceed %"PRIu64", given %s\n",
@@ -1858,7 +1873,7 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
 
     offset = cvtnum(argv[1]);
     if (offset < 0) {
-        printf("non-numeric offset argument -- %s\n", argv[1]);
+        print_cvtnum_err(offset, argv[1]);
         return 0;
     } else if (offset & 0x1ff) {
         printf("offset %" PRId64 " is not sector aligned\n",
@@ -1869,7 +1884,7 @@ static int alloc_f(BlockBackend *blk, int argc, char **argv)
     if (argc == 3) {
         nb_sectors = cvtnum(argv[2]);
         if (nb_sectors < 0) {
-            printf("non-numeric length argument -- %s\n", argv[2]);
+            print_cvtnum_err(nb_sectors, argv[2]);
             return 0;
         } else if (nb_sectors > INT_MAX) {
             printf("length argument cannot exceed %d, given %s\n",
@@ -2214,7 +2229,7 @@ static int sigraise_f(BlockBackend *blk, int argc, char **argv)
 {
     int64_t sig = cvtnum(argv[1]);
     if (sig < 0) {
-        printf("non-numeric signal number argument -- %s\n", argv[1]);
+        print_cvtnum_err(sig, argv[1]);
         return 0;
     } else if (sig > NSIG) {
         printf("signal argument '%s' is too large to be a valid signal\n",
-- 
2.4.3

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] qemu-io: fix cvtnum lval types
  2015-11-04  0:17 ` [Qemu-devel] [PATCH v3 1/3] qemu-io: fix cvtnum lval types John Snow
@ 2015-11-04 10:35   ` Kevin Wolf
  0 siblings, 0 replies; 5+ messages in thread
From: Kevin Wolf @ 2015-11-04 10:35 UTC (permalink / raw)
  To: John Snow; +Cc: qemu-devel, qemu-block, mreitz

Am 04.11.2015 um 01:17 hat John Snow geschrieben:
> cvtnum() returns int64_t: we should not be storing this
> result inside of an int.
> 
> In a few cases, we need an extra sprinkling of error handling
> where we expect to pass this number on towards a function that
> expects something smaller than int64_t.
> 
> Reported-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  qemu-io-cmds.c | 88 +++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 53 insertions(+), 35 deletions(-)

> v3:
> - pulled a lot of loose yarn, now missing my sweater
>   (Updated patch 1 even further, reported-by Kevin)

I'm afraid you'll have to start using up another sweater.

> diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
> index 6e5d1e4..f04c1db 100644
> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -294,7 +294,7 @@ static void qemu_io_free(void *p)
>      qemu_vfree(p);
>  }
>  
> -static void dump_buffer(const void *buffer, int64_t offset, int len)
> +static void dump_buffer(const void *buffer, int64_t offset, int64_t len)
>  {
>      int i, j;
>      const uint8_t *p;

One more line of context:

    for (i = 0, p = buffer; i < len; i += 16) {

For len > INT_MAX, this is an endless loop. The same way, i + j a few
lines below can wrap around.

> @@ -393,8 +393,8 @@ fail:
>      return buf;
>  }
>  
> -static int do_read(BlockBackend *blk, char *buf, int64_t offset, int count,
> -                   int *total)
> +static int do_read(BlockBackend *blk, char *buf, int64_t offset, int64_t count,
> +                   int64_t *total)
>  {
>      int ret;

Again, one more line of context:

    ret = blk_read(blk, offset >> 9, (uint8_t *)buf, count >> 9);

count is silently truncated if it's larger than INT_MAX << 9. I think we
should return an error (ERANGE? EINVAL? EFBIG?) instead.

Same for do_write, do_pread, do_pwrite, co_write_zeroes_entry,
do_write_compressed, do_load_vmstate, do_save_vmstate.

Kevin

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-11-04 10:35 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-04  0:17 [Qemu-devel] [PATCH v3 0/3] qemu-io: clean up cvtnum usage John Snow
2015-11-04  0:17 ` [Qemu-devel] [PATCH v3 1/3] qemu-io: fix cvtnum lval types John Snow
2015-11-04 10:35   ` Kevin Wolf
2015-11-04  0:17 ` [Qemu-devel] [PATCH v3 2/3] qemu-io: Check for trailing chars John Snow
2015-11-04  0:17 ` [Qemu-devel] [PATCH v3 3/3] qemu-io: Correct error messages John Snow

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.