libvncserver: indicate client thread end by other means than socket

Before, the client-to-server thread would end when the client's socket
was set to RFB_INVALID_SOCKET. However, there's a race condition where
the client's socket can be set to invalid while the client-to-server
thread is in select(), causing
bk138/droidVNC-NG#8.

Fix this by:

* introducing an additional RFB_SHUTDOWN state for clients
* use this as an end condition in the client-to-server thread instead
  of the socket
* move the self-pipe notification to rfbCloseClient from
  rfbShutdownServer.

Author: bk138

libvncserver/main.c

@@ -461,7 +461,7 @@ clientOutput(void *data)
     while (1) {
         haveUpdate = false;
         while (!haveUpdate) {
-		if (cl->sock == RFB_INVALID_SOCKET) {
+		if (cl->sock == RFB_INVALID_SOCKET || cl->state == RFB_SHUTDOWN) {
 			/* Client has disconnected. */
 			return THREAD_ROUTINE_RETURN_VALUE;
 		}
@@ -528,7 +528,7 @@ clientInput(void *data)
     uintptr_t output_thread = _beginthread(clientOutput, 0, cl);
 #endif
 
-    while (1) {
+    while (cl->state != RFB_SHUTDOWN) {
 	fd_set rfds, wfds, efds;
 	struct timeval tv;
 	int n;
@@ -604,6 +604,10 @@ clientInput(void *data)
     UNLOCK(cl->updateMutex);
     THREAD_JOIN(output_thread);
 
+    /* Close client sock */
+    rfbCloseSocket(cl->sock);
+    cl->sock = RFB_INVALID_SOCKET;
+
     rfbClientConnectionGone(cl);
 
     return THREAD_ROUTINE_RETURN_VALUE;
@@ -620,8 +624,16 @@ listenerRun(void *data)
     socklen_t len;
     fd_set listen_fds;  /* temp file descriptor list for select() */
 
+    /*
+       Only checking socket state here and not using rfbIsActive()
+       because: When rfbShutdownServer() is called by the client, it runs in
+       the client-to-server thread's context, resulting in itself calling
+       its own the pthread_join(), returning immediately, leaving the
+       client-to-server thread to actually terminate _after_ the listener thread
+       is terminated, leaving the client list still populated.
+     */
     /* TODO: HTTP is not handled */
-    while (rfbIsActive(screen)) {
+    while (screen->socketState != RFB_SOCKET_SHUTDOWN) {
         client_fd = -1;
         FD_ZERO(&listen_fds);
 	if(screen->listenSock != RFB_INVALID_SOCKET)
@@ -1173,22 +1185,14 @@ void rfbShutdownServer(rfbScreenInfoPtr screen,rfbBool disconnectClients) {
         rfbCloseClient(currentCl);
       }
 
-#ifdef LIBVNCSERVER_HAVE_LIBPTHREAD
-    if(currentCl->screen->backgroundLoop) {
       /*
-	Notify the thread. This simply writes a NULL byte to the notify pipe in order to get past the select()
-	in clientInput(), the loop in there will then break because the rfbCloseClient() above has set
-	currentCl->sock to -1.
-      */
-      write(currentCl->pipe_notify_client_thread[1], "\x00", 1);
-      /* And wait for it to finish. */
-      pthread_join(currentCl->client_thread, NULL);
-    } else {
-      rfbClientConnectionGone(currentCl);
-    }
-#else
-      rfbClientConnectionGone(currentCl);
+	 In threaded mode, rfbClientConnectionGone() is called by the client-to-server thread.
+	 Only need to call this here for non-threaded mode.
+       */
+#if defined(LIBVNCSERVER_HAVE_LIBPTHREAD) || defined(LIBVNCSERVER_HAVE_WIN32THREADS)
+    if(!currentCl->screen->backgroundLoop)
 #endif
+	rfbClientConnectionGone(currentCl);
 
       currentCl = nextCl;
     }
@@ -1200,6 +1204,7 @@ void rfbShutdownServer(rfbScreenInfoPtr screen,rfbBool disconnectClients) {
   rfbShutdownSockets(screen);
 
 #ifdef LIBVNCSERVER_HAVE_LIBPTHREAD
+  screen->socketState = RFB_SOCKET_SHUTDOWN; /* Set this so the listener thread ends */
   if (screen->backgroundLoop) {
       /*
 	Notify the listener thread. This simply writes a NULL byte to the notify pipe in order to get past the select()

libvncserver/sockets.c

@@ -556,24 +556,47 @@ rfbCloseClient(rfbClientPtr cl)
     if (cl->sock != RFB_INVALID_SOCKET)
 #endif
       {
+	/* Remove client sock from allFds and adapt maxFd */
 	FD_CLR(cl->sock,&(cl->screen->allFds));
 	if(cl->sock==cl->screen->maxFd)
 	  while(cl->screen->maxFd>0
 		&& !FD_ISSET(cl->screen->maxFd,&(cl->screen->allFds)))
 	    cl->screen->maxFd--;
 #ifdef LIBVNCSERVER_WITH_WEBSOCKETS
+	/* Has to happen before socket close as the SSL implementation might send a goodbye */
 	if (cl->sslctx)
 	    rfbssl_destroy(cl);
 	free(cl->wspath);
 #endif
-#ifndef __MINGW32__
-	shutdown(cl->sock,SHUT_RDWR);
-#endif
-	rfbCloseSocket(cl->sock);
-	cl->sock = RFB_INVALID_SOCKET;
       }
     TSIGNAL(cl->updateCond);
     UNLOCK(cl->updateMutex);
+
+    /*
+       Client socket closing, either via the client-to-server thread or directly.
+    */
+#if defined(LIBVNCSERVER_HAVE_LIBPTHREAD) || defined(LIBVNCSERVER_HAVE_WIN32THREADS)
+    if(cl->screen->backgroundLoop) {
+	/* Indicate to client-to-server thread that it should not go on */
+	cl->state = RFB_SHUTDOWN;
+#ifdef LIBVNCSERVER_HAVE_LIBPTHREAD
+	/*
+	  Notify the thread. This simply writes a NULL byte to the notify pipe in order to get past the select()
+	  in clientInput(), the loop in there will then break because the client state has been set to
+	  RFB_SHUTDOWN. Client socket closing will be done by the thread.
+	*/
+	write(cl->pipe_notify_client_thread[1], "\x00", 1);
+	/* And wait for it to finish. */
+	pthread_join(cl->client_thread, NULL);
+#endif
+    } else
+#endif
+	/* Either no threading support or threading support with screen->backgroundloop == false */
+	{
+	    /* Close client sock */
+	    rfbCloseSocket(cl->sock);
+	    cl->sock = RFB_INVALID_SOCKET;
+	}
 }
 
 

rfb/rfb.h

@@ -491,7 +491,8 @@ typedef struct _rfbClientRec {
         /* Ephemeral internal-use states that will never be seen by software
          * using LibVNCServer to provide services: */
 
-        RFB_INITIALISATION_SHARED /**< sending initialisation messages with implicit shared-flag already true */
+        RFB_INITIALISATION_SHARED, /**< sending initialisation messages with implicit shared-flag already true */
+        RFB_SHUTDOWN            /**< shutting down */
     } state;
 
     rfbBool reverseConnection;