From 198a0039c5fca224a77e9761e2350dd9cc102ad0 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann Date: Tue, 16 Jun 2009 14:19:48 +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 Signed-off-by: Anthony Liguori --- vnc.c | 120 ++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 75 insertions(+), 45 deletions(-) diff --git a/vnc.c b/vnc.c index e1ca9f8c75..2006280ff2 100644 --- a/vnc.c +++ b/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)