| Message ID | 1405979077-18163-1-git-send-email-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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
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>
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.
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
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 :)
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
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,
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(-)