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&gt; >> --- >> >> 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.