Hi all- qemu-kvm quite reliably crashes when running with a VNC viewer connected at 1400x1050. (The crash happens when changing resolution *from* 1400x1050 or disconnecting and reconnecting a client.)
The problem is that vnc_refresh_server_surface overruns server->data and corrupts heap metadata, causing glibc to explode when the buffer is freed. The patch below (obviously only for testing) demonstrates the problem and prevents the crash, but it introduces a black line 8 pixels wide on the right when running at 1400x1050. I'm not sure what's going on -- either I messed something up (entirely possible) or there's another bug somewhere else. Note: I've only reproduced this on Avi's qemu-kvm and on Fedora's packages -- upstream qemu crashes somewhere else. The code in question looks the same upstream, though. --Andy diff --git a/vnc.c b/vnc.c index e678fcc..f9d0ad3 100644 --- a/vnc.c +++ b/vnc.c @@ -527,6 +527,15 @@ static void vnc_dpy_resize(DisplayState *ds) vd->server->data = qemu_mallocz(vd->server->linesize * vd->server->height); + printf("vnc_dpy_resize: linesize = %d, height = %d, bpp = %d, " + "width = %d, height = %d, width%%16 = %d, data = %p\n", + vd->server->linesize, vd->server->height, + ds_get_bytes_per_pixel(ds), + ds_get_width(ds), ds_get_height(ds), + ds_get_width(ds) % 16, + vd->server->data); + + /* guest surface */ if (!vd->guest.ds) vd->guest.ds = qemu_mallocz(sizeof(*vd->guest.ds)); @@ -1740,7 +1749,7 @@ static void framebuffer_update_request(VncState *vs, int incremental, vs->force_update = 1; for (i = 0; i < h; i++) { vnc_set_bits(vs->dirty[y_position + i], - (ds_get_width(vs->ds) / 16), VNC_DIRTY_WORDS); + ((ds_get_width(vs->ds) + 15) / 16), VNC_DIRTY_WORDS); } } } @@ -2320,8 +2329,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd) * Check and copy modified bits from guest to server surface. * Update server dirty map. */ - vnc_set_bits(width_mask, (ds_get_width(vd->ds) / 16), VNC_DIRTY_WORDS); - cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds); + vnc_set_bits(width_mask, ((ds_get_width(vd->ds) + 15) / 16), VNC_DIRTY_WORDS); guest_row = vd->guest.ds->data; server_row = vd->server->data; for (y = 0; y < vd->guest.ds->height; y++) { @@ -2332,12 +2340,20 @@ static int vnc_refresh_server_surface(VncDisplay *vd) guest_ptr = guest_row; server_ptr = server_row; + int bpp = ds_get_bytes_per_pixel(vd->ds); + cmp_bytes = 16 * bpp; for (x = 0; x < vd->guest.ds->width; x += 16, guest_ptr += cmp_bytes, server_ptr += cmp_bytes) { if (!vnc_get_bit(vd->guest.dirty[y], (x / 16))) continue; vnc_clear_bit(vd->guest.dirty[y], (x / 16)); + + // If this happens, we'll overrun the line. If it + // happens on the last row, we'll corrupt the heap. + if (cmp_bytes > bpp * (vd->guest.ds->width - x)) + cmp_bytes = bpp * (vd->guest.ds->width - x); + if (memcmp(server_ptr, guest_ptr, cmp_bytes) == 0) continue; memcpy(server_ptr, guest_ptr, cmp_bytes);