From 3df7537a301a3502f81f55b8261e92a97c9bdd45 Mon Sep 17 00:00:00 2001
From: Christian Beier <dontmind@freeshell.org>
Date: Wed, 26 Oct 2011 18:41:19 +0200
Subject: [PATCH] Fix deadlock in threaded mode when using nested
 rfbClientIteratorNext() calls.

Lengthy explanation follows...

First, the scenario before this patch:

We have three clients 1,2,3 connected. The main thread loops through
them using rfbClientIteratorNext() (loop L1) and is currently at
client 2 i.e. client 2's cl_2->refCount is 1. At this point we need to
loop again through the clients, with cl_2->refCount == 1, i.e. do a
loop L2 nested within loop L1.

BUT: Now client 2 disconnects, it's clientInput thread terminates its
clientOutput thread and calls rfbClientConnectionGone(). This LOCKs
clientListMutex and WAITs for cl_2->refCount to become 0. This means
this thread waits for the main thread to release cl_2. Waiting, with
clientListMutex LOCKed!

Meanwhile, the main thread is about to begin the inner
rfbClientIteratorNext() loop L2. The first call to rfbClientIteratorNext()
LOCKs clientListMutex. BAAM. This mutex is locked by cl2's clientInput
thread and is only released when cl_2->refCount becomes 0. The main thread
would decrement cl_2->refCount when it would continue with loop L1. But
it's waiting for cl2's clientInput thread to release clientListMutex. Which
never happens since this one's waiting for the main thread to decrement
cl_2->refCount. DEADLOCK.

Now, situation with this patch:

Same as above, but when client 2 disconnects it's clientInput thread
rfbClientConnectionGone(). This again LOCKs clientListMutex, removes cl_2
from the linked list and UNLOCKS clientListMutex. The WAIT for
cl_2->refCount to become 0 is _after_ that. Waiting, with
clientListMutex UNLOCKed!

Therefore, the main thread can continue, do the inner loop L2 (now only
looping through 1,3 - 2 was removed from the linked list) and continue with
loop L1, finally decrementing cl_2->refCount, allowing cl2's clientInput
thread to continue and terminate. The resources held by cl2 are not free()'d
by rfbClientConnectionGone until cl2->refCount becomes 0, i.e. loop L1 has
released cl2.
---
 libvncserver/rfbserver.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/libvncserver/rfbserver.c b/libvncserver/rfbserver.c
index cb37da3..b42a5ea 100644
--- a/libvncserver/rfbserver.c
+++ b/libvncserver/rfbserver.c
@@ -494,6 +494,21 @@ rfbClientConnectionGone(rfbClientPtr cl)
     if (cl->next)
         cl->next->prev = cl->prev;
 
+    UNLOCK(rfbClientListMutex);
+
+#ifdef LIBVNCSERVER_HAVE_LIBPTHREAD
+    if(cl->screen->backgroundLoop != FALSE) {
+      int i;
+      do {
+	LOCK(cl->refCountMutex);
+	i=cl->refCount;
+	if(i>0)
+	  WAIT(cl->deleteCond,cl->refCountMutex);
+	UNLOCK(cl->refCountMutex);
+      } while(i>0);
+    }
+#endif
+
     if(cl->sock>=0)
 	close(cl->sock);
 
@@ -510,21 +525,6 @@ rfbClientConnectionGone(rfbClientPtr cl)
     free(cl->beforeEncBuf);
     free(cl->afterEncBuf);
 
-#ifdef LIBVNCSERVER_HAVE_LIBPTHREAD
-    if(cl->screen->backgroundLoop != FALSE) {
-      int i;
-      do {
-	LOCK(cl->refCountMutex);
-	i=cl->refCount;
-	if(i>0)
-	  WAIT(cl->deleteCond,cl->refCountMutex);
-	UNLOCK(cl->refCountMutex);
-      } while(i>0);
-    }
-#endif
-
-    UNLOCK(rfbClientListMutex);
-
     if(cl->sock>=0)
        FD_CLR(cl->sock,&(cl->screen->allFds));