Skip to content

Commit

Permalink
Thread safety for zrle, zlib, tight.
Browse files Browse the repository at this point in the history 
Proposed tight security type fix for debian bug 517422.
  • Loading branch information
runge committed May 21, 2009
1 parent 2cd4833 commit 804335f
Show file tree
Hide file tree
Showing 10 changed files with 187 additions and 47 deletions.
13 changes: 13 additions & 0 deletions ChangeLog
View file
@@ -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

Expand Down
7 changes: 7 additions & 0 deletions configure.ac
View file
Expand Up @@ -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=""
Expand Down
46 changes: 30 additions & 16 deletions libvncserver/main.c
View file
Expand Up @@ -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
Expand All @@ -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);
Expand Down
21 changes: 21 additions & 0 deletions libvncserver/rfbserver.c
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
}

Expand All @@ -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);

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down
45 changes: 30 additions & 15 deletions libvncserver/tight.c
View file
Expand Up @@ -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).
Expand Down Expand Up @@ -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. */

Expand All @@ -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;
}
}

Expand Down Expand Up @@ -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)
Expand Down
24 changes: 24 additions & 0 deletions libvncserver/tightvnc-filetransfer/rfbtightserver.c
View file
Expand Up @@ -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).
*/
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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;
}
Expand Down
24 changes: 18 additions & 6 deletions libvncserver/zlib.c
View file
Expand Up @@ -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)
{
Expand Down

0 comments on commit 804335f

Please sign in to comment.