Re: [PATCH] Paravirt framebuffer backend tools [2/5]

["Daniel P. Berrange" <berrange@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2383 bytes --]

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 <katzj@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>

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 <berrange@redhat.com>

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  -=| 

[-- Attachment #2: xen-libvncserver-threads.patch --]
[-- Type: text/plain, Size: 2884 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel