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

[Luiz Capitulino]

On Fri, 17 Jun 2011 12:17:36 +0530
Amit Shah <address@hidden> wrote:

> On (Thu) 16 Jun 2011 [13:38:49], 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);
> 
> Strange, I've not seen it so far in my testing (neither in the daily
> test runs of the virtio-serial testsuite).
> 
> > I've also bisected this and git points out to commit:
> > 
> >   commit a15bb0d6a981de749452a5180fc8084d625671da
> >   Author: Markus Armbruster <address@hidden>
> >   Date:   Wed May 25 14:21:13 2011 +0200
> > 
> >       virtio-serial: Drop redundant VirtIOSerialPort member info
> > 
> > I think what's happening is that the device is not initialized on a
> > VIRTIO_CONSOLE_DEVICE_READY event. Moving the DO_UPCAST() call to
> > the other events fixes the problem to me.
> > 
> > Signed-off-by: Luiz Capitulino <address@hidden>
> > ---
> >  hw/virtio-serial-bus.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> > index 9a12104..579f676 100644
> > --- a/hw/virtio-serial-bus.c
> > +++ b/hw/virtio-serial-bus.c
> > @@ -332,8 +332,6 @@ static void handle_control_message(VirtIOSerial *vser, 
> > void *buf, size_t len)
> >      if (!port && cpkt.event != VIRTIO_CONSOLE_DEVICE_READY)
> >          return;
> >  
> > -    info = DO_UPCAST(VirtIOSerialPortInfo, qdev, port->dev.info);
> > -
> 
> Ah - this missed the !port check.  It should be possible to do this in
> a 'if (port)' block instead of replicating in the individual case
> statements.
> 
> Thanks for the debugging and patch; please update with the above and
> I'll apply it to the virtio-serial tree.

What about moving the VIRTIO_CONSOLE_DEVICE_READY handling out of the
switch, like the patch below? This way the function is divided in a way
that related events are handled together.

I'll implement your first suggestion if you don't like this...

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 579f676..5f96245 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -325,19 +325,12 @@ static void handle_control_message(VirtIOSerial *vser, 
void *buf, size_t len)
         return;
     }
 
-    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;
-
-    switch(cpkt.event) {
-    case VIRTIO_CONSOLE_DEVICE_READY:
+    cpkt.event = lduw_p(&gcpkt->event);
+    if (cpkt.event == VIRTIO_CONSOLE_DEVICE_READY) {
         if (!cpkt.value) {
-            error_report("virtio-serial-bus: Guest failure in adding device 
%s\n",
-                         vser->bus.qbus.name);
-            break;
+            error_report("virtio-serial-bus: Guest failure in adding device 
%s\n", vser->bus.qbus.name);
+            return;
         }
         /*
          * The device is up, we can now tell the device about all the
@@ -346,8 +339,13 @@ 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));
+    assert(port != NULL);
+
+    switch(cpkt.event) {
     case VIRTIO_CONSOLE_PORT_READY:
         if (!cpkt.value) {
             error_report("virtio-serial-bus: Guest failure in adding port %u 
for device %s\n",