[libvirt] [PATCH] blkdeviotune: check for overflow when parsing XML

[Martin Kletzander]

On Tue, Aug 26, 2014 at 10:17:22AM +0200, Erik Skultety wrote:
>According to docs/schemas/domaincommon.rng and _virDomainBlockIoTuneInfo
>all the iotune values are interpreted as unsigned long long, however
>according to qemu_monitor_json.c, qemu silently truncates numbers
>larger than LLONG_MAX. There's really not much of a usage for such
>large numbers anyway yet. This patch provides the same overflow
>check during domain XML parsing phase as it does during setting
>a blkdeviotune element in qemu_driver.c and thus reports an error when
>a larger number than LLONG_MAX is detected.
>---
> src/conf/domain_conf.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 22a7f7e..4cc900f 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -5711,6 +5711,18 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt,
>                     def->blkdeviotune.write_iops_sec = 0;
>                 }
>
>+                if (def->blkdeviotune.total_bytes_sec > LLONG_MAX ||
>+                    def->blkdeviotune.read_bytes_sec > LLONG_MAX ||
>+                    def->blkdeviotune.write_bytes_sec > LLONG_MAX ||
>+                    def->blkdeviotune.total_iops_sec > LLONG_MAX ||
>+                    def->blkdeviotune.read_iops_sec > LLONG_MAX ||
>+                    def->blkdeviotune.write_iops_sec > LLONG_MAX) {
>+                    virReportError(VIR_ERR_OVERFLOW,
>+                                  _("block I/O throttle limit must "
>+                                    "be less than %llu"), LLONG_MAX);
>+                    goto error;
>+                }
>+
>                 if ((def->blkdeviotune.total_bytes_sec &&
>                      def->blkdeviotune.read_bytes_sec) ||
>                     (def->blkdeviotune.total_bytes_sec &&
>--
>1.9.3
>

Oh, c**p, so if someone has that in the XML, then such domain will
disappear with the upgrade of libvirt daemon, right.

We should instead put this in the starting phase (and any API that
touches that data, hotplug, blkdeviotune, etc.), so such domains don't
disappear.  Fortunately that makes sense, too, because only QEMU
truncates the value, but in case any other hypervisor may accept the
values properly (in the future for example).

Test case in tests/qemuxml2argvtest.c could be nice way to show that
this is fixed ;)

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140826/ef96b617/attachment-0001.sig>