3:24 a.m.
While looking at a use-after-free situation going through how the QEMU
monitor is set up I noticed some things. These cleanups and the fix
for the use-after-free are the result of that.
Marc Hartmayer (5):
qemu: Fix two use-after-free situations
qemu: Turn qemuDomainLogContext into virObject
qemu: Implement qemuMonitorRegister()
qemu: remove ATTRIBUTE_UNUSED in qemuProcessHandleMonitorEOF
refactoring: Use the return value of virObjectRef directly
src/datatypes.c | 6 ++--
src/qemu/qemu_domain.c | 72 ++++++++++++++++++++------------------
src/qemu/qemu_domain.h | 2 --
src/qemu/qemu_monitor.c | 82 ++++++++++++++++++++++++++++++++++----------
src/qemu/qemu_monitor.h | 6 ++++
src/qemu/qemu_process.c | 12 +++----
src/rpc/virnetclientstream.c | 4 +--
src/rpc/virnetserver.c | 9 ++---
tests/qemumonitortestutils.c | 3 +-
9 files changed, 121 insertions(+), 75 deletions(-)
--
2.5.5
3:24 a.m.
New subject: [libvirt] [PATCH 1/5] qemu: Fix two use-after-free situations
There were multiple race conditions that could lead to segmentation
faults. The first precondition for this is qemuProcessLaunch must fail
sometime shortly after starting the new QEMU process. The second
precondition for the segmentation faults is that the new QEMU process
dies - or to be more precise the QEMU monitor has to be closed
irregularly. If both happens during qemuProcessStart (starting a
domain) there are race windows between the thread with the event
loop (T1) and the thread that is starting the domain (T2).
First segmentation fault scenario:
If qemuProcessLaunch fails during qemuProcessStart the code branches
to the 'stop' path where 'qemuMonitorSetDomainLog(priv->mon, NULL,
NULL, NULL)' will set the log function of the monitor to NULL (done in
T2). In the meantime the event loop of T1 will wake up with an EOF
event for the QEMU monitor because the QEMU process has died. The
crash occurs if T1 has checked 'mon->logFunc != NULL' in qemuMonitorIO
just before the logFunc was set to NULL by T2. If this situation
occurs T1 will try to call mon->logFunc which leads to the
segmentation fault.
Solution:
Require the monitor lock for setting the log function.
Backtrace:
0 0x0000000000000000 in ?? ()
1 0x000003ffe9e45316 in qemuMonitorIO (watch=<optimized out>,
fd=<optimized out>, events=<optimized out>, opaque=0x3ffe08aa860) at
../../src/qemu/qemu_monitor.c:727
2 0x000003fffda2e1a4 in virEventPollDispatchHandles (nfds=<optimized
out>, fds=0x2aa000fd980) at ../../src/util/vireventpoll.c:508
3 0x000003fffda2e398 in virEventPollRunOnce () at
../../src/util/vireventpoll.c:657
4 0x000003fffda2ca10 in virEventRunDefaultImpl () at
../../src/util/virevent.c:314
5 0x000003fffdba9366 in virNetDaemonRun (dmn=0x2aa000cc550) at
../../src/rpc/virnetdaemon.c:818
6 0x000002aa00024668 in main (argc=<optimized out>, argv=<optimized
out>) at ../../daemon/libvirtd.c:1541
Second segmentation fault scenario:
If qemuProcessLaunch fails it will unref the log context and with
invoking qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL)
qemuDomainLogContextFree() will be invoked. qemuDomainLogContextFree()
invokes virNetClientClose() to close the client and cleans everything
up (including unref of _virLogManager.client) when virNetClientClose()
returns. When T1 is now trying to report 'qemu unexpectedly closed the
monitor' libvirtd will crash because the client has already been
freed.
Solution:
As the critical section in qemuMonitorIO is protected with the monitor
lock we can use the same solution as proposed for the first
segmentation fault.
Backtrace:
0 virClassIsDerivedFrom (klass=0x3100979797979797,
parent=0x2aa000d92f0) at ../../src/util/virobject.c:169
1 0x000003fffda659e6 in virObjectIsClass (anyobj=<optimized out>,
klass=<optimized out>) at ../../src/util/virobject.c:365
2 0x000003fffda65a24 in virObjectLock (anyobj=0x3ffe08c1db0) at
../../src/util/virobject.c:317
3 0x000003fffdba4688 in
virNetClientIOEventLoop (client=client@entry=0x3ffe08c1db0,
thiscall=thiscall@entry=0x2aa000fbfa0) at
../../src/rpc/virnetclient.c:1668
4 0x000003fffdba4b4c in
virNetClientIO (client=client@entry=0x3ffe08c1db0,
thiscall=0x2aa000fbfa0) at ../../src/rpc/virnetclient.c:1944
5 0x000003fffdba4d42 in
virNetClientSendInternal (client=client@entry=0x3ffe08c1db0,
msg=msg@entry=0x2aa000cc710, expectReply=expectReply@entry=true,
nonBlock=nonBlock@entry=false) at ../../src/rpc/virnetclient.c:2116
6 0x000003fffdba6268 in
virNetClientSendWithReply (client=0x3ffe08c1db0, msg=0x2aa000cc710) at
../../src/rpc/virnetclient.c:2144
7 0x000003fffdba6e8e in virNetClientProgramCall (prog=0x3ffe08c1120,
client=<optimized out>, serial=<optimized out>, proc=<optimized out>,
noutfds=<optimized out>, outfds=0x0, ninfds=0x0, infds=0x0,
args_filter=0x3fffdb64440
<xdr_virLogManagerProtocolDomainReadLogFileArgs>, args=0x3ffffffe010,
ret_filter=0x3fffdb644c0
<xdr_virLogManagerProtocolDomainReadLogFileRet>, ret=0x3ffffffe008) at
../../src/rpc/virnetclientprogram.c:329
8 0x000003fffdb64042 in
virLogManagerDomainReadLogFile (mgr=<optimized out>, path=<optimized
out>, inode=<optimized out>, offset=<optimized out>, maxlen=<optimized
out>, flags=0) at ../../src/logging/log_manager.c:272
9 0x000003ffe9e0315c in qemuDomainLogContextRead (ctxt=0x3ffe08c2980,
msg=0x3ffffffe1c0) at ../../src/qemu/qemu_domain.c:4422
10 0x000003ffe9e280a8 in qemuProcessReadLog (logCtxt=<optimized out>,
msg=msg@entry=0x3ffffffe288) at ../../src/qemu/qemu_process.c:1800
11 0x000003ffe9e28206 in qemuProcessReportLogError (logCtxt=<optimized
out>, msgprefix=0x3ffe9ec276a "qemu unexpectedly closed the monitor")
at ../../src/qemu/qemu_process.c:1836
12 0x000003ffe9e28306 in
qemuProcessMonitorReportLogError (mon=mon@entry=0x3ffe085cf10,
msg=<optimized out>, opaque=<optimized out>) at
../../src/qemu/qemu_process.c:1856
13 0x000003ffe9e452b6 in qemuMonitorIO (watch=<optimized out>,
fd=<optimized out>, events=<optimized out>, opaque=0x3ffe085cf10) at
../../src/qemu/qemu_monitor.c:726
14 0x000003fffda2e1a4 in virEventPollDispatchHandles (nfds=<optimized
out>, fds=0x2aa000fd980) at ../../src/util/vireventpoll.c:508
15 0x000003fffda2e398 in virEventPollRunOnce () at
../../src/util/vireventpoll.c:657
16 0x000003fffda2ca10 in virEventRunDefaultImpl () at
../../src/util/virevent.c:314
17 0x000003fffdba9366 in virNetDaemonRun (dmn=0x2aa000cc550) at
../../src/rpc/virnetdaemon.c:818
18 0x000002aa00024668 in main (argc=<optimized out>, argv=<optimized
out>) at ../../daemon/libvirtd.c:1541
Other code parts where the same problem was possible to occur are
fixed as well (qemuMigrationFinish, qemuProcessStart, and
qemuDomainSaveImageStartVM).
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reported-by: Sascha Silbe <silbe(a)linux.vnet.ibm.com>
---
src/qemu/qemu_monitor.c | 44 ++++++++++++++++++++++++++++++++++----------
src/qemu/qemu_monitor.h | 4 ++++
2 files changed, 38 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index a4fa6ec..b41aaed 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -963,7 +963,7 @@ qemuMonitorClose(qemuMonitorPtr mon)
PROBE(QEMU_MONITOR_CLOSE,
"mon=%p refs=%d", mon, mon->parent.parent.u.s.refs);
- qemuMonitorSetDomainLog(mon, NULL, NULL, NULL);
+ qemuMonitorSetDomainLogLocked(mon, NULL, NULL, NULL);
if (mon->fd >= 0) {
qemuMonitorUnregister(mon);
@@ -4035,20 +4035,21 @@ qemuMonitorGetDeviceAliases(qemuMonitorPtr mon,
/**
- * qemuMonitorSetDomainLog:
- * Set the file descriptor of the open VM log file to report potential
- * early startup errors of qemu.
- *
- * @mon: Monitor object to set the log file reading on
+ * qemuMonitorSetDomainLogLocked:
+ * @mon: Locked monitor object to set the log file reading on
* @func: the callback to report errors
* @opaque: data to pass to @func
* @destroy: optional callback to free @opaque
+ *
+ * Set the file descriptor of the open VM log file to report potential
+ * early startup errors of qemu. This function requires @mon to be
+ * locked already!
*/
void
-qemuMonitorSetDomainLog(qemuMonitorPtr mon,
- qemuMonitorReportDomainLogError func,
- void *opaque,
- virFreeCallback destroy)
+qemuMonitorSetDomainLogLocked(qemuMonitorPtr mon,
+ qemuMonitorReportDomainLogError func,
+ void *opaque,
+ virFreeCallback destroy)
{
if (mon->logDestroy && mon->logOpaque)
mon->logDestroy(mon->logOpaque);
@@ -4060,6 +4061,29 @@ qemuMonitorSetDomainLog(qemuMonitorPtr mon,
/**
+ * qemuMonitorSetDomainLog:
+ * @mon: Unlocked monitor object to set the log file reading on
+ * @func: the callback to report errors
+ * @opaque: data to pass to @func
+ * @destroy: optional callback to free @opaque
+ *
+ * Set the file descriptor of the open VM log file to report potential
+ * early startup errors of qemu. This functions requires @mon to be
+ * unlocked.
+ */
+void
+qemuMonitorSetDomainLog(qemuMonitorPtr mon,
+ qemuMonitorReportDomainLogError func,
+ void *opaque,
+ virFreeCallback destroy)
+{
+ virObjectLock(mon);
+ qemuMonitorSetDomainLogLocked(mon, func, opaque, destroy);
+ virObjectUnlock(mon);
+}
+
+
+/**
* qemuMonitorJSONGetGuestCPU:
* @mon: Pointer to the monitor
* @arch: arch of the guest
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 91ab905..2e42d16 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -1067,6 +1067,10 @@ int qemuMonitorGetDeviceAliases(qemuMonitorPtr mon,
typedef void (*qemuMonitorReportDomainLogError)(qemuMonitorPtr mon,
const char *msg,
void *opaque);
+void qemuMonitorSetDomainLogLocked(qemuMonitorPtr mon,
+ qemuMonitorReportDomainLogError func,
+ void *opaque,
+ virFreeCallback destroy);
void qemuMonitorSetDomainLog(qemuMonitorPtr mon,
qemuMonitorReportDomainLogError func,
void *opaque,
--
2.5.5
3:24 a.m.
New subject: [libvirt] [PATCH 2/5] qemu: Turn qemuDomainLogContext into virObject
This way qemuDomainLogContextRef() and qemuDomainLogContextFree() is
no longer needed. The naming qemuDomainLogContextFree() was also
somewhat misleading. Additionally, it's easier to turn
qemuDomainLogContext in a self-locking object.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
---
src/qemu/qemu_domain.c | 72 ++++++++++++++++++++++++++-----------------------
src/qemu/qemu_domain.h | 2 --
src/qemu/qemu_process.c | 10 +++----
3 files changed, 44 insertions(+), 40 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b733505..6be7a4e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -111,7 +111,8 @@ VIR_ENUM_IMPL(qemuDomainNamespace, QEMU_DOMAIN_NS_LAST,
struct _qemuDomainLogContext {
- int refs;
+ virObject parent;
+
int writefd;
int readfd; /* Only used if manager == NULL */
off_t pos;
@@ -120,6 +121,36 @@ struct _qemuDomainLogContext {
virLogManagerPtr manager;
};
+static virClassPtr qemuDomainLogContextClass;
+
+static void qemuDomainLogContextDispose(void *obj);
+
+static int
+qemuDomainLogContextOnceInit(void)
+{
+ if (!(qemuDomainLogContextClass = virClassNew(virClassForObject(),
+ "qemuDomainLogContext",
+ sizeof(qemuDomainLogContext),
+ qemuDomainLogContextDispose)))
+ return -1;
+
+ return 0;
+}
+
+VIR_ONCE_GLOBAL_INIT(qemuDomainLogContext)
+
+static void
+qemuDomainLogContextDispose(void *obj)
+{
+ qemuDomainLogContextPtr ctxt = obj;
+ VIR_DEBUG("ctxt=%p", ctxt);
+
+ virLogManagerFree(ctxt->manager);
+ VIR_FREE(ctxt->path);
+ VIR_FORCE_CLOSE(ctxt->writefd);
+ VIR_FORCE_CLOSE(ctxt->readfd);
+}
+
const char *
qemuDomainAsyncJobPhaseToString(qemuDomainAsyncJob job,
int phase ATTRIBUTE_UNUSED)
@@ -4175,7 +4206,7 @@ void qemuDomainObjTaint(virQEMUDriverPtr driver,
cleanup:
VIR_FREE(timestamp);
if (closeLog)
- qemuDomainLogContextFree(logCtxt);
+ virObjectUnref(logCtxt);
if (orig_err) {
virSetError(orig_err);
virFreeError(orig_err);
@@ -4287,13 +4318,15 @@ qemuDomainLogContextPtr qemuDomainLogContextNew(virQEMUDriverPtr
driver,
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
qemuDomainLogContextPtr ctxt = NULL;
- if (VIR_ALLOC(ctxt) < 0)
- goto error;
+ if (qemuDomainLogContextInitialize() < 0)
+ goto cleanup;
+
+ if (!(ctxt = virObjectNew(qemuDomainLogContextClass)))
+ goto cleanup;
VIR_DEBUG("Context new %p stdioLogD=%d", ctxt, cfg->stdioLogD);
ctxt->writefd = -1;
ctxt->readfd = -1;
- virAtomicIntSet(&ctxt->refs, 1);
if (virAsprintf(&ctxt->path, "%s/%s.log", cfg->logDir,
vm->def->name) < 0)
goto error;
@@ -4361,7 +4394,7 @@ qemuDomainLogContextPtr qemuDomainLogContextNew(virQEMUDriverPtr
driver,
return ctxt;
error:
- qemuDomainLogContextFree(ctxt);
+ virObjectUnref(ctxt);
ctxt = NULL;
goto cleanup;
}
@@ -4530,39 +4563,12 @@ void qemuDomainLogContextMarkPosition(qemuDomainLogContextPtr
ctxt)
}
-void qemuDomainLogContextRef(qemuDomainLogContextPtr ctxt)
-{
- VIR_DEBUG("Context ref %p", ctxt);
- virAtomicIntInc(&ctxt->refs);
-}
-
-
virLogManagerPtr qemuDomainLogContextGetManager(qemuDomainLogContextPtr ctxt)
{
return ctxt->manager;
}
-void qemuDomainLogContextFree(qemuDomainLogContextPtr ctxt)
-{
- bool lastRef;
-
- if (!ctxt)
- return;
-
- lastRef = virAtomicIntDecAndTest(&ctxt->refs);
- VIR_DEBUG("Context free %p lastref=%d", ctxt, lastRef);
- if (!lastRef)
- return;
-
- virLogManagerFree(ctxt->manager);
- VIR_FREE(ctxt->path);
- VIR_FORCE_CLOSE(ctxt->writefd);
- VIR_FORCE_CLOSE(ctxt->readfd);
- VIR_FREE(ctxt);
-}
-
-
/* Locate an appropriate 'qemu-img' binary. */
const char *
qemuFindQemuImgBinary(virQEMUDriverPtr driver)
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 91573ff..caac5d5 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -540,8 +540,6 @@ ssize_t qemuDomainLogContextRead(qemuDomainLogContextPtr ctxt,
char **msg);
int qemuDomainLogContextGetWriteFD(qemuDomainLogContextPtr ctxt);
void qemuDomainLogContextMarkPosition(qemuDomainLogContextPtr ctxt);
-void qemuDomainLogContextRef(qemuDomainLogContextPtr ctxt);
-void qemuDomainLogContextFree(qemuDomainLogContextPtr ctxt);
virLogManagerPtr qemuDomainLogContextGetManager(qemuDomainLogContextPtr ctxt);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index e450d06..028f0c5 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1692,7 +1692,7 @@ static void
qemuProcessMonitorLogFree(void *opaque)
{
qemuDomainLogContextPtr logCtxt = opaque;
- qemuDomainLogContextFree(logCtxt);
+ virObjectUnref(logCtxt);
}
static int
@@ -1731,7 +1731,7 @@ qemuConnectMonitor(virQEMUDriverPtr driver, virDomainObjPtr vm, int
asyncJob,
driver);
if (mon && logCtxt) {
- qemuDomainLogContextRef(logCtxt);
+ virObjectRef(logCtxt);
qemuMonitorSetDomainLog(mon,
qemuProcessMonitorReportLogError,
logCtxt,
@@ -5871,7 +5871,7 @@ qemuProcessLaunch(virConnectPtr conn,
cleanup:
qemuDomainSecretDestroy(vm);
virCommandFree(cmd);
- qemuDomainLogContextFree(logCtxt);
+ virObjectUnref(logCtxt);
virObjectUnref(cfg);
virObjectUnref(caps);
VIR_FREE(nicindexes);
@@ -6667,7 +6667,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
goto error;
}
- qemuDomainLogContextFree(logCtxt);
+ virObjectUnref(logCtxt);
VIR_FREE(seclabel);
VIR_FREE(sec_managers);
virObjectUnref(cfg);
@@ -6687,7 +6687,7 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED,
qemuMonitorClose(priv->mon);
priv->mon = NULL;
- qemuDomainLogContextFree(logCtxt);
+ virObjectUnref(logCtxt);
VIR_FREE(seclabel);
VIR_FREE(sec_managers);
if (seclabelgen)
--
2.5.5
7:52 a.m.
New subject: [libvirt] [PATCH 2/5] qemu: Turn qemuDomainLogContext into virObject
On 04/03/2017 10:24 AM, Marc Hartmayer wrote:
This way qemuDomainLogContextRef() and qemuDomainLogContextFree() is
no longer needed. The naming qemuDomainLogContextFree() was also
somewhat misleading. Additionally, it's easier to turn
qemuDomainLogContext in a self-locking object.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
---
src/qemu/qemu_domain.c | 72 ++++++++++++++++++++++++++-----------------------
src/qemu/qemu_domain.h | 2 --
src/qemu/qemu_process.c | 10 +++----
3 files changed, 44 insertions(+), 40 deletions(-)
Ha ha, why we've had reimplemeneted virObject anyway? :-) Nice catch.
Michal
3:24 a.m.
New subject: [libvirt] [PATCH 3/5] qemu: Implement qemuMonitorRegister()
Implement qemuMonitorRegister() as there is already a
qemuMonitorUnregister() function. This way it may be easier to
understand the code paths.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
---
src/qemu/qemu_monitor.c | 38 +++++++++++++++++++++++++++++---------
src/qemu/qemu_monitor.h | 2 ++
2 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index b41aaed..34037ac 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -837,15 +837,7 @@ qemuMonitorOpenInternal(virDomainObjPtr vm,
virObjectLock(mon);
- virObjectRef(mon);
- if ((mon->watch = virEventAddHandle(mon->fd,
- VIR_EVENT_HANDLE_HANGUP |
- VIR_EVENT_HANDLE_ERROR |
- VIR_EVENT_HANDLE_READABLE,
- qemuMonitorIO,
- mon,
- virObjectFreeCallback)) < 0) {
- virObjectUnref(mon);
+ if (!qemuMonitorRegister(mon)) {
virObjectUnlock(mon);
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("unable to register monitor events"));
@@ -944,6 +936,34 @@ qemuMonitorOpenFD(virDomainObjPtr vm,
}
+/**
+ * qemuMonitorRegister:
+ * @mon: QEMU monitor
+ *
+ * Registers the monitor in the event loop. The caller has to hold the
+ * lock for @mon.
+ *
+ * Returns true in case of success, false otherwise
+ */
+bool
+qemuMonitorRegister(qemuMonitorPtr mon)
+{
+ virObjectRef(mon);
+ if ((mon->watch = virEventAddHandle(mon->fd,
+ VIR_EVENT_HANDLE_HANGUP |
+ VIR_EVENT_HANDLE_ERROR |
+ VIR_EVENT_HANDLE_READABLE,
+ qemuMonitorIO,
+ mon,
+ virObjectFreeCallback)) < 0) {
+ virObjectUnref(mon);
+ return false;
+ }
+
+ return true;
+}
+
+
void
qemuMonitorUnregister(qemuMonitorPtr mon)
{
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 2e42d16..12f98be 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -296,6 +296,8 @@ qemuMonitorPtr qemuMonitorOpenFD(virDomainObjPtr vm,
void *opaque)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(4);
+bool qemuMonitorRegister(qemuMonitorPtr mon)
+ ATTRIBUTE_NONNULL(1);
void qemuMonitorUnregister(qemuMonitorPtr mon)
ATTRIBUTE_NONNULL(1);
void qemuMonitorClose(qemuMonitorPtr mon);
--
2.5.5
3:24 a.m.
New subject: [libvirt] [PATCH 4/5] qemu: remove ATTRIBUTE_UNUSED in qemuProcessHandleMonitorEOF
This attribute is not needed here, since @mon is in use.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
---
src/qemu/qemu_process.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 028f0c5..c060847 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -279,7 +279,7 @@ qemuConnectAgent(virQEMUDriverPtr driver, virDomainObjPtr vm)
* performed
*/
static void
-qemuProcessHandleMonitorEOF(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
+qemuProcessHandleMonitorEOF(qemuMonitorPtr mon,
virDomainObjPtr vm,
void *opaque)
{
--
2.5.5
3:24 a.m.
New subject: [libvirt] [PATCH 5/5] refactoring: Use the return value of virObjectRef directly
Use the return value of virObjectRef directly. This way, it's easier
for another reader to identify the reason why the additional reference
is required.
Signed-off-by: Marc Hartmayer <mhartmay(a)linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk(a)linux.vnet.ibm.com>
---
src/datatypes.c | 6 ++----
src/rpc/virnetclientstream.c | 4 +---
src/rpc/virnetserver.c | 9 +++------
tests/qemumonitortestutils.c | 3 +--
4 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/src/datatypes.c b/src/datatypes.c
index 3e3148d..59ba956 100644
--- a/src/datatypes.c
+++ b/src/datatypes.c
@@ -196,8 +196,7 @@ void
virConnectCloseCallbackDataRegister(virConnectCloseCallbackDataPtr closeDat
return;
}
- closeData->conn = conn;
- virObjectRef(closeData->conn);
+ closeData->conn = virObjectRef(conn);
closeData->callback = cb;
closeData->opaque = opaque;
closeData->freeCallback = freecb;
@@ -985,8 +984,7 @@
virAdmConnectCloseCallbackDataRegister(virAdmConnectCloseCallbackDataPtr cbdata,
goto cleanup;
}
- virObjectRef(conn);
- cbdata->conn = conn;
+ cbdata->conn = virObjectRef(conn);
cbdata->callback = cb;
cbdata->opaque = opaque;
cbdata->freeCallback = freecb;
diff --git a/src/rpc/virnetclientstream.c b/src/rpc/virnetclientstream.c
index 34989a9..2105bd0 100644
--- a/src/rpc/virnetclientstream.c
+++ b/src/rpc/virnetclientstream.c
@@ -145,12 +145,10 @@ virNetClientStreamPtr virNetClientStreamNew(virNetClientProgramPtr
prog,
if (!(st = virObjectLockableNew(virNetClientStreamClass)))
return NULL;
- st->prog = prog;
+ st->prog = virObjectRef(prog);
st->proc = proc;
st->serial = serial;
- virObjectRef(prog);
-
return st;
}
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index f06643a..c02db74 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -213,8 +213,7 @@ static int virNetServerDispatchNewMessage(virNetServerClientPtr
client,
job->msg = msg;
if (prog) {
- virObjectRef(prog);
- job->prog = prog;
+ job->prog = virObjectRef(prog);
priority = virNetServerProgramGetPriority(prog, msg->header.proc);
}
@@ -284,8 +283,7 @@ int virNetServerAddClient(virNetServerPtr srv,
if (VIR_EXPAND_N(srv->clients, srv->nclients, 1) < 0)
goto error;
- srv->clients[srv->nclients-1] = client;
- virObjectRef(client);
+ srv->clients[srv->nclients-1] = virObjectRef(client);
if (virNetServerClientNeedAuth(client))
virNetServerTrackPendingAuthLocked(srv);
@@ -695,8 +693,7 @@ int virNetServerAddService(virNetServerPtr srv,
}
}
- srv->services[srv->nservices-1] = svc;
- virObjectRef(svc);
+ srv->services[srv->nservices-1] = virObjectRef(svc);
virNetServerServiceSetDispatcher(svc,
virNetServerDispatchNewClient,
diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c
index 89857a6..5e30fb0 100644
--- a/tests/qemumonitortestutils.c
+++ b/tests/qemumonitortestutils.c
@@ -1064,8 +1064,7 @@ qemuMonitorCommonTestNew(virDomainXMLOptionPtr xmlopt,
goto error;
if (vm) {
- virObjectRef(vm);
- test->vm = vm;
+ test->vm = virObjectRef(vm);
} else {
test->vm = virDomainObjNew(xmlopt);
if (!test->vm)
--
2.5.5
7:52 a.m.
On 04/03/2017 10:24 AM, Marc Hartmayer wrote:
While looking at a use-after-free situation going through how the
QEMU
monitor is set up I noticed some things. These cleanups and the fix
for the use-after-free are the result of that.
Marc Hartmayer (5):
qemu: Fix two use-after-free situations
qemu: Turn qemuDomainLogContext into virObject
qemu: Implement qemuMonitorRegister()
qemu: remove ATTRIBUTE_UNUSED in qemuProcessHandleMonitorEOF
refactoring: Use the return value of virObjectRef directly
src/datatypes.c | 6 ++--
src/qemu/qemu_domain.c | 72 ++++++++++++++++++++------------------
src/qemu/qemu_domain.h | 2 --
src/qemu/qemu_monitor.c | 82 ++++++++++++++++++++++++++++++++++----------
src/qemu/qemu_monitor.h | 6 ++++
src/qemu/qemu_process.c | 12 +++----
src/rpc/virnetclientstream.c | 4 +--
src/rpc/virnetserver.c | 9 ++---
tests/qemumonitortestutils.c | 3 +-
9 files changed, 121 insertions(+), 75 deletions(-)
ACKed and pushed. Thanks.
Michal
3:43 a.m.
On Mon, Apr 10, 2017 at 02:52 PM +0200, Michal Privoznik <mprivozn(a)redhat.com>
wrote:
On 04/03/2017 10:24 AM, Marc Hartmayer wrote:
> While looking at a use-after-free situation going through how the QEMU
> monitor is set up I noticed some things. These cleanups and the fix
> for the use-after-free are the result of that.
>
> Marc Hartmayer (5):
> qemu: Fix two use-after-free situations
> qemu: Turn qemuDomainLogContext into virObject
> qemu: Implement qemuMonitorRegister()
> qemu: remove ATTRIBUTE_UNUSED in qemuProcessHandleMonitorEOF
> refactoring: Use the return value of virObjectRef directly
>
> src/datatypes.c | 6 ++--
> src/qemu/qemu_domain.c | 72 ++++++++++++++++++++------------------
> src/qemu/qemu_domain.h | 2 --
> src/qemu/qemu_monitor.c | 82 ++++++++++++++++++++++++++++++++++----------
> src/qemu/qemu_monitor.h | 6 ++++
> src/qemu/qemu_process.c | 12 +++----
> src/rpc/virnetclientstream.c | 4 +--
> src/rpc/virnetserver.c | 9 ++---
> tests/qemumonitortestutils.c | 3 +-
> 9 files changed, 121 insertions(+), 75 deletions(-)
>
ACKed and pushed. Thanks.
Thanks.
Michal
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
2775
days inactive
2790
days old
8 comments
2 participants
participants (2)
-
Marc Hartmayer -
Michal Privoznik