[Qemu-devel] [PATCH 1/1] virtio-serial-bus: send_control_msg should not deal with cpkts

[Qemu-devel] [PATCH 1/1] virtio-serial-bus: send_control_msg should not deal with cpkts

[Amit Shah]

Stuff the cpkt before calling send_control_msg().  it should not be
concerned about contents, just send across the buffer.

A few code refactorings recently have made mkaing this change easier
than earlier.

Coverity and clang have flagged this code several times in the past
(cpkt->id not set before send_control_event() passed it on to
send_control_msg()).  This will finally eliminate the false-positive.

CC: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 155da58..47d0481 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -215,13 +215,12 @@ static void flush_queued_data(VirtIOSerialPort *port)
     do_flush_queued_data(port, port->ovq, &port->vser->vdev);
 }
 
-static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len)
+static size_t send_control_msg(VirtIOSerial *vser, void *buf, size_t len)
 {
     VirtQueueElement elem;
     VirtQueue *vq;
-    struct virtio_console_control *cpkt;
 
-    vq = port->vser->c_ivq;
+    vq = vser->c_ivq;
     if (!virtio_queue_ready(vq)) {
         return 0;
     }
@@ -229,25 +228,24 @@ static size_t send_control_msg(VirtIOSerialPort *port, void *buf, size_t len)
         return 0;
     }
 
-    cpkt = (struct virtio_console_control *)buf;
-    stl_p(&cpkt->id, port->id);
     memcpy(elem.in_sg[0].iov_base, buf, len);
 
     virtqueue_push(vq, &elem, len);
-    virtio_notify(&port->vser->vdev, vq);
+    virtio_notify(&vser->vdev, vq);
     return len;
 }
 
-static size_t send_control_event(VirtIOSerialPort *port, uint16_t event,
-                                 uint16_t value)
+static size_t send_control_event(VirtIOSerial *vser, uint32_t port_id,
+                                 uint16_t event, uint16_t value)
 {
     struct virtio_console_control cpkt;
 
+    stl_p(&cpkt.id, port_id);
     stw_p(&cpkt.event, event);
     stw_p(&cpkt.value, value);
 
-    trace_virtio_serial_send_control_event(port->id, event, value);
-    return send_control_msg(port, &cpkt, sizeof(cpkt));
+    trace_virtio_serial_send_control_event(port_id, event, value);
+    return send_control_msg(vser, &cpkt, sizeof(cpkt));
 }
 
 /* Functions for use inside qemu to open and read from/write to ports */
@@ -259,7 +257,7 @@ int virtio_serial_open(VirtIOSerialPort *port)
     }
     /* Send port open notification to the guest */
     port->host_connected = true;
-    send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
+    send_control_event(port->vser, port->id, VIRTIO_CONSOLE_PORT_OPEN, 1);
 
     return 0;
 }
@@ -274,7 +272,7 @@ int virtio_serial_close(VirtIOSerialPort *port)
     port->throttled = false;
     discard_vq_data(port->ovq, &port->vser->vdev);
 
-    send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 0);
+    send_control_event(port->vser, port->id, VIRTIO_CONSOLE_PORT_OPEN, 0);
 
     return 0;
 }
@@ -363,7 +361,7 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
          * ports we have here.
          */
         QTAILQ_FOREACH(port, &vser->ports, next) {
-            send_control_event(port, VIRTIO_CONSOLE_PORT_ADD, 1);
+            send_control_event(vser, port->id, VIRTIO_CONSOLE_PORT_ADD, 1);
         }
         return;
     }
@@ -394,10 +392,11 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
          * up to hvc.
          */
         if (vsc->is_console) {
-            send_control_event(port, VIRTIO_CONSOLE_CONSOLE_PORT, 1);
+            send_control_event(vser, port->id, VIRTIO_CONSOLE_CONSOLE_PORT, 1);
         }
 
         if (port->name) {
+            stl_p(&cpkt.id, port->id);
             stw_p(&cpkt.event, VIRTIO_CONSOLE_PORT_NAME);
             stw_p(&cpkt.value, 1);
 
@@ -408,12 +407,12 @@ static void handle_control_message(VirtIOSerial *vser, void *buf, size_t len)
             memcpy(buffer + sizeof(cpkt), port->name, strlen(port->name));
             buffer[buffer_len - 1] = 0;
 
-            send_control_msg(port, buffer, buffer_len);
+            send_control_msg(vser, buffer, buffer_len);
             g_free(buffer);
         }
 
         if (port->host_connected) {
-            send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, 1);
+            send_control_event(vser, port->id, VIRTIO_CONSOLE_PORT_OPEN, 1);
         }
 
         /*
@@ -650,7 +649,7 @@ static void virtio_serial_post_load_timer_cb(void *opaque)
              * We have to let the guest know of the host connection
              * status change
              */
-            send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN,
+            send_control_event(s, port->id, VIRTIO_CONSOLE_PORT_OPEN,
                                port->host_connected);
         }
     }
@@ -815,9 +814,7 @@ static void mark_port_added(VirtIOSerial *vser, uint32_t port_id)
 static void add_port(VirtIOSerial *vser, uint32_t port_id)
 {
     mark_port_added(vser, port_id);
-
-    send_control_event(find_port_by_id(vser, port_id),
-                       VIRTIO_CONSOLE_PORT_ADD, 1);
+    send_control_event(vser, port_id, VIRTIO_CONSOLE_PORT_ADD, 1);
 }
 
 static void remove_port(VirtIOSerial *vser, uint32_t port_id)
@@ -832,7 +829,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
     /* Flush out any unconsumed buffers first */
     discard_vq_data(port->ovq, &port->vser->vdev);
 
-    send_control_event(port, VIRTIO_CONSOLE_PORT_REMOVE, 1);
+    send_control_event(vser, port->id, VIRTIO_CONSOLE_PORT_REMOVE, 1);
 }
 
 static int virtser_port_qdev_init(DeviceState *qdev)
-- 
1.8.0.2

Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: send_control_msg should not deal with cpkts

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> Stuff the cpkt before calling send_control_msg().  it should not be
> concerned about contents, just send across the buffer.
>
> A few code refactorings recently have made mkaing this change easier
> than earlier.
>
> Coverity and clang have flagged this code several times in the past
> (cpkt->id not set before send_control_event() passed it on to
> send_control_msg()).  This will finally eliminate the false-positive.
>
> CC: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

Patch makes sense to me, and the Coverity defect report is gone.
However, it now worries find_port_by_id() in remove_port() could return
a null pointer, which is then dereferenced.  No idea why it didn't
report that before.  Obvious suppressor:

    diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
    index 47d0481..7ff7505 100644
    --- a/hw/virtio-serial-bus.c
    +++ b/hw/virtio-serial-bus.c
    @@ -826,6 +826,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
         vser->ports_map[i] &= ~(1U << (port_id % 32));

         port = find_port_by_id(vser, port_id);
    +    assert(port);
         /* Flush out any unconsumed buffers first */
         discard_vq_data(port->ovq, &port->vser->vdev);

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: send_control_msg should not deal with cpkts

[Amit Shah]

On (Mon) 17 Dec 2012 [14:14:17], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > Stuff the cpkt before calling send_control_msg().  it should not be
> > concerned about contents, just send across the buffer.
> >
> > A few code refactorings recently have made mkaing this change easier
> > than earlier.

Ugh, I'll fix the typo and incoherent language here before merging.

> > Coverity and clang have flagged this code several times in the past
> > (cpkt->id not set before send_control_event() passed it on to
> > send_control_msg()).  This will finally eliminate the false-positive.
> >
> > CC: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> 
> Patch makes sense to me, and the Coverity defect report is gone.

Thanks for checking!

> However, it now worries find_port_by_id() in remove_port() could return
> a null pointer, which is then dereferenced.  No idea why it didn't
> report that before.  Obvious suppressor:
> 
>     diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>     index 47d0481..7ff7505 100644
>     --- a/hw/virtio-serial-bus.c
>     +++ b/hw/virtio-serial-bus.c
>     @@ -826,6 +826,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
>          vser->ports_map[i] &= ~(1U << (port_id % 32));
> 
>          port = find_port_by_id(vser, port_id);
>     +    assert(port);
>          /* Flush out any unconsumed buffers first */
>          discard_vq_data(port->ovq, &port->vser->vdev);

remove_port() is called by the hot-unplug qdev callback, and if the
port's missing from our tailq, something's gone wrong anyway.  So this
patch makes sense too.

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

		Amit

Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: send_control_msg should not deal with cpkts

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Mon) 17 Dec 2012 [14:14:17], Markus Armbruster wrote:
>> Amit Shah <amit.shah@redhat.com> writes:
>> 
>> > Stuff the cpkt before calling send_control_msg().  it should not be
>> > concerned about contents, just send across the buffer.
>> >
>> > A few code refactorings recently have made mkaing this change easier
>> > than earlier.
>
> Ugh, I'll fix the typo and incoherent language here before merging.
>
>> > Coverity and clang have flagged this code several times in the past
>> > (cpkt->id not set before send_control_event() passed it on to
>> > send_control_msg()).  This will finally eliminate the false-positive.
>> >
>> > CC: Markus Armbruster <armbru@redhat.com>
>> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
>> 
>> Patch makes sense to me, and the Coverity defect report is gone.
>
> Thanks for checking!
>
>> However, it now worries find_port_by_id() in remove_port() could return
>> a null pointer, which is then dereferenced.  No idea why it didn't
>> report that before.  Obvious suppressor:
>> 
>>     diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>>     index 47d0481..7ff7505 100644
>>     --- a/hw/virtio-serial-bus.c
>>     +++ b/hw/virtio-serial-bus.c
>>     @@ -826,6 +826,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
>>          vser->ports_map[i] &= ~(1U << (port_id % 32));
>> 
>>          port = find_port_by_id(vser, port_id);
>>     +    assert(port);
>>          /* Flush out any unconsumed buffers first */
>>          discard_vq_data(port->ovq, &port->vser->vdev);
>
> remove_port() is called by the hot-unplug qdev callback, and if the
> port's missing from our tailq, something's gone wrong anyway.  So this
> patch makes sense too.

Will you take care of that, or do you want me to post the patch?

Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: send_control_msg should not deal with cpkts

[Amit Shah]

On (Mon) 17 Dec 2012 [18:23:53], Markus Armbruster wrote:
> >> However, it now worries find_port_by_id() in remove_port() could return
> >> a null pointer, which is then dereferenced.  No idea why it didn't
> >> report that before.  Obvious suppressor:
> >> 
> >>     diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> >>     index 47d0481..7ff7505 100644
> >>     --- a/hw/virtio-serial-bus.c
> >>     +++ b/hw/virtio-serial-bus.c
> >>     @@ -826,6 +826,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
> >>          vser->ports_map[i] &= ~(1U << (port_id % 32));
> >> 
> >>          port = find_port_by_id(vser, port_id);
> >>     +    assert(port);
> >>          /* Flush out any unconsumed buffers first */
> >>          discard_vq_data(port->ovq, &port->vser->vdev);
> >
> > remove_port() is called by the hot-unplug qdev callback, and if the
> > port's missing from our tailq, something's gone wrong anyway.  So this
> > patch makes sense too.
> 
> Will you take care of that, or do you want me to post the patch?

I was going to, but if you want to, go ahead -- you already have the
patch ready :)

		Amit

Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: send_control_msg should not deal with cpkts

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> On (Mon) 17 Dec 2012 [18:23:53], Markus Armbruster wrote:
>> >> However, it now worries find_port_by_id() in remove_port() could return
>> >> a null pointer, which is then dereferenced.  No idea why it didn't
>> >> report that before.  Obvious suppressor:
>> >> 
>> >>     diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
>> >>     index 47d0481..7ff7505 100644
>> >>     --- a/hw/virtio-serial-bus.c
>> >>     +++ b/hw/virtio-serial-bus.c
>> >>     @@ -826,6 +826,7 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
>> >>          vser->ports_map[i] &= ~(1U << (port_id % 32));
>> >> 
>> >>          port = find_port_by_id(vser, port_id);
>> >>     +    assert(port);
>> >>          /* Flush out any unconsumed buffers first */
>> >>          discard_vq_data(port->ovq, &port->vser->vdev);
>> >
>> > remove_port() is called by the hot-unplug qdev callback, and if the
>> > port's missing from our tailq, something's gone wrong anyway.  So this
>> > patch makes sense too.
>> 
>> Will you take care of that, or do you want me to post the patch?
>
> I was going to, but if you want to, go ahead -- you already have the
> patch ready :)

Happy to leave it to you.

[Qemu-devel] [PATCH 1/1] virtio-serial-bus: assert port is non-null in remove_port()

[Amit Shah]

remove_port() is called from qdev's unplug callback, and we're certain
the port will be found in our list of ports.  Adding an assert()
documents this.

This was flagged by Coverity, fix suggested by Markus.

CC: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-serial-bus.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
index 3ea95b8..ce4556f 100644
--- a/hw/virtio-serial-bus.c
+++ b/hw/virtio-serial-bus.c
@@ -852,6 +852,12 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
     vser->ports_map[i] &= ~(1U << (port_id % 32));
 
     port = find_port_by_id(vser, port_id);
+    /*
+     * This function is only called from qdev's unplug callback; if we
+     * get a NULL port here, we're in trouble.
+     */
+    assert(port);
+
     /* Flush out any unconsumed buffers first */
     discard_vq_data(port->ovq, &port->vser->vdev);
 
-- 
1.8.0.2

Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: assert port is non-null in remove_port()

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> remove_port() is called from qdev's unplug callback, and we're certain
> the port will be found in our list of ports.  Adding an assert()
> documents this.
>
> This was flagged by Coverity, fix suggested by Markus.
>
> CC: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
>  hw/virtio-serial-bus.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c
> index 3ea95b8..ce4556f 100644
> --- a/hw/virtio-serial-bus.c
> +++ b/hw/virtio-serial-bus.c
> @@ -852,6 +852,12 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
>      vser->ports_map[i] &= ~(1U << (port_id % 32));
>  
>      port = find_port_by_id(vser, port_id);
> +    /*
> +     * This function is only called from qdev's unplug callback; if we
> +     * get a NULL port here, we're in trouble.
> +     */
> +    assert(port);
> +
>      /* Flush out any unconsumed buffers first */
>      discard_vq_data(port->ovq, &port->vser->vdev);

Leaving it to you got me a nice comment!

Reviewed-by: Markus Armbruster <armbru@redhat.com>