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

[John Ferlan]

On 02/05/2014 05:12 PM, Eric Blake wrote:
> 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 at 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.
> 
> 

I agree - I started going through this, got interrupted by some meeting
:-), then dealt with the 8"+ of snow outside in my driveway, and well
got sidetracked a bit.

John