[Qemu-devel] [PATCH v3] virtio-rng: Add human-readable error message for negative max-bytes parameter

[Qemu-devel] [PATCH v3] virtio-rng: Add human-readable error message for negative max-bytes parameter

[John Snow]

If a negative integer is used for the max_bytes parameter, QEMU currently
calls abort() and leaves behind a core dump. This patch adds a simple
error message to make the reason for the termination clearer.

There is an underlying insufficiency in the parameter parsing code of QEMU
that renders it unable to reject negative values for unsigned properties,
thus the error message "a non-negative integer below 2^63" is the most
user-friendly and correct message we can give until the underlying 
insufficiency is corrected.

Signed-off-by: John Snow <jsnow@redhat.com>
---
v3: Adjusted the error message to be more semantically meaningful, but
while acknowledging the limitations of the current unsigned integer
parsing routines.

 hw/virtio/virtio-rng.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
index 1356aca..7c5a675 100644
--- a/hw/virtio/virtio-rng.c
+++ b/hw/virtio/virtio-rng.c
@@ -181,7 +181,13 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
 
     vrng->vq = virtio_add_queue(vdev, 8, handle_input);
 
-    assert(vrng->conf.max_bytes <= INT64_MAX);
+    /* Workaround: Property parsing does not enforce unsigned integers,
+     * So this is a hack to reject such numbers. */
+    if (vrng->conf.max_bytes > INT64_MAX) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "max-bytes",
+                  "a non-negative integer below 2^63");
+        return;
+    }
     vrng->quota_remaining = vrng->conf.max_bytes;
 
     vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
-- 
1.9.3

Re: [Qemu-devel] [PATCH v3] virtio-rng: Add human-readable error message for negative max-bytes parameter

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> If a negative integer is used for the max_bytes parameter, QEMU currently
> calls abort() and leaves behind a core dump. This patch adds a simple
> error message to make the reason for the termination clearer.

It also avoids the abort, doesn't it?

> There is an underlying insufficiency in the parameter parsing code of QEMU
> that renders it unable to reject negative values for unsigned properties,
> thus the error message "a non-negative integer below 2^63" is the most
> user-friendly and correct message we can give until the underlying 
> insufficiency is corrected.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> v3: Adjusted the error message to be more semantically meaningful, but
> while acknowledging the limitations of the current unsigned integer
> parsing routines.
>
>  hw/virtio/virtio-rng.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> index 1356aca..7c5a675 100644
> --- a/hw/virtio/virtio-rng.c
> +++ b/hw/virtio/virtio-rng.c
> @@ -181,7 +181,13 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
>  
>      vrng->vq = virtio_add_queue(vdev, 8, handle_input);
>  
> -    assert(vrng->conf.max_bytes <= INT64_MAX);
> +    /* Workaround: Property parsing does not enforce unsigned integers,
> +     * So this is a hack to reject such numbers. */
> +    if (vrng->conf.max_bytes > INT64_MAX) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "max-bytes",
> +                  "a non-negative integer below 2^63");
> +        return;
> +    }
>      vrng->quota_remaining = vrng->conf.max_bytes;
>  
>      vrng->rate_limit_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,

Patch looks good now.

Re: [Qemu-devel] [PATCH v3] virtio-rng: Add human-readable error message for negative max-bytes parameter

[Amit Shah]

On (Mon) 21 Jul 2014 [17:44:37], John Snow wrote:
> If a negative integer is used for the max_bytes parameter, QEMU currently
> calls abort() and leaves behind a core dump. This patch adds a simple
> error message to make the reason for the termination clearer.

It avoids the abort(), which in the case of device hot-plug means
there's no termination either.  That's the bigger advantage, because
there's no excuse for qemu to just quit when passing nonsensical
values during device hotplug.

> There is an underlying insufficiency in the parameter parsing code of QEMU
> that renders it unable to reject negative values for unsigned properties,
> thus the error message "a non-negative integer below 2^63" is the most
> user-friendly and correct message we can give until the underlying 
> insufficiency is corrected.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> v3: Adjusted the error message to be more semantically meaningful, but
> while acknowledging the limitations of the current unsigned integer
> parsing routines.

Thanks, I think this qualifies for 2.1, so I'll send it on after
adjusting the commit message a bit.

		Amit

Re: [Qemu-devel] [PATCH v3] virtio-rng: Add human-readable error message for negative max-bytes parameter

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Mon) 21 Jul 2014 [17:44:37], John Snow wrote:
>> If a negative integer is used for the max_bytes parameter, QEMU currently
>> calls abort() and leaves behind a core dump. This patch adds a simple
>> error message to make the reason for the termination clearer.
>
> It avoids the abort(), which in the case of device hot-plug means
> there's no termination either.  That's the bigger advantage, because
> there's no excuse for qemu to just quit when passing nonsensical
> values during device hotplug.
>
>> There is an underlying insufficiency in the parameter parsing code of QEMU
>> that renders it unable to reject negative values for unsigned properties,
>> thus the error message "a non-negative integer below 2^63" is the most
>> user-friendly and correct message we can give until the underlying 
>> insufficiency is corrected.
>> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> v3: Adjusted the error message to be more semantically meaningful, but
>> while acknowledging the limitations of the current unsigned integer
>> parsing routines.
>
> Thanks, I think this qualifies for 2.1, so I'll send it on after
> adjusting the commit message a bit.

You may add

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v3] virtio-rng: Add human-readable error message for negative max-bytes parameter

[Amit Shah]

On (Mon) 21 Jul 2014 [17:44:37], John Snow wrote:
> If a negative integer is used for the max_bytes parameter, QEMU currently
> calls abort() and leaves behind a core dump. This patch adds a simple
> error message to make the reason for the termination clearer.
> 
> There is an underlying insufficiency in the parameter parsing code of QEMU
> that renders it unable to reject negative values for unsigned properties,
> thus the error message "a non-negative integer below 2^63" is the most
> user-friendly and correct message we can give until the underlying 
> insufficiency is corrected.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> v3: Adjusted the error message to be more semantically meaningful, but
> while acknowledging the limitations of the current unsigned integer
> parsing routines.
> 
>  hw/virtio/virtio-rng.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> index 1356aca..7c5a675 100644
> --- a/hw/virtio/virtio-rng.c
> +++ b/hw/virtio/virtio-rng.c
> @@ -181,7 +181,13 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
>  
>      vrng->vq = virtio_add_queue(vdev, 8, handle_input);
>  
> -    assert(vrng->conf.max_bytes <= INT64_MAX);
> +    /* Workaround: Property parsing does not enforce unsigned integers,
> +     * So this is a hack to reject such numbers. */
> +    if (vrng->conf.max_bytes > INT64_MAX) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "max-bytes",
> +                  "a non-negative integer below 2^63");

Huh, why do we allow 0?  There's no reason to have 0 as a max-bytes
value as well...

		Amit

Re: [Qemu-devel] [PATCH v3] virtio-rng: Add human-readable error message for negative max-bytes parameter

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Mon) 21 Jul 2014 [17:44:37], John Snow wrote:
>> If a negative integer is used for the max_bytes parameter, QEMU currently
>> calls abort() and leaves behind a core dump. This patch adds a simple
>> error message to make the reason for the termination clearer.
>> 
>> There is an underlying insufficiency in the parameter parsing code of QEMU
>> that renders it unable to reject negative values for unsigned properties,
>> thus the error message "a non-negative integer below 2^63" is the most
>> user-friendly and correct message we can give until the underlying 
>> insufficiency is corrected.
>> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>> v3: Adjusted the error message to be more semantically meaningful, but
>> while acknowledging the limitations of the current unsigned integer
>> parsing routines.
>> 
>>  hw/virtio/virtio-rng.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
>> index 1356aca..7c5a675 100644
>> --- a/hw/virtio/virtio-rng.c
>> +++ b/hw/virtio/virtio-rng.c
>> @@ -181,7 +181,13 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
>>  
>>      vrng->vq = virtio_add_queue(vdev, 8, handle_input);
>>  
>> -    assert(vrng->conf.max_bytes <= INT64_MAX);
>> +    /* Workaround: Property parsing does not enforce unsigned integers,
>> +     * So this is a hack to reject such numbers. */
>> +    if (vrng->conf.max_bytes > INT64_MAX) {
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "max-bytes",
>> +                  "a non-negative integer below 2^63");
>
> Huh, why do we allow 0?  There's no reason to have 0 as a max-bytes
> value as well...

Could be treated as separate problem.

Re: [Qemu-devel] [PATCH v3] virtio-rng: Add human-readable error message for negative max-bytes parameter

[Amit Shah]

On (Tue) 22 Jul 2014 [13:41:43], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > On (Mon) 21 Jul 2014 [17:44:37], John Snow wrote:
> >> If a negative integer is used for the max_bytes parameter, QEMU currently
> >> calls abort() and leaves behind a core dump. This patch adds a simple
> >> error message to make the reason for the termination clearer.
> >> 
> >> There is an underlying insufficiency in the parameter parsing code of QEMU
> >> that renders it unable to reject negative values for unsigned properties,
> >> thus the error message "a non-negative integer below 2^63" is the most
> >> user-friendly and correct message we can give until the underlying 
> >> insufficiency is corrected.
> >> 
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> ---
> >> v3: Adjusted the error message to be more semantically meaningful, but
> >> while acknowledging the limitations of the current unsigned integer
> >> parsing routines.
> >> 
> >>  hw/virtio/virtio-rng.c | 8 +++++++-
> >>  1 file changed, 7 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> >> index 1356aca..7c5a675 100644
> >> --- a/hw/virtio/virtio-rng.c
> >> +++ b/hw/virtio/virtio-rng.c
> >> @@ -181,7 +181,13 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
> >>  
> >>      vrng->vq = virtio_add_queue(vdev, 8, handle_input);
> >>  
> >> -    assert(vrng->conf.max_bytes <= INT64_MAX);
> >> +    /* Workaround: Property parsing does not enforce unsigned integers,
> >> +     * So this is a hack to reject such numbers. */
> >> +    if (vrng->conf.max_bytes > INT64_MAX) {
> >> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "max-bytes",
> >> +                  "a non-negative integer below 2^63");
> >
> > Huh, why do we allow 0?  There's no reason to have 0 as a max-bytes
> > value as well...
> 
> Could be treated as separate problem.

Yep, don't mean to hold this up for that one.

Thanks for the reviewed-by.

		Amit

Re: [Qemu-devel] [PATCH v3] virtio-rng: Add human-readable error message for negative max-bytes parameter

[John Snow]

On 07/22/2014 07:48 AM, Amit Shah wrote:
> On (Tue) 22 Jul 2014 [13:41:43], Markus Armbruster wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>>
>>> On (Mon) 21 Jul 2014 [17:44:37], John Snow wrote:
>>>> If a negative integer is used for the max_bytes parameter, QEMU currently
>>>> calls abort() and leaves behind a core dump. This patch adds a simple
>>>> error message to make the reason for the termination clearer.
>>>>
>>>> There is an underlying insufficiency in the parameter parsing code of QEMU
>>>> that renders it unable to reject negative values for unsigned properties,
>>>> thus the error message "a non-negative integer below 2^63" is the most
>>>> user-friendly and correct message we can give until the underlying
>>>> insufficiency is corrected.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>> ---
>>>> v3: Adjusted the error message to be more semantically meaningful, but
>>>> while acknowledging the limitations of the current unsigned integer
>>>> parsing routines.
>>>>
>>>>   hw/virtio/virtio-rng.c | 8 +++++++-
>>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
>>>> index 1356aca..7c5a675 100644
>>>> --- a/hw/virtio/virtio-rng.c
>>>> +++ b/hw/virtio/virtio-rng.c
>>>> @@ -181,7 +181,13 @@ static void virtio_rng_device_realize(DeviceState *dev, Error **errp)
>>>>   
>>>>       vrng->vq = virtio_add_queue(vdev, 8, handle_input);
>>>>   
>>>> -    assert(vrng->conf.max_bytes <= INT64_MAX);
>>>> +    /* Workaround: Property parsing does not enforce unsigned integers,
>>>> +     * So this is a hack to reject such numbers. */
>>>> +    if (vrng->conf.max_bytes > INT64_MAX) {
>>>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "max-bytes",
>>>> +                  "a non-negative integer below 2^63");
>>> Huh, why do we allow 0?  There's no reason to have 0 as a max-bytes
>>> value as well...
>> Could be treated as separate problem.
> Yep, don't mean to hold this up for that one.
>
> Thanks for the reviewed-by.
>
> 		Amit
Yes, 0 makes no sense, but there are a lot of extremely low values that 
cause problems. The current release allows you to input 0 so I left it 
as-is. The decision for what a reasonable minimum might be is perhaps up 
to the user, unless a better technical limit is found (like 1K? 2K? 4K?)

We could also change parsing for this property to use the "size" 
attribute (instead of unsigned integers) to allow users to specify e.g, 
4K/ms or 16K/ms and so on. It changes the nature of the sign problem for 
this property, though that problem for parsing in general still needs to 
be addressed in a future release.

Thanks :)

Re: [Qemu-devel] [PATCH v3] virtio-rng: Add human-readable error message for negative max-bytes parameter

[Amit Shah]

On (Tue) 22 Jul 2014 [11:30:28], John Snow wrote:
> 
> On 07/22/2014 07:48 AM, Amit Shah wrote:

> >>>>-    assert(vrng->conf.max_bytes <= INT64_MAX);
> >>>>+    /* Workaround: Property parsing does not enforce unsigned integers,
> >>>>+     * So this is a hack to reject such numbers. */
> >>>>+    if (vrng->conf.max_bytes > INT64_MAX) {
> >>>>+        error_set(errp, QERR_INVALID_PARAMETER_VALUE, "max-bytes",
> >>>>+                  "a non-negative integer below 2^63");
> >>>Huh, why do we allow 0?  There's no reason to have 0 as a max-bytes
> >>>value as well...
> >>Could be treated as separate problem.
> >Yep, don't mean to hold this up for that one.
> >
> >Thanks for the reviewed-by.
>
> Yes, 0 makes no sense, but there are a lot of extremely low values that
> cause problems.

0 makes no sense, but other low values (even 1) is just a very frugal
host admin trying to preserve his entropy pool.  But for the guest,
something is better than nothing.

I don't see how such low values would cause problems.

> The current release allows you to input 0 so I left it
> as-is.

Yes, the right thing to do for this patch.

> The decision for what a reasonable minimum might be is perhaps up to
> the user, unless a better technical limit is found (like 1K? 2K? 4K?)

That's policy, and we should leave that to the admins.

		Amit