Re: [Qemu-devel] [PATCH v2 1/4] display: ensure qxl log_buf is a nul terminated string

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Jan 21, 2019 at 10:45:45AM +0000, Stefan Hajnoczi wrote:
> On Fri, Jan 18, 2019 at 05:31:00PM +0000, Daniel P. Berrangé wrote:
> > diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> > index 8e9a65e75b..eefdf4baac 100644
> > --- a/hw/display/qxl.c
> > +++ b/hw/display/qxl.c
> > @@ -1763,7 +1763,8 @@ async_common:
> >          qxl_set_mode(d, val, 0);
> >          break;
> >      case QXL_IO_LOG:
> > -        trace_qxl_io_log(d->id, d->ram->log_buf);
> > +        d->ram->log_buf[sizeof(d->ram->log_buf) - 1] = '\0';
> > +        trace_qxl_io_log(d->id, (const char *)d->ram->log_buf);
> 
> This is a PCI BAR shared with the guest?  Then NUL termination is
> subject to races with vcpu threads that modify log_buf[] while we access
> it.

Doh, yes, it is racy.

> The safe way to do this is to copy in log_buf[] and then NUL-terminate
> the local copy.

log_buf is 4k in size, which I don't really want to allocate on the
stack. Using malloc would impose a perf penalty on logging but not
sure if that's significant enough to worry about. Alternatively we
could just drop the tracepoint, given that you can already use the
'guestdebug' option to get these fprintf'd to stderr.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|