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

[Wen Congyang]

At 08/02/2011 03:53 AM, Eric Blake Write:
> 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.

With this patch, libvirtd does not crash.

> 
> * 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.

I agree with this patch. But give the chance to danpb to give the final
ack 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);
>  }
> 
> @@ -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);
>  }
>