Re: [PATCH] qemu: cleanup error checking on agent replies

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

 



On Tue, Apr 01, 2014 at 08:26:57AM -0600, Eric Blake wrote:
On 04/01/2014 07:22 AM, Martin Kletzander wrote:
On all the places where qemuAgentComand() was called, we did a check
for errors in the reply.  Unfortunately, some of the places called
qemuAgentCheckError() without checking for non-null reply which might
have resulted in a crash.

So this patch makes the error-checking part of qemuAgentCommand()
itself, which:

 a) makes it look better,

 b) makes the check mandatory and, most importantly,

 c) checks for the errors if and only if it is appropriate.

This actually fixes a potential crashers when qemuAgentComand()
returned 0, but reply was NULL.  Having said that, it *should* fix the
following bug:

https://bugzilla.redhat.com/show_bug.cgi?id=1058149

Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
---
 src/qemu/qemu_agent.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)



+static int qemuAgentCheckError(virJSONValuePtr cmd, virJSONValuePtr reply);
+

Is it worth hoisting this function into topological order, so we don't
need a forward declaration?   But that's better as a followup patch
(no-op code motion should be separate from refactoring).

ACK


Thank you, I've pushed it.  Would the following suffice as the mentioned
follow-up?  Or do you want me to send a new standalone patch for that?

Martin

diff --git i/src/qemu/qemu_agent.c w/src/qemu/qemu_agent.c
index 4c83d7d..92573bd 100644
--- i/src/qemu/qemu_agent.c
+++ w/src/qemu/qemu_agent.c
@@ -117,8 +117,6 @@ struct _qemuAgent {
    qemuAgentEvent await_event;
};

-static int qemuAgentCheckError(virJSONValuePtr cmd, virJSONValuePtr reply);
-
static virClassPtr qemuAgentClass;
static void qemuAgentDispose(void *obj);

@@ -967,61 +965,6 @@ qemuAgentGuestSync(qemuAgentPtr mon)
    return ret;
}

-static int
-qemuAgentCommand(qemuAgentPtr mon,
-                 virJSONValuePtr cmd,
-                 virJSONValuePtr *reply,
-                 int seconds)
-{
-    int ret = -1;
-    qemuAgentMessage msg;
-    char *cmdstr = NULL;
-    int await_event = mon->await_event;
-
-    *reply = NULL;
-
-    if (qemuAgentGuestSync(mon) < 0)
-        return -1;
-
-    memset(&msg, 0, sizeof(msg));
-
-    if (!(cmdstr = virJSONValueToString(cmd, false)))
-        goto cleanup;
-    if (virAsprintf(&msg.txBuffer, "%s" LINE_ENDING, cmdstr) < 0)
-        goto cleanup;
-    msg.txLength = strlen(msg.txBuffer);
-
-    VIR_DEBUG("Send command '%s' for write, seconds = %d", cmdstr, seconds);
-
-    ret = qemuAgentSend(mon, &msg, seconds);
-
-    VIR_DEBUG("Receive command reply ret=%d rxObject=%p",
-              ret, msg.rxObject);
-
-    if (ret == 0) {
-        /* If we haven't obtained any reply but we wait for an
-         * event, then don't report this as error */
-        if (!msg.rxObject) {
-            if (await_event) {
-                VIR_DEBUG("Woken up by event %d", await_event);
-            } else {
-                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                               _("Missing monitor reply object"));
-                ret = -1;
-            }
-        } else {
-            *reply = msg.rxObject;
-            ret = qemuAgentCheckError(cmd, *reply);
-        }
-    }
-
- cleanup:
-    VIR_FREE(cmdstr);
-    VIR_FREE(msg.txBuffer);
-
-    return ret;
-}
-
static const char *
qemuAgentStringifyErrorClass(const char *klass)
{
@@ -1133,6 +1076,61 @@ qemuAgentCheckError(virJSONValuePtr cmd,
    return 0;
}

+static int
+qemuAgentCommand(qemuAgentPtr mon,
+                 virJSONValuePtr cmd,
+                 virJSONValuePtr *reply,
+                 int seconds)
+{
+    int ret = -1;
+    qemuAgentMessage msg;
+    char *cmdstr = NULL;
+    int await_event = mon->await_event;
+
+    *reply = NULL;
+
+    if (qemuAgentGuestSync(mon) < 0)
+        return -1;
+
+    memset(&msg, 0, sizeof(msg));
+
+    if (!(cmdstr = virJSONValueToString(cmd, false)))
+        goto cleanup;
+    if (virAsprintf(&msg.txBuffer, "%s" LINE_ENDING, cmdstr) < 0)
+        goto cleanup;
+    msg.txLength = strlen(msg.txBuffer);
+
+    VIR_DEBUG("Send command '%s' for write, seconds = %d", cmdstr, seconds);
+
+    ret = qemuAgentSend(mon, &msg, seconds);
+
+    VIR_DEBUG("Receive command reply ret=%d rxObject=%p",
+              ret, msg.rxObject);
+
+    if (ret == 0) {
+        /* If we haven't obtained any reply but we wait for an
+         * event, then don't report this as error */
+        if (!msg.rxObject) {
+            if (await_event) {
+                VIR_DEBUG("Woken up by event %d", await_event);
+            } else {
+                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                               _("Missing monitor reply object"));
+                ret = -1;
+            }
+        } else {
+            *reply = msg.rxObject;
+            ret = qemuAgentCheckError(cmd, *reply);
+        }
+    }
+
+ cleanup:
+    VIR_FREE(cmdstr);
+    VIR_FREE(msg.txBuffer);
+
+    return ret;
+}
+
static virJSONValuePtr ATTRIBUTE_SENTINEL
qemuAgentMakeCommand(const char *cmdname,
                     ...)
--

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