From 4143972fb4bea0a6a6f9c11e95b5b6a5c8b4d48b Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 16 Jun 2009 14:28:18 +0200
Subject: [PATCH 2/2] vnc: rework VncState release workflow.

Hi,

Split socket closing and releasing of VncState into two steps.  First
close the socket and set the variable to -1 to indicate shutdown in
progress.  Do the actual release in a few places where we can be sure it
doesn't cause trouble in form of use-after-free.  Add some checks for a
valid socket handle to make sure we don't try to use the closed socket.

BZ #505641.
Upstream status: broken too, just mailed a similar patch to qemu-devel.

please ack,
   Gerd

>From 4fa3a74a71d4f2a8b0e3389d2275b03c4d582cff Mon Sep 17 00:00:00 2001
From: Gerd Hoffmann <kraxel@redhat.com>
Date: Tue, 16 Jun 2009 11:49:33 +0200
Subject: [PATCH] vnc: rework VncState release workflow.

Split socket closing and releasing of VncState into two steps.  First
close the socket and set the variable to -1 to indicate shutdown in
progress.  Do the actual release in a few places where we can be sure it
doesn't cause trouble in form of use-after-free.  Add some checks for a
valid socket handle to make sure we don't try to use the closed socket.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Message-ID: <4A378FE2.5070706@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Bugzilla: 505641
RH-Upstream-status: similar-patch-submitted(qemu-devel)
Acked-by: john cooper <john.cooper@redhat.com>
Acked-by: Juan Quintela <quintela@redhat.com>
Acked-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu/vnc.c |  112 +++++++++++++++++++++++++++++++++++++-----------------------
 1 files changed, 69 insertions(+), 43 deletions(-)

diff --git a/qemu/vnc.c b/qemu/vnc.c
index 4a46bae..e0ce4cf 100644
--- a/qemu/vnc.c
+++ b/qemu/vnc.c
@@ -228,6 +228,8 @@ static void vnc_update_client(void *opaque);
 static void vnc_client_read(void *opaque);
 
 static void vnc_colordepth(VncState *vs, int depth);
+static void vnc_disconnect_start(VncState *vs);
+static void vnc_disconnect_finish(VncState *vs);
 
 static inline void vnc_set_bit(uint32_t *d, int k)
 {
@@ -558,11 +560,14 @@ static void vnc_copy_msg(VncState *vs, int src_x, int src_y, int dst_x, int dst_
 static void vnc_dpy_copy(DisplayState *ds, int src_x, int src_y, int dst_x, int dst_y, int w, int h)
 {
     VncDisplay *vd = ds->opaque;
-    VncState *vs;
+    VncState *vs, *vn;
 
-    for (vs = vd->clients; vs != NULL; vs = vs->next) {
-        if (vs->has_copyrect)
+    for (vs = vd->clients; vs != NULL; vs = vn) {
+        vn = vs->next;
+        if (vs->has_copyrect) {
             vnc_update_client(vs);
+            /* vs might be free()ed here */
+        }
     }
 
     vnc_copy_data(ds->data, ds->linesize, ds->depth / 8,
@@ -685,8 +690,9 @@ static void vnc_update_client(void *opaque)
 
     if (vs->csock != -1) {
         qemu_mod_timer(vs->timer, qemu_get_clock(rt_clock) + VNC_REFRESH_INTERVAL);
+    } else {
+        vnc_disconnect_finish(vs);
     }
-
 }
 
 static void buffer_reserve(Buffer *buffer, size_t len)
@@ -782,6 +788,51 @@ static void audio_del(VncState *vs)
     }
 }
 
+static void vnc_disconnect_start(VncState *vs)
+{
+    if (vs->csock == -1)
+        return;
+    qemu_set_fd_handler2(vs->csock, NULL, NULL, NULL, NULL);
+    closesocket(vs->csock);
+    vs->csock = -1;
+}
+
+static void vnc_disconnect_finish(VncState *vs)
+{
+    VncState *p, *parent = NULL;
+
+    qemu_del_timer(vs->timer);
+    qemu_free_timer(vs->timer);
+    if (vs->input.buffer)
+        qemu_free(vs->input.buffer);
+    if (vs->output.buffer)
+        qemu_free(vs->output.buffer);
+#ifdef CONFIG_VNC_TLS
+    if (vs->tls_session) {
+        gnutls_deinit(vs->tls_session);
+        vs->tls_session = NULL;
+    }
+#endif /* CONFIG_VNC_TLS */
+    audio_del(vs);
+    if (vs->vd->display)
+        term_printf_async(VNC_ASYNC_EVENT, "VNC: Closing down connection %s\n", vs->vd->display);
+
+    for (p = vs->vd->clients; p != NULL; p = p->next) {
+        if (p == vs) {
+            if (parent)
+                parent->next = p->next;
+            else
+                vs->vd->clients = p->next;
+            break;
+        }
+        parent = p;
+    }
+    if (!vs->vd->clients)
+        vs->ds->idle = 1;
+    qemu_free(vs->old_data);
+    qemu_free(vs);
+}
+
 static int vnc_client_io_error(VncState *vs, int ret, int last_errno)
 {
     if (ret == 0 || ret == -1) {
@@ -798,40 +849,8 @@ static int vnc_client_io_error(VncState *vs, int ret, int last_errno)
             }
         }
 
-	VNC_DEBUG("Closing down client sock %d %d\n", ret, ret < 0 ? last_errno : 0);
-	qemu_set_fd_handler2(vs->csock, NULL, NULL, NULL, NULL);
-	closesocket(vs->csock);
-        qemu_del_timer(vs->timer);
-        qemu_free_timer(vs->timer);
-        if (vs->input.buffer)
-            qemu_free(vs->input.buffer);
-        if (vs->output.buffer)
-            qemu_free(vs->output.buffer);
-#ifdef CONFIG_VNC_TLS
-	if (vs->tls_session) {
-	    gnutls_deinit(vs->tls_session);
-	    vs->tls_session = NULL;
-	}
-#endif /* CONFIG_VNC_TLS */
-        audio_del(vs);
-        if (vs->vd->display)
-            term_printf_async(VNC_ASYNC_EVENT, "VNC: Closing down connection %s\n", vs->vd->display);
-
-        VncState *p, *parent = NULL;
-        for (p = vs->vd->clients; p != NULL; p = p->next) {
-            if (p == vs) {
-                if (parent)
-                    parent->next = p->next;
-                else
-                    vs->vd->clients = p->next;
-                break;
-            }
-            parent = p;
-        }
-        if (!vs->vd->clients)
-            vs->ds->idle = 1;
-        qemu_free(vs->old_data);
-        qemu_free(vs);
+        VNC_DEBUG("Closing down client sock %d %d\n", ret, ret < 0 ? last_errno : 0);
+        vnc_disconnect_start(vs);
 
 	return 0;
     }
@@ -900,8 +919,11 @@ static void vnc_client_read(void *opaque)
 #endif /* CONFIG_VNC_TLS */
 	ret = recv(vs->csock, buffer_end(&vs->input), 4096, 0);
     ret = vnc_client_io_error(vs, ret, socket_error());
-    if (!ret)
+    if (!ret) {
+        if (vs->csock == -1)
+            vnc_disconnect_finish(vs);
 	return;
+    }
 
     vs->input.offset += ret;
 
@@ -910,8 +932,10 @@ static void vnc_client_read(void *opaque)
 	int ret;
 
 	ret = vs->read_handler(vs, vs->input.buffer, len);
-	if (vs->csock == -1)
+	if (vs->csock == -1) {
+            vnc_disconnect_finish(vs);
 	    return;
+        }
 
 	if (!ret) {
 	    memmove(vs->input.buffer, vs->input.buffer + len, (vs->input.offset - len));
@@ -926,7 +950,7 @@ static void vnc_write(VncState *vs, const void *data, size_t len)
 {
     buffer_reserve(&vs->output, len);
 
-    if (buffer_empty(&vs->output)) {
+    if (vs->csock != -1 && buffer_empty(&vs->output)) {
 	qemu_set_fd_handler2(vs->csock, NULL, vnc_client_read, vnc_client_write, vs);
     }
 
@@ -967,7 +991,7 @@ static void vnc_write_u8(VncState *vs, uint8_t value)
 
 static void vnc_flush(VncState *vs)
 {
-    if (vs->output.offset)
+    if (vs->csock != -1 && vs->output.offset)
 	vnc_client_write(vs);
 }
 
@@ -2364,11 +2388,13 @@ static void vnc_connect(VncDisplay *vd, int csock)
     vs->has_hextile = 0;
     vs->has_WMVi = 0;
     vs->has_copyrect = 0;
-    vnc_update_client(vs);
     reset_keys(vs);
 
     vs->next = vd->clients;
     vd->clients = vs;
+
+    vnc_update_client(vs);
+    /* vs might be free()ed here */
 }
 
 static void vnc_listen_read(void *opaque)
-- 
1.6.3.rc4.29.g8146