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> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

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> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Gerd Hoffmann · Anthony Liguori
1 parent 11a1feb6
Showing 1 changed file with 75 additions and 45 deletions
vnc.c
@@ -216,6 +216,8 @@ static inline uint32_t vnc_has_feature(VncState *vs, int feature) {
 */
 static void vnc_update_client(void *opaque);
+static void vnc_disconnect_start(VncState *vs);
+static void vnc_disconnect_finish(VncState *vs);
 static void vnc_colordepth(VncState *vs);
@@ -652,9 +654,6 @@ static void send_framebuffer_update(VncState *vs, int x, int y, int w, int h)
 static void vnc_copy(VncState *vs, int src_x, int src_y, int dst_x, int dst_y, int w, int h)
 {
-    vs->force_update = 1;
-    vnc_update_client(vs);
-
     vnc_write_u8(vs, 0);  /* msg id */
     vnc_write_u8(vs, 0);
     vnc_write_u16(vs, 1); /* number of rects */
@@ -667,13 +666,22 @@ static void vnc_copy(VncState *vs, int src_x, int src_y, int dst_x, int dst_y, i
 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 = vd->clients;
-    while (vs != NULL) {
+    VncState *vs, *vn;
+
+    for (vs = vd->clients; vs != NULL; vs = vn) {
+        vn = vs->next;
+        if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT)) {
+            vs->force_update = 1;
+            vnc_update_client(vs);
+            /* vs might be free()ed here */
+        }
+    }
+
+    for (vs = vd->clients; vs != NULL; vs = vs->next) {
         if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT))
             vnc_copy(vs, src_x, src_y, dst_x, dst_y, w, h);
         else /* TODO */
             vnc_update(vs, dst_x, dst_y, w, h);
-        vs = vs->next;
     }
 }
@@ -798,6 +806,8 @@ 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);
     }
 }
@@ -868,6 +878,48 @@ 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)
+{
+    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
+    vnc_tls_client_cleanup(vs);
+#endif /* CONFIG_VNC_TLS */
+#ifdef CONFIG_VNC_SASL
+    vnc_sasl_client_cleanup(vs);
+#endif /* CONFIG_VNC_SASL */
+    audio_del(vs);
+
+    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)
+        dcl->idle = 1;
+
+    qemu_free(vs->server.ds->data);
+    qemu_free(vs->server.ds);
+    qemu_free(vs->guest.ds);
+    qemu_free(vs);
+}
 int vnc_client_io_error(VncState *vs, int ret, int last_errno)
 {
@@ -885,39 +937,9 @@ 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
-        vnc_tls_client_cleanup(vs);
-#endif /* CONFIG_VNC_TLS */
-#ifdef CONFIG_VNC_SASL
-        vnc_sasl_client_cleanup(vs);
-#endif /* CONFIG_VNC_SASL */
-        audio_del(vs);
-
-        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)
-            dcl->idle = 1;
-
-        qemu_free(vs->server.ds->data);
-        qemu_free(vs->server.ds);
-        qemu_free(vs->guest.ds);
-        qemu_free(vs);
+        VNC_DEBUG("Closing down client sock: ret %d, errno %d\n",
+                  ret, ret < 0 ? last_errno : 0);
+        vnc_disconnect_start(vs);
         return 0;
     }
@@ -927,7 +949,8 @@ int vnc_client_io_error(VncState *vs, int ret, int last_errno)
 void vnc_client_error(VncState *vs)
 {
-    vnc_client_io_error(vs, -1, EINVAL);
+    VNC_DEBUG("Closing down client sock: protocol error\n");
+    vnc_disconnect_start(vs);
 }
@@ -1110,16 +1133,21 @@ void vnc_client_read(void *opaque)
     else
 #endif /* CONFIG_VNC_SASL */
         ret = vnc_client_read_plain(vs);
-    if (!ret)
+    if (!ret) {
+        if (vs->csock == -1)
+            vnc_disconnect_finish(vs);
         return;
+    }
     while (vs->read_handler && vs->input.offset >= vs->read_handler_expect) {
         size_t len = vs->read_handler_expect;
         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));
@@ -1134,7 +1162,7 @@ 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);
     }
@@ -1175,7 +1203,7 @@ void vnc_write_u8(VncState *vs, uint8_t value)
 void vnc_flush(VncState *vs)
 {
-    if (vs->output.offset)
+    if (vs->csock != -1 && vs->output.offset)
         vnc_client_write(vs);
 }
@@ -2009,11 +2037,13 @@ static void vnc_connect(VncDisplay *vd, int csock)
     vnc_write(vs, "RFB 003.008\n", 12);
     vnc_flush(vs);
     vnc_read_when(vs, protocol_version, 12);
-    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)