[Qemu-devel] [PATCH 0/9] Fix Coverity warning reports

[Qemu-devel] [PATCH 0/9] Fix Coverity warning reports

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Those reports come from scan.coverity.com for Qemu,
which Paolo told me. There are so many defects that
I can't repair all of them. That's will be great
if more and more people to join us. :) 

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: zhanghailiang <zhang.zhanghailiang@huawei.com>
Cc: qemu-trivial@nongnu.org

Gonglei (9):
  l2tpv3: fix fd leak
  mips_mipssim: fix use-after-free for filename
  qga: fix false negative argument passing
  loader: fix NEGATIVE_RETURNS
  nvme: remove superfluous check
  acl: fix memory leak
  qemu-char: fix MISSING_COMMA
  shpc: fix dead code
  hcd-musb: fix dereference null return value

 hw/block/nvme.c        |  3 +--
 hw/core/loader.c       | 13 +++++++++++++
 hw/mips/mips_mipssim.c |  2 +-
 hw/pci/shpc.c          |  3 ++-
 hw/usb/hcd-musb.c      |  4 ++++
 net/l2tpv3.c           |  2 +-
 qemu-char.c            |  2 +-
 qga/main.c             |  4 ++--
 util/acl.c             | 10 +++++-----
 9 files changed, 30 insertions(+), 13 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

In this false branch, fd will leak when it is zero.
Change the testing condition.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 net/l2tpv3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/l2tpv3.c b/net/l2tpv3.c
index 528d95b..b2b0fc0 100644
--- a/net/l2tpv3.c
+++ b/net/l2tpv3.c
@@ -746,7 +746,7 @@ int net_init_l2tpv3(const NetClientOptions *opts,
     return 0;
 outerr:
     qemu_del_net_client(nc);
-    if (fd > 0) {
+    if (fd != -1) {
         close(fd);
     }
     if (result) {
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]

On Sat, Nov 15, 2014 at 06:06:40PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> In this false branch, fd will leak when it is zero.
> Change the testing condition.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  net/l2tpv3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak

[Michael Tokarev]

15.11.2014 13:06, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> In this false branch, fd will leak when it is zero.
> Change the testing condition.

Why fd==0 is a concern here?  It is a very unlikely
situation that fd0 will be picked - firstly because
fd0 is almost always open, and second - even if it
isn't open, it will be picked much earlier than this
code path, ie, some other file will use fd0 before.

But even if the concern is real (after all, better
stay correct than spread bad code pattern, even if
in reality we don't care as this can't happen), why
not add 0 to the equality?

Why people especially compare with -1?  Any negative
value is illegal here and in lots of other places,
and many software packages used to return -errno in
error cases, which is definitely != -1.  I'm not
saying that comparing with -1 is bad in _this_
particular case, but why not do it generally in
all cases?

More, comparing with 0 is faster and shorter than
comparing with -1...

So if it were me, I'd change it to >= 0, not to
== -1.  Here and in all other millions of places
in qemu code where it compares with -1... ;)

Thanks,

/mjt
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  net/l2tpv3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/l2tpv3.c b/net/l2tpv3.c
> index 528d95b..b2b0fc0 100644
> --- a/net/l2tpv3.c
> +++ b/net/l2tpv3.c
> @@ -746,7 +746,7 @@ int net_init_l2tpv3(const NetClientOptions *opts,
>      return 0;
>  outerr:
>      qemu_del_net_client(nc);
> -    if (fd > 0) {
> +    if (fd != -1) {
>          close(fd);
>      }
>      if (result) {
> 

Re: [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak

[Markus Armbruster]

Michael Tokarev <mjt@tls.msk.ru> writes:

> 15.11.2014 13:06, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>> 
>> In this false branch, fd will leak when it is zero.
>> Change the testing condition.
>
> Why fd==0 is a concern here?  It is a very unlikely
> situation that fd0 will be picked - firstly because
> fd0 is almost always open, and second - even if it
> isn't open, it will be picked much earlier than this
> code path, ie, some other file will use fd0 before.
>
> But even if the concern is real (after all, better
> stay correct than spread bad code pattern, even if
> in reality we don't care as this can't happen), why
> not add 0 to the equality?
>
> Why people especially compare with -1?  Any negative
> value is illegal here and in lots of other places,
> and many software packages used to return -errno in
> error cases, which is definitely != -1.  I'm not
> saying that comparing with -1 is bad in _this_
> particular case, but why not do it generally in
> all cases?
>
> More, comparing with 0 is faster and shorter than
> comparing with -1...
>
> So if it were me, I'd change it to >= 0, not to
> == -1.  Here and in all other millions of places
> in qemu code where it compares with -1... ;)

Yup.

In the case of close(), I wouldn't even bother to check, except Coverity
gets excited when it sees an invalid fd flow into close().  Rightfully
so when the invalid fd is non-negative, overeager when it's negative.

Re: [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak

[Gonglei]

On 2014/11/18 15:50, Markus Armbruster wrote:

> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> 15.11.2014 13:06, arei.gonglei@huawei.com wrote:
>>> From: Gonglei <arei.gonglei@huawei.com>
>>>
>>> In this false branch, fd will leak when it is zero.
>>> Change the testing condition.
>>
>> Why fd==0 is a concern here?  It is a very unlikely
>> situation that fd0 will be picked - firstly because
>> fd0 is almost always open, and second - even if it
>> isn't open, it will be picked much earlier than this
>> code path, ie, some other file will use fd0 before.
>>
>> But even if the concern is real (after all, better
>> stay correct than spread bad code pattern, even if
>> in reality we don't care as this can't happen), why
>> not add 0 to the equality?
>>
>> Why people especially compare with -1?  Any negative
>> value is illegal here and in lots of other places,
>> and many software packages used to return -errno in
>> error cases, which is definitely != -1.  I'm not
>> saying that comparing with -1 is bad in _this_
>> particular case, but why not do it generally in
>> all cases?
>>
>> More, comparing with 0 is faster and shorter than
>> comparing with -1...
>>
>> So if it were me, I'd change it to >= 0, not to
>> == -1.  Here and in all other millions of places
>> in qemu code where it compares with -1... ;)
> 
> Yup.
> 
> In the case of close(), I wouldn't even bother to check, except Coverity
> gets excited when it sees an invalid fd flow into close().  Rightfully
> so when the invalid fd is non-negative, overeager when it's negative.

Thank you, guys.
Paolo had fixed it :)

Best regards,
-Gonglei

[Qemu-devel] [PATCH 2/9] mips_mipssim: fix use-after-free for filename

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

May pass freed pointer filename as an argument to error_report.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/mips/mips_mipssim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/mips/mips_mipssim.c b/hw/mips/mips_mipssim.c
index 7ea0b9a..5d44c3f 100644
--- a/hw/mips/mips_mipssim.c
+++ b/hw/mips/mips_mipssim.c
@@ -197,7 +197,7 @@ mips_mipssim_init(MachineState *machine)
         !kernel_filename && !qtest_enabled()) {
         /* Bail out if we have neither a kernel image nor boot vector code. */
         error_report("Could not load MIPS bios '%s', and no "
-                     "-kernel argument was specified", filename);
+                     "-kernel argument was specified", bios_name);
         exit(1);
     } else {
         /* We have a boot vector start address. */
-- 
1.7.12.4

[Qemu-devel] [PATCH 3/9] qga: fix false negative argument passing

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Function send_response(s, &qdict->base) returns a negative number
when any faliures occured. But strerror()'s parameter cannot be
negative. Let's change the testing condition and pass '-ret' to
strerr().

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 qga/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qga/main.c b/qga/main.c
index 227f2bd..9939a2b 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -603,8 +603,8 @@ static void process_event(JSONMessageParser *parser, QList *tokens)
             error_free(err);
         }
         ret = send_response(s, QOBJECT(qdict));
-        if (ret) {
-            g_warning("error sending error response: %s", strerror(ret));
+        if (ret < 0) {
+            g_warning("error sending error response: %s", strerror(-ret));
         }
     }
 
-- 
1.7.12.4

[Qemu-devel] [PATCH 4/9] loader: fix NEGATIVE_RETURNS

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

lseek will return -1 on error, g_malloc0(size) and read(,,size)
paramenters cannot be negative. We should add a check for return
value of lseek().

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/core/loader.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index bbe6eb3..fc15535 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -80,6 +80,13 @@ int load_image(const char *filename, uint8_t *addr)
     if (fd < 0)
         return -1;
     size = lseek(fd, 0, SEEK_END);
+    if (size == -1) {
+        fprintf(stderr, "file %-20s: get size error: %s\n",
+                filename, strerror(errno));
+        close(fd);
+        return -1;
+    }
+
     lseek(fd, 0, SEEK_SET);
     if (read(fd, addr, size) != size) {
         close(fd);
@@ -748,6 +755,12 @@ int rom_add_file(const char *file, const char *fw_dir,
     }
     rom->addr     = addr;
     rom->romsize  = lseek(fd, 0, SEEK_END);
+    if (rom->romsize == -1) {
+        fprintf(stderr, "rom: file %-20s: get size error: %s\n",
+                rom->name, strerror(errno));
+        goto err;
+    }
+
     rom->datasize = rom->romsize;
     rom->data     = g_malloc0(rom->datasize);
     lseek(fd, 0, SEEK_SET);
-- 
1.7.12.4

[Qemu-devel] [PATCH 5/9] nvme: remove superfluous check

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Operands don't affect result (CONSTANT_EXPRESSION_RESULT)
((n->bar.aqa >> AQA_ASQS_SHIFT) & AQA_ASQS_MASK) > 4095
is always false regardless of the values of its operands.
This occurs as the logical second operand of '||'.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/block/nvme.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index b6263dc..1327658 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -583,8 +583,7 @@ static int nvme_start_ctrl(NvmeCtrl *n)
             NVME_CC_IOCQES(n->bar.cc) > NVME_CTRL_CQES_MAX(n->id_ctrl.cqes) ||
             NVME_CC_IOSQES(n->bar.cc) < NVME_CTRL_SQES_MIN(n->id_ctrl.sqes) ||
             NVME_CC_IOSQES(n->bar.cc) > NVME_CTRL_SQES_MAX(n->id_ctrl.sqes) ||
-            !NVME_AQA_ASQS(n->bar.aqa) || NVME_AQA_ASQS(n->bar.aqa) > 4095 ||
-            !NVME_AQA_ACQS(n->bar.aqa) || NVME_AQA_ACQS(n->bar.aqa) > 4095) {
+            !NVME_AQA_ASQS(n->bar.aqa) || !NVME_AQA_ACQS(n->bar.aqa)) {
         return -1;
     }
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 5/9] nvme: remove superfluous check

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

On Sat, Nov 15, 2014 at 06:06:44PM +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Operands don't affect result (CONSTANT_EXPRESSION_RESULT)
> ((n->bar.aqa >> AQA_ASQS_SHIFT) & AQA_ASQS_MASK) > 4095
> is always false regardless of the values of its operands.
> This occurs as the logical second operand of '||'.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/block/nvme.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH 6/9] acl: fix memory leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

If 'i != index' for all acl->entries, variable
entry leaks the storage it points to.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 util/acl.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/util/acl.c b/util/acl.c
index 938b7ae..571d686 100644
--- a/util/acl.c
+++ b/util/acl.c
@@ -132,7 +132,6 @@ int qemu_acl_insert(qemu_acl *acl,
                     const char *match,
                     int index)
 {
-    qemu_acl_entry *entry;
     qemu_acl_entry *tmp;
     int i = 0;
 
@@ -142,13 +141,14 @@ int qemu_acl_insert(qemu_acl *acl,
         return qemu_acl_append(acl, deny, match);
     }
 
-    entry = g_malloc(sizeof(*entry));
-    entry->match = g_strdup(match);
-    entry->deny = deny;
-
     QTAILQ_FOREACH(tmp, &acl->entries, next) {
         i++;
         if (i == index) {
+            qemu_acl_entry *entry;
+            entry = g_malloc(sizeof(*entry));
+            entry->match = g_strdup(match);
+            entry->deny = deny;
+
             QTAILQ_INSERT_BEFORE(tmp, entry, next);
             acl->nentries++;
             break;
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 6/9] acl: fix memory leak

[Paolo Bonzini]

On 15/11/2014 11:06, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> If 'i != index' for all acl->entries, variable
> entry leaks the storage it points to.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  util/acl.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/util/acl.c b/util/acl.c
> index 938b7ae..571d686 100644
> --- a/util/acl.c
> +++ b/util/acl.c
> @@ -132,7 +132,6 @@ int qemu_acl_insert(qemu_acl *acl,
>                      const char *match,
>                      int index)
>  {
> -    qemu_acl_entry *entry;
>      qemu_acl_entry *tmp;
>      int i = 0;
>  
> @@ -142,13 +141,14 @@ int qemu_acl_insert(qemu_acl *acl,
>          return qemu_acl_append(acl, deny, match);
>      }
>  
> -    entry = g_malloc(sizeof(*entry));
> -    entry->match = g_strdup(match);
> -    entry->deny = deny;
> -
>      QTAILQ_FOREACH(tmp, &acl->entries, next) {
>          i++;
>          if (i == index) {
> +            qemu_acl_entry *entry;
> +            entry = g_malloc(sizeof(*entry));
> +            entry->match = g_strdup(match);
> +            entry->deny = deny;
> +
>              QTAILQ_INSERT_BEFORE(tmp, entry, next);
>              acl->nentries++;
>              break;
> 

This should never happen, but the patch doesn't hurt either.

Paolo

[Qemu-devel] [PATCH 7/9] qemu-char: fix MISSING_COMMA

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 qemu-char.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-char.c b/qemu-char.c
index bd0709b..4a76f0f 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -464,7 +464,7 @@ static const char * const mux_help[] = {
     "% h    print this help\n\r",
     "% x    exit emulator\n\r",
     "% s    save disk data back to file (if -snapshot)\n\r",
-    "% t    toggle console timestamps\n\r"
+    "% t    toggle console timestamps\n\r",
     "% b    send break (magic sysrq)\n\r",
     "% c    switch between console and monitor\n\r",
     "% %  sends %\n\r",
-- 
1.7.12.4

[Qemu-devel] [PATCH 8/9] shpc: fix dead code

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/pci/shpc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 65b2f51..9a39060 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -559,8 +559,9 @@ void shpc_device_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
     uint8_t led;
     int slot;
 
-    shpc_device_hotplug_common(PCI_DEVICE(dev), &slot, shpc, errp);
+    shpc_device_hotplug_common(PCI_DEVICE(dev), &slot, shpc, &local_err);
     if (local_err) {
+        error_propagate(errp, local_err);
         return;
     }
 
-- 
1.7.12.4

[Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/usb/hcd-musb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
index 66bc61a..f2cb73c 100644
--- a/hw/usb/hcd-musb.c
+++ b/hw/usb/hcd-musb.c
@@ -624,6 +624,10 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
 
     /* A wild guess on the FADDR semantics... */
     dev = usb_find_device(&s->port, ep->faddr[idx]);
+    if (!dev) {
+        TRACE("Do not find an usb device");
+        return;
+    }
     uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
     usb_packet_setup(&ep->packey[dir].p, pid, uep, 0,
                      (dev->addr << 16) | (uep->nr << 8) | pid, false, true);
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value

[Paolo Bonzini]

On 15/11/2014 11:06, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/usb/hcd-musb.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
> index 66bc61a..f2cb73c 100644
> --- a/hw/usb/hcd-musb.c
> +++ b/hw/usb/hcd-musb.c
> @@ -624,6 +624,10 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
>  
>      /* A wild guess on the FADDR semantics... */
>      dev = usb_find_device(&s->port, ep->faddr[idx]);
> +    if (!dev) {
> +        TRACE("Do not find an usb device");
> +        return;
> +    }
>      uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
>      usb_packet_setup(&ep->packey[dir].p, pid, uep, 0,
>                       (dev->addr << 16) | (uep->nr << 8) | pid, false, true);
> 

I think this patch is not the real fix.  usb_ep_get and
usb_handle_packet can deal with a NULL device, but we have to avoid 
dereferencing NULL pointers when building the id.

Paolo

diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
index 66bc61a..40809f6 100644
--- a/hw/usb/hcd-musb.c
+++ b/hw/usb/hcd-musb.c
@@ -608,6 +608,7 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
     USBDevice *dev;
     USBEndpoint *uep;
     int idx = epnum && dir;
+    int id;
     int ttype;
 
     /* ep->type[0,1] contains:
@@ -625,8 +626,11 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
     /* A wild guess on the FADDR semantics... */
     dev = usb_find_device(&s->port, ep->faddr[idx]);
     uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
-    usb_packet_setup(&ep->packey[dir].p, pid, uep, 0,
-                     (dev->addr << 16) | (uep->nr << 8) | pid, false, true);
+    id = pid;
+    if (uep) {
+        id |= (dev->addr << 16) | (uep->nr << 8);
+    }
+    usb_packet_setup(&ep->packey[dir].p, pid, uep, 0, id, false, true);
     usb_packet_addbuf(&ep->packey[dir].p, ep->buf[idx], len);
     ep->packey[dir].ep = ep;
     ep->packey[dir].dir = dir;

Re: [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value

[Gonglei]

On 2014/11/17 18:58, Paolo Bonzini wrote:

> 
> 
> On 15/11/2014 11:06, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  hw/usb/hcd-musb.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
>> index 66bc61a..f2cb73c 100644
>> --- a/hw/usb/hcd-musb.c
>> +++ b/hw/usb/hcd-musb.c
>> @@ -624,6 +624,10 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
>>  
>>      /* A wild guess on the FADDR semantics... */
>>      dev = usb_find_device(&s->port, ep->faddr[idx]);
>> +    if (!dev) {
>> +        TRACE("Do not find an usb device");
>> +        return;
>> +    }
>>      uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
>>      usb_packet_setup(&ep->packey[dir].p, pid, uep, 0,
>>                       (dev->addr << 16) | (uep->nr << 8) | pid, false, true);
>>
> 
> I think this patch is not the real fix.  usb_ep_get and
> usb_handle_packet can deal with a NULL device, but we have to avoid 
> dereferencing NULL pointers when building the id.
> 

Good catch :)

> Paolo
> 
> diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
> index 66bc61a..40809f6 100644
> --- a/hw/usb/hcd-musb.c
> +++ b/hw/usb/hcd-musb.c
> @@ -608,6 +608,7 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
>      USBDevice *dev;
>      USBEndpoint *uep;
>      int idx = epnum && dir;
> +    int id;
>      int ttype;
>  
>      /* ep->type[0,1] contains:
> @@ -625,8 +626,11 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
>      /* A wild guess on the FADDR semantics... */
>      dev = usb_find_device(&s->port, ep->faddr[idx]);
>      uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
> -    usb_packet_setup(&ep->packey[dir].p, pid, uep, 0,
> -                     (dev->addr << 16) | (uep->nr << 8) | pid, false, true);
> +    id = pid;
> +    if (uep) {
> +        id |= (dev->addr << 16) | (uep->nr << 8);
> +    }
> +    usb_packet_setup(&ep->packey[dir].p, pid, uep, 0, id, false, true);
>      usb_packet_addbuf(&ep->packey[dir].p, ep->buf[idx], len);
>      ep->packey[dir].ep = ep;
>      ep->packey[dir].dir = dir;

This is a good approach, id is just a identifying. Thanks,

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value

[Gonglei]

On 2014/11/17 19:18, Gonglei wrote:

> On 2014/11/17 18:58, Paolo Bonzini wrote:
> 
>>
>>
>> On 15/11/2014 11:06, arei.gonglei@huawei.com wrote:
>>> From: Gonglei <arei.gonglei@huawei.com>
>>>
>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>>> ---
>>>  hw/usb/hcd-musb.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
>>> index 66bc61a..f2cb73c 100644
>>> --- a/hw/usb/hcd-musb.c
>>> +++ b/hw/usb/hcd-musb.c
>>> @@ -624,6 +624,10 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
>>>  
>>>      /* A wild guess on the FADDR semantics... */
>>>      dev = usb_find_device(&s->port, ep->faddr[idx]);
>>> +    if (!dev) {
>>> +        TRACE("Do not find an usb device");
>>> +        return;
>>> +    }
>>>      uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
>>>      usb_packet_setup(&ep->packey[dir].p, pid, uep, 0,
>>>                       (dev->addr << 16) | (uep->nr << 8) | pid, false, true);
>>>
>>
>> I think this patch is not the real fix.  usb_ep_get and
>> usb_handle_packet can deal with a NULL device, but we have to avoid 
>> dereferencing NULL pointers when building the id.
>>
> 
> Good catch :)
> 
>> Paolo
>>
>> diff --git a/hw/usb/hcd-musb.c b/hw/usb/hcd-musb.c
>> index 66bc61a..40809f6 100644
>> --- a/hw/usb/hcd-musb.c
>> +++ b/hw/usb/hcd-musb.c
>> @@ -608,6 +608,7 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
>>      USBDevice *dev;
>>      USBEndpoint *uep;
>>      int idx = epnum && dir;
>> +    int id;
>>      int ttype;
>>  
>>      /* ep->type[0,1] contains:
>> @@ -625,8 +626,11 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
>>      /* A wild guess on the FADDR semantics... */
>>      dev = usb_find_device(&s->port, ep->faddr[idx]);
>>      uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
>> -    usb_packet_setup(&ep->packey[dir].p, pid, uep, 0,
>> -                     (dev->addr << 16) | (uep->nr << 8) | pid, false, true);
>> +    id = pid;
>> +    if (uep) {
>> +        id |= (dev->addr << 16) | (uep->nr << 8);
>> +    }
>> +    usb_packet_setup(&ep->packey[dir].p, pid, uep, 0, id, false, true);
>>      usb_packet_addbuf(&ep->packey[dir].p, ep->buf[idx], len);
>>      ep->packey[dir].ep = ep;
>>      ep->packey[dir].dir = dir;
> 
> This is a good approach, id is just a identifying. Thanks,
> 

Let me split the patch from this series as a separate patch
and add Paolo's signed-off-by.

Asking for Gerd's reviewing, Thanks.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value

[Gerd Hoffmann]

> >> @@ -625,8 +626,11 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
> >>      /* A wild guess on the FADDR semantics... */
> >>      dev = usb_find_device(&s->port, ep->faddr[idx]);
> >>      uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
> >> -    usb_packet_setup(&ep->packey[dir].p, pid, uep, 0,
> >> -                     (dev->addr << 16) | (uep->nr << 8) | pid, false, true);
> >> +    id = pid;
> >> +    if (uep) {
> >> +        id |= (dev->addr << 16) | (uep->nr << 8);
> >> +    }
> >> +    usb_packet_setup(&ep->packey[dir].p, pid, uep, 0, id, false, true);
> >>      usb_packet_addbuf(&ep->packey[dir].p, ep->buf[idx], len);
> >>      ep->packey[dir].ep = ep;
> >>      ep->packey[dir].dir = dir;
> > 
> > This is a good approach, id is just a identifying. Thanks,
> > 
> 
> Let me split the patch from this series as a separate patch
> and add Paolo's signed-off-by.
> 
> Asking for Gerd's reviewing, Thanks.

Looks good to me.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value

[Paolo Bonzini]

 17/11/2014 14:36, Gerd Hoffmann wrote:
>>>> @@ -625,8 +626,11 @@ static void musb_packet(MUSBState *s, MUSBEndPoint *ep,
>>>>      /* A wild guess on the FADDR semantics... */
>>>>      dev = usb_find_device(&s->port, ep->faddr[idx]);
>>>>      uep = usb_ep_get(dev, pid, ep->type[idx] & 0xf);
>>>> -    usb_packet_setup(&ep->packey[dir].p, pid, uep, 0,
>>>> -                     (dev->addr << 16) | (uep->nr << 8) | pid, false, true);
>>>> +    id = pid;
>>>> +    if (uep) {
>>>> +        id |= (dev->addr << 16) | (uep->nr << 8);
>>>> +    }
>>>> +    usb_packet_setup(&ep->packey[dir].p, pid, uep, 0, id, false, true);
>>>>      usb_packet_addbuf(&ep->packey[dir].p, ep->buf[idx], len);
>>>>      ep->packey[dir].ep = ep;
>>>>      ep->packey[dir].dir = dir;
>>>
>>> This is a good approach, id is just a identifying. Thanks,
>>>
>>
>> Let me split the patch from this series as a separate patch
>> and add Paolo's signed-off-by.
>>
>> Asking for Gerd's reviewing, Thanks.
> 
> Looks good to me.

Thanks, then I'll pick it up.

Paolo