From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O6Tq1-0001ml-6S for qemu-devel@nongnu.org; Mon, 26 Apr 2010 15:19:25 -0400 Received: from [140.186.70.92] (port=34595 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O6Tpx-0001id-Tk for qemu-devel@nongnu.org; Mon, 26 Apr 2010 15:19:24 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O6Tpv-0004Xu-Tj for qemu-devel@nongnu.org; Mon, 26 Apr 2010 15:19:21 -0400 Received: from mail-qy0-f188.google.com ([209.85.221.188]:56445) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O6Tpv-0004Xq-KY for qemu-devel@nongnu.org; Mon, 26 Apr 2010 15:19:19 -0400 Received: by qyk26 with SMTP id 26so5640408qyk.19 for ; Mon, 26 Apr 2010 12:19:19 -0700 (PDT) Message-ID: <4BD5E733.7040501@codemonkey.ws> Date: Mon, 26 Apr 2010 14:19:15 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] Possible race condition in VNC display resizing References: <20100426181937.GF12919@redhat.com> In-Reply-To: <20100426181937.GF12919@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: qemu-devel On 04/26/2010 01:19 PM, Daniel P. Berrange wrote: > In tracking down a rare crash in GTK-VNC clients, I think I've discovered > a race in the way QEMU processes resize events during initial connection > handshake > > The symptom of the problem is that the client receives a framebuffer > update which stretches outside the boundaries of the guest framebuffer, > as known to the VNC client. > > This GTK-VNC debug trace shows the initial framebuffer size is '640x480' > but the first framebuffer update received is for a region of size '720x400' > > (virt-viewer:20171): gtk-vnc-DEBUG: Pixel format BPP: 32, Depth: 24, Byte order: 1234, True color: 1 > Mask red: 255, green: 255, blue: 255 > Shift red: 16, green: 8, blue: 0 > (virt-viewer:20171): gtk-vnc-DEBUG: Display name 'QEMU (f14i686)' > (virt-viewer:20171): gtk-vnc-DEBUG: Setting depth color to 24 (32 bpp) > (virt-viewer:20171): gtk-vnc-DEBUG: Do resize 0x117ec10 1 640 480 0 > (virt-viewer:20171): gtk-vnc-DEBUG: Visual mask: 16711680 65280 255 > shift: 16 8 0 > (virt-viewer:20171): gtk-vnc-DEBUG: Mask local: 255 255 255 > remote: 255 255 255 > merged: 255 255 255 > (virt-viewer:20171): gtk-vnc-DEBUG: Pixel shifts > right: 16 8 0 > left: 16 8 0 > (virt-viewer:20171): gtk-vnc-DEBUG: Running main loop > (virt-viewer:20171): gtk-vnc-DEBUG: Expose 0x0 @ 640,480 > (virt-viewer:20171): gtk-vnc-DEBUG: FramebufferUpdate(-258, 0, 0, 720, 400) > (virt-viewer:20171): gtk-vnc-DEBUG: Using evdev keycode mapping > (virt-viewer:20171): gtk-vnc-DEBUG: FramebufferUpdate(-257, 0, 0, 720, 400) > (virt-viewer:20171): gtk-vnc-DEBUG: FramebufferUpdate(5, 0, 0, 720, 400) > (virt-viewer:20171): gtk-vnc-DEBUG: Framebuffer update 720x400 outside boundary 640x480 > > > I can reproduce this perhaps 1 time in 5, if I connect the VNC client at > exactly the time the QEMU guest starts while also using XSync. > > At a protocol level the initial startup sequence is > > 1. Client& server negotiate version > 2. Client& server negotiate auth > 3. Client sets the 'shared flag' > 4. Server sends width + height > 5. Server sends pixel format > 6. Server sends display name > 7. Client sends its supported framebuffer update encodings > 8. Client requests first framebuffer update > 9. Server sends first framebuffer update > > What I believe I am seeing is the guest resizing its display at some point > between steps 4& 9, and QEMU forgetting to send a DESKTOPRESIZE message > before doing step 9. > > Looking at the QEMU code seems to confirm this hypothesis. In vnc.c in the > desktop-resize messages are triggered from vnc_dpy_resize(). This code > checks: > > if (size_changed) { > if (vs->csock != -1&& vnc_has_feature(vs, VNC_FEATURE_RESIZE)) { > ....send the resize message .... > } > } > This is tough. Technically speaking, we're under no obligation to send a DesktopResize because the event happened before the SetEncodings message was received. I agree with you though that it's not terribly useful if we don't. I think in the very least, we ought to send a DesktopResize immediately after we receive a SetEncodings message. > The VNC_FEATURE_RESIZE feature does not get set until step 7. So there is > a clear window between step 4 and 7 when vnc_dpy_resize() can be invoked > where the test against VNC_FEATURE_RESIZE returns false& thus the client > does not get notified of the resize even though it is capable of handling > it. > > Furthermore in the case where desktop resize is not supported, QEMU is not > clipping its framebuffer updates to the clients' view of the framebuffer > size. > > A crude fix would be to send an immediate DESKTOPRESIZE update the moment > the client tells the server is supports the DESKTOPRESIZE psuedo encoding. > > The more involved fix is to record the clients expected width+height in > the VncState struct per client. Instead of sending the DESKTOPRESIZE > updates directly from the vnc_dpy_resize() method, simply update the > VncState struct with new width/height. Then at the send_framebuffer_update() > method check to see if a DESKTOPRESIZE needs to be triggered, or if not > supported, clip the update to the client's boundary. > I'm surprised we don't clip--and that we get away with that. I think practically speaking, all clients support DesktopResize. I think we should clip, but I don't think that negates the need to generate a DesktopResize after receiving a SetEncoding. Waiting until the next framebuffer update doesn't seem like it buys us that much. Regards, Anthony Liguori > Thoughts ? > > Regards, > Daniel >