[libvirt] [PATCH 0/2] Fix numeric overflow in qemu block job handling
The JSON generator and qemu QMP protocol limit numeric values to LLONG_MAX even for unsigned values. A bug in the code caused numeric overflow when passing through speed parameters to qemu block job commands. Background: https://bugzilla.redhat.com/show_bug.cgi?id=927156 Peter Krempa (2): qemu-JSON: Error out if number is out of range instead of overflowing to negative qemu-blockjob: Fix limit of bandwidth for block jobs to supported value src/qemu/qemu_monitor.c | 21 ++++++++++++--------- src/qemu/qemu_monitor_json.c | 10 ++++++++-- 2 files changed, 20 insertions(+), 11 deletions(-) -- 1.8.1.5
Commit 78eb8b60d59662271c4a9a1be8c9002ee84dc8cf works around qemu's inability to parse unsigned 64 bit integers by representing them as signed. This introduces a bug where if the requested integer is greater than LLONG_MAX the result is wrapped to negative numbers. This patch adds a check to avoid the wrap for unsigned numbers and error out rather than passing them along. --- src/qemu/qemu_monitor_json.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 1bf8baf..6cc21ee 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -460,10 +460,16 @@ qemuMonitorJSONMakeCommandRaw(bool wrap, const char *cmdname, ...) case 'U': { /* qemu silently truncates numbers larger than LLONG_MAX, * so passing the full range of unsigned 64 bit integers - * is not safe here. Pass them as signed 64 bit integers - * instead. + * is not safe here. Limit them to LLONG_MAX. */ long long val = va_arg(args, long long); + if (val < 0) { + virReportError(VIR_ERR_OVERFLOW, + _("Value of '%s' can't be represented in JSON: " + "value too big (%llu > %lld)"), + key, val, LLONG_MAX); + goto error; + } ret = virJSONValueObjectAppendNumberLong(jargs, key, val); } break; case 'd': { -- 1.8.1.5
On 04/03/2013 02:46 AM, Peter Krempa wrote:
Commit 78eb8b60d59662271c4a9a1be8c9002ee84dc8cf works around qemu's inability to parse unsigned 64 bit integers by representing them as signed. This introduces a bug where if the requested integer is greater than LLONG_MAX the result is wrapped to negative numbers.
But then qemu, on the receiving end, turns the negative number back into the appropriate bit pattern for an unsigned value, which lets us get around the fact that passing a positive unsigned value > LLONG_MAX would be truncated.
This patch adds a check to avoid the wrap for unsigned numbers and error out rather than passing them along. --- src/qemu/qemu_monitor_json.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
This would prevent us from sending bit patterns that qemu will interpret correctly, even if the JSON used in sending the bit pattern is expressed as a negative number, where we could previously send such a pattern. I'm inclined to NACK this patch; unless you have a more concrete example of qemu no longer interpreting a negative string as the correct unsigned bit value. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/03/13 15:29, Eric Blake wrote:
On 04/03/2013 02:46 AM, Peter Krempa wrote:
Commit 78eb8b60d59662271c4a9a1be8c9002ee84dc8cf works around qemu's inability to parse unsigned 64 bit integers by representing them as signed. This introduces a bug where if the requested integer is greater than LLONG_MAX the result is wrapped to negative numbers.
But then qemu, on the receiving end, turns the negative number back into the appropriate bit pattern for an unsigned value, which lets us get around the fact that passing a positive unsigned value > LLONG_MAX would be truncated.
Hmmm, then we should at least comment that this is intentional and that qemu should be able to reconstruct the correct value from this as it looks wrong at the first look. Also qemu apparently uses this code wrong at least for the bandwidth limits for block-* operations.
This patch adds a check to avoid the wrap for unsigned numbers and error out rather than passing them along. --- src/qemu/qemu_monitor_json.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
This would prevent us from sending bit patterns that qemu will interpret correctly, even if the JSON used in sending the bit pattern is expressed
I wasn't aware that this is supposed to work this way and I neither learned that from the mail thread linked in the original patch. From the issue in the linked BZ I inferred that we are passing negative values to qemu and started to looking why that was happening.
as a negative number, where we could previously send such a pattern. I'm inclined to NACK this patch; unless you have a more concrete example of qemu no longer interpreting a negative string as the correct unsigned
I'm okay with not putting this in, but then I'll document that the signed value is intentional and correctly parsed (in some cases) in qemu.
bit value.
Peter
The JSON generator is able to represent only values less than LLONG_MAX, fix the bandwidth limit checks when converting to value to catch overflows before they reach the parser. --- src/qemu/qemu_monitor.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 30f7820..d1c6690 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2889,12 +2889,13 @@ qemuMonitorDriveMirror(qemuMonitorPtr mon, "flags=%x", mon, device, file, NULLSTR(format), bandwidth, flags); - /* Convert bandwidth MiB to bytes */ + /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is + * limited to LLONG_MAX also for unsigned values */ speed = bandwidth; - if (speed > ULLONG_MAX / 1024 / 1024) { + if (speed > LLONG_MAX >> 20) { virReportError(VIR_ERR_OVERFLOW, _("bandwidth must be less than %llu"), - ULLONG_MAX / 1024 / 1024); + LLONG_MAX >> 20); return -1; } speed <<= 20; @@ -2936,12 +2937,13 @@ qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device, VIR_DEBUG("mon=%p, device=%s, top=%s, base=%s, bandwidth=%ld", mon, device, NULLSTR(top), NULLSTR(base), bandwidth); - /* Convert bandwidth MiB to bytes */ + /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is + * limited to LLONG_MAX also for unsigned values */ speed = bandwidth; - if (speed > ULLONG_MAX / 1024 / 1024) { + if (speed > LLONG_MAX >> 20) { virReportError(VIR_ERR_OVERFLOW, _("bandwidth must be less than %llu"), - ULLONG_MAX / 1024 / 1024); + LLONG_MAX >> 20); return -1; } speed <<= 20; @@ -3056,12 +3058,13 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, "modern=%d", mon, device, NULLSTR(base), bandwidth, info, mode, modern); - /* Convert bandwidth MiB to bytes */ + /* Convert bandwidth MiB to bytes - unfortunately the JSON QMP protocol is + * limited to LLONG_MAX also for unsigned values */ speed = bandwidth; - if (speed > ULLONG_MAX / 1024 / 1024) { + if (speed > LLONG_MAX >> 20) { virReportError(VIR_ERR_OVERFLOW, _("bandwidth must be less than %llu"), - ULLONG_MAX / 1024 / 1024); + LLONG_MAX >> 20); return -1; } speed <<= 20; -- 1.8.1.5
On 04/03/2013 02:46 AM, Peter Krempa wrote:
The JSON generator is able to represent only values less than LLONG_MAX, fix the bandwidth limit checks when converting to value to catch overflows before they reach the parser. --- src/qemu/qemu_monitor.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
ACK. This one is safe in isolation, even if we drop 1/2. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/03/13 15:32, Eric Blake wrote:
On 04/03/2013 02:46 AM, Peter Krempa wrote:
The JSON generator is able to represent only values less than LLONG_MAX, fix the bandwidth limit checks when converting to value to catch overflows before they reach the parser. --- src/qemu/qemu_monitor.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)
ACK. This one is safe in isolation, even if we drop 1/2.
I pushed this one. Thanks for the review. Peter
participants (2)
-
Eric Blake -
Peter Krempa