From patchwork Wed Aug 19 22:47:39 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chris Webb X-Patchwork-Id: 42797 Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n7JMmRTu025867 for ; Wed, 19 Aug 2009 22:48:27 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753397AbZHSWr6 (ORCPT ); Wed, 19 Aug 2009 18:47:58 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753356AbZHSWr6 (ORCPT ); Wed, 19 Aug 2009 18:47:58 -0400 Received: from alpha.arachsys.com ([91.203.57.7]:43717 "EHLO alpha.arachsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753354AbZHSWr5 (ORCPT ); Wed, 19 Aug 2009 18:47:57 -0400 Received: from arachsys.demon.co.uk ([83.104.159.199] helo=miranda.arachsys.com) by alpha.arachsys.com with esmtpa (Exim 4.52) id 1MdtwS-0003oK-R5; Wed, 19 Aug 2009 23:47:41 +0100 Received: from chris by miranda.arachsys.com with local (Exim 4.52) id 1MdtwR-00081X-KF; Wed, 19 Aug 2009 23:47:39 +0100 Date: Wed, 19 Aug 2009 23:47:39 +0100 From: Chris Webb To: Avi Kivity , Gerd Hoffmann Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org Subject: Re: qemu-kvm segfaults in qemu_del_timer (0.10.5 and 0.10.6) Message-ID: <20090819224739.GB17276@arachsys.com> References: <20090812150159.GW5348@arachsys.com> <4A82E200.3040107@redhat.com> <20090812162401.GB8115@arachsys.com> <20090813122333.GA2863@arachsys.com> <4A840A3E.1040400@redhat.com> <20090813124350.GA21678@arachsys.com> <20090813124546.GB21678@arachsys.com> <4A840DE0.2060202@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4A840DE0.2060202@redhat.com> User-Agent: Mutt/1.5.19 (2009-01-05) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org Avi Kivity writes: > master branch has a patch that fixes a use-after-free when > disconnecting. Unfortunately it doesn't port cleanly to stable-0.10. I've collected quite a few more core dumps from segfaults of client virtual machines now, all of which are VNC related and could quite plausibly be use of a VncState after it has been freed. I looked at Gerd's patch [198a00: vnc: rework VncState release workflow] and have taken a stab at the equivalent patch for stable qemu & qemu-kvm 0.10. With the following applied, VNC connections and disconnections still work correctly, so it doesn't horribly break anything, but I can't immediately confirm whether it will cure the rare segfaults as I haven't yet found a rapid way of reproducing the crashes other than by waiting for one. --- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/vnc.c b/vnc.c --- a/vnc.c +++ b/vnc.c @@ -200,6 +200,8 @@ static void vnc_write_u8(VncState *vs, uint8_t value); static void vnc_flush(VncState *vs); static void vnc_update_client(void *opaque); +static void vnc_disconnect_start(VncState *vs); +static void vnc_disconnect_finish(VncState *vs); static void vnc_client_read(void *opaque); static void vnc_colordepth(VncState *vs); @@ -633,8 +635,6 @@ static void vnc_copy(VncState *vs, int src_x, int src_y, int dst_x, int dst_y, int w, int h) { - vnc_update_client(vs); - vnc_write_u8(vs, 0); /* msg id */ vnc_write_u8(vs, 0); vnc_write_u16(vs, 1); /* number of rects */ @@ -647,13 +647,21 @@ 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)) { + 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; } } @@ -763,6 +771,8 @@ if (vs->csock != -1) { qemu_mod_timer(vs->timer, qemu_get_clock(rt_clock) + VNC_REFRESH_INTERVAL); + } else { + vnc_disconnect_finish(vs); } } @@ -832,6 +842,47 @@ } } +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 + if (vs->tls_session) { + gnutls_deinit(vs->tls_session); + vs->tls_session = NULL; + } +#endif /* CONFIG_VNC_TLS */ + 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->old_data); + qemu_free(vs); +} + static int vnc_client_io_error(VncState *vs, int ret, int last_errno) { if (ret == 0 || ret == -1) { @@ -849,36 +900,7 @@ } 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); - - 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->old_data); - qemu_free(vs); + vnc_disconnect_start(vs); return 0; } @@ -887,7 +909,8 @@ static 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); } static void vnc_client_write(void *opaque) @@ -947,8 +970,11 @@ #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; @@ -957,8 +983,10 @@ 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)); @@ -973,7 +1001,7 @@ { 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); } @@ -1014,7 +1042,7 @@ static void vnc_flush(VncState *vs) { - if (vs->output.offset) + if (vs->csock != -1 && vs->output.offset) vnc_client_write(vs); } @@ -2282,11 +2310,13 @@ vnc_read_when(vs, protocol_version, 12); memset(vs->old_data, 0, ds_get_linesize(vs->ds) * ds_get_height(vs->ds)); memset(vs->dirty_row, 0xFF, sizeof(vs->dirty_row)); - 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)