fix vnc regression
PATCH RESEND v2 1/2 fix vnc regression
This patch fix the following two regressions:
1. we should use bitmap_set() and bitmap_clear() to replace vnc_set_bits().
2. The unit of bitmap_intersects()'third parameter is bit, not words. But we pass the num of words to bitmap_intersects().
Changes from v1 to v2:
1. fix the third argument of bitmap_clear()
--- ui/vnc.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-)
ui/vnc.c +8 -3
@@ -1645,17 +1645,21 @@ static void framebuffer_update_request(VncState *vs, int incremental, int x_position, int y_position, int w, int h) { + int i; +const size_t width =ds_get_width (vs->ds) /16 ; +if (y_position >ds_get_height (vs->ds)) y_position =ds_get_height (vs->ds);if (y_position + h >=ds_get_height (vs->ds)) h =ds_get_height (vs->ds) - y_position; -int i; vs->need_update =1 ;if (!incremental) { vs->force_update =1 ;for (i =0 ; i < h; i++) { -bitmap_set (vs->dirty[y_position + i], x_position /16 , w /16 ); +bitmap_set (vs->dirty[y_position + i],0 , width); +bitmap_clear (vs->dirty[y_position + i], width, +VNC_DIRTY_WORDS *BITS_PER_LONG - width); } } }
@@ -2406,7 +2410,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd) guest_row = vd->guest.ds->data; server_row = vd->server->data;for (y =0 ; y < vd->guest.ds->height; y++) { -if (bitmap_intersects (vd->guest.dirty[y], width_mask,VNC_DIRTY_WORDS )) { +if (bitmap_intersects (vd->guest.dirty[y], width_mask, +VNC_DIRTY_WORDS *BITS_PER_LONG )) {int x;uint8_t *guest_ptr;uint8_t *server_ptr;
Signature · 1 line
1.7.1
PATCH RESEND 2/2 vnc: Fix heap corruption
This bug is reported by Stefan Weil:
========
Commit bc2429b9174ac2d3c56b7fd35884b0d89ec7fb02 introduced a severe bug (heap corruption).
bitmap_clear was called with a wrong argument
which caused out-of-bound writes to width_mask.
This bug was detected with QEMU running on windows. It also occurs with wine:
*** stack smashing detected ***: terminated
wine: Unhandled illegal instruction at address 0x6115c7 (thread 0009), starting debugger...
The bug is not windows specific!
========
The third argument of bitmap_clear() is number of bits to be cleared, but we pass the end bits to be cleared to bitmap_clear().
--- ui/vnc.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
ui/vnc.c +3 -3
@@ -2390,6 +2390,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)unsigned long width_mask[VNC_DIRTY_WORDS ];VncState *vs;int has_dirty =0 ; +const size_t width =ds_get_width (vd->ds) /16 ;struct timeval tv = {0 ,0 };
@@ -2403,9 +2404,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)* Check and copy modified bits from guest to server surface. * Update server dirty map. */ -bitmap_set (width_mask,0 , (ds_get_width (vd->ds) /16 )); -bitmap_clear (width_mask, (ds_get_width (vd->ds) /16 ), -VNC_DIRTY_WORDS *BITS_PER_LONG ); +bitmap_set (width_mask,0 , width); +bitmap_clear (width_mask, width,VNC_DIRTY_WORDS *BITS_PER_LONG - width); cmp_bytes =16 *ds_get_bytes_per_pixel (vd->ds); guest_row = vd->guest.ds->data; server_row = vd->server->data;
Signature · 1 line
1.7.1
[Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
Quoted 14 lines · Wen Congyang
This bug is reported by Stefan Weil:
========
Commitbc2429b9174ac2d3c56b7fd35884b0d89ec7fb02introduced a severe bug (heap corruption).
bitmap_clearwas called with a wrong argument
which caused out-of-bound writes towidth_mask.This bug was detected with QEMU running on windows. It also occurs with wine:
*** stack smashing detected ***: terminated
wine: Unhandled illegal instruction at address0x6115c7(thread 0009), starting debugger...
Quoted 8 lines
The bug is not windows specific!
========The third argument of
bitmap_clear()is number of bits to be cleared, but we pass the end bits to be cleared tobitmap_clear().Signed-off-by Wen Congyang <[email protected]>Reported-by Stefan Weil <[email protected]>
Quoted 32 lines
---
ui/vnc.c | 6 +++---1 files changed, 3 insertions(+), 3 deletions(-)ui/vnc.c +3
-4@@ -2390,6 +2390,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd) unsigned long width_mask[VNC_DIRTY_WORDS]; VncState *vs; int has_dirty = 0; + const size_t width = ds_get_width(vd->ds) / 16; struct timeval tv = { 0, 0 };@@ -2403,9 +2404,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)* Check and copy modified bits from guest to server surface. * Update server dirty map. */ - bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16)); - bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16), - VNC_DIRTY_WORDS * BITS_PER_LONG); + bitmap_set(width_mask, 0, width); + bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width); cmp_bytes = 16 * ds_get_bytes_per_pixel(vd->ds); guest_row = vd->guest.ds->data; server_row = vd->server->data; --1.7.1
Signature · 2 lines
Corentin Chary http://xf.iksaif.net
[Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
Am 02.03.2011 11:57, schrieb Corentin Chary:
Quoted 18 lines · Wen Congyang
Quoted 17 lines
This bug is reported by Stefan Weil:
========
Commitbc2429b9174ac2d3c56b7fd35884b0d89ec7fb02introduced a severe bug (heap corruption).
bitmap_clearwas called with a wrong argument
which caused out-of-bound writes towidth_mask.This bug was detected with QEMU running on windows. It also occurs with wine:
*** stack smashing detected ***: terminated
wine: Unhandled illegal instruction at address0x6115c7(thread 0009), starting debugger...The bug is not windows specific!
========
Quoted 8 lines
Quoted 5 lines
The third argument of
bitmap_clear()is number of bits to be cleared, but we pass the end bits to be cleared tobitmap_clear().Signed-off-by Wen Congyang <[email protected]>Reported-by Stefan Weil <[email protected]>Acked-by Corentin Chary <[email protected]>
No. I dont't think that the third parameter of bitmap_clear is ok like that. See my patch for the correct value.
My own patch is also incomplete, so I'll send an update.
Stefan
Quoted 35 lines
Quoted 32 lines
--- ui/vnc.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)ui/vnc.c +3
-4@@ -2390,6 +2390,7 @@ static int vnc_refresh_server_surface(VncDisplay *vd)unsigned long width_mask[VNC_DIRTY_WORDS ];VncState *vs;int has_dirty =0 ; +const size_t width =ds_get_width (vd->ds) /16 ;struct timeval tv = {0 ,0 };@@ -2403,9 +2404,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd)* Check and copy modified bits from guest to server surface. * Update server dirty map. */ -bitmap_set (width_mask,0 , (ds_get_width (vd->ds) /16 )); -bitmap_clear (width_mask, (ds_get_width (vd->ds) /16 ), -VNC_DIRTY_WORDS *BITS_PER_LONG ); +bitmap_set (width_mask,0 , width); +bitmap_clear (width_mask, width,VNC_DIRTY_WORDS *BITS_PER_LONG - width); cmp_bytes =16 *ds_get_bytes_per_pixel (vd->ds); guest_row = vd->guest.ds->data; server_row = vd->server->data; --1.7.1
Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
Quoted 2 lines · Stefan Weil
No. I dont't think that the third parameter of
bitmap_clearis ok like that. See my patch for the correct value.
Wen's patch:
+ const size_t width = ds_get_width(vd->ds) / 16;
[...]
- bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16));
- bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
- VNC_DIRTY_WORDS * BITS_PER_LONG);
+ bitmap_set(width_mask, 0, width);
+ bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);
Your patch:
bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
- VNC_DIRTY_WORDS * BITS_PER_LONG);
+ (VNC_MAX_WIDTH - ds_get_width(vd->ds)) / 16);
Since ui/vnc.h has:
#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG))
the third parameter to bitmap_clear is the same value in both cases, isn't it? Or is this a rounding bug?
-- PMM
Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
Am 02.03.2011 19:47, schrieb Peter Maydell:
Quoted 21 lines · Stefan Weil
Quoted 2 lines
No. I dont't think that the third parameter of
bitmap_clearis ok like that. See my patch for the correct value.Wen's patch:
+ const
size_twidth =ds_get_width(vd->ds)/ 16;
[...]ui/vnc.c +2
-3-bitmap_set (width_mask,0 , (ds_get_width (vd->ds) /16 )); -bitmap_clear (width_mask, (ds_get_width (vd->ds) /16 ), -VNC_DIRTY_WORDS *BITS_PER_LONG ); +bitmap_set (width_mask,0 , width); + bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG -width);
Your patch:
bitmap_clear(width_mask, (
ds_get_width(vd->ds)/ 16),
-VNC_DIRTY_WORDS*BITS_PER_LONG);
+ (VNC_MAX_WIDTH-ds_get_width(vd->ds)) / 16);
Quoted 8 lines
Since
ui/vnc.hhas:#define
VNC_DIRTY_WORDS(VNC_MAX_WIDTH/ (16 *BITS_PER_LONG))the third parameter to
bitmap_clearis the same value in both cases, isn't it? Or is this a rounding bug?-- PMM
Because of rounding effects, both values can be different.
The part missing in my patch is correct handling of another rounding effect:
VNC_DIRTY_WORDS is exact for 32 bit long values (and the "old" code which used uint32_t until some weeks ago), where VNC_DIRTY_WORDS = 2560/16/32 = 5.
For 64 bit values, VNC_DIRTY_WORDS = 2560/16/64 = 2 (rounded)!
Stefan W.
Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
Am 02.03.2011 23:01, schrieb Stefan Weil:
Quoted 36 lines
Am 02.03.2011 19:47, schrieb Peter Maydell:
Quoted 21 lines · Stefan Weil
Quoted 2 lines
No. I dont't think that the third parameter of
bitmap_clearis ok like that. See my patch for the correct value.Wen's patch:
+ const
size_twidth =ds_get_width(vd->ds)/ 16;
[...]ui/vnc.c +2
-4-bitmap_set (width_mask,0 , (ds_get_width (vd->ds) /16 )); -bitmap_clear (width_mask, (ds_get_width (vd->ds) /16 ), -VNC_DIRTY_WORDS *BITS_PER_LONG ); +bitmap_set (width_mask,0 , width); + bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);Your patch:
bitmap_clear(width_mask, (
ds_get_width(vd->ds)/ 16),
-VNC_DIRTY_WORDS*BITS_PER_LONG);
+ (VNC_MAX_WIDTH-ds_get_width(vd->ds)) / 16);Quoted 8 lines
Since
ui/vnc.hhas:#define
VNC_DIRTY_WORDS(VNC_MAX_WIDTH/ (16 *BITS_PER_LONG))the third parameter to
bitmap_clearis the same value in both cases, isn't it? Or is this a rounding bug?-- PMM
Because of rounding effects, both values can be different.
The part missing in my patch is correct handling of another rounding effect:
Quoted 7 lines
VNC_DIRTY_WORDSis exact for 32 bit long values (and the "old" code which useduint32_tuntil some weeks ago), whereVNC_DIRTY_WORDS= 2560/16/32 = 5.For 64 bit values,
VNC_DIRTY_WORDS= 2560/16/64 = 2 (rounded)!Stefan W.
Is bitmap_clear() really needed here? Meanwhile I think it is not, so this might be a new patch variant...
Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
At 03/03/2011 06:27 AM, Stefan Weil Write:
Quoted 41 lines
Am 02.03.2011 23:01, schrieb Stefan Weil:
Quoted 40 lines
Am 02.03.2011 19:47, schrieb Peter Maydell:
Quoted 21 lines · Stefan Weil
Quoted 2 lines
No. I dont't think that the third parameter of
bitmap_clearis ok like that. See my patch for the correct value.Wen's patch:
+ const
size_twidth =ds_get_width(vd->ds)/ 16;
[...]ui/vnc.c +2
-4-bitmap_set (width_mask,0 , (ds_get_width (vd->ds) /16 )); -bitmap_clear (width_mask, (ds_get_width (vd->ds) /16 ), -VNC_DIRTY_WORDS *BITS_PER_LONG ); +bitmap_set (width_mask,0 , width); + bitmap_clear(width_mask, width, VNC_DIRTY_WORDS * BITS_PER_LONG - width);Your patch:
bitmap_clear(width_mask, (
ds_get_width(vd->ds)/ 16),
-VNC_DIRTY_WORDS*BITS_PER_LONG);
+ (VNC_MAX_WIDTH-ds_get_width(vd->ds)) / 16);Quoted 8 lines
Since
ui/vnc.hhas:#define
VNC_DIRTY_WORDS(VNC_MAX_WIDTH/ (16 *BITS_PER_LONG))the third parameter to
bitmap_clearis the same value in both cases, isn't it? Or is this a rounding bug?-- PMM
Because of rounding effects, both values can be different.
The part missing in my patch is correct handling of another rounding effect:
VNC_DIRTY_WORDSis exact for 32 bit long values (and the "old" code which useduint32_tuntil some weeks ago), whereVNC_DIRTY_WORDS= 2560/16/32 = 5.
Quoted 7 lines
Quoted 3 lines
For 64 bit values,
VNC_DIRTY_WORDS= 2560/16/64 = 2 (rounded)!Stefan W.
Is
bitmap_clear()really needed here? Meanwhile I think it is not, so this might be a new patch variant...
I do not know why we call bitmap_clear() hear. I only know it is the same as the old code:
-static inline void vnc_set_bits(uint32_t *d, int n, int nb_words) -{
- int j;
-
- j = 0;
- while (n >= 32) {
- d[j++] = -1;
- n -= 32;
- }
- if (n > 0)
- d[j++] = (1 << n) - 1;
- while (j < nb_words) <=== bitmap_clear()
- d[j++] = 0;
-}
- vnc_set_bits(width_mask, (ds_get_width(vd->ds) / 16), VNC_DIRTY_WORDS); + bitmap_set(width_mask, 0, (ds_get_width(vd->ds) / 16)); + bitmap_clear(width_mask, (ds_get_width(vd->ds) / 16),
+ VNC_DIRTY_WORDS * BITS_PER_LONG);
Quoted 3 lines
Re: [Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption
In reply to Stefan WeilQuoted 8 lines · Stefan Weil
The part missing in my patch is correct handling of another rounding effect:
VNC_DIRTY_WORDSis exact for 32 bit long values (and the "old" code which useduint32_tuntil some weeks ago), whereVNC_DIRTY_WORDS= 2560/16/32 = 5.For 64 bit values,
VNC_DIRTY_WORDS= 2560/16/64 = 2 (rounded)!
Yes, I noticed that after I posted. Given that we have arrays like
unsigned long dirty[VNC_MAX_HEIGHT][VNC_DIRTY_WORDS];
rounding down rather than up looks a bit suspicious...
(Can we just make VNC_MAX_WIDTH a multiple of 64, or is it baked into the VNC protocol?)
-- PMM
Quoted 9 lines · Wen Congyang
This patch fix the following two regressions:
1. we should usebitmap_set()andbitmap_clear()to replacevnc_set_bits().
2. The unit of bitmap_intersects()'third parameter is bit, not words. But we pass the num of words tobitmap_intersects().Changes from v1 to v2:
1. fix the third argument ofbitmap_clear()Signed-off-by Wen Congyang <[email protected]>
Quoted 45 lines
---
ui/vnc.c | 11 ++++++++---1 files changed, 8 insertions(+), 3 deletions(-)ui/vnc.c +8
-4@@ -1645,17 +1645,21 @@ static void framebuffer_update_request(VncState *vs, int incremental, int x_position, int y_position, int w, int h) { + int i; + const size_t width = ds_get_width(vs->ds) / 16; + if (y_position > ds_get_height(vs->ds)) y_position = ds_get_height(vs->ds); if (y_position + h >= ds_get_height(vs->ds)) h = ds_get_height(vs->ds) - y_position; - int i; vs->need_update = 1; if (!incremental) { vs->force_update = 1; for (i = 0; i < h; i++) { - bitmap_set(vs->dirty[y_position + i], x_position / 16, w / 16); + bitmap_set(vs->dirty[y_position + i], 0, width); + bitmap_clear(vs->dirty[y_position + i], width, + VNC_DIRTY_WORDS * BITS_PER_LONG - width); } } }@@ -2406,7 +2410,8 @@ static int vnc_refresh_server_surface(VncDisplay *vd) guest_row = vd->guest.ds->data; server_row = vd->server->data; for (y = 0; y < vd->guest.ds->height; y++) { - if (bitmap_intersects(vd->guest.dirty[y], width_mask, VNC_DIRTY_WORDS)) { + if (bitmap_intersects(vd->guest.dirty[y], width_mask, + VNC_DIRTY_WORDS * BITS_PER_LONG)) { int x; uint8_t *guest_ptr; uint8_t *server_ptr; --1.7.1
Signature · 2 lines
Corentin Chary http://xf.iksaif.net
PATCH 3/3 correct VNC_DIRTY_WORDS on 64 bit machine
In reply to Wen CongyangVNC_DIRTY_WORDS is wrong on 64 bit long machine.
--- ui/vnc.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
ui/vnc.h +2 -1
@@ -81,7 +81,8 @@ typedef void VncSendHextileTile(VncState *vs,#define VNC_MAX_WIDTH 2560#define VNC_MAX_HEIGHT 2048 -#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG)) +#define divideup (x, y) (((x) + ((y) - 1)) / (y)) +#define VNC_DIRTY_WORDS (divideup(VNC_MAX_WIDTH, (16 * BITS_PER_LONG)))#define VNC_STAT_RECT 64#define VNC_STAT_COLS (VNC_MAX_WIDTH / VNC_STAT_RECT)
Signature · 1 line
1.7.1
[Qemu-devel] Re: [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine
Quoted 13 lines · Wen Congyang
VNC_DIRTY_WORDSis wrong on 64 bit long machine.Signed-off-by Wen Congyang <[email protected]>---
ui/vnc.h | 3 ++-1 files changed, 2 insertions(+), 1 deletions(-)ui/vnc.h +0
-0
ui/vnc.h typedef void VncSendHextileTile(VncState *vs, · line 84
Quoted 5 lines
ui/vnc.h +1
-1@@ -81,7 +81,8 @@ typedef void VncSendHextileTile(VncState *vs, #define VNC_MAX_WIDTH 2560 #define VNC_MAX_HEIGHT 2048 -#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG)) +#define divideup (x, y) (((x) + ((y) - 1)) / (y))
osdep.h:#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
Signature · 2 lines
Corentin Chary http://xf.iksaif.net
[Qemu-devel] Re: [PATCH 3/3] correct VNC_DIRTY_WORDS on 64 bit machine
At 03/03/2011 02:41 PM, Corentin Chary Write:
Quoted 15 lines · Wen Congyang
Quoted 14 lines
VNC_DIRTY_WORDSis wrong on 64 bit long machine.Signed-off-by Wen Congyang <[email protected]>--- ui/vnc.h | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)ui/vnc.h +0
-0@@ -81,7 +81,8 @@ typedef void VncSendHextileTile(VncState *vs,
Quoted 6 lines
ui/vnc.h typedef void VncSendHextileTile(VncState *vs, · line 84
Quoted 4 lines
ui/vnc.h +1
-1#define VNC_MAX_WIDTH 2560#define VNC_MAX_HEIGHT 2048 -#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG)) +#define divideup (x, y) (((x) + ((y) - 1)) / (y))osdep.h:#define
DIV_ROUND_UP(n,d)(((n) + (d) - 1) / (d))
Thank you for pointing out it. I will update my patch.
Quoted 2 lines
PATCH v2 3/3 correct VNC_DIRTY_WORDS on 64 bit machine
In reply to Corentin CharyVNC_DIRTY_WORDS is wrong on 64 bit long machine.
--- ui/vnc.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
ui/vnc.h +1 -1
@@ -81,7 +81,7 @@ typedef void VncSendHextileTile(VncState *vs,#define VNC_MAX_WIDTH 2560#define VNC_MAX_HEIGHT 2048 -#define VNC_DIRTY_WORDS (VNC_MAX_WIDTH / (16 * BITS_PER_LONG)) +#define VNC_DIRTY_WORDS (DIV_ROUND_UP(VNC_MAX_WIDTH, (16 * BITS_PER_LONG)))#define VNC_STAT_RECT 64#define VNC_STAT_COLS (VNC_MAX_WIDTH / VNC_STAT_RECT)
Signature · 1 line
1.7.1