Re: [Qemu-devel] [PATCH v2] virtio-serial: Fix segfault on guest boot

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 22 Jun 2011 09:49:22 +0530
Amit Shah <amit.shah@redhat.com> wrote:

> On (Fri) 17 Jun 2011 [15:58:08], Luiz Capitulino wrote:
> > If I start qemu with:
> > 
> >   # qemu -hda disks/test.img -enable-kvm -m 1G -snapshot \
> >          -device virtio-serial \
> >          -chardev socket,host=localhost,port=1234,server,nowait,id=foo \
> >          -device virtserialport,chardev=foo,name=org.qemu.guest_agent
> > 
> > I get a segfault when booting a Fedora 14 guest. The backtrace says:
> > 
> >   Program terminated with signal 11, Segmentation fault.
> >     #0  0x0000000000420850 in handle_control_message (vser=0x3732bd0, buf=0x2c173e0, len=8) at /home/lcapitulino/src/qmp-unstable/hw/virtio-serial-bus.c:335
> >     335     info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> > 
> > What's happening is VIRTIO_CONSOLE_DEVICE_READY is a message for the
> > whole device, not for an individual port. So port is NULL. This bug was
> > introduced by commit a15bb0d6a981de749452a5180fc8084d625671da.
> > 
> > This commit fixes that by making the port returned by find_port_by_id()
> > be used only by the VIRTIO_CONSOLE_PORT_READY and
> > VIRTIO_CONSOLE_PORT_OPEN messages.
> > 
> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> > ---
> >  hw/virtio-serial-bus.c |   24 ++++++++++++++----------
> >  1 files changed, 14 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> > index 9a12104..33a6f61 100644
> > --- a/hw/virtio-serial-bus.c
> > +++ b/hw/virtio-serial-bus.c
> > @@ -328,18 +328,11 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
> >      cpkt.event = lduw_p(&gcpkt->event);
> >      cpkt.value = lduw_p(&gcpkt->value);
> >  
> > -    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> > -    if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
> > -        return;
> > -
> > -    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> > -
> > -    switch(cpkt.event) {
> > -    case VIRTIO_CONSOLE_DEVICE_READY:
> > +    if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) {
> 
> What we lose after this re-arrangement is the check that port is NULL
> when this message is received.  i.e., a guest bug where port is set to
> a valid value when this message arrives.  (I think I pointed this out
> in a previous mail?)

I'm not sure I follow you here, the current code doesn't return if
cpkt.event == VIRTIO_CONSOLE_DEVICE_READY:

    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
    if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
        return;

> 
> >          if (!cpkt.value) {
> >              error_report("virtio-serial-bus: Guest failure in adding device %s\n",
> >                           vser->bus.qbus.name);
> > -            break;
> > +            return;
> >          }
> >          /*
> >           * The device is up, we can now tell the device about all the
> > @@ -348,8 +341,19 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
> >          QTAILQ_FOREACH(port, &vser->ports, next) {
> >              send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1);
> >          }
> > -        break;
> > +        return;
> > +    }
> >  
> > +    port = find_port_by_id(vser, ldl_p(&gcpkt->id));
> > +    if (!port) {
> > +        error_report("virtio-serial-bus: Expected port id %d for device %s\n",
> > +                     ldl_p(&gcpkt->id), vser->bus.qbus.name);
> 
> It's actually 'unexpected port id...' :-)
> 
> Also, port id is %u, not %d.
> 
> Fixed these two and committed to virtio-serial tree.
> 
> Thanks,
> 		Amit
>