On 02/25/2015 02:10 AM, Martin Kletzander wrote: > On Tue, Feb 24, 2015 at 04:28:18PM +0100, Erik Skultety wrote: >> We do parse and represent period collection as unsigned int in our >> internal structures, however commit >> d5c67e7f4523450023b89b69c16472582c85eeaf converts this to int, thus >> wrapping >> around inputs greater than INT_MAX which results in an error from QEMU. >> This patch adds a check into QEMU driver, because period attribute is >> only supported by QEMU. >> > > Well, there are couple more things broken. > > 1) Change to this patch: > It is written in the description that period can me 0 or more, so > it would make sense to guard all the drivers (ideally at one > place) together. If that changes, the check can be moved to > individual drivers, but for now anything outside <0,INT_MAX> > doesn't make sense. correct... for that matter a really long period doesn't make much sense, but if someone wants one, then they can have it... > > 2) Curiosity: > <membaloon model='virtio'> > <stats period='5'/> > <address .../> > </membaloon> > > This ^^ fails validation because <stats/> must come *after* > <address/> (WAT) even though all other device elements *require* > the address to be last (why would we even do that!). Probably because I was still getting used to the XML/RNG formatting/syntax "rules".... This would be easily dealt with by an <interleave .../>, right? Then again, review missed it too ;-) > > 3) Invalid number gets still parsed in the XML and it only wraps to > negative when sending to the monitor at the guest's start. > The input parsing for that value was created (or copied from some other attribute) before the virStrToLong_ui[p] API's were created... I think at the time those API's were created there was a comment about maybe spending some cycles looking at all the various parsed 'int' or 'uint' attributes to convert them to use the newer API, but that never got anywhere. John > > Since these are all small things, I expect them to be fixed as well > (be sure the BZ will come back anyway if you don't do that :) ), > although not necessarily in the same commit. One note: we must be > able to parse old XMLs, so you can either reject the number at start > (more pain then security) or you can just make period < 0 behave like > period == 0 or many other variants. You might even wrap everything to > unsigned as it is unsigned in qemu anyways. The other thing you can't > do is to fix our API without creating a new one. I wonder if wraping > negative values could be written in the docs so we have more options. > But anyway, who's going to request stats gathering period greater than > 68 years except QE, right? ;) > >> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1140958 >> --- >> src/qemu/qemu_driver.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index bec05d4..46bd880 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -2414,6 +2414,12 @@ static int >> qemuDomainSetMemoryStatsPeriod(virDomainPtr dom, int period, >> /* Set the balloon driver collection interval */ >> priv = vm->privateData; >> >> + if (period < 0) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("invalid value for collection period")); >> + goto endjob; >> + } >> + >> if (flags & VIR_DOMAIN_AFFECT_LIVE) { >> >> qemuDomainObjEnterMonitor(driver, vm); >> -- >> 1.9.3 >> >> -- >> libvir-list mailing list >> libvir-list@xxxxxxxxxx >> https://www.redhat.com/mailman/listinfo/libvir-list > > > -- > libvir-list mailing list > libvir-list@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list