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

[Erik Skultety]

Thank you very much for review. You're right, except for the part about 
adding a test case I think, it's fine to do it locally, but you can't 
predict data type sizes on all architectures, so in terms of creating a 
static XML description, you can't rely on a specific number being 
long/short enough to pass/fail the test.

On 08/26/2014 11:46 AM, Martin Kletzander wrote:
> 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
>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>