Re: [PATCH] qemu: forbid large value wraparound in balloon period collection

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Feb 26, 2015 at 05:42:10PM +0100, Erik Skultety wrote:


On 02/25/2015 08: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.

Well stats collection period is only used by QEMU, but the issue here is
it's quite impossible to do it unified for all drivers, possibly at one
place, because you can't do it right after the domain gets parsed
(otherwise domain disappears after daemon's restart) and we already do
check this at client's side (both virsh cmd routine and our public API
virDomainSetMemoryStatsPeriod). However, this patch doesn't fix the
issue anyway, as the check is at a wrong place.


You're right, I didn't know we have checks in APIs on the way already.

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!).

Will include fix for this one in the next versions :).


Great!  That'd be nice (although it's not an error per se).

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.

We can't do much about this, can we?


Sure we can, the question is whether that makes sense.  I'd either
shout at the time when qemu is starting the domain and fail when int
period <= 0 or ignore such values quietly (with VIR_DEBUG() at most).

When it comes to where to check this, it'd be best to check this
inside qemuMonitorSetMemoryStatsPeriod(), but beware!  We are screwing
up the error messages all over the place and leaving them, duplicating
them, etc.  So even though I'd accept a simple fix in that function,
I'd like to see (at least later on) some cleanup to this.

Thank you for your review :),
Erik

You're welcome ;)

Attachment: pgp4SnbZDEngk.pgp
Description: PGP signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]