Re: [libvirt] [PATCH v2] qemu: Fix crash in virDomainMemoryStats with old qemu
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
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