From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Daniel P. Berrange" Subject: Re: [PATCH] Paravirt framebuffer backend tools [2/5] Date: Tue, 12 Sep 2006 19:55:36 +0100 Message-ID: <20060912185536.GE16855@redhat.com> References: <1157227102.11059.40.camel@aglarond.local> Reply-To: "Daniel P. Berrange" Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="0eh6TmSyL6TZE2Uz" Return-path: Content-Disposition: inline In-Reply-To: <1157227102.11059.40.camel@aglarond.local> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Jeremy Katz Cc: aliguori , xen-devel , Markus Armbruster List-Id: xen-devel@lists.xenproject.org --0eh6TmSyL6TZE2Uz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sat, Sep 02, 2006 at 03:58:22PM -0400, Jeremy Katz wrote: > Initially from Anthony Liguori and then modified for > * Integration with xenstore (armrbu) > * Adding option parsing (katzj) > * Integration with Xen makefiles > * Error handling (armbru) > * Future-proof layout of shared page (armbru) > * Memory barriers (armbru) > * Support for vncunused and vnclisten as support in qemu-dm vnc (katzj) > > Signed-off-by: Jeremy Katz > Cc: Markus Armbruster > Cc: Anthony Liguori I've been stress testing the xen-vncfb daemon and found some very nasty problems. First of all the code in LibVNCServer which deals with pthreads condition variables is utterly broken. It passes unlocked mutexes into pthread_cond_wait, and in some places also tries to lock the mutex just after existing the wait call. The former leads to a race condition & missed conditions, the latter completely deadlocks the daemon. To make matters worse the library fails to check return values form pthread_* calls pretty much everywhere. Carrying on, the code uses mutexes to synchronize changes to the struct members in the client state structure, but then fails to do any locking for reads, leading to yet more race conditions & death by SIGBUS in a number of places. Basically LibVNCServer should be considered extremely harmful :-( the QEMU VNC code for HVM guests doesn't use LibVNCServer, instead following a more sane select() reactor model multiplexing all I/O in a single process. I think it would be highly desirable to replace the LibVNCServer code with Anthony's code from QEMU, because I don't have any confidence in running something based on LibVNCServer as an unprivileged daemon, let alone root. FWIW, I'm attaching a patch to LibVNCServer which fixes as many of the issues I've found so far as possible. If you want a stable xen-vncfb daemon then I recommend applying these before trying to test the VNC bits of paravirt framebuffer. Signed-off by: Daniel berrange Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=| --0eh6TmSyL6TZE2Uz Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="xen-libvncserver-threads.patch" diff -ru xen-unstable-11405.orig/LibVNCServer-0.8.2/libvncserver/main.c xen-unstable-11405/LibVNCServer-0.8.2/libvncserver/main.c --- xen-unstable-11405.orig/LibVNCServer-0.8.2/libvncserver/main.c 2006-05-28 16:38:26.000000000 -0400 +++ xen-unstable-11405/LibVNCServer-0.8.2/libvncserver/main.c 2006-09-12 14:23:09.000000000 -0400 @@ -454,13 +454,10 @@ updateRegion = sraRgnCreateRgn(cl->modifiedRegion); haveUpdate = sraRgnAnd(updateRegion,cl->requestedRegion); sraRgnDestroy(updateRegion); - } - UNLOCK(cl->updateMutex); - if (!haveUpdate) { WAIT(cl->updateCond, cl->updateMutex); - UNLOCK(cl->updateMutex); /* we really needn't lock now. */ } + UNLOCK(cl->updateMutex); } /* OK, now, to save bandwidth, wait a little while for more @@ -497,21 +494,25 @@ while (1) { fd_set rfds, wfds, efds; struct timeval tv; - int n; + int n, sock; + + LOCK(cl->updateMutex); + sock = cl->sock; + UNLOCK(cl->updateMutex); FD_ZERO(&rfds); - FD_SET(cl->sock, &rfds); + FD_SET(sock, &rfds); FD_ZERO(&efds); - FD_SET(cl->sock, &efds); + FD_SET(sock, &efds); /* Are we transferring a file in the background? */ FD_ZERO(&wfds); if ((cl->fileTransfer.fd!=-1) && (cl->fileTransfer.sending==1)) - FD_SET(cl->sock, &wfds); + FD_SET(sock, &wfds); tv.tv_sec = 60; /* 1 minute */ tv.tv_usec = 0; - n = select(cl->sock + 1, &rfds, &wfds, &efds, &tv); + n = select(sock + 1, &rfds, &wfds, &efds, &tv); if (n < 0) { rfbLogPerror("ReadExact: select"); break; @@ -523,16 +524,19 @@ } /* We have some space on the transmit queue, send some data */ - if (FD_ISSET(cl->sock, &wfds)) + if (FD_ISSET(sock, &wfds)) rfbSendFileTransferChunk(cl); - if (FD_ISSET(cl->sock, &rfds) || FD_ISSET(cl->sock, &efds)) + if (FD_ISSET(sock, &rfds) || FD_ISSET(sock, &efds)) rfbProcessClientMessage(cl); - if (cl->sock == -1) { - /* Client has disconnected. */ - break; - } + LOCK(cl->updateMutex); + if (cl->sock == -1) { + UNLOCK(cl->updateMutex); + /* Client has disconnected. */ + break; + } + UNLOCK(cl->updateMutex); } /* Get rid of the output thread. */ diff -ru xen-unstable-11405.orig/LibVNCServer-0.8.2/libvncserver/rfbserver.c xen-unstable-11405/LibVNCServer-0.8.2/libvncserver/rfbserver.c --- xen-unstable-11405.orig/LibVNCServer-0.8.2/libvncserver/rfbserver.c 2006-06-05 14:50:45.000000000 -0400 +++ xen-unstable-11405/LibVNCServer-0.8.2/libvncserver/rfbserver.c 2006-09-12 13:02:06.000000000 -0400 @@ -492,9 +492,9 @@ do { LOCK(cl->refCountMutex); i=cl->refCount; - UNLOCK(cl->refCountMutex); if(i>0) WAIT(cl->deleteCond,cl->refCountMutex); + UNLOCK(cl->refCountMutex); } while(i>0); } #endif --0eh6TmSyL6TZE2Uz Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --0eh6TmSyL6TZE2Uz--