From 804335f9d296440bb708ca844f5d89b58b50b0c6 Mon Sep 17 00:00:00 2001 From: runge Date: Thu, 21 May 2009 10:32:18 -0400 Subject: Thread safety for zrle, zlib, tight. Proposed tight security type fix for debian bug 517422. --- ChangeLog | 13 ++++++ configure.ac | 7 ++++ libvncserver/main.c | 46 ++++++++++++++-------- libvncserver/rfbserver.c | 21 ++++++++++ libvncserver/tight.c | 45 ++++++++++++++------- .../tightvnc-filetransfer/rfbtightserver.c | 24 +++++++++++ libvncserver/zlib.c | 24 ++++++++--- libvncserver/zrle.c | 27 ++++++++++--- libvncserver/zrleencodetemplate.c | 16 ++++++-- rfb/rfb.h | 11 ++++++ 10 files changed, 187 insertions(+), 47 deletions(-) diff --git a/ChangeLog b/ChangeLog index 99a2a20..4a797fc 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,16 @@ +2009-05-21 Karl Runge + * 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 * client_examples/SDLvncviewer.c: support mouse wheel operations diff --git a/configure.ac b/configure.ac index 3f48b26..65e258c 100644 --- a/configure.ac +++ b/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="" diff --git a/libvncserver/main.c b/libvncserver/main.c index 52bd4e7..1c9a4e7 100644 --- a/libvncserver/main.c +++ b/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); diff --git a/libvncserver/rfbserver.c b/libvncserver/rfbserver.c index 1fc90c1..bb9c665 100644 --- a/libvncserver/rfbserver.c +++ b/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); diff --git a/libvncserver/tight.c b/libvncserver/tight.c index 8a594c2..54eb921 100644 --- a/libvncserver/tight.c +++ b/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) diff --git a/libvncserver/tightvnc-filetransfer/rfbtightserver.c b/libvncserver/tightvnc-filetransfer/rfbtightserver.c index a24666b..ef29514 100644 --- a/libvncserver/tightvnc-filetransfer/rfbtightserver.c +++ b/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; } diff --git a/libvncserver/zlib.c b/libvncserver/zlib.c index 7b20f74..ac20c9c 100644 --- a/libvncserver/zlib.c +++ b/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) { diff --git a/libvncserver/zrle.c b/libvncserver/zrle.c index 2475fc0..e1f1447 100644 --- a/libvncserver/zrle.c +++ b/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; } diff --git a/libvncserver/zrleencodetemplate.c b/libvncserver/zrleencodetemplate.c index 6e81b3c..3a6f117 100644 --- a/libvncserver/zrleencodetemplate.c +++ b/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 diff --git a/rfb/rfb.h b/rfb/rfb.h index 4f3a664..b28a863 100644 --- a/rfb/rfb.h +++ b/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; /* -- cgit v1.2.3