[libvirt] [PATCH v2] qemu: Fix crash in virDomainMemoryStats with old qemu
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@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 src/qemu/qemu_monitor.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a968901..a2769db 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1019,7 +1019,7 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon, virDomainObjPtr vm, const char *curpath) { - size_t i, j, npaths = 0, nprops = 0; + ssize_t i, j, npaths = 0, nprops = 0; int ret = 0; char *nextpath = NULL; qemuMonitorJSONListPathPtr *paths = NULL; @@ -1045,6 +1045,8 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon, VIR_DEBUG("Searching for Balloon Object Path starting at %s", curpath); npaths = qemuMonitorJSONGetObjectListPaths(mon, curpath, &paths); + if (npaths < 0) + return -1; 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; + } + for (j = 0; j < nprops; j++) { if (STREQ(bprops[j]->name, "guest-stats-polling-interval")) { VIR_DEBUG("Found Balloon Object Path %s", nextpath); -- 1.8.5.3
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@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
src/qemu/qemu_monitor.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a968901..a2769db 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1019,7 +1019,7 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon, virDomainObjPtr vm, const char *curpath) { - size_t i, j, npaths = 0, nprops = 0; + ssize_t i, j, npaths = 0, nprops = 0; int ret = 0; char *nextpath = NULL; qemuMonitorJSONListPathPtr *paths = NULL; @@ -1045,6 +1045,8 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon, VIR_DEBUG("Searching for Balloon Object Path starting at %s", curpath);
npaths = qemuMonitorJSONGetObjectListPaths(mon, curpath, &paths); + if (npaths < 0) + return -1;
Returning 0 from this function isn't necessarily bad - it means not found still looking... It's a recursive nightmare. 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. For the qemuMonitorSetMemoryStatsPeriod() returning 0 means don't even try to set.
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. 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. John
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@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
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@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
On Wed, Feb 05, 2014 at 15:12:03 -0700, 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@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
... I see no problem with returning -1 if qom-list doesn't exist. I'm happy with the patch as-is:
ACK.
Thanks and pushed. Jirka
On 2014-02-05 14:19, 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@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
src/qemu/qemu_monitor.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a968901..a2769db 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1019,7 +1019,7 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon, virDomainObjPtr vm, const char *curpath) { - size_t i, j, npaths = 0, nprops = 0; + ssize_t i, j, npaths = 0, nprops = 0; int ret = 0; char *nextpath = NULL; qemuMonitorJSONListPathPtr *paths = NULL; @@ -1045,6 +1045,8 @@ qemuMonitorFindBalloonObjectPath(qemuMonitorPtr mon, VIR_DEBUG("Searching for Balloon Object Path starting at %s", curpath);
npaths = qemuMonitorJSONGetObjectListPaths(mon, curpath, &paths); + if (npaths < 0) + return -1;
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; + } + for (j = 0; j < nprops; j++) { if (STREQ(bprops[j]->name, "guest-stats-polling-interval")) { VIR_DEBUG("Found Balloon Object Path %s", nextpath);
I tested this patch and so far it seems ok: libvirtd hasn't crashed during 1 hour. I'll leave it running during the night to be sure. I only see this now in the logs: 2014-02-05 14:54:41.280+0000: 8104: error : qemuMonitorJSONCheckError:354 : internal error: unable to execute QEMU command 'qom-list': The command qom-list has not been found 2014-02-05 14:54:41.306+0000: 8103: error : qemuMonitorJSONCheckError:354 : internal error: unable to execute QEMU command 'qom-list': The command qom-list has not been found 2014-02-05 14:54:41.333+0000: 8106: error : qemuMonitorJSONCheckError:354 : internal error: unable to execute QEMU command 'qom-list': The command qom-list has not been found 2014-02-05 14:54:41.358+0000: 8105: error : qemuMonitorJSONCheckError:354 : internal error: unable to execute QEMU command 'qom-list': The command qom-list has not been found but no crashes because of that. Franky
participants (4)
-
Eric Blake -
Franky Van Liedekerke -
Jiri Denemark -
John Ferlan