From 96e163bdae65aa2c68e4301cf9ebe29e9f53f3d9 Mon Sep 17 00:00:00 2001 From: Quentin BUATHIER Date: Wed, 8 Aug 2018 16:14:39 +0200 Subject: [PATCH 1/3] Fix use-after-free --- libvncserver/main.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/libvncserver/main.c b/libvncserver/main.c index 05b4b13..106ebab 100644 --- a/libvncserver/main.c +++ b/libvncserver/main.c @@ -1081,15 +1081,21 @@ void rfbInitServer(rfbScreenInfoPtr screen) void rfbShutdownServer(rfbScreenInfoPtr screen,rfbBool disconnectClients) { if(disconnectClients) { - rfbClientPtr cl; rfbClientIteratorPtr iter = rfbGetClientIterator(screen); - while( (cl = rfbClientIteratorNext(iter)) ) { - if (cl->sock > -1) { - /* we don't care about maxfd here, because the server goes away */ - rfbCloseClient(cl); - rfbClientConnectionGone(cl); + rfbClientPtr nextCl, currentCl = rfbClientIteratorNext(iter); + + while(currentCl) { + nextCl = rfbClientIteratorNext(iter); + if (currentCl->sock > -1) { + /* we don't care about maxfd here, because the server goes away */ + rfbCloseClient(currentCl); } + + rfbClientConnectionGone(currentCl); + + currentCl = nextCl; } + rfbReleaseClientIterator(iter); } From cedae6e6f97b14f5df3ea7c5f7efd59f2bc9ad82 Mon Sep 17 00:00:00 2001 From: Quentin BUATHIER Date: Thu, 9 Aug 2018 09:33:59 +0200 Subject: [PATCH 2/3] Fix the concurrent issue hapenning between the freeing of the client and the clientOutput thread --- libvncserver/main.c | 29 ++++++++++++++++++++++++++--- libvncserver/rfbserver.c | 5 +++++ rfb/rfb.h | 1 + 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/libvncserver/main.c b/libvncserver/main.c index 106ebab..c34ae7e 100644 --- a/libvncserver/main.c +++ b/libvncserver/main.c @@ -33,6 +33,7 @@ #include #include #include +#include #endif #include @@ -533,6 +534,7 @@ clientInput(void *data) FD_ZERO(&rfds); FD_SET(cl->sock, &rfds); + FD_SET(cl->pipe_notify_client_thread[0], &rfds); FD_ZERO(&efds); FD_SET(cl->sock, &efds); @@ -541,9 +543,13 @@ clientInput(void *data) if ((cl->fileTransfer.fd!=-1) && (cl->fileTransfer.sending==1)) FD_SET(cl->sock, &wfds); + int nfds = cl->pipe_notify_client_thread[0] > cl->sock ? cl->pipe_notify_client_thread[0] : cl->sock; + tv.tv_sec = 60; /* 1 minute */ tv.tv_usec = 0; - n = select(cl->sock + 1, &rfds, &wfds, &efds, &tv); + + n = select(nfds + 1, &rfds, &wfds, &efds, &tv); + if (n < 0) { rfbLogPerror("ReadExact: select"); break; @@ -558,6 +564,13 @@ clientInput(void *data) if (FD_ISSET(cl->sock, &wfds)) rfbSendFileTransferChunk(cl); + if (FD_ISSET(cl->pipe_notify_client_thread[0], &rfds)) + { + // Reset the pipe + char buf; + while (read(cl->pipe_notify_client_thread[0], &buf, sizeof(buf)) == sizeof(buf)); + } + if (FD_ISSET(cl->sock, &rfds) || FD_ISSET(cl->sock, &efds)) { #ifdef LIBVNCSERVER_WITH_WEBSOCKETS @@ -628,8 +641,12 @@ rfbStartOnHoldClient(rfbClientPtr cl) { cl->onHold = FALSE; #ifdef LIBVNCSERVER_HAVE_LIBPTHREAD - if(cl->screen->backgroundLoop) - pthread_create(&cl->client_thread, NULL, clientInput, (void *)cl); + if(cl->screen->backgroundLoop) { + pipe(cl->pipe_notify_client_thread); + fcntl(cl->pipe_notify_client_thread[0], F_SETFL, O_NONBLOCK); + + pthread_create(&cl->client_thread, NULL, clientInput, (void *)cl); + } #endif } @@ -1091,7 +1108,13 @@ void rfbShutdownServer(rfbScreenInfoPtr screen,rfbBool disconnectClients) { rfbCloseClient(currentCl); } +#ifdef LIBVNCSERVER_HAVE_LIBPTHREAD + // Notify the thread and join it + write(currentCl->pipe_notify_client_thread[1], "\x00", 1); + pthread_join(currentCl->client_thread, NULL); +#else rfbClientConnectionGone(currentCl); +#endif currentCl = nextCl; } diff --git a/libvncserver/rfbserver.c b/libvncserver/rfbserver.c index 7af6aed..f13050d 100644 --- a/libvncserver/rfbserver.c +++ b/libvncserver/rfbserver.c @@ -621,6 +621,11 @@ rfbClientConnectionGone(rfbClientPtr cl) UNLOCK(cl->sendMutex); TINI_MUTEX(cl->sendMutex); +#ifdef LIBVNCSERVER_HAVE_LIBPTHREAD + close(cl->pipe_notify_client_thread[0]); + close(cl->pipe_notify_client_thread[1]); +#endif + rfbPrintStats(cl); rfbResetStats(cl); diff --git a/rfb/rfb.h b/rfb/rfb.h index 3d6d31e..9c60f3d 100644 --- a/rfb/rfb.h +++ b/rfb/rfb.h @@ -465,6 +465,7 @@ typedef struct _rfbClientRec { int protocolMinorVersion; #ifdef LIBVNCSERVER_HAVE_LIBPTHREAD + int pipe_notify_client_thread[2]; pthread_t client_thread; #endif From 00bae113d54014bafcf20c9f4c8c296e3e91bde5 Mon Sep 17 00:00:00 2001 From: Quentin BUATHIER Date: Thu, 6 Dec 2018 09:16:51 +0100 Subject: [PATCH 3/3] Check the return code of pipe --- libvncserver/main.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libvncserver/main.c b/libvncserver/main.c index c34ae7e..17bef7e 100644 --- a/libvncserver/main.c +++ b/libvncserver/main.c @@ -642,7 +642,10 @@ rfbStartOnHoldClient(rfbClientPtr cl) cl->onHold = FALSE; #ifdef LIBVNCSERVER_HAVE_LIBPTHREAD if(cl->screen->backgroundLoop) { - pipe(cl->pipe_notify_client_thread); + if (pipe(cl->pipe_notify_client_thread) == -1) { + cl->pipe_notify_client_thread[0] = -1; + cl->pipe_notify_client_thread[1] = -1; + } fcntl(cl->pipe_notify_client_thread[0], F_SETFL, O_NONBLOCK); pthread_create(&cl->client_thread, NULL, clientInput, (void *)cl);