From 0eaa59dce10d593a7b5470bfd7175e245e337cdc Mon Sep 17 00:00:00 2001
From: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Date: Thu, 21 Dec 2017 15:29:03 +0100
Subject: [PATCH] rpc: Correct locking and simplify the function

The lock for @client must not only be held for the duration of
checking whether the client wants to close, but also for as long as
we're closing the client. The same applies to the tracking of
authentications.

Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
---
 src/libvirt_remote.syms      |  5 +++--
 src/rpc/virnetserver.c       | 20 ++++++++++++++------
 src/rpc/virnetserverclient.c | 35 ++++++++++++++++++-----------------
 src/rpc/virnetserverclient.h |  5 +++--
 4 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index cecd71c49e..4e684ef695 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -125,6 +125,7 @@ virNetServerUpdateServices;
 # rpc/virnetserverclient.h
 virNetServerClientAddFilter;
 virNetServerClientClose;
+virNetServerClientCloseLocked;
 virNetServerClientDelayedClose;
 virNetServerClientGetAuth;
 virNetServerClientGetFD;
@@ -138,7 +139,7 @@ virNetServerClientGetUNIXIdentity;
 virNetServerClientImmediateClose;
 virNetServerClientInit;
 virNetServerClientInitKeepAlive;
-virNetServerClientIsClosed;
+virNetServerClientIsClosedLocked;
 virNetServerClientIsLocal;
 virNetServerClientIsSecure;
 virNetServerClientLocalAddrStringSASL;
@@ -156,7 +157,7 @@ virNetServerClientSetCloseHook;
 virNetServerClientSetDispatcher;
 virNetServerClientSetReadonly;
 virNetServerClientStartKeepAlive;
-virNetServerClientWantClose;
+virNetServerClientWantCloseLocked;
 
 
 # rpc/virnetservermdns.h
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 43f889e2a0..57cbfb59ab 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -285,8 +285,10 @@ int virNetServerAddClient(virNetServerPtr srv,
         goto error;
     srv->clients[srv->nclients-1] = virObjectRef(client);
 
-    if (virNetServerClientNeedAuth(client))
+    virObjectLock(client);
+    if (virNetServerClientNeedAuthLocked(client))
         virNetServerTrackPendingAuthLocked(srv);
+    virObjectUnlock(client);
 
     virNetServerCheckLimits(srv);
 
@@ -847,6 +849,7 @@ void
 virNetServerProcessClients(virNetServerPtr srv)
 {
     size_t i;
+    virNetServerClientPtr client;
 
     virObjectLock(srv);
 
@@ -855,15 +858,18 @@ virNetServerProcessClients(virNetServerPtr srv)
         /* Coverity 5.3.0 couldn't see that srv->clients is non-NULL
          * if srv->nclients is non-zero.  */
         sa_assert(srv->clients);
-        if (virNetServerClientWantClose(srv->clients[i]))
-            virNetServerClientClose(srv->clients[i]);
-        if (virNetServerClientIsClosed(srv->clients[i])) {
-            virNetServerClientPtr client = srv->clients[i];
 
+        client = srv->clients[i];
+        virObjectLock(client);
+        if (virNetServerClientWantCloseLocked(client))
+            virNetServerClientCloseLocked(client);
+
+        if (virNetServerClientIsClosedLocked(client)) {
             VIR_DELETE_ELEMENT(srv->clients, i, srv->nclients);
 
-            if (virNetServerClientNeedAuth(client))
+            if (virNetServerClientNeedAuthLocked(client))
                 virNetServerTrackCompletedAuthLocked(srv);
+            virObjectUnlock(client);
 
             virNetServerCheckLimits(srv);
 
@@ -872,6 +878,8 @@ virNetServerProcessClients(virNetServerPtr srv)
             virObjectLock(srv);
 
             goto reprocess;
+        } else {
+            virObjectUnlock(client);
         }
     }
 
diff --git a/src/rpc/virnetserverclient.c b/src/rpc/virnetserverclient.c
index afe4fb47a2..dee94450df 100644
--- a/src/rpc/virnetserverclient.c
+++ b/src/rpc/virnetserverclient.c
@@ -987,17 +987,15 @@ void virNetServerClientDispose(void *obj)
  * Full free of the client is done later in a safe point
  * where it can be guaranteed it is no longer in use
  */
-void virNetServerClientClose(virNetServerClientPtr client)
+void
+virNetServerClientCloseLocked(virNetServerClientPtr client)
 {
     virNetServerClientCloseFunc cf;
     virKeepAlivePtr ka;
 
-    virObjectLock(client);
     VIR_DEBUG("client=%p", client);
-    if (!client->sock) {
-        virObjectUnlock(client);
+    if (!client->sock)
         return;
-    }
 
     if (client->keepalive) {
         virKeepAliveStop(client->keepalive);
@@ -1048,20 +1046,25 @@ void virNetServerClientClose(virNetServerClientPtr client)
         virObjectUnref(client->sock);
         client->sock = NULL;
     }
-
-    virObjectUnlock(client);
 }
 
 
-bool virNetServerClientIsClosed(virNetServerClientPtr client)
+void
+virNetServerClientClose(virNetServerClientPtr client)
 {
-    bool closed;
     virObjectLock(client);
-    closed = client->sock == NULL ? true : false;
+    virNetServerClientCloseLocked(client);
     virObjectUnlock(client);
-    return closed;
 }
 
+
+bool
+virNetServerClientIsClosedLocked(virNetServerClientPtr client)
+{
+    return client->sock == NULL ? true : false;
+}
+
+
 void virNetServerClientDelayedClose(virNetServerClientPtr client)
 {
     virObjectLock(client);
@@ -1076,13 +1079,11 @@ void virNetServerClientImmediateClose(virNetServerClientPtr client)
     virObjectUnlock(client);
 }
 
-bool virNetServerClientWantClose(virNetServerClientPtr client)
+
+bool
+virNetServerClientWantCloseLocked(virNetServerClientPtr client)
 {
-    bool wantClose;
-    virObjectLock(client);
-    wantClose = client->wantClose;
-    virObjectUnlock(client);
-    return wantClose;
+    return client->wantClose;
 }
 
 
diff --git a/src/rpc/virnetserverclient.h b/src/rpc/virnetserverclient.h
index 1182d53c70..b7752a61fa 100644
--- a/src/rpc/virnetserverclient.h
+++ b/src/rpc/virnetserverclient.h
@@ -124,11 +124,12 @@ void virNetServerClientSetDispatcher(virNetServerClientPtr client,
                                      virNetServerClientDispatchFunc func,
                                      void *opaque);
 void virNetServerClientClose(virNetServerClientPtr client);
-bool virNetServerClientIsClosed(virNetServerClientPtr client);
+void virNetServerClientCloseLocked(virNetServerClientPtr client);
+bool virNetServerClientIsClosedLocked(virNetServerClientPtr client);
 
 void virNetServerClientDelayedClose(virNetServerClientPtr client);
 void virNetServerClientImmediateClose(virNetServerClientPtr client);
-bool virNetServerClientWantClose(virNetServerClientPtr client);
+bool virNetServerClientWantCloseLocked(virNetServerClientPtr client);
 
 int virNetServerClientInit(virNetServerClientPtr client);
 
-- 
GitLab