[libvirt] [PATCH] rpc: avoid libvirtd crash on unexpected client close

[Daniel P. Berrange]

On Mon, Aug 01, 2011 at 01:53:19PM -0600, Eric Blake wrote:
> Steps to reproduce this problem (vm1 is not running):
> for i in `seq 50`; do virsh managedsave vm1& done; killall virsh
> 
> Pre-patch, virNetServerClientClose could end up setting client->sock
> to NULL prior to other cleanup functions trying to use client->sock.
> This fixes things by checking for NULL in more places, and by deferring
> the cleanup until after all queued messages have been served.
> 
> * src/rpc/virnetserverclient.c (virNetServerClientRegisterEvent)
> (virNetServerClientGetFD, virNetServerClientIsSecure)
> (virNetServerClientLocalAddrString)
> (virNetServerClientRemoteAddrString): Check for closed socket.
> (virNetServerClientClose): Rearrange close sequence.
> Analysis from Wen Congyang.
> ---
> 
> This fixes the problem using the reproduction formula (that is,
> pre-patch I reproduced the failure; valgrind showed it at:
> ==29390== Thread 4:
> ==29390== Invalid read of size 4
> ==29390==    at 0x3D16608FA0: pthread_mutex_lock (pthread_mutex_lock.c:50)
> ==29390==    by 0x4E9FED2: virMutexLock (threads-pthread.c:85)
> ==29390==    by 0x45DA5E: virNetSocketGetLocalIdentity (virnetsocket.c:741)
> ==29390==    by 0x45554A: virNetServerClientGetLocalIdentity (virnetserverclient.c:401)
> ==29390==    by 0x4420E3: remoteDispatchAuthList (remote.c:1682)
> ==29390==    by 0x4224E4: remoteDispatchAuthListHelper (remote_dispatch.h:19)
> ==29390==    by 0x453EFD: virNetServerProgramDispatchCall (virnetserverprogram.c:375)
> ==29390==    by 0x4539FF: virNetServerProgramDispatch (virnetserverprogram.c:252)
> ==29390==    by 0x456B20: virNetServerHandleJob (virnetserver.c:155)
> ==29390==    by 0x4EA06A3: virThreadPoolWorker (threadpool.c:98)
> ==29390==    by 0x4EA01D6: virThreadHelper (threads-pthread.c:157)
> ==29390==    by 0x3D16606CCA: start_thread (pthread_create.c:301)
> ==29390==  Address 0x10 is not stack'd, malloc'd or (recently) free'd
> 
> 
> post-patch, libvirtd stays alive and valgrind no longer reports
> an invalid read).  But I'd really like danpb's opinion, if there
> is time to get that before 0.9.4.
> 
> /me note to self 'git send-email --cc=...' uses cc as well as
> honoring .git/config --to to the list; but the list manager
> sometimes strips cc: lines.  Converserly, 'git send-email --to=...'
> overrides .git/config settings, leaving out the list.  I can't
> win; sorry for the private mails on my previous send attempt.
> 
>  src/rpc/virnetserverclient.c |   29 +++++++++++++++++++----------
>  1 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
> index 3c0dba8..2f6c040 100644
> --- a/src/rpc/virnetserverclient.c
> +++ b/src/rpc/virnetserverclient.c
> @@ -177,7 +177,8 @@ static int virNetServerClientRegisterEvent(virNetServerClientPtr client)
> 
>      client->refs++;
>      VIR_DEBUG("Registering client event callback %d", mode);
> -    if (virNetSocketAddIOCallback(client->sock,
> +    if (!client->sock ||
> +        virNetSocketAddIOCallback(client->sock,
>                                    mode,
>                                    virNetServerClientDispatchEvent,
>                                    client,
> @@ -386,9 +387,10 @@ int virNetServerClientGetTLSKeySize(virNetServerClientPtr client)
> 
>  int virNetServerClientGetFD(virNetServerClientPtr client)
>  {
> -    int fd = 0;
> +    int fd = -1;
>      virNetServerClientLock(client);
> -    fd = virNetSocketGetFD(client->sock);
> +    if (client->sock)
> +        fd = virNetSocketGetFD(client->sock);
>      virNetServerClientUnlock(client);
>      return fd;
>  }
> @@ -396,9 +398,10 @@ int virNetServerClientGetFD(virNetServerClientPtr client)
>  int virNetServerClientGetLocalIdentity(virNetServerClientPtr client,
>                                         uid_t *uid, pid_t *pid)
>  {
> -    int ret;
> +    int ret = -1;
>      virNetServerClientLock(client);
> -    ret = virNetSocketGetLocalIdentity(client->sock, uid, pid);
> +    if (client->sock)
> +        ret = virNetSocketGetLocalIdentity(client->sock, uid, pid);
>      virNetServerClientUnlock(client);
>      return ret;
>  }
> @@ -413,7 +416,7 @@ bool virNetServerClientIsSecure(virNetServerClientPtr client)
>      if (client->sasl)
>          secure = true;
>  #endif
> -    if (virNetSocketIsLocal(client->sock))
> +    if (client->sock && virNetSocketIsLocal(client->sock))
>          secure = true;
>      virNetServerClientUnlock(client);
>      return secure;
> @@ -502,12 +505,16 @@ void virNetServerClientSetDispatcher(virNetServerClientPtr client,
> 
>  const char *virNetServerClientLocalAddrString(virNetServerClientPtr client)
>  {
> +    if (!client->sock)
> +        return NULL;
>      return virNetSocketLocalAddrString(client->sock);
>  }
> 
> 
>  const char *virNetServerClientRemoteAddrString(virNetServerClientPtr client)
>  {
> +    if (!client->sock)
> +        return NULL;
>      return virNetSocketRemoteAddrString(client->sock);
>  }
> 


All thes changes look correct & desirable.


> @@ -570,10 +577,7 @@ void virNetServerClientClose(virNetServerClientPtr client)
>          virNetTLSSessionFree(client->tls);
>          client->tls = NULL;
>      }
> -    if (client->sock) {
> -        virNetSocketFree(client->sock);
> -        client->sock = NULL;
> -    }
> +    client->wantClose = true;
> 
>      while (client->rx) {
>          virNetMessagePtr msg
> @@ -586,6 +590,11 @@ void virNetServerClientClose(virNetServerClientPtr client)
>          virNetMessageFree(msg);
>      }
> 
> +    if (client->sock) {
> +        virNetSocketFree(client->sock);
> +        client->sock = NULL;
> +    }
> +
>      virNetServerClientUnlock(client);
>  }

I'm curious why you have these last 2 hunks of the patc ?  The entire
of the virNetServerClientClose() is executed under the mutex, so AFAICT
moving these 4 lines to later in the function can have no effect. The
assignment to wantClose ought to not be needed at this point either,
since this flag is used by the server to decide to invoke the
virNetServerClientClose method.

That all said, these 2 hunks are at worst harmless, so ACK to the
patch.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|