[libvirt] [PATCH 0/3] Fix crash when relaying objects and closing connection at the same time
Fix the crash and clean up two related places. Peter Krempa (3): util: identity: Harden virIdentitySetCurrent() daemon: Clear fake domain def object that is used to check ACL prior to use rpc: Don't unref identity object while callbacks still can be executed daemon/remote.c | 1 + src/rpc/virnetserverclient.c | 4 ++-- src/util/viridentity.c | 4 +++- 3 files changed, 6 insertions(+), 3 deletions(-) -- 2.2.2
Don't unref the old identity unless we set the new one correctly and unref the new one on failure to set it so that we don't leak any references or use invalid pointers. --- src/util/viridentity.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/viridentity.c b/src/util/viridentity.c index 6f3baee..9b8ba4a 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -111,15 +111,17 @@ int virIdentitySetCurrent(virIdentityPtr ident) return -1; old = virThreadLocalGet(&virIdentityCurrent); - virObjectUnref(old); if (virThreadLocalSet(&virIdentityCurrent, virObjectRef(ident)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to set thread local identity")); + virObjectUnref(ident); return -1; } + virObjectUnref(old); + return 0; } -- 2.2.2
The fake object is used to pass the domain name and UUID to the ACL code for events where we don't have the full domain def when dispatching events. The rest of the entries would be left uninitialized. While this is not a problem code-wise as the used fields are initialized it looks ugly in the debugger. --- daemon/remote.c | 1 + 1 file changed, 1 insertion(+) diff --git a/daemon/remote.c b/daemon/remote.c index 62a4728..8646b06 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -150,6 +150,7 @@ remoteRelayDomainEventCheckACL(virNetServerClientPtr client, /* For now, we just create a virDomainDef with enough contents to * satisfy what viraccessdriverpolkit.c references. This is a bit * fragile, but I don't know of anything better. */ + memset(&def, 0, sizeof(def)); def.name = dom->name; memcpy(def.uuid, dom->uuid, VIR_UUID_BUFLEN); -- 2.2.2
While this thread is cleaning up the client and connection objects: #2 virFileReadAll (path=0x7f28780012b0 "/proc/1319/stat", maxlen=maxlen@entry=1024, buf=buf@entry=0x7f289c60fc40) at util/virfile.c:1287 #3 0x00007f28adbb1539 in virProcessGetStartTime (pid=<optimized out>, timestamp=timestamp@entry=0x7f289c60fc98) at util/virprocess.c:838 #4 0x00007f28adb91981 in virIdentityGetSystem () at util/viridentity.c:151 #5 0x00007f28ae73f17c in remoteClientFreeFunc (data=<optimized out>) at remote.c:1131 #6 0x00007f28adcb7f33 in virNetServerClientDispose (obj=0x7f28aecad180) at rpc/virnetserverclient.c:858 #7 0x00007f28adba8eeb in virObjectUnref (anyobj=<optimized out>) at util/virobject.c:265 #8 0x00007f28ae74ad05 in virNetServerHandleJob (jobOpaque=<optimized out>, opaque=0x7f28aec93ff0) at rpc/virnetserver.c:205 #9 0x00007f28adbbef4e in virThreadPoolWorker (opaque=opaque@entry=0x7f28aec88030) at util/virthreadpool.c:145 In stack frame #6 the client->identity object got unref'd, but the code that removes the event callbacks in frame #5 did not run yet as we are trying to obtain the system identity (frames #4, #3, #2). In other thead: #0 virObjectUnref (anyobj=anyobj@entry=0x7f288c162c60) at util/virobject.c:264 klass = 0xdeadbeef obj = 0x7f288c162c60 #1 0x00007f28ae71c709 in remoteRelayDomainEventCheckACL (client=<optimized out>, conn=<optimized out>, dom=dom@entry=0x7f28aecaafc0) at remote.c:164 #2 0x00007f28ae71fc83 in remoteRelayDomainEventTrayChange (conn=<optimized out>, dom=0x7f28aecaafc0, ... ) at remote.c:717 #3 0x00007f28adc04e53 in virDomainEventDispatchDefaultFunc (conn=0x7f287c0009a0, event=0x7f28aecab1a0, ...) at conf/domain_event.c:1455 #4 0x00007f28adc03831 in virObjectEventStateDispatchCallbacks (callbacks=<optimized out>, ....) at conf/object_event.c:724 #5 virObjectEventStateQueueDispatch (callbacks=0x7f288c083730, queue=0x7fff51f90030, state=0x7f288c18da20) at conf/object_event.c:738 #6 virObjectEventStateFlush (state=0x7f288c18da20) at conf/object_event.c:816 #7 virObjectEventTimer (timer=<optimized out>, opaque=0x7f288c18da20) at conf/object_event.c:562 #8 0x00007f28adb859cd in virEventPollDispatchTimeouts () at util/vireventpoll.c:459 Frame #0 is unrefing an invalid identity object while frame #2 hints that the client is still dispatching the event. For untrimmed backtrace see the bugzilla attachment. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1203030 --- src/rpc/virnetserverclient.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c index d36e80c..34c1994 100644 --- a/src/rpc/virnetserverclient.c +++ b/src/rpc/virnetserverclient.c @@ -850,12 +850,12 @@ void virNetServerClientDispose(void *obj) PROBE(RPC_SERVER_CLIENT_DISPOSE, "client=%p", client); - virObjectUnref(client->identity); - if (client->privateData && client->privateDataFreeFunc) client->privateDataFreeFunc(client->privateData); + virObjectUnref(client->identity); + #if WITH_SASL virObjectUnref(client->sasl); #endif -- 2.2.2
On Wed, Mar 25, 2015 at 09:35:05AM +0100, Peter Krempa wrote:
Fix the crash and clean up two related places.
Peter Krempa (3): util: identity: Harden virIdentitySetCurrent() daemon: Clear fake domain def object that is used to check ACL prior to use rpc: Don't unref identity object while callbacks still can be executed
daemon/remote.c | 1 + src/rpc/virnetserverclient.c | 4 ++-- src/util/viridentity.c | 4 +++- 3 files changed, 6 insertions(+), 3 deletions(-)
ACK series, thanks for chasing that down.
-- 2.2.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 25, 2015 at 13:32:25 +0100, Martin Kletzander wrote:
On Wed, Mar 25, 2015 at 09:35:05AM +0100, Peter Krempa wrote:
Fix the crash and clean up two related places.
Peter Krempa (3): util: identity: Harden virIdentitySetCurrent() daemon: Clear fake domain def object that is used to check ACL prior to use rpc: Don't unref identity object while callbacks still can be executed
daemon/remote.c | 1 + src/rpc/virnetserverclient.c | 4 ++-- src/util/viridentity.c | 4 +++- 3 files changed, 6 insertions(+), 3 deletions(-)
ACK series, thanks for chasing that down.
Pushed; Thanks. Peter
participants (2)
-
Martin Kletzander -
Peter Krempa