Re: [PATCH v2] qemu: Fix crash in virDomainMemoryStats with old qemu

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

 



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@xxxxxxxxxx>
>> ---
>>
>> 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

Attachment: signature.asc
Description: OpenPGP digital 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]