7:51 a.m.
Rework patches/logic most recently posted here:
https://www.redhat.com/archives/libvir-list/2017-March/msg00072.html
John Ferlan (2):
daemon: Rework remoteClientFreeFunc cleanup loops into C macro
remote: Fix possible use-after-free when sending event message
daemon/remote.c | 158 +++++++++++++++++++-------------------------------------
1 file changed, 52 insertions(+), 106 deletions(-)
--
2.9.3
7:51 a.m.
New subject: [libvirt] [PATCH 1/2] daemon: Rework remoteClientFreeFunc cleanup loops into C macro
Rather than 'n' repetitive code segments, let's create a single macro
which will make the code easier to read.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
daemon/remote.c | 120 +++++++++++++++-----------------------------------------
1 file changed, 31 insertions(+), 89 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c
index 1c9708c..6fdafce 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -1623,6 +1623,24 @@ void remoteRelayConnectionClosedEvent(virConnectPtr conn
ATTRIBUTE_UNUSED, int r
&msg);
}
+#define CLEAN_EVENT_CALLBACK(conn, eventCallbacks, neventCallbacks, name) \
+ do { \
+ size_t i; \
+ for (i = 0; i < neventCallbacks; i++) { \
+ int callbackID = eventCallbacks[i]->callbackID; \
+ if (callbackID < 0) { \
+ VIR_WARN("unexpected incomplete %s callback %zu", name, i); \
+ continue; \
+ } \
+ VIR_DEBUG("Deregistering remote %s event relay %d", \
+ name, callbackID); \
+ eventCallbacks[i]->callbackID = -1; \
+ if (virConnectDomainEventDeregisterAny(conn, callbackID) < 0) \
+ VIR_WARN("unexpected %s event deregister failure", name); \
+ } \
+ VIR_FREE(eventCallbacks); \
+ } while (0);
+
/*
* You must hold lock for at least the client
* We don't free stuff here, merely disconnect the client's
@@ -1637,98 +1655,21 @@ void remoteClientFreeFunc(void *data)
/* Deregister event delivery callback */
if (priv->conn) {
virIdentityPtr sysident = virIdentityGetSystem();
- size_t i;
virIdentitySetCurrent(sysident);
- for (i = 0; i < priv->ndomainEventCallbacks; i++) {
- int callbackID = priv->domainEventCallbacks[i]->callbackID;
- if (callbackID < 0) {
- VIR_WARN("unexpected incomplete domain callback %zu", i);
- continue;
- }
- VIR_DEBUG("Deregistering remote domain event relay %d",
- callbackID);
- priv->domainEventCallbacks[i]->callbackID = -1;
- if (virConnectDomainEventDeregisterAny(priv->conn, callbackID) < 0)
- VIR_WARN("unexpected domain event deregister failure");
- }
- VIR_FREE(priv->domainEventCallbacks);
-
- for (i = 0; i < priv->nnetworkEventCallbacks; i++) {
- int callbackID = priv->networkEventCallbacks[i]->callbackID;
- if (callbackID < 0) {
- VIR_WARN("unexpected incomplete network callback %zu", i);
- continue;
- }
- VIR_DEBUG("Deregistering remote network event relay %d",
- callbackID);
- priv->networkEventCallbacks[i]->callbackID = -1;
- if (virConnectNetworkEventDeregisterAny(priv->conn,
- callbackID) < 0)
- VIR_WARN("unexpected network event deregister failure");
- }
- VIR_FREE(priv->networkEventCallbacks);
-
- for (i = 0; i < priv->nstorageEventCallbacks; i++) {
- int callbackID = priv->storageEventCallbacks[i]->callbackID;
- if (callbackID < 0) {
- VIR_WARN("unexpected incomplete storage pool callback %zu",
i);
- continue;
- }
- VIR_DEBUG("Deregistering remote storage pool event relay %d",
- callbackID);
- priv->storageEventCallbacks[i]->callbackID = -1;
- if (virConnectStoragePoolEventDeregisterAny(priv->conn,
- callbackID) < 0)
- VIR_WARN("unexpected storage pool event deregister failure");
- }
- VIR_FREE(priv->storageEventCallbacks);
-
- for (i = 0; i < priv->nnodeDeviceEventCallbacks; i++) {
- int callbackID = priv->nodeDeviceEventCallbacks[i]->callbackID;
- if (callbackID < 0) {
- VIR_WARN("unexpected incomplete node device callback %zu", i);
- continue;
- }
- VIR_DEBUG("Deregistering remote node device event relay %d",
- callbackID);
- priv->nodeDeviceEventCallbacks[i]->callbackID = -1;
- if (virConnectNodeDeviceEventDeregisterAny(priv->conn,
- callbackID) < 0)
- VIR_WARN("unexpected node device event deregister failure");
- }
- VIR_FREE(priv->nodeDeviceEventCallbacks);
-
- for (i = 0; i < priv->nsecretEventCallbacks; i++) {
- int callbackID = priv->secretEventCallbacks[i]->callbackID;
- if (callbackID < 0) {
- VIR_WARN("unexpected incomplete secret callback %zu", i);
- continue;
- }
- VIR_DEBUG("Deregistering remote secret event relay %d",
- callbackID);
- priv->secretEventCallbacks[i]->callbackID = -1;
- if (virConnectSecretEventDeregisterAny(priv->conn,
- callbackID) < 0)
- VIR_WARN("unexpected secret event deregister failure");
- }
- VIR_FREE(priv->secretEventCallbacks);
-
- for (i = 0; i < priv->nqemuEventCallbacks; i++) {
- int callbackID = priv->qemuEventCallbacks[i]->callbackID;
- if (callbackID < 0) {
- VIR_WARN("unexpected incomplete qemu monitor callback %zu",
i);
- continue;
- }
- VIR_DEBUG("Deregistering remote qemu monitor event relay %d",
- callbackID);
- priv->qemuEventCallbacks[i]->callbackID = -1;
- if (virConnectDomainQemuMonitorEventDeregister(priv->conn,
- callbackID) < 0)
- VIR_WARN("unexpected qemu monitor event deregister failure");
- }
- VIR_FREE(priv->qemuEventCallbacks);
+ CLEAN_EVENT_CALLBACK(priv->conn, priv->domainEventCallbacks,
+ priv->ndomainEventCallbacks, "domain");
+ CLEAN_EVENT_CALLBACK(priv->conn, priv->networkEventCallbacks,
+ priv->nnetworkEventCallbacks, "network");
+ CLEAN_EVENT_CALLBACK(priv->conn, priv->storageEventCallbacks,
+ priv->nstorageEventCallbacks, "storage");
+ CLEAN_EVENT_CALLBACK(priv->conn, priv->nodeDeviceEventCallbacks,
+ priv->nnodeDeviceEventCallbacks, "node
device");
+ CLEAN_EVENT_CALLBACK(priv->conn, priv->secretEventCallbacks,
+ priv->nsecretEventCallbacks, "secret");
+ CLEAN_EVENT_CALLBACK(priv->conn, priv->qemuEventCallbacks,
+ priv->nqemuEventCallbacks, "qemu monitor");
if (priv->closeRegistered) {
if (virConnectUnregisterCloseCallback(priv->conn,
@@ -1744,6 +1685,7 @@ void remoteClientFreeFunc(void *data)
VIR_FREE(priv);
}
+#undef CLEAN_EVENT_CALLBACK
static void remoteClientCloseFunc(virNetServerClientPtr client)
--
2.9.3
7:36 p.m.
New subject: [libvirt] [PATCH 1/2] daemon: Rework remoteClientFreeFunc cleanup loops into C macro
On 03/25/2017 08:51 AM, John Ferlan wrote:
Rather than 'n' repetitive code segments, let's create a
single macro
which will make the code easier to read.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
daemon/remote.c | 120 +++++++++++++++-----------------------------------------
1 file changed, 31 insertions(+), 89 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c
index 1c9708c..6fdafce 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -1623,6 +1623,24 @@ void remoteRelayConnectionClosedEvent(virConnectPtr conn
ATTRIBUTE_UNUSED, int r
&msg);
}
+#define CLEAN_EVENT_CALLBACK(conn, eventCallbacks, neventCallbacks, name) \
+ do { \
+ size_t i; \
+ for (i = 0; i < neventCallbacks; i++) { \
+ int callbackID = eventCallbacks[i]->callbackID; \
+ if (callbackID < 0) { \
+ VIR_WARN("unexpected incomplete %s callback %zu", name, i); \
+ continue; \
+ } \
+ VIR_DEBUG("Deregistering remote %s event relay %d", \
+ name, callbackID); \
+ eventCallbacks[i]->callbackID = -1; \
+ if (virConnectDomainEventDeregisterAny(conn, callbackID) < 0) \
This call is different for each loop, and you've made them all
identical. You need to make the deregister function an argument to the
macro.
Otherwise it seems like a nice de-duplication of code. ACK if you fix that.
7:51 a.m.
New subject: [libvirt] [PATCH 2/2] remote: Fix possible use-after-free when sending event message
Based upon an idea and some research by Wang King <king.wang(a)huawei.com>
and xinhua.Cao <caoxinhua(a)huawei.com>.
Since we're assigning the 'client' to our callback event lookaside list,
it's imperative that we grab a reference to the object; otherwise, when
the object is unref'd during virNetServerProcessClients when it's determined
that the virNetServerClientIsClosed and the memory is free'd before perhaps
the object event state callbacks are run. When a virObjectLock() is run,
before sending the message the following trace occurs;
#0 0x00007fda223d66d8 in virClassIsDerivedFrom
(klass=0xdeadbeef, parent=0x7fda24c81b40)
at util/virobject.c:169
#1 0x00007fda223d6a1e in virObjectIsClass
(anyobj=anyobj@entry=0x7fd9e575b400, klass=<optimized out>)
at util/virobject.c:365
#2 0x00007fda223d6a44 in virObjectLock
(anyobj=0x7fd9e575b400)
at util/virobject.c:317
#3 0x00007fda22507f71 in virNetServerClientSendMessage
(client=client@entry=0x7fd9e575b400, msg=msg@entry=0x7fd9ec30de90)
at rpc/virnetserverclient.c:1422
#4 0x00007fda230d714d in remoteDispatchObjectEventSend
(client=0x7fd9e575b400, program=0x7fda24c844e0, procnr=348,
proc=0x7fda2310e5e0 <xdr_remote_domain_event_callback_tunable_msg>,
data=0x7ffc3857fdb0)
at remote.c:3803
#5 0x00007fda230dd71b in remoteRelayDomainEventTunable
(conn=<optimized out>, dom=0x7fda27cd7660, params=0x7fda27f3aae0,
nparams=1,opaque=0x7fd9e6c99e00)
at remote.c:1033
#6 0x00007fda224484cb in virDomainEventDispatchDefaultFunc
(conn=0x7fda27cd0120, event=0x7fda2736ea00, cb=0x7fda230dd610
<remoteRelayDomainEventTunable>, cbopaque=0x7fd9e6c99e00)
at conf/domain_event.c:1910
#7 0x00007fda22446871 in virObjectEventStateDispatchCallbacks
(callbacks=<optimized out>, callbacks=<optimized out>,
event=0x7fda2736ea00,state=0x7fda24ca3960)
at conf/object_event.c:722
#8 virObjectEventStateQueueDispatch
(callbacks=0x7fda24c65800, queue=0x7ffc3857fe90, state=0x7fda24ca3960)
at conf/object_event.c:736
#9 virObjectEventStateFlush (state=0x7fda24ca3960)
at conf/object_event.c:814
#10 virObjectEventTimer (timer=<optimized out>, opaque=0x7fda24ca3960)
at conf/object_event.c:560
#11 0x00007fda223ae8b9 in virEventPollDispatchTimeouts ()
at util/vireventpoll.c:458
#12 virEventPollRunOnce ()
at util/vireventpoll.c:654
#13 0x00007fda223ad1d2 in virEventRunDefaultImpl ()
at util/virevent.c:314
#14 0x00007fda225046cd in virNetDaemonRun (dmn=0x7fda24c775c0)
at rpc/virnetdaemon.c:818
#15 0x00007fda230d6351 in main (argc=<optimized out>, argv=<optimized
out>)
at libvirtd.c:1623
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
daemon/remote.c | 38 +++++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c
index 6fdafce..6bb6239 100644
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -124,7 +124,11 @@ remoteDispatchObjectEventSend(virNetServerClientPtr client,
static void
remoteEventCallbackFree(void *opaque)
{
- VIR_FREE(opaque);
+ daemonClientEventCallbackPtr callback = opaque;
+ if (!callback)
+ return;
+ virObjectUnref(callback->client);
+ VIR_FREE(callback);
}
@@ -3845,7 +3849,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
ATTRIBUTE_UNUSED
*/
if (VIR_ALLOC(callback) < 0)
goto cleanup;
- callback->client = client;
+ callback->client = virObjectRef(client);
callback->eventID = VIR_DOMAIN_EVENT_ID_LIFECYCLE;
callback->callbackID = -1;
callback->legacy = true;
@@ -3872,7 +3876,7 @@ remoteDispatchConnectDomainEventRegister(virNetServerPtr server
ATTRIBUTE_UNUSED
rv = 0;
cleanup:
- VIR_FREE(callback);
+ remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virMutexUnlock(&priv->lock);
@@ -4080,7 +4084,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
goto cleanup;
- callback->client = client;
+ callback->client = virObjectRef(client);
callback->eventID = args->eventID;
callback->callbackID = -1;
callback->legacy = true;
@@ -4107,7 +4111,7 @@ remoteDispatchConnectDomainEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
rv = 0;
cleanup:
- VIR_FREE(callback);
+ remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virMutexUnlock(&priv->lock);
@@ -4156,7 +4160,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr
server ATTRI
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
goto cleanup;
- callback->client = client;
+ callback->client = virObjectRef(client);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -4183,7 +4187,7 @@ remoteDispatchConnectDomainEventCallbackRegisterAny(virNetServerPtr
server ATTRI
rv = 0;
cleanup:
- VIR_FREE(callback);
+ remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virObjectUnref(dom);
@@ -5666,7 +5670,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UN
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
goto cleanup;
- callback->client = client;
+ callback->client = virObjectRef(client);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5693,7 +5697,7 @@ remoteDispatchConnectNetworkEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UN
rv = 0;
cleanup:
- VIR_FREE(callback);
+ remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virObjectUnref(net);
@@ -5788,7 +5792,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server ATTRIBUT
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
goto cleanup;
- callback->client = client;
+ callback->client = virObjectRef(client);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5815,7 +5819,7 @@ remoteDispatchConnectStoragePoolEventRegisterAny(virNetServerPtr
server ATTRIBUT
rv = 0;
cleanup:
- VIR_FREE(callback);
+ remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virObjectUnref(pool);
@@ -5909,7 +5913,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server ATTRIBUTE
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
goto cleanup;
- callback->client = client;
+ callback->client = virObjectRef(client);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -5936,7 +5940,7 @@ remoteDispatchConnectNodeDeviceEventRegisterAny(virNetServerPtr
server ATTRIBUTE
rv = 0;
cleanup:
- VIR_FREE(callback);
+ remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virObjectUnref(dev);
@@ -6030,7 +6034,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
goto cleanup;
- callback->client = client;
+ callback->client = virObjectRef(client);
callback->eventID = args->eventID;
callback->callbackID = -1;
ref = callback;
@@ -6057,7 +6061,7 @@ remoteDispatchConnectSecretEventRegisterAny(virNetServerPtr server
ATTRIBUTE_UNU
rv = 0;
cleanup:
- VIR_FREE(callback);
+ remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virObjectUnref(secret);
@@ -6146,7 +6150,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server
ATTRIBUTE_U
* success, we use 'ref' to save a copy of the pointer. */
if (VIR_ALLOC(callback) < 0)
goto cleanup;
- callback->client = client;
+ callback->client = virObjectRef(client);
callback->callbackID = -1;
ref = callback;
if (VIR_APPEND_ELEMENT(priv->qemuEventCallbacks,
@@ -6173,7 +6177,7 @@ qemuDispatchConnectDomainMonitorEventRegister(virNetServerPtr server
ATTRIBUTE_U
rv = 0;
cleanup:
- VIR_FREE(callback);
+ remoteEventCallbackFree(callback);
if (rv < 0)
virNetMessageSaveError(rerr);
virObjectUnref(dom);
--
2.9.3
2801
days inactive
2803
days old
3 comments
2 participants
participants (2)
-
John Ferlan -
Laine Stump