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.