[trace+patch] VBoxHeadless crashes when connected by vncviewer if VNC/VRDE is enabled at runtime => Fixed in SVN

[Dennis Chen]

VBoxHeadless crashes when connected by vncviewer if VNC/VRDE is enabled at runtime.

Root Cause

VNC screen's frame buffer is not initialized properly when VNCServerImpl::VRDEEnableConnections is invoked without the first call to VNCServerImpl::VRDEResize, which causes the VNC client output thread to crash on null frame buffer.

Test Environment

4.3.6 and 4.2.20 on CentOS 6.2 x86_64

Affected Versions

4.3.x and 4.2.x

Reproducible Steps

1. Create a live snapshot of Windows VM with VRDE disabled.
2. Resume the Windows VM (e.g., win7), then enable VRDE with the following commands:
   • # VBoxManage controlvm win7 vrdeport 5901
   • # VBoxManage controlvm win7 vrde on
3. Use a vncviewer (e.g., tightvnc) to connect to the VM console.
4. VBoxHeadless crashes at the client output thread with SIGSEGV.

Stack Trace

# gdb
GNU gdb Fedora (6.8-37.el5)
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-redhat-linux-gnu".
(gdb) file VBoxHeadless
Reading symbols from /home/tests/virtualbox.4.2.20/bin/VBoxHeadless...done.
(gdb) r -s win7 -vrde config
Starting program: /home/tests/virtualbox.4.2.20/bin/VBoxHeadless -s win7 -vrde config
[Thread debugging using libthread_db enabled]
[New Thread 0x2b541d243190 (LWP 28344)]
[New Thread 0x40020940 (LWP 28347)]
[New Thread 0x40041940 (LWP 28348)]
[Thread 0x40041940 (LWP 28348) exited]
[Thread 0x40020940 (LWP 28347) exited]
Oracle VM VirtualBox Headless Interface 4.2.20_OSE
(C) 2008-2014 Oracle Corporation
All rights reserved.

Detaching after fork from child process 28349.
[New Thread 0x40020940 (LWP 28353)]
[New Thread 0x40041940 (LWP 28354)]
Detaching after fork from child process 28355.
[New Thread 0x400c2940 (LWP 28388)]
[New Thread 0x40143940 (LWP 28390)]
[New Thread 0x401c4940 (LWP 28391)]
[New Thread 0x40245940 (LWP 28392)]
[New Thread 0x40346940 (LWP 28393)]
[New Thread 0x403c7940 (LWP 28394)]
[New Thread 0x40448940 (LWP 28395)]
[New Thread 0x404c9940 (LWP 28396)]
[New Thread 0x4054a940 (LWP 28397)]
[New Thread 0x405cb940 (LWP 28398)]
[New Thread 0x4064c940 (LWP 28399)]
[New Thread 0x406cd940 (LWP 28400)]
[New Thread 0x406ee940 (LWP 28401)]
[New Thread 0x4070f940 (LWP 28402)]
[New Thread 0x40730940 (LWP 28403)]
[New Thread 0x407b1940 (LWP 28404)]
[New Thread 0x40832940 (LWP 28405)]
[Thread 0x40832940 (LWP 28405) exited]
[Thread 0x401c4940 (LWP 28391) exited]

Program received signal SIGINT, Interrupt.
0x00000036992cced2 in select () from /lib64/libc.so.6
(gdb) b VBoxVNC.cpp:667
No source file named VBoxVNC.cpp.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (VBoxVNC.cpp:667) pending.
(gdb) c
Continuing.
[New Thread 0x407c2940 (LWP 31663)]
14/01/2014 02:51:23 Listening for VNC connections on TCP port 5901
VRDE server is listening on port 5901.
[Switching to Thread 0x40041940 (LWP 29752)]

Breakpoint 1, VNCServerImpl::VRDEEnableConnections (hServer=<value optimized out>, fEnable=<value optimized out>)
    at /home/tests/VirtualBox-4.2.20/src/VBox/ExtPacks/VNC/VBoxVNC.cpp:667
667	/home/tests/VirtualBox-4.2.20/src/VBox/ExtPacks/VNC/VBoxVNC.cpp: No such file or directory.
	in /home/tests/VirtualBox-4.2.20/src/VBox/ExtPacks/VNC/VBoxVNC.cpp
(gdb) p vncServer->frameBuffer
$1 = 0x0
(gdb) c
Continuing.
[New Thread 0x40843940 (LWP 29785)]
14/01/2014 02:01:10 Listening for VNC connections on TCP port 5901
VRDE server is listening on port 5901.
[New Thread 0x41044940 (LWP 29786)]
14/01/2014 02:01:18   other clients:
[New Thread 0x41845940 (LWP 29865)]
[New Thread 0x42046940 (LWP 29866)]
14/01/2014 02:01:18 Client Protocol Version 3.8
14/01/2014 02:01:18 Protocol version sent 3.8, using 3.8
14/01/2014 02:01:18 rfbProcessClientSecurityType: executing handler for type 2
14/01/2014 02:01:19 Using image quality level 6 for client 127.0.0.1
14/01/2014 02:01:19 Enabling NewFBSize protocol extension for client 127.0.0.1
14/01/2014 02:01:19 Enabling LastRect protocol extension for client 127.0.0.1
14/01/2014 02:01:19 Enabling cursor position updates for client 127.0.0.1
14/01/2014 02:01:19 Enabling full-color cursor updates for client 127.0.0.1
14/01/2014 02:01:19 Using raw encoding for client 127.0.0.1
14/01/2014 02:01:19 Pixel format for client 127.0.0.1:
14/01/2014 02:01:19   32 bpp, depth 24, little endian
14/01/2014 02:01:19   true colour: max r 255 g 255 b 255, shift r 16 g 8 b 0

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x42046940 (LWP 29866)]
rfbTranslateWithRGBTables32to32 (table=0x6fe240 "", in=0x6e97a8, out=<value optimized out>, iptr=0x0, optr=0x6f20dc "���\021���",
    bytesBetweenInputLines=<value optimized out>, width=800, height=9) at tabletranstemplate.c:104
104     tabletranstemplate.c: No such file or directory.
        in tabletranstemplate.c
Current language:  auto; currently c
(gdb) bt
#0  rfbTranslateWithRGBTables32to32 (table=0x6fe240 "", in=0x6e97a8, out=<value optimized out>, iptr=0x0, optr=0x6f20dc "???\021???", 
    bytesBetweenInputLines=<value optimized out>, width=800, height=9) at tabletranstemplate.c:104
#1  0x00002aaaefe22e93 in rfbSendRectEncodingRaw (cl=0x6f1ff0, x=<value optimized out>, y=<value optimized out>, w=800, h=600) at rfbserver.c:3010
#2  0x00002aaaefe24505 in rfbSendFramebufferUpdate (cl=0x6f1ff0, givenUpdateRegion=<value optimized out>) at rfbserver.c:2840
#3  0x00002aaaefe21a3e in clientOutput (data=<value optimized out>) at main.c:497
#4  0x0000003699a064a7 in start_thread () from /lib64/libpthread.so.0
#5  0x00000036992d3c2d in clone () from /lib64/libc.so.6
(gdb) info locals
ip = (uint32_t *) 0x0
op = (uint32_t *) 0x6f20dc
ipextra = 0
opLineEnd = (uint32_t *) 0x6f2d5c
greenTable = (uint32_t *) 0x6fe640
blueTable = (uint32_t *) 0x6fea40
(gdb) up
#1  0x00002aaaefe22e93 in rfbSendRectEncodingRaw (cl=0x6f1ff0, x=<value optimized out>, y=<value optimized out>, w=800, h=600) at rfbserver.c:3010
3010    rfbserver.c: No such file or directory.
        in rfbserver.c
(gdb) p cl->scaledScreen->frameBuffer
$6 = 0x0
(gdb) p cl->screen->frameBuffer
$7 = 0x0
(gdb) 

Patch

This patch also fixes memory leaks with VNC handle when enabling/disabling VNC connections.

diff -Naur VirtualBox-4.3.6.orig/src/VBox/ExtPacks/VNC/VBoxVNC.cpp VirtualBox-4.3.6/src/VBox/ExtPacks/VNC/VBoxVNC.cpp
--- VirtualBox-4.3.6.orig/src/VBox/ExtPacks/VNC/VBoxVNC.cpp	2013-11-29 02:34:11.000000000 +0800
+++ VirtualBox-4.3.6/src/VBox/ExtPacks/VNC/VBoxVNC.cpp	2014-01-06 10:19:27.000000000 +0800
@@ -66,6 +66,7 @@
 public:
     VNCServerImpl()
     {
+        mVNCServer = NULL;
         mFrameBuffer = NULL;
         mScreenBuffer = NULL;
         mCursor = NULL;
@@ -79,6 +80,9 @@
         if (mCursor)
             rfbFreeCursor(mCursor);
         memset(szVNCPassword, '\0', sizeof(szVNCPassword));
+
+        if (mVNCServer)
+            rfbScreenCleanup(mVNCServer);
     }
 
     int Init(const VRDEINTERFACEHDR *pCallbacks, void *pvCallback);
@@ -257,11 +261,18 @@
 #endif
     LogFlowFunc(("enter\n"));
 
+    if (instance->mVNCServer)
+        rfbShutdownServer(instance->mVNCServer, TRUE);
+
+    if (!fEnable)
+        return VINF_SUCCESS;
+
     // query server for the framebuffer
     VRDEFRAMEBUFFERINFO info;
     int rc = instance->mCallbacks->VRDECallbackFramebufferQuery(instance->mCallback, 0, &info);
 
     rfbScreenInfoPtr vncServer = rfbGetScreen(0, NULL, info.cWidth, info.cHeight, 8, 3, VNC_SIZEOFRGBA);
+    rfbScreenInfoPtr oldVNCServer = instance->mVNCServer;
     instance->mVNCServer = vncServer;
     vncServer->serverFormat.redShift = 16;
     vncServer->serverFormat.greenShift = 8;
@@ -269,6 +280,11 @@
     vncServer->screenData = (void *)instance;
     vncServer->desktopName = "VBoxVNC";
 
+    VRDEResize(hServer);
+
+    if (oldVNCServer)
+        rfbScreenCleanup(oldVNCServer);
+
 #ifndef VBOX_USE_IPV6
 
     // get listen address

[Yonathan Randolph]

VRDEResize contains a bug: it uses the wrong comparison on the return value of VRDECallbackFramebufferQuery (which returns true or false, not negative error codes). When VirtualBox is restoring saved state, VRDECallbackFramebufferQuery returns false but VRDEResize thinks it succeeded and uses width, height, screen pointer from uninitialized stack anyway. In my testing, this resulted in sending the VNC client incorrect width=27167, height=0, which crashes my VNC client.

Once I fixed VRDEResize, it returns early so I can’t use it within VRDEEnableConnections to allocate the frame buffer. So you have to unconditionally allocate some frame buffer within VRDEEnableConnections. I recommend allocating a constant-sized frame buffer 640x480 (as opposed to 0x0) to avoid crashing VNC clients. When VirtualBox sends the first VRDEResize event, we will get the true size of the screen.

I made the following patch on r54144 of ​http://www.virtualbox.org/svn/vbox/trunk

diff --git a/src/VBox/ExtPacks/VNC/VBoxVNC.cpp b/src/VBox/ExtPacks/VNC/VBoxVNC.cpp
index d725a67..9c71579 100644
--- a/src/VBox/ExtPacks/VNC/VBoxVNC.cpp
+++ b/src/VBox/ExtPacks/VNC/VBoxVNC.cpp
@@ -260,12 +260,24 @@ DECLCALLBACK(int) VNCServerImpl::VRDEEnableConnections(HVRDESERVER hServer, bool
 #endif
     LogFlowFunc(("enter\n"));
 
-    // query server for the framebuffer
-    VRDEFRAMEBUFFERINFO info;
-    int rc = instance->mCallbacks->VRDECallbackFramebufferQuery(instance->mCallback, 0, &info);
+    // At this point, VRDECallbackFramebufferQuery will not succeed.
+    // Initialize VNC with 640x480 and wait for VRDEResize to get actual size.
+    int dummyWidth = 640, dummyHeight = 480;
 
-    rfbScreenInfoPtr vncServer = rfbGetScreen(0, NULL, info.cWidth, info.cHeight, 8, 3, VNC_SIZEOFRGBA);
+    rfbScreenInfoPtr vncServer = rfbGetScreen(0, NULL, dummyWidth, dummyHeight, 8, 3, VNC_SIZEOFRGBA);
     instance->mVNCServer = vncServer;
+
+    VRDEFRAMEBUFFERINFO info;
+    memset(&info, 0, sizeof(info));
+    info.cWidth = dummyWidth, info.cHeight = dummyHeight;
+    info.cBitsPerPixel = 24;
+    info.pu8Bits = NULL;
+    unsigned char *FrameBuffer = (unsigned char *)RTMemAlloc(info.cWidth * info.cHeight * VNC_SIZEOFRGBA); // RGBA
+    rfbNewFramebuffer(instance->mVNCServer, (char *)FrameBuffer, info.cWidth, info.cHeight, 8, 3, VNC_SIZEOFRGBA);
+    instance->mFrameBuffer = FrameBuffer;
+    instance->mScreenBuffer = (unsigned char *)info.pu8Bits;
+    instance->FrameInfo = info;
+
     vncServer->serverFormat.redShift = 16;
     vncServer->serverFormat.greenShift = 8;
     vncServer->serverFormat.blueShift = 0;
@@ -277,7 +289,7 @@ DECLCALLBACK(int) VNCServerImpl::VRDEEnableConnections(HVRDESERVER hServer, bool
     // get listen address
     char szAddress[VNC_ADDRESSSIZE + 1] = {0};
     uint32_t cbOut = 0;
-    rc = instance->mCallbacks->VRDECallbackProperty(instance->mCallback,
+    int rc = instance->mCallbacks->VRDECallbackProperty(instance->mCallback,
                                                     VRDE_QP_NETWORK_ADDRESS,
                                                     &szAddress, sizeof(szAddress), &cbOut);
     Assert(cbOut <= sizeof(szAddress));
@@ -715,8 +727,8 @@ DECLCALLBACK(void) VNCServerImpl::VRDEResize(HVRDESERVER hServer)
 {
     VNCServerImpl *instance = (VNCServerImpl *)hServer;
     VRDEFRAMEBUFFERINFO info;
-    int rc = instance->mCallbacks->VRDECallbackFramebufferQuery(instance->mCallback, 0, &info);
-    if (!RT_SUCCESS(rc))
+    bool available = instance->mCallbacks->VRDECallbackFramebufferQuery(instance->mCallback, 0, &info);
+    if (!available)
     {
         return;
     }
-- 

[Frank Mehnert]

Thank you for your analysis. As written on vbox-dev, a slightly modified version of the patch was committed to the repository and will be part of the next 4.3.x maintenance release.