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

Re: [libvirt] [PATCH] qemu: monitor: Add check that monitor is non-null to specific commands



On Fri, Apr 10, 2015 at 04:45:09PM +0200, Peter Krempa wrote:
Our monitor locking and cleanup code is weird as it allows to remove the
monitor object pointer while we are in the monitor. Additionally EVERY
api is using the vm private data after @vm was unlocked. This leads to
an unpleasant situation, where when qemu crashes during a monitor
session while a command is executed (and locks are down), the priv->mon
pointer gets cleared (@vm is unlocked).

The next monitor call then crashes on commands that did not check if the
monitor is non-NULL.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1209813 and few
other possible crashes
---
Since this is a workaround rather than a proper fix I'd normally not go this
way, but untangling the monitor mess won't be easy so I'll fix the symptoms
first.

src/qemu/qemu_monitor.c | 42 ++++++++++++++++++++++++++++++++++++++++++
src/qemu/qemu_monitor.h | 12 +++++-------
2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 2b0e1a5..e8403be 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1171,6 +1171,12 @@ qemuMonitorUpdateVideoMemorySize(qemuMonitorPtr mon,
    int ret = -1;
    char *path = NULL;

+    if (!mon) {
+        virReportError(VIR_ERR_INVALID_ARG, "%s",
+                       _("monitor must not be NULL"));
+        return -1;
+    }
+
    if (mon->json) {
        ret = qemuMonitorFindObjectPath(mon, "/", videoName, &path);
        if (ret < 0) {
@@ -1886,6 +1892,12 @@ qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon,
{
    VIR_DEBUG("mon=%p, stats=%p, backing=%d", mon, stats, backingChain);

+    if (!mon) {
+        virReportError(VIR_ERR_INVALID_ARG, "%s",
+                       _("monitor must not be NULL"));
+        return -1;
+    }
+
    if (!mon->json) {
        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
                       _("block capacity/size info requires JSON monitor"));
@@ -3246,6 +3258,12 @@ qemuMonitorAddObject(qemuMonitorPtr mon,
              mon, type, objalias, props);
    int ret = -1;

+    if (!mon) {
+        virReportError(VIR_ERR_INVALID_ARG, "%s",
+                       _("monitor must not be NULL"));
+        return -1;
+    }
+
    if (mon->json) {
        ret = qemuMonitorJSONAddObject(mon, type, objalias, props);
    } else {
@@ -3265,6 +3283,12 @@ qemuMonitorDelObject(qemuMonitorPtr mon,
    VIR_DEBUG("mon=%p objalias=%s", mon, objalias);
    int ret = -1;

+    if (!mon) {
+        virReportError(VIR_ERR_INVALID_ARG, "%s",
+                       _("monitor must not be NULL"));
+        return -1;
+    }
+
    if (mon->json)
        ret = qemuMonitorJSONDelObject(mon, objalias);
    else
@@ -3453,6 +3477,12 @@ qemuMonitorBlockCommit(qemuMonitorPtr mon, const char *device,
              "bandwidth=%llu",
              mon, device, top, base, NULLSTR(backingName), bandwidth);

+    if (!mon) {
+        virReportError(VIR_ERR_INVALID_ARG, "%s",
+                       _("monitor must not be NULL"));
+        return -1;
+    }
+
    if (mon->json)
        ret = qemuMonitorJSONBlockCommit(mon, device, top, base,
                                         backingName, bandwidth);
@@ -3482,6 +3512,12 @@ qemuMonitorDiskNameLookup(qemuMonitorPtr mon,
                          virStorageSourcePtr top,
                          virStorageSourcePtr target)
{
+    if (!mon) {
+        virReportError(VIR_ERR_INVALID_ARG, "%s",
+                       _("monitor must not be NULL"));
+        return NULL;
+    }
+
    if (!mon->json) {
        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
                       _("JSON monitor is required"));
@@ -3592,6 +3628,12 @@ qemuMonitorBlockJob(qemuMonitorPtr mon,
              mon, device, NULLSTR(base), NULLSTR(backingName),
              bandwidth, mode, modern);

+    if (!mon) {
+        virReportError(VIR_ERR_INVALID_ARG, "%s",
+                       _("monitor must not be NULL"));
+        return -1;
+    }
+
    if (mon->json)
        ret = qemuMonitorJSONBlockJob(mon, device, base, backingName,
                                      bandwidth, mode, modern);
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index a3514cf..7e62139 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -246,7 +246,7 @@ void qemuMonitorSetOptions(qemuMonitorPtr mon, virJSONValuePtr options)
int qemuMonitorUpdateVideoMemorySize(qemuMonitorPtr mon,
                                     virDomainVideoDefPtr video,
                                     const char *videName)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
int qemuMonitorHMPCommandWithFd(qemuMonitorPtr mon,
                                const char *cmd,
                                int scm_fd,
@@ -381,7 +381,7 @@ int qemuMonitorGetAllBlockStatsInfo(qemuMonitorPtr mon,
int qemuMonitorBlockStatsUpdateCapacity(qemuMonitorPtr mon,
                                        virHashTablePtr stats,
                                        bool backingChain)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
+    ATTRIBUTE_NONNULL(2);

int qemuMonitorGetBlockExtent(qemuMonitorPtr mon,
                              const char *dev_name,
@@ -730,15 +730,13 @@ int qemuMonitorBlockCommit(qemuMonitorPtr mon,
                           const char *base,
                           const char *backingName,
                           unsigned long long bandwidth)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
-    ATTRIBUTE_NONNULL(4);
+    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);
bool qemuMonitorSupportsActiveCommit(qemuMonitorPtr mon);
char *qemuMonitorDiskNameLookup(qemuMonitorPtr mon,
                                const char *device,
                                virStorageSourcePtr top,
                                virStorageSourcePtr target)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3)
-    ATTRIBUTE_NONNULL(4);
+    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);

int qemuMonitorArbitraryCommand(qemuMonitorPtr mon,
                                const char *cmd,

You could've updated the ArbitraryCommand too.  I haven't checked you
fixed all of them, but most of them could use a run of attribute
clean-ups ;)

@@ -774,7 +772,7 @@ int qemuMonitorBlockJobInfo(qemuMonitorPtr mon,
                            const char *device,
                            virDomainBlockJobInfoPtr info,
                            unsigned long long *bandwidth)
-    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);
+    ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3);


You removed the attribute but haven't fixed the function itself.

ACK with at least functions qemuMonitorBlockJobInfo() and
qemuMonitorArbitraryCommand() fixed as well.

Pity there isn't an easy way of testing all the functions with NULLs.

Attachment: signature.asc
Description: PGP signature


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