Re: [libvirt] [PATCH v2] qemu: Fix crash in virDomainMemoryStats with old qemu
On 02/05/2014 07:58 AM, John Ferlan wrote:
On 02/05/2014 08:19 AM, Jiri Denemark wrote:
> If virDomainMemoryStats was run on a domain with virtio balloon driver
> running on an old qemu which supports QMP but does not support qom-list
> QMP command, libvirtd would crash. The reason is we did not check if
> qemuMonitorJSONGetObjectListPaths failed and moreover we even stored its
> result in an unsigned integer type.
>
> Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
> ---
>
> Notes:
> version 2:
> - use signed type for i and j to avoid comparison between signed and
> unsigned types; gcc-- for not complaining about it
>
Returning 0 from this function isn't necessarily bad - it means not
found still looking... It's a recursive nightmare.
On the recursive path - we can't recurse unless qom-list existed in the
first place - either qemu is new enough or it is not. So exiting with
-1 avoids the recursion, and if we DO recurse, we wouldn't be failing
because of a missing qom-list.
As for the non recursive callers...
For the qemuMonitorGetMemoryStats() path that's OK - it's allowed to
fallback to trying the "older" former method of calling
"query-balloon"
in qemuMonitorJSONGetMemoryStats(). Returning -1 if "qom-list" isn't
found means we won't go the fallback route.
Let's look at the callers:
ignore_value(qemuMonitorFindBalloonObjectPath(mon, mon->vm, "/"));
mon->ballooninit = true;
ret = qemuMonitorJSONGetMemoryStats(mon, mon->balloonpath,
stats, nr_stats);
so we don't care whether it returns -1 or 0 or 1; we only care whether
mon->balloonpath was set (it is set if we returned 1; and there are no
stats to get if it returns -1 or 0).
For the qemuMonitorSetMemoryStatsPeriod() returning 0 means don't even
try to set.
But again, the code does:
if (qemuMonitorFindBalloonObjectPath(mon, mon->vm, "/") == 1) {
ret = qemuMonitorJSONSetMemoryStatsPeriod(mon, mon->balloonpath,
period);
}
so we don't care about the difference between -1 and 0. The only place
that cares about the difference is in recursion, but I already argued we
aren't recursing if qom-list is missing.
>
> for (i = 0; i < npaths && ret == 0; i++) {
>
> @@ -1061,6 +1063,11 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon,
> * then this version of qemu/kvm does not support the feature.
> */
> nprops = qemuMonitorJSONGetObjectListPaths(mon, nextpath, &bprops);
> + if (nprops < 0) {
> + ret = -1;
> + goto cleanup;
> + }
> +
Failure here wouldn't be because 'qom-list' doesn't exist, rather there
was some other property error or malformed return object. Since not
finding "guest-stats-polling-interval" property for a
"link<virtio-balloon-pci>" object.
Indeed - the only way to fail here if the outer loop succeeded is for a
failure unrelated to qom-list not existing. But it is still failure.
After the for loop that error is reported. So if nprops <= 0, then we
fall through to that. The other errors are still logged (right?), but
we report the error below.
> for (j = 0; j < nprops; j++) {
> if (STREQ(bprops[j]->name,
"guest-stats-polling-interval")) {
> VIR_DEBUG("Found Balloon Object Path %s", nextpath);
>
FWIW: To Dan's comment - not sure how simple it would be to add a test
for this condition.
I'm still thinking the change in types is all that is necessary as there
is cause for this function to return 0 if "qom-list" doesn't exist.
I see no problem with returning -1 if qom-list doesn't exist. I'm happy
with the patch as-is:
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachments:
- signature.asc (application/pgp-signature — 604 bytes)