[Qemu-devel] [PATCH v2 0/2] block: Reject negative values for throttling options

[Qemu-devel] [PATCH v2 0/2] block: Reject negative values for throttling options

[Fam Zheng]

v2: Check the value range and report an appropriate error. [Berto]

Now the negative values are silently converted to a huge positive number
because we are doing implicit casting from uint64_t to double. Fix it and add a
test case (this was once fixed in 7d81c1413c9 but regressed when the block
device option parsing code was changed).


Fam Zheng (2):
  blockdev: Error out on negative throttling option values
  iotests: Test that negative throttle values are rejected

 blockdev.c                    |  3 ++-
 include/qemu/throttle.h       |  2 ++
 tests/qemu-iotests/051        | 11 +++++++++++
 tests/qemu-iotests/051.out    | 21 +++++++++++++++++++++
 tests/qemu-iotests/051.pc.out | 21 +++++++++++++++++++++
 util/throttle.c               | 16 ++++++----------
 6 files changed, 63 insertions(+), 11 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH v2 1/2] blockdev: Error out on negative throttling option values

[Fam Zheng]

The implicit casting from unsigned int to double changes negative values
into large positive numbers and accepts them.  We should instead print
an error.

Check the number range so this case is catched and reported.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c              |  3 ++-
 include/qemu/throttle.h |  2 ++
 util/throttle.c         | 16 ++++++----------
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 2df0c6d..1afef87 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -348,7 +348,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp)
     }
 
     if (!throttle_is_valid(cfg)) {
-        error_setg(errp, "bps/iops/maxs values must be 0 or greater");
+        error_setg(errp, "bps/iops/maxs values must be within [0, %" PRId64
+                         ")", (int64_t)THROTTLE_VALUE_MAX);
         return false;
     }
 
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h
index 12faaad..b5ddddd 100644
--- a/include/qemu/throttle.h
+++ b/include/qemu/throttle.h
@@ -29,6 +29,8 @@
 #include "qemu-common.h"
 #include "qemu/timer.h"
 
+#define THROTTLE_VALUE_MAX 1000000000000000L
+
 typedef enum {
     THROTTLE_BPS_TOTAL,
     THROTTLE_BPS_READ,
diff --git a/util/throttle.c b/util/throttle.c
index 1113671..af4bc95 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -282,22 +282,18 @@ bool throttle_conflicting(ThrottleConfig *cfg)
  */
 bool throttle_is_valid(ThrottleConfig *cfg)
 {
-    bool invalid = false;
     int i;
 
     for (i = 0; i < BUCKETS_COUNT; i++) {
-        if (cfg->buckets[i].avg < 0) {
-            invalid = true;
+        if (cfg->buckets[i].avg < 0 ||
+            cfg->buckets[i].max < 0 ||
+            cfg->buckets[i].avg > THROTTLE_VALUE_MAX ||
+            cfg->buckets[i].max > THROTTLE_VALUE_MAX) {
+            return false;
         }
     }
 
-    for (i = 0; i < BUCKETS_COUNT; i++) {
-        if (cfg->buckets[i].max < 0) {
-            invalid = true;
-        }
-    }
-
-    return !invalid;
+    return true;
 }
 
 /* check if bps_max/iops_max is used without bps/iops
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 1/2] blockdev: Error out on negative throttling option values

[Alberto Garcia]

On Wed 13 Jan 2016 01:52:29 AM CET, Fam Zheng wrote:

> The implicit casting from unsigned int to double changes negative values
> into large positive numbers and accepts them.  We should instead print
> an error.
>
> Check the number range so this case is catched and reported.

I still don't know why qemu_opt_get_number() convert silently negative
numbers into positive ones, shouldn't it just fail with an "invalid
parameter" error?

> +#define THROTTLE_VALUE_MAX 1000000000000000L

This is larger than LONG_MAX in 32-bit systems, I don't know if you need
to use LL instead.

Berto

Re: [Qemu-devel] [PATCH v2 1/2] blockdev: Error out on negative throttling option values

[Fam Zheng]

On Wed, 01/13 11:17, Alberto Garcia wrote:
> On Wed 13 Jan 2016 01:52:29 AM CET, Fam Zheng wrote:
> 
> > The implicit casting from unsigned int to double changes negative values
> > into large positive numbers and accepts them.  We should instead print
> > an error.
> >
> > Check the number range so this case is catched and reported.
> 
> I still don't know why qemu_opt_get_number() convert silently negative
> numbers into positive ones, shouldn't it just fail with an "invalid
> parameter" error?

Because the parsing is done with strtoull(3) and unfortunately its man page
says "negative values are considered valid input and are silently converted to
the equivalent unsigned long int value."

> 
> > +#define THROTTLE_VALUE_MAX 1000000000000000L
> 
> This is larger than LONG_MAX in 32-bit systems, I don't know if you need
> to use LL instead.

I assume a compiler will handle that okay but yes it's safer to use LL.

Fam

Re: [Qemu-devel] [PATCH v2 1/2] blockdev: Error out on negative throttling option values

[Alberto Garcia]

On Wed 13 Jan 2016 12:02:00 PM CET, Fam Zheng wrote:

>> > Check the number range so this case is catched and reported.
>> 
>> I still don't know why qemu_opt_get_number() convert silently
>> negative numbers into positive ones, shouldn't it just fail with an
>> "invalid parameter" error?
>
> Because the parsing is done with strtoull(3) and unfortunately its man
> page says "negative values are considered valid input and are silently
> converted to the equivalent unsigned long int value."

I see... parse_uint() from cutils.c handles that by making an explicit
check for negative numbers. It probably makes sense to apply the same
solution (or even merge the code to the extent to which it's possible).

I also noticed that there's a couple of places where we're calling
qemu_opt_get_number() passing -1 as default value, so maybe that API
needs to be reviewed anyway.

Berto

Re: [Qemu-devel] [PATCH v2 1/2] blockdev: Error out on negative throttling option values

[Fam Zheng]

On Wed, 01/13 12:13, Alberto Garcia wrote:
> On Wed 13 Jan 2016 12:02:00 PM CET, Fam Zheng wrote:
> 
> >> > Check the number range so this case is catched and reported.
> >> 
> >> I still don't know why qemu_opt_get_number() convert silently
> >> negative numbers into positive ones, shouldn't it just fail with an
> >> "invalid parameter" error?
> >
> > Because the parsing is done with strtoull(3) and unfortunately its man
> > page says "negative values are considered valid input and are silently
> > converted to the equivalent unsigned long int value."
> 
> I see... parse_uint() from cutils.c handles that by making an explicit
> check for negative numbers. It probably makes sense to apply the same
> solution (or even merge the code to the extent to which it's possible).
> 
> I also noticed that there's a couple of places where we're calling
> qemu_opt_get_number() passing -1 as default value, so maybe that API
> needs to be reviewed anyway.

Those callers rely on casting preserves the MSB as the sign, but that's ugly.
Anyway I'd leave the API change for a separate series and keep this patch local
to fix this particular regression. :)

Fam

Re: [Qemu-devel] [PATCH v2 1/2] blockdev: Error out on negative throttling option values

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1026 bytes --]

On 01/13/2016 03:17 AM, Alberto Garcia wrote:
> On Wed 13 Jan 2016 01:52:29 AM CET, Fam Zheng wrote:
> 
>> The implicit casting from unsigned int to double changes negative values
>> into large positive numbers and accepts them.  We should instead print
>> an error.
>>
>> Check the number range so this case is catched and reported.

s/catched/caught/

> 
> I still don't know why qemu_opt_get_number() convert silently negative
> numbers into positive ones, shouldn't it just fail with an "invalid
> parameter" error?

Passing -1 as a synonym for ULLONG_MAX can be convenient.  But rejecting
it outright rather than doing wraparound wouldn't hurt libvirt too badly.

> 
>> +#define THROTTLE_VALUE_MAX 1000000000000000L
> 
> This is larger than LONG_MAX in 32-bit systems, I don't know if you need
> to use LL instead.

You do need LL, not for C99, but for older compilers (hello mingw).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH v2 2/2] iotests: Test that negative throttle values are rejected

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 tests/qemu-iotests/051        | 11 +++++++++++
 tests/qemu-iotests/051.out    | 21 +++++++++++++++++++++
 tests/qemu-iotests/051.pc.out | 21 +++++++++++++++++++++
 3 files changed, 53 insertions(+)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index d91f80b..2c9b07e 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -263,6 +263,17 @@ run_qemu -drive file="$TEST_IMG",iops_size=1234,throttling.iops-size=5678
 run_qemu -drive file="$TEST_IMG",readonly=on,read-only=off
 
 echo
+echo === Catching nagative throttling values ===
+echo
+
+run_qemu -drive file="$TEST_IMG",iops=-1
+run_qemu -drive file="$TEST_IMG",bps=-2
+run_qemu -drive file="$TEST_IMG",bps_rd=-3
+run_qemu -drive file="$TEST_IMG",bps_rd_max=-3
+run_qemu -drive file="$TEST_IMG",throttling.iops-total=-4
+run_qemu -drive file="$TEST_IMG",throttling.bps-total=-5
+
+echo
 echo === Parsing protocol from file name ===
 echo
 
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index bf886ce..82d2121 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -285,6 +285,27 @@ Testing: -drive file=TEST_DIR/t.qcow2,readonly=on,read-only=off
 QEMU_PROG: -drive file=TEST_DIR/t.qcow2,readonly=on,read-only=off: 'read-only' and its alias 'readonly' can't be used at the same time
 
 
+=== Catching nagative throttling values ===
+
+Testing: -drive file=TEST_DIR/t.qcow2,iops=-1
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,iops=-1: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=-2
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps=-2: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps_rd=-3
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps_rd=-3: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps_rd_max=-3
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps_rd_max=-3: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,throttling.iops-total=-4
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,throttling.iops-total=-4: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,throttling.bps-total=-5
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,throttling.bps-total=-5: bps/iops/maxs values must be within [0, 1000000000000000)
+
+
 === Parsing protocol from file name ===
 
 Testing: -hda foo:bar
diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out
index a5dfc33..4b29289 100644
--- a/tests/qemu-iotests/051.pc.out
+++ b/tests/qemu-iotests/051.pc.out
@@ -379,6 +379,27 @@ Testing: -drive file=TEST_DIR/t.qcow2,readonly=on,read-only=off
 QEMU_PROG: -drive file=TEST_DIR/t.qcow2,readonly=on,read-only=off: 'read-only' and its alias 'readonly' can't be used at the same time
 
 
+=== Catching nagative throttling values ===
+
+Testing: -drive file=TEST_DIR/t.qcow2,iops=-1
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,iops=-1: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps=-2
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps=-2: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps_rd=-3
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps_rd=-3: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,bps_rd_max=-3
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,bps_rd_max=-3: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,throttling.iops-total=-4
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,throttling.iops-total=-4: bps/iops/maxs values must be within [0, 1000000000000000)
+
+Testing: -drive file=TEST_DIR/t.qcow2,throttling.bps-total=-5
+QEMU_PROG: -drive file=TEST_DIR/t.qcow2,throttling.bps-total=-5: bps/iops/maxs values must be within [0, 1000000000000000)
+
+
 === Parsing protocol from file name ===
 
 Testing: -hda foo:bar
-- 
2.4.3

Re: [Qemu-devel] [PATCH v2 2/2] iotests: Test that negative throttle values are rejected

[Alberto Garcia]

On Wed 13 Jan 2016 01:52:30 AM CET, Fam Zheng <famz@redhat.com> wrote:

> +echo === Catching nagative throttling values ===

s/nagative/negative/

(there are several of these in the patch)

You could also test the upper limits now.

Berto

Re: [Qemu-devel] [PATCH v2 2/2] iotests: Test that negative throttle values are rejected

[Fam Zheng]

On Wed, 01/13 11:02, Alberto Garcia wrote:
> On Wed 13 Jan 2016 01:52:30 AM CET, Fam Zheng <famz@redhat.com> wrote:
> 
> > +echo === Catching nagative throttling values ===
> 
> s/nagative/negative/
> 
> (there are several of these in the patch)

Will fix.

> 
> You could also test the upper limits now.
> 

Yes, I will add one.

Fam