From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1M8Vcn-0000UI-0l
	for qemu-devel@nongnu.org; Mon, 25 May 2009 04:33:37 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1M8Vci-0000Tw-Tt
	for qemu-devel@nongnu.org; Mon, 25 May 2009 04:33:36 -0400
Received: from [199.232.76.173] (port=49260 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1M8Vci-0000Tt-PE
	for qemu-devel@nongnu.org; Mon, 25 May 2009 04:33:32 -0400
Received: from mx2.redhat.com ([66.187.237.31]:36031)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <markmc@redhat.com>) id 1M8Vci-0003OQ-9f
	for qemu-devel@nongnu.org; Mon, 25 May 2009 04:33:32 -0400
Subject: Re: [Qemu-devel] [STABLE] [BUG] VNC mode can crash QEMU
From: Mark McLoughlin <markmc@redhat.com>
In-Reply-To: <4A19A95A.2050801@mail.berlios.de>
References: <4A19A95A.2050801@mail.berlios.de>
Content-Type: text/plain
Date: Mon, 25 May 2009 09:33:24 +0100
Message-Id: <1243240404.18067.6.camel@blaa>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Reply-To: Mark McLoughlin <markmc@redhat.com>
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Stefan Weil <weil@mail.berlios.de>
Cc: QEMU Developers <qemu-devel@nongnu.org>

On Sun, 2009-05-24 at 22:08 +0200, Stefan Weil wrote:
> Hello,
> 
> this scenario crashs the latest QEMU HEAD on Windows
> (Linux users, please note that the bug is not Windows related,
> so don't stop reading!):
> 
> * run qemu.exe -vnc :0
> * connect using UltraVnc
> * select fuzzy screen mode in UltraVnc
> 
> => segfault of qemu.exe
> 
> The crash is caused by VNC protocols which are unsupported
> by QEMU - in my case it was the fuzzy screen mode protocol.
> These protocols trigger a call stack which releases the
> VncState vs:
> 
> qemu_free(vs)
> vnc_client_io_error(vs, ...)
> vnc_client_error(vs, ...)
> protocol_client_msg(vs, ...)
> vnc_client_read
> main_loop_wait
> main_loop
> 
> The default handlers for unimplemented protocols in
> protocol_client_msg call vnc_client_error which finally
> calls qemu_free for the current VncState vs.
> 
> vs is then used in protocol_client_msg and vnc_client_read
> although it is no longer valid. On Windows, this results
> in a crash, for other host platforms, the result depends
> on implementation details of the C library.
> 
> In any case, access to a data structure after a free()
> is a bug.

Yep, I looked into a similar report recently:

  https://bugzilla.redhat.com/show_bug.cgi?id=501131

Basically, vnc_flush() or vnc_update_client() will handle an error by
freeing VncState, yet we will not detect this error and happily continue
on using the freed VncState.

The approach I tried in the patch attached to the bug isn't going to
work, I think. Instead, we should just mark the VncState with a
"deleted" flag on I/O error and only free it later on when the
in-progress protocol step has completed.

Cheers,
Mark.