Revert "libvncserver: indicate client thread end by other means than socket"

This reverts commit 336fc84.

Waiting for the client-to-server thread to finish in rfbCloseClient()
caused deadlocks in production environments.

Revert the patch and redo in a little bit less intrusive way.

Author: bk138

libvncserver/main.c

@@ -461,7 +461,7 @@ clientOutput(void *data)
     while (1) {
         haveUpdate = false;
         while (!haveUpdate) {
-		if (cl->sock == RFB_INVALID_SOCKET || cl->state == RFB_SHUTDOWN) {
+		if (cl->sock == RFB_INVALID_SOCKET) {
 			/* 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 (cl->state != RFB_SHUTDOWN) {
+    while (1) {
 	fd_set rfds, wfds, efds;
 	struct timeval tv;
 	int n;
@@ -604,10 +604,6 @@ 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;
@@ -624,16 +620,8 @@ 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 (screen->socketState != RFB_SOCKET_SHUTDOWN) {
+    while (rfbIsActive(screen)) {
         client_fd = -1;
         FD_ZERO(&listen_fds);
 	if(screen->listenSock != RFB_INVALID_SOCKET)
@@ -1185,14 +1173,22 @@ void rfbShutdownServer(rfbScreenInfoPtr screen,rfbBool disconnectClients) {
         rfbCloseClient(currentCl);
       }
 
+#ifdef LIBVNCSERVER_HAVE_LIBPTHREAD
+    if(currentCl->screen->backgroundLoop) {
       /*
-	 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)
+	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);
 #endif
-	rfbClientConnectionGone(currentCl);
 
       currentCl = nextCl;
     }
@@ -1204,7 +1200,6 @@ 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,47 +556,24 @@ 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,8 +491,7 @@ 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_SHUTDOWN            /**< shutting down */
+        RFB_INITIALISATION_SHARED /**< sending initialisation messages with implicit shared-flag already true */
     } state;
 
     rfbBool reverseConnection;