Thread safety for zrle, zlib, tight.

Proposed tight security type fix for debian bug 517422.

ChangeLog

@@ -1,3 +1,16 @@
+2009-05-21  Karl Runge <runge@karlrunge.com>
+	* configure.ac: check for __thread.
+	* libvncserver/main.c, libvncserver/rfbserver.c: various
+	  thread safe corrections including sendMutex guard.
+	* libvncserver/zrle.c, libvncserver/zrleencodetemplate.c:
+	  thread safety via per-client buffers.
+	* libvncserver/tight.c, libvncserver/zlib.c: thread safety
+	  corrections via thread local storage using __thread.
+	* rfb/rfb.h: new members for threaded usage.
+	* tightvnc-filetransfer/rfbtightserver.c: fix (currently disabled)
+	  for tight security type for RFB 3.8 (debian bug 517422.)
+	  NEEDS AUDIT.
+
 2009-03-12  Johannes E. Schindelin  <Johannes.Schindelin@gmx.de>
 	* client_examples/SDLvncviewer.c: support mouse wheel operations
 

configure.ac

@@ -628,6 +628,13 @@ if test "x$with_pthread" != "xno"; then
 fi
 AM_CONDITIONAL(HAVE_LIBPTHREAD, test ! -z "$HAVE_LIBPTHREAD")
 
+AC_MSG_CHECKING([for __thread])
+AC_LINK_IFELSE([AC_LANG_PROGRAM(, [static __thread int p = 0])],
+	       [AC_DEFINE(HAVE_TLS, 1,
+		      Define to 1 if compiler supports __thread)
+		AC_MSG_RESULT([yes])],
+	       [AC_MSG_RESULT([no])])
+
 # tightvnc-filetransfer implemented using threads:
 if test -z "$HAVE_LIBPTHREAD"; then
 	with_tightvnc_filetransfer=""

libvncserver/main.c

@@ -444,22 +444,34 @@ clientOutput(void *data)
     while (1) {
         haveUpdate = false;
         while (!haveUpdate) {
-            if (cl->sock == -1) {
-                /* Client has disconnected. */
-                return NULL;
-            }
-	    LOCK(cl->updateMutex);
-	    haveUpdate = FB_UPDATE_PENDING(cl);
-	    if(!haveUpdate) {
-		updateRegion = sraRgnCreateRgn(cl->modifiedRegion);
-		haveUpdate = sraRgnAnd(updateRegion,cl->requestedRegion);
-		sraRgnDestroy(updateRegion);
-	    }
-
-            if (!haveUpdate) {
-                WAIT(cl->updateCond, cl->updateMutex);
-            }
-	    UNLOCK(cl->updateMutex);
+		if (cl->sock == -1) {
+			/* Client has disconnected. */
+			return NULL;
+		}
+		if (cl->state != RFB_NORMAL || cl->onHold) {
+			/* just sleep until things get normal */
+			usleep(cl->screen->deferUpdateTime * 1000);
+			continue;
+		}
+
+		LOCK(cl->updateMutex);
+
+		if (sraRgnEmpty(cl->requestedRegion)) {
+			; /* always require a FB Update Request (otherwise can crash.) */
+		} else {
+			haveUpdate = FB_UPDATE_PENDING(cl);
+			if(!haveUpdate) {
+				updateRegion = sraRgnCreateRgn(cl->modifiedRegion);
+				haveUpdate   = sraRgnAnd(updateRegion,cl->requestedRegion);
+				sraRgnDestroy(updateRegion);
+			}
+		}
+
+		if (!haveUpdate) {
+			WAIT(cl->updateCond, cl->updateMutex);
+		}
+
+		UNLOCK(cl->updateMutex);
         }
         
         /* OK, now, to save bandwidth, wait a little while for more
@@ -476,7 +488,9 @@ clientOutput(void *data)
 
         /* Now actually send the update. */
 	rfbIncrClientRef(cl);
+        LOCK(cl->sendMutex);
         rfbSendFramebufferUpdate(cl, updateRegion);
+        UNLOCK(cl->sendMutex);
 	rfbDecrClientRef(cl);
 
 	sraRgnDestroy(updateRegion);

libvncserver/rfbserver.c

@@ -327,6 +327,7 @@ rfbNewTCPOrUDPClient(rfbScreenInfoPtr rfbScreen,
 
       INIT_MUTEX(cl->outputMutex);
       INIT_MUTEX(cl->refCountMutex);
+      INIT_MUTEX(cl->sendMutex);
       INIT_COND(cl->deleteCond);
 
       cl->state = RFB_PROTOCOL_VERSION;
@@ -550,6 +551,10 @@ rfbClientConnectionGone(rfbClientPtr cl)
     UNLOCK(cl->outputMutex);
     TINI_MUTEX(cl->outputMutex);
 
+    LOCK(cl->sendMutex);
+    UNLOCK(cl->sendMutex);
+    TINI_MUTEX(cl->sendMutex);
+
 #ifdef CORBA
     destroyConnection(cl);
 #endif
@@ -1102,9 +1107,11 @@ rfbBool rfbSendFileTransferMessage(rfbClientPtr cl, uint8_t contentType, uint8_t
     /*
     rfbLog("rfbSendFileTransferMessage( %dtype, %dparam, %dsize, %dlen, %p)\n", contentType, contentParam, size, length, buffer);
     */
+    LOCK(cl->sendMutex);
     if (rfbWriteExact(cl, (char *)&ft, sz_rfbFileTransferMsg) < 0) {
         rfbLogPerror("rfbSendFileTransferMessage: write");
         rfbCloseClient(cl);
+        UNLOCK(cl->sendMutex);
         return FALSE;
     }
 
@@ -1113,9 +1120,11 @@ rfbBool rfbSendFileTransferMessage(rfbClientPtr cl, uint8_t contentType, uint8_t
         if (rfbWriteExact(cl, buffer, length) < 0) {
             rfbLogPerror("rfbSendFileTransferMessage: write");
             rfbCloseClient(cl);
+            UNLOCK(cl->sendMutex);
             return FALSE;
         }
     }
+    UNLOCK(cl->sendMutex);
 
     rfbStatRecordMessageSent(cl, rfbFileTransfer, sz_rfbFileTransferMsg+length, sz_rfbFileTransferMsg+length);
 
@@ -1525,12 +1534,15 @@ rfbBool rfbProcessFileTransfer(rfbClientPtr cl, uint8_t contentType, uint8_t con
 
         /* TODO: finish 64-bit file size support */
         sizeHtmp = 0;        
+        LOCK(cl->sendMutex);
         if (rfbWriteExact(cl, (char *)&sizeHtmp, 4) < 0) {
           rfbLogPerror("rfbProcessFileTransfer: write");
           rfbCloseClient(cl);
+          UNLOCK(cl->sendMutex);
           if (buffer!=NULL) free(buffer);
           return FALSE;
         }
+        UNLOCK(cl->sendMutex);
         break;
 
     case rfbFileHeader:
@@ -2122,6 +2134,7 @@ rfbProcessClientNormalMessage(rfbClientPtr cl)
 	    if (!cl->format.trueColour) {
 		if (!rfbSetClientColourMap(cl, 0, 0)) {
 		    sraRgnDestroy(tmpRegion);
+		    TSIGNAL(cl->updateCond);
 		    UNLOCK(cl->updateMutex);
 		    return;
 		}
@@ -3103,12 +3116,15 @@ rfbSendSetColourMapEntries(rfbClientPtr cl,
 
     len += nColours * 3 * 2;
 
+    LOCK(cl->sendMutex);
     if (rfbWriteExact(cl, wbuf, len) < 0) {
 	rfbLogPerror("rfbSendSetColourMapEntries: write");
 	rfbCloseClient(cl);
         if (wbuf != buf) free(wbuf);
+        UNLOCK(cl->sendMutex);
 	return FALSE;
     }
+    UNLOCK(cl->sendMutex);
 
     rfbStatRecordMessageSent(cl, rfbSetColourMapEntries, len, len);
     if (wbuf != buf) free(wbuf);
@@ -3129,10 +3145,12 @@ rfbSendBell(rfbScreenInfoPtr rfbScreen)
     i = rfbGetClientIterator(rfbScreen);
     while((cl=rfbClientIteratorNext(i))) {
 	b.type = rfbBell;
+        LOCK(cl->sendMutex);
 	if (rfbWriteExact(cl, (char *)&b, sz_rfbBellMsg) < 0) {
 	    rfbLogPerror("rfbSendBell: write");
 	    rfbCloseClient(cl);
 	}
+        UNLOCK(cl->sendMutex);
     }
     rfbStatRecordMessageSent(cl, rfbBell, sz_rfbBellMsg, sz_rfbBellMsg);
     rfbReleaseClientIterator(i);
@@ -3154,16 +3172,19 @@ rfbSendServerCutText(rfbScreenInfoPtr rfbScreen,char *str, int len)
     while ((cl = rfbClientIteratorNext(iterator)) != NULL) {
         sct.type = rfbServerCutText;
         sct.length = Swap32IfLE(len);
+        LOCK(cl->sendMutex);
         if (rfbWriteExact(cl, (char *)&sct,
                        sz_rfbServerCutTextMsg) < 0) {
             rfbLogPerror("rfbSendServerCutText: write");
             rfbCloseClient(cl);
+            UNLOCK(cl->sendMutex);
             continue;
         }
         if (rfbWriteExact(cl, str, len) < 0) {
             rfbLogPerror("rfbSendServerCutText: write");
             rfbCloseClient(cl);
         }
+        UNLOCK(cl->sendMutex);
         rfbStatRecordMessageSent(cl, rfbServerCutText, sz_rfbServerCutTextMsg+len, sz_rfbServerCutTextMsg+len);
     }
     rfbReleaseClientIterator(iterator);

libvncserver/tight.c

@@ -47,9 +47,20 @@
 /* May be set to TRUE with "-lazytight" Xvnc option. */
 rfbBool rfbTightDisableGradient = FALSE;
 
-/* This variable is set on every rfbSendRectEncodingTight() call. */
-static rfbBool usePixelFormat24;
+/*
+ * There is so much access of the Tight encoding static data buffers
+ * that we resort to using thread local storage instead of having
+ * per-client data.
+ */
+#if LIBVNCSERVER_HAVE_LIBPTHREAD && LIBVNCSERVER_HAVE_TLS && !defined(TLS) && defined(__linux__)
+#define TLS __thread
+#endif
+#ifndef TLS
+#define TLS
+#endif
 
+/* This variable is set on every rfbSendRectEncodingTight() call. */
+static TLS rfbBool usePixelFormat24 = FALSE;
 
 /* Compression level stuff. The following array contains various
    encoder parameters for each of 10 compression levels (0..9).
@@ -77,8 +88,8 @@ static TIGHT_CONF tightConf[10] = {
     { 65536, 2048,  32,  8192, 9, 9, 9, 6, 200, 500,  96, 80,   200,   500 }
 };
 
-static int compressLevel;
-static int qualityLevel;
+static TLS int compressLevel = 0;
+static TLS int qualityLevel = 0;
 
 /* Stuff dealing with palettes. */
 
@@ -100,29 +111,33 @@ typedef struct PALETTE_s {
 } PALETTE;
 
 /* TODO: move into rfbScreen struct */
-static int paletteNumColors, paletteMaxColors;
-static uint32_t monoBackground, monoForeground;
-static PALETTE palette;
+static TLS int paletteNumColors = 0;
+static TLS int paletteMaxColors = 0;
+static TLS uint32_t monoBackground = 0;
+static TLS uint32_t monoForeground = 0;
+static TLS PALETTE palette;
 
 /* Pointers to dynamically-allocated buffers. */
 
-static int tightBeforeBufSize = 0;
-static char *tightBeforeBuf = NULL;
+static TLS int tightBeforeBufSize = 0;
+static TLS char *tightBeforeBuf = NULL;
 
-static int tightAfterBufSize = 0;
-static char *tightAfterBuf = NULL;
+static TLS int tightAfterBufSize = 0;
+static TLS char *tightAfterBuf = NULL;
 
-static int *prevRowBuf = NULL;
+static TLS int *prevRowBuf = NULL;
 
 void rfbTightCleanup(rfbScreenInfoPtr screen)
 {
   if(tightBeforeBufSize) {
     free(tightBeforeBuf);
     tightBeforeBufSize=0;
+    tightBeforeBuf = NULL;
   }
   if(tightAfterBufSize) {
     free(tightAfterBuf);
     tightAfterBufSize=0;
+    tightAfterBuf = NULL;
   }
 }
 
@@ -1627,9 +1642,9 @@ DEFINE_DETECT_FUNCTION(32)
  * JPEG compression stuff.
  */
 
-static struct jpeg_destination_mgr jpegDstManager;
-static rfbBool jpegError;
-static int jpegDstDataLen;
+static TLS struct jpeg_destination_mgr jpegDstManager;
+static TLS rfbBool jpegError = FALSE;
+static TLS int jpegDstDataLen = 0;
 
 static rfbBool
 SendJpegRect(rfbClientPtr cl, int x, int y, int w, int h, int quality)

libvncserver/tightvnc-filetransfer/rfbtightserver.c

@@ -74,6 +74,24 @@ rfbVncAuthSendChallenge(rfbClientPtr cl)
 
 }
 
+/*
+ * LibVNCServer has a bug WRT Tight SecurityType and RFB 3.8
+ * It should send auth result even for rfbAuthNone.
+ * See http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=517422
+ * For testing set USE_SECTYPE_TIGHT_FOR_RFB_3_8 when compiling
+ * or set it here.
+ */
+#define SECTYPE_TIGHT_FOR_RFB_3_8 \
+	if (cl->protocolMajorVersion==3 && cl->protocolMinorVersion > 7) { \
+		uint32_t authResult; \
+		rfbLog("rfbProcessClientSecurityType: returning securityResult for client rfb version >= 3.8\n"); \
+		authResult = Swap32IfLE(rfbVncAuthOK); \
+		if (rfbWriteExact(cl, (char *)&authResult, 4) < 0) { \
+			rfbLogPerror("rfbAuthProcessClientMessage: write"); \
+			rfbCloseClient(cl); \
+			return; \
+		} \
+	}
 /*
  * Read client's preferred authentication type (protocol 3.7t).
  */
@@ -117,6 +135,9 @@ rfbProcessClientAuthType(rfbClientPtr cl)
     switch (auth_type) {
     case rfbAuthNone:
 	/* Dispatch client input to rfbProcessClientInitMessage. */
+#ifdef USE_SECTYPE_TIGHT_FOR_RFB_3_8
+	SECTYPE_TIGHT_FOR_RFB_3_8
+#endif
 	cl->state = RFB_INITIALISATION;
 	break;
     case rfbAuthVNC:
@@ -188,6 +209,9 @@ rfbSendAuthCaps(rfbClientPtr cl)
 	/* Call the function for authentication from here */
 	rfbProcessClientAuthType(cl);
     } else {
+#ifdef USE_SECTYPE_TIGHT_FOR_RFB_3_8
+	SECTYPE_TIGHT_FOR_RFB_3_8
+#endif
 	/* Dispatch client input to rfbProcessClientInitMessage. */
 	cl->state = RFB_INITIALISATION;
     }

libvncserver/zlib.c

@@ -40,12 +40,24 @@
  * raw encoding is used instead.
  */
 
-static int zlibBeforeBufSize = 0;
-static char *zlibBeforeBuf = NULL;
-
-static int zlibAfterBufSize = 0;
-static char *zlibAfterBuf = NULL;
-static int zlibAfterBufLen;
+/*
+ * Out of lazyiness, we use thread local storage for zlib as we did for
+ * tight.  N.B. ZRLE does it the traditional way with per-client storage
+ * (and so at least ZRLE will work threaded on older systems.)
+ */
+#if LIBVNCSERVER_HAVE_LIBPTHREAD && LIBVNCSERVER_HAVE_TLS && !defined(TLS) && defined(__linux__)
+#define TLS __thread
+#endif
+#ifndef TLS
+#define TLS
+#endif
+
+static TLS int zlibBeforeBufSize = 0;
+static TLS char *zlibBeforeBuf = NULL;
+
+static TLS int zlibAfterBufSize = 0;
+static TLS char *zlibAfterBuf = NULL;
+static TLS int zlibAfterBufLen = 0;
 
 void rfbZlibCleanup(rfbScreenInfoPtr screen)
 {

libvncserver/zrle.c

@@ -97,21 +97,25 @@
  */
 
 /* TODO: put into rfbClient struct */
-static char zrleBeforeBuf[rfbZRLETileWidth * rfbZRLETileHeight * 4 + 4];
-
 
+static char zrleBeforeBuf[rfbZRLETileWidth * rfbZRLETileHeight * 4 + 4];
 
 /*
  * rfbSendRectEncodingZRLE - send a given rectangle using ZRLE encoding.
  */
 
-
 rfbBool rfbSendRectEncodingZRLE(rfbClientPtr cl, int x, int y, int w, int h)
 {
   zrleOutStream* zos;
   rfbFramebufferUpdateRectHeader rect;
   rfbZRLEHeader hdr;
   int i;
+  char *zrleBeforeBuf;
+
+  if (cl->zrleBeforeBuf == NULL) {
+	cl->zrleBeforeBuf = (char *) malloc(rfbZRLETileWidth * rfbZRLETileHeight * 4 + 4);
+  }
+  zrleBeforeBuf = cl->zrleBeforeBuf;
 
   if (cl->preferredEncoding == rfbEncodingZYWRLE) {
 	  if (cl->tightQualityLevel < 0) {
@@ -238,8 +242,19 @@ rfbBool rfbSendRectEncodingZRLE(rfbClientPtr cl, int x, int y, int w, int h)
 
 void rfbFreeZrleData(rfbClientPtr cl)
 {
-  if (cl->zrleData)
-    zrleOutStreamFree(cl->zrleData);
-  cl->zrleData = NULL;
+	if (cl->zrleData) {
+		zrleOutStreamFree(cl->zrleData);
+	}
+	cl->zrleData = NULL;
+
+	if (cl->zrleBeforeBuf) {
+		free(cl->zrleBeforeBuf);
+	}
+	cl->zrleBeforeBuf = NULL;
+
+	if (cl->paletteHelper) {
+		free(cl->paletteHelper);
+	}
+	cl->paletteHelper = NULL;
 }
 

libvncserver/zrleencodetemplate.c

@@ -89,7 +89,7 @@ static zrlePaletteHelper paletteHelper;
 #endif /* ZRLE_ONCE */
 
 void ZRLE_ENCODE_TILE (PIXEL_T* data, int w, int h, zrleOutStream* os,
-		int zywrle_level, int *zywrleBuf);
+		int zywrle_level, int *zywrleBuf, void *paletteHelper);
 
 #if BPP!=8
 #define ZYWRLE_ENCODE
@@ -111,8 +111,12 @@ static void ZRLE_ENCODE (int x, int y, int w, int h,
 
       GET_IMAGE_INTO_BUF(tx,ty,tw,th,buf);
 
+      if (cl->paletteHelper == NULL) {
+          cl->paletteHelper = (void *) calloc(sizeof(zrlePaletteHelper), 1);
+      }
+
       ZRLE_ENCODE_TILE((PIXEL_T*)buf, tw, th, os,
-		      cl->zywrleLevel, cl->zywrleBuf);
+		      cl->zywrleLevel, cl->zywrleBuf, cl->paletteHelper);
     }
   }
   zrleOutStreamFlush(os);
@@ -120,7 +124,7 @@ static void ZRLE_ENCODE (int x, int y, int w, int h,
 
 
 void ZRLE_ENCODE_TILE(PIXEL_T* data, int w, int h, zrleOutStream* os,
-	int zywrle_level, int *zywrleBuf)
+	int zywrle_level, int *zywrleBuf,  void *paletteHelper)
 {
   /* First find the palette and the number of runs */
 
@@ -140,7 +144,11 @@ void ZRLE_ENCODE_TILE(PIXEL_T* data, int w, int h, zrleOutStream* os,
   PIXEL_T* end = ptr + h * w;
   *end = ~*(end-1); /* one past the end is different so the while loop ends */
 
+#if 0
   ph = &paletteHelper;
+#else
+  ph = (zrlePaletteHelper *) paletteHelper;
+#endif
   zrlePaletteHelperInit(ph);
 
   while (ptr < end) {
@@ -289,7 +297,7 @@ void ZRLE_ENCODE_TILE(PIXEL_T* data, int w, int h, zrleOutStream* os,
 #if BPP!=8
       if (zywrle_level > 0 && !(zywrle_level & 0x80)) {
         ZYWRLE_ANALYZE(data, data, w, h, w, zywrle_level, zywrleBuf);
-	ZRLE_ENCODE_TILE(data, w, h, os, zywrle_level | 0x80, zywrleBuf);
+	ZRLE_ENCODE_TILE(data, w, h, os, zywrle_level | 0x80, zywrleBuf, paletteHelper);
       }
       else
 #endif

rfb/rfb.h

@@ -589,6 +589,17 @@ typedef struct _rfbClientRec {
     int progressiveSliceY;
 
     rfbExtensionData* extensions;
+
+    /* for threaded zrle */
+    char *zrleBeforeBuf;
+    void *paletteHelper;
+
+    /* for thread safety for rfbSendFBUpdate() */
+#ifdef LIBVNCSERVER_HAVE_LIBPTHREAD
+#define LIBVNCSERVER_SEND_MUTEX
+    MUTEX(sendMutex);
+#endif
+
 } rfbClientRec, *rfbClientPtr;
 
 /*