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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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