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

[Eric Blake]

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);
 }

@@ -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);
 }

-- 
1.7.4.4