6:38 p.m.
From: "Daniel P. Berrange" <berrange(a)redhat.com>
The lifetime of the virDomainEventState object is tied to
the lifetime of the driver, which in stateless drivers is
tied to the lifetime of the virConnectPtr.
If we add & remove a timer when allocating/freeing the
virDomainEventState object, we can get a situation where
the timer still triggers once after virDomainEventState
has been freed. The timeout callback can't keep a ref
on the event state though, since that would be a circular
reference.
The trick is to only register the timer when a callback
is registered with the event state & remove the timer
when the callback is unregistered.
The demo for the bug is to run
while true ; do date ; ../tools/virsh -q -c test:///default 'shutdown test; undefine
test; dominfo test' ; done
prior to this fix, it will frequently hang and / or
crash, or corrupt memory
---
src/conf/domain_event.c | 87 ++++++++++++++++++++++++++++++++------------
src/conf/domain_event.h | 2 +-
src/libxl/libxl_driver.c | 2 +-
src/lxc/lxc_driver.c | 2 +-
src/qemu/qemu_driver.c | 2 +-
src/remote/remote_driver.c | 2 +-
src/test/test_driver.c | 2 +-
src/uml/uml_driver.c | 2 +-
src/vbox/vbox_tmpl.c | 2 +-
src/xen/xen_driver.c | 2 +-
10 files changed, 72 insertions(+), 33 deletions(-)
diff --git a/src/conf/domain_event.c b/src/conf/domain_event.c
index da99d9f..26b4967 100644
--- a/src/conf/domain_event.c
+++ b/src/conf/domain_event.c
@@ -631,13 +631,9 @@ static void virDomainEventTimer(int timer ATTRIBUTE_UNUSED, void
*opaque)
/**
* virDomainEventStateNew:
- * @requireTimer: If true, return an error if registering the timer fails.
- * This is fatal for drivers that sit behind the daemon
- * (qemu, lxc), since there should always be a event impl
- * registered.
*/
virDomainEventStatePtr
-virDomainEventStateNew(bool requireTimer)
+virDomainEventStateNew(void)
{
virDomainEventStatePtr state = NULL;
@@ -658,23 +654,10 @@ virDomainEventStateNew(bool requireTimer)
goto error;
}
- if (!(state->queue = virDomainEventQueueNew())) {
+ if (!(state->queue = virDomainEventQueueNew()))
goto error;
- }
- if ((state->timer = virEventAddTimeout(-1,
- virDomainEventTimer,
- state,
- NULL)) < 0) {
- if (requireTimer == false) {
- VIR_DEBUG("virEventAddTimeout failed: No addTimeoutImpl defined. "
- "continuing without events.");
- } else {
- eventReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("could not initialize domain event timer"));
- goto error;
- }
- }
+ state->timer = -1;
return state;
@@ -1311,7 +1294,6 @@ virDomainEventStateFlush(virDomainEventStatePtr state)
state->queue->count = 0;
state->queue->events = NULL;
virEventUpdateTimeout(state->timer, -1);
- virDomainEventStateUnlock(state);
virDomainEventQueueDispatch(&tempQueue,
state->callbacks,
@@ -1319,7 +1301,6 @@ virDomainEventStateFlush(virDomainEventStatePtr state)
state);
/* Purge any deleted callbacks */
- virDomainEventStateLock(state);
virDomainEventCallbackListPurgeMarked(state->callbacks);
state->isDispatching = false;
@@ -1346,10 +1327,32 @@ int virDomainEventStateRegister(virConnectPtr conn,
void *opaque,
virFreeCallback freecb)
{
- int ret;
+ int ret = -1;
+
virDomainEventStateLock(state);
+
+ if ((state->callbacks->count == 0) &&
+ (state->timer == -1) &&
+ (state->timer = virEventAddTimeout(-1,
+ virDomainEventTimer,
+ state,
+ NULL)) < 0) {
+ eventReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("could not initialize domain event timer"));
+ goto cleanup;
+ }
+
ret = virDomainEventCallbackListAdd(conn, state->callbacks,
callback, opaque, freecb);
+
+ if (ret == -1 &&
+ state->callbacks->count == 0 &&
+ state->timer != -1) {
+ virEventRemoveTimeout(state->timer);
+ state->timer = -1;
+ }
+
+cleanup:
virDomainEventStateUnlock(state);
return ret;
}
@@ -1379,11 +1382,33 @@ int virDomainEventStateRegisterID(virConnectPtr conn,
virFreeCallback freecb,
int *callbackID)
{
- int ret;
+ int ret = -1;
+
virDomainEventStateLock(state);
+
+ if ((state->callbacks->count == 0) &&
+ (state->timer == -1) &&
+ (state->timer = virEventAddTimeout(-1,
+ virDomainEventTimer,
+ state,
+ NULL)) < 0) {
+ eventReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("could not initialize domain event timer"));
+ goto cleanup;
+ }
+
ret = virDomainEventCallbackListAddID(conn, state->callbacks,
dom, eventID, cb, opaque, freecb,
callbackID);
+
+ if (ret == -1 &&
+ state->callbacks->count == 0 &&
+ state->timer != -1) {
+ virEventRemoveTimeout(state->timer);
+ state->timer = -1;
+ }
+
+cleanup:
virDomainEventStateUnlock(state);
return ret;
}
@@ -1413,6 +1438,13 @@ virDomainEventStateDeregister(virConnectPtr conn,
state->callbacks, callback);
else
ret = virDomainEventCallbackListRemove(conn, state->callbacks, callback);
+
+ if (state->callbacks->count == 0 &&
+ state->timer != -1) {
+ virEventRemoveTimeout(state->timer);
+ state->timer = -1;
+ }
+
virDomainEventStateUnlock(state);
return ret;
}
@@ -1443,6 +1475,13 @@ virDomainEventStateDeregisterID(virConnectPtr conn,
else
ret = virDomainEventCallbackListRemoveID(conn,
state->callbacks, callbackID);
+
+ if (state->callbacks->count == 0 &&
+ state->timer != -1) {
+ virEventRemoveTimeout(state->timer);
+ state->timer = -1;
+ }
+
virDomainEventStateUnlock(state);
return ret;
}
diff --git a/src/conf/domain_event.h b/src/conf/domain_event.h
index a6c3562..0e7cd75 100644
--- a/src/conf/domain_event.h
+++ b/src/conf/domain_event.h
@@ -119,7 +119,7 @@ void virDomainEventFree(virDomainEventPtr event);
void virDomainEventStateFree(virDomainEventStatePtr state);
virDomainEventStatePtr
-virDomainEventStateNew(bool requireTimer);
+virDomainEventStateNew(void);
void
virDomainEventStateQueue(virDomainEventStatePtr state,
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 04392da..0500ed0 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -928,7 +928,7 @@ libxlStartup(int privileged) {
}
VIR_FREE(log_file);
- libxl_driver->domainEventState = virDomainEventStateNew(true);
+ libxl_driver->domainEventState = virDomainEventStateNew();
if (!libxl_driver->domainEventState)
goto error;
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
index 6d32ed2..3baff19 100644
--- a/src/lxc/lxc_driver.c
+++ b/src/lxc/lxc_driver.c
@@ -2418,7 +2418,7 @@ static int lxcStartup(int privileged)
if (virDomainObjListInit(&lxc_driver->domains) < 0)
goto cleanup;
- lxc_driver->domainEventState = virDomainEventStateNew(true);
+ lxc_driver->domainEventState = virDomainEventStateNew();
if (!lxc_driver->domainEventState)
goto cleanup;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b2a8525..5a876de 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -431,7 +431,7 @@ qemudStartup(int privileged) {
goto out_of_memory;
/* Init domain events */
- qemu_driver->domainEventState = virDomainEventStateNew(true);
+ qemu_driver->domainEventState = virDomainEventStateNew();
if (!qemu_driver->domainEventState)
goto error;
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 03bd424..f266574 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -726,7 +726,7 @@ doRemoteOpen (virConnectPtr conn,
}
}
- if (!(priv->domainEventState = virDomainEventStateNew(false)))
+ if (!(priv->domainEventState = virDomainEventStateNew()))
goto failed;
/* Successful. */
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 5ec4190..9e00b1c 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1137,7 +1137,7 @@ static virDrvOpenStatus testOpen(virConnectPtr conn,
privconn = conn->privateData;
testDriverLock(privconn);
- privconn->domainEventState = virDomainEventStateNew(false);
+ privconn->domainEventState = virDomainEventStateNew();
if (!privconn->domainEventState) {
testDriverUnlock(privconn);
testClose(conn);
diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c
index 360f0ce..671216e 100644
--- a/src/uml/uml_driver.c
+++ b/src/uml/uml_driver.c
@@ -413,7 +413,7 @@ umlStartup(int privileged)
if (virDomainObjListInit(¨_driver->domains) < 0)
goto error;
- uml_driver->domainEventState = virDomainEventStateNew(true);
+ uml_driver->domainEventState = virDomainEventStateNew();
if (!uml_driver->domainEventState)
goto error;
diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index a10f5ae..0339123 100644
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -1034,7 +1034,7 @@ static virDrvOpenStatus vboxOpen(virConnectPtr conn,
#else /* !(VBOX_API_VERSION == 2002) */
- if (!(data->domainEvents = virDomainEventStateNew(true))) {
+ if (!(data->domainEvents = virDomainEventStateNew())) {
vboxUninitialize(data);
return VIR_DRV_OPEN_ERROR;
}
diff --git a/src/xen/xen_driver.c b/src/xen/xen_driver.c
index ab49c2b..20671c0 100644
--- a/src/xen/xen_driver.c
+++ b/src/xen/xen_driver.c
@@ -325,7 +325,7 @@ xenUnifiedOpen (virConnectPtr conn, virConnectAuthPtr auth, unsigned
int flags)
return VIR_DRV_OPEN_ERROR;
}
- if (!(priv->domainEvents = virDomainEventStateNew(true))) {
+ if (!(priv->domainEvents = virDomainEventStateNew())) {
virMutexDestroy(&priv->lock);
VIR_FREE(priv);
return VIR_DRV_OPEN_ERROR;
--
1.7.7.4