[Qemu-devel] Re: [PATCH RESEND 2/2] vnc: Fix heap corruption

[Stefan Weil]

Am 02.03.2011 11:57, schrieb Corentin Chary:
> On Wed, Mar 2, 2011 at 3:58 AM, Wen Congyang <address@hidden> wrote:
> > 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().
> >
> > Signed-off-by: Wen Congyang <address@hidden>
> > Reported-by: Stefan Weil <address@hidden>
>
> Acked-by: Corentin Chary <address@hidden>

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


> > ---
> >   ui/vnc.c |    6 +++---
> >   1 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/ui/vnc.c b/ui/vnc.c
> > index e3761b0..e7d0b5b 100644
> > --- a/ui/vnc.c
> > +++ b/ui/vnc.c
> > @@ -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