[libvirt] [PATCH 1/2] remote: Avoid the thread race condition

[Daniel P. Berrange]

On Wed, Dec 05, 2012 at 10:48:43PM +0800, Osier Yang wrote:
> From: Daniel P. Berrange <berrange at redhat.com>
> 
> 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