[libvirt] [PATCH] qemu: monitor: Add check that monitor is non-null to specific commands
Martin Kletzander
mkletzan at redhat.com
Tue Apr 14 12:09:45 UTC 2015
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150414/489ebe54/attachment-0001.sig>
More information about the libvir-list
mailing list