On Wed, Dec 05, 2012 at 10:48:43PM +0800, Osier Yang wrote: > From: Daniel P. Berrange <berrange@xxxxxxxxxx> > > https://bugzilla.redhat.com/show_bug.cgi?id=866524 > > Since the virConnect object is not locked wholely when doing > virConenctDispose, a thread can get the lock and thus might > cause the race. > > Detected by valgrind: > > ==23687== Invalid read of size 4 > ==23687== at 0x38BAA091EC: pthread_mutex_lock (pthread_mutex_lock.c:61) > ==23687== by 0x3FBA919E36: remoteClientCloseFunc (remote_driver.c:337) > ==23687== by 0x3FBA936BF2: virNetClientCloseLocked (virnetclient.c:688) > ==23687== by 0x3FBA9390D8: virNetClientIncomingEvent (virnetclient.c:1859) > ==23687== by 0x3FBA851AAE: virEventPollRunOnce (event_poll.c:485) > ==23687== by 0x3FBA850846: virEventRunDefaultImpl (event.c:247) > ==23687== by 0x40CD61: vshEventLoop (virsh.c:2128) > ==23687== by 0x3FBA8626F8: virThreadHelper (threads-pthread.c:161) > ==23687== by 0x38BAA077F0: start_thread (pthread_create.c:301) > ==23687== by 0x33F68E570C: clone (clone.S:115) > ==23687== Address 0x4ca94e0 is 144 bytes inside a block of size 312 free'd > ==23687== at 0x4A0595D: free (vg_replace_malloc.c:366) > ==23687== by 0x3FBA8588B8: virFree (memory.c:309) > ==23687== by 0x3FBA86AAFC: virObjectUnref (virobject.c:145) > ==23687== by 0x3FBA8EA767: virConnectClose (libvirt.c:1458) > ==23687== by 0x40C8B8: vshDeinit (virsh.c:2584) > ==23687== by 0x41071E: main (virsh.c:3022) > > The above race is caused by the eventLoop thread tries to handle > the net client event by calling the callback set by: > virNetClientSetCloseCallback(priv->client, > remoteClientCloseFunc, > conn, NULL); > > I.E. remoteClientCloseFunc, which lock/unlock the virConnect object. > > This patch is to fix the bug by setting the callback to NULL when > doRemoteClose. > --- > src/remote/remote_driver.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c > index e5d4af3..5cc7e32 100644 > --- a/src/remote/remote_driver.c > +++ b/src/remote/remote_driver.c > @@ -1003,6 +1003,10 @@ doRemoteClose(virConnectPtr conn, struct private_data *priv) > > virObjectUnref(priv->tls); > priv->tls = NULL; > + virNetClientSetCloseCallback(priv->client, > + NULL, > + NULL, > + NULL); > virNetClientClose(priv->client); > virObjectUnref(priv->client); > priv->client = NULL; ACK, this avoids the race condition of concurrent access. THe only other way to avoid it would be to have the remote driver hold an extra reference on the virConnectPtr, but this causes a circular reference, meaning the connection would never be able to be freed. Daniel -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list