[Qemu-devel] [PATCH v2 0/3] usb: fix bugs reported by Clang Static Analyzer

[Qemu-devel] [PATCH v2 0/3] usb: fix bugs reported by Clang Static Analyzer

[Philippe Mathieu-Daudé]

Since v1 [1]:
- zero-initialize c->argv[] as suggested by Gerd Hoffmann
- drop 4th patch, bug already fixed in 53735bef108

[1] http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg07453.html

Philippe Mathieu-Daudé (3):
  usb: correctly handle Zero Length Packets
  usb/dev-mtp: Fix use of uninitialized values
  usb/dev-mtp: Fix use of uninitialized values

 hw/usb/dev-mtp.c  | 11 ++++++++++-
 hw/usb/redirect.c |  2 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH v2 1/3] usb: correctly handle Zero Length Packets

[Philippe Mathieu-Daudé]

USB Specification Revision 2.0, §5.5.3:
  The Data stage of a control transfer from an endpoint to the host is complete when the endpoint does one of the following:
  • Has transferred exactly the amount of data specified during the Setup stage
  • Transfers a packet with a payload size less than wMaxPacketSize or transfers a zero-length packet"

hw/usb/redirect.c:802:9: warning: Declared variable-length array (VLA) has zero size
        uint8_t buf[size];
        ^~~~~~~~~~~ ~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/usb/redirect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/redirect.c b/hw/usb/redirect.c
index 65a9196c1a..58e8f7f5bd 100644
--- a/hw/usb/redirect.c
+++ b/hw/usb/redirect.c
@@ -795,7 +795,7 @@ static void usbredir_handle_bulk_data(USBRedirDevice *dev, USBPacket *p,
            usbredirparser_peer_has_cap(dev->parser,
                                        usb_redir_cap_32bits_bulk_length));
 
-    if (ep & USB_DIR_IN) {
+    if (ep & USB_DIR_IN || size == 0) {
         usbredirparser_send_bulk_packet(dev->parser, p->id,
                                         &bulk_packet, NULL, 0);
     } else {
-- 
2.17.1

[Qemu-devel] [PATCH v2 2/3] usb/dev-mtp: Fix use of uninitialized values

[Philippe Mathieu-Daudé]

This fixes:

  hw/usb/dev-mtp.c:971:5: warning: 4th function call argument is an uninitialized value
      trace_usb_mtp_op_get_partial_object(s->dev.addr, o->handle, o->path,
                                           c->argv[1], c->argv[2]);
                                                       ^~~~~~~~~~
and:

  hw/usb/dev-mtp.c:981:12: warning: Assigned value is garbage or undefined
      offset = c->argv[1];
               ^ ~~~~~~~~~~

Reported-by: Clang Static Analyzer
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/usb/dev-mtp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 560c61c7c1..b0ab6a7912 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1017,12 +1017,16 @@ static MTPData *usb_mtp_get_object(MTPState *s, MTPControl *c,
 static MTPData *usb_mtp_get_partial_object(MTPState *s, MTPControl *c,
                                            MTPObject *o)
 {
-    MTPData *d = usb_mtp_data_alloc(c);
+    MTPData *d;
     off_t offset;
 
+    if (c->argc <= 2) {
+        return NULL;
+    }
     trace_usb_mtp_op_get_partial_object(s->dev.addr, o->handle, o->path,
                                         c->argv[1], c->argv[2]);
 
+    d = usb_mtp_data_alloc(c);
     d->fd = open(o->path, O_RDONLY);
     if (d->fd == -1) {
         usb_mtp_data_free(d);
-- 
2.17.1

[Qemu-devel] [PATCH v2 3/3] usb/dev-mtp: Fix use of uninitialized values

[Philippe Mathieu-Daudé]

This fixes:

  hw/usb/dev-mtp.c:1212:13: warning: 2nd function call argument is an uninitialized value
      o = usb_mtp_object_lookup(s, c->argv[0]);
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Clang Static Analyzer
Suggested-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/usb/dev-mtp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index b0ab6a7912..dd96c91cf9 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1281,6 +1281,7 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
     MTPData *data_in = NULL;
     MTPObject *o = NULL;
     uint32_t nres = 0, res0 = 0;
+    int i;
 
     /* sanity checks */
     if (c->code >= CMD_CLOSE_SESSION && s->session == 0) {
@@ -1289,6 +1290,10 @@ static void usb_mtp_command(MTPState *s, MTPControl *c)
         return;
     }
 
+    for (i = c->argc; i < ARRAY_SIZE(c->argv); i++) {
+        c->argv[i] = 0;
+    }
+
     /* process commands */
     switch (c->code) {
     case CMD_GET_DEVICE_INFO:
-- 
2.17.1

Re: [Qemu-devel] [PATCH v2 3/3] usb/dev-mtp: Fix use of uninitialized values

[Gerd Hoffmann]

> +    for (i = c->argc; i < ARRAY_SIZE(c->argv); i++) {
> +        c->argv[i] = 0;
> +    }

I think the code filling c->argv (in usb_mtp_handle_data) should so
that.  Or just memset(0) cmd in usb_mtp_handle_data ...

cheers,
  Gerd

PS: the other patches are fine.