* [Qemu-devel] [PATCH 0/9] Fix Coverity warning reports
@ 2014-11-15 10:06 arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak arei.gonglei
` (8 more replies)
0 siblings, 9 replies; 21+ messages in thread
From: arei.gonglei @ 2014-11-15 10:06 UTC (permalink / raw)
To: qemu-devel
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, mjt,
peter.huangpeng, Gonglei, pbonzini
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak
2014-11-15 10:06 [Qemu-devel] [PATCH 0/9] Fix Coverity warning reports arei.gonglei
@ 2014-11-15 10:06 ` arei.gonglei
2014-11-17 16:51 ` Stefan Hajnoczi
2014-11-17 16:58 ` Michael Tokarev
2014-11-15 10:06 ` [Qemu-devel] [PATCH 2/9] mips_mipssim: fix use-after-free for filename arei.gonglei
` (7 subsequent siblings)
8 siblings, 2 replies; 21+ messages in thread
From: arei.gonglei @ 2014-11-15 10:06 UTC (permalink / raw)
To: qemu-devel
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, mjt,
peter.huangpeng, Gonglei, pbonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 2/9] mips_mipssim: fix use-after-free for filename
2014-11-15 10:06 [Qemu-devel] [PATCH 0/9] Fix Coverity warning reports arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak arei.gonglei
@ 2014-11-15 10:06 ` arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 3/9] qga: fix false negative argument passing arei.gonglei
` (6 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: arei.gonglei @ 2014-11-15 10:06 UTC (permalink / raw)
To: qemu-devel
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, mjt,
peter.huangpeng, Gonglei, pbonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 3/9] qga: fix false negative argument passing
2014-11-15 10:06 [Qemu-devel] [PATCH 0/9] Fix Coverity warning reports arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 2/9] mips_mipssim: fix use-after-free for filename arei.gonglei
@ 2014-11-15 10:06 ` arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 4/9] loader: fix NEGATIVE_RETURNS arei.gonglei
` (5 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: arei.gonglei @ 2014-11-15 10:06 UTC (permalink / raw)
To: qemu-devel
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, mjt,
peter.huangpeng, Gonglei, pbonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 4/9] loader: fix NEGATIVE_RETURNS
2014-11-15 10:06 [Qemu-devel] [PATCH 0/9] Fix Coverity warning reports arei.gonglei
` (2 preceding siblings ...)
2014-11-15 10:06 ` [Qemu-devel] [PATCH 3/9] qga: fix false negative argument passing arei.gonglei
@ 2014-11-15 10:06 ` arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 5/9] nvme: remove superfluous check arei.gonglei
` (4 subsequent siblings)
8 siblings, 0 replies; 21+ messages in thread
From: arei.gonglei @ 2014-11-15 10:06 UTC (permalink / raw)
To: qemu-devel
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, mjt,
peter.huangpeng, Gonglei, pbonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 5/9] nvme: remove superfluous check
2014-11-15 10:06 [Qemu-devel] [PATCH 0/9] Fix Coverity warning reports arei.gonglei
` (3 preceding siblings ...)
2014-11-15 10:06 ` [Qemu-devel] [PATCH 4/9] loader: fix NEGATIVE_RETURNS arei.gonglei
@ 2014-11-15 10:06 ` arei.gonglei
2014-11-17 16:55 ` Stefan Hajnoczi
2014-11-15 10:06 ` [Qemu-devel] [PATCH 6/9] acl: fix memory leak arei.gonglei
` (3 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: arei.gonglei @ 2014-11-15 10:06 UTC (permalink / raw)
To: qemu-devel
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, mjt,
peter.huangpeng, Gonglei, pbonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 6/9] acl: fix memory leak
2014-11-15 10:06 [Qemu-devel] [PATCH 0/9] Fix Coverity warning reports arei.gonglei
` (4 preceding siblings ...)
2014-11-15 10:06 ` [Qemu-devel] [PATCH 5/9] nvme: remove superfluous check arei.gonglei
@ 2014-11-15 10:06 ` arei.gonglei
2014-11-17 10:48 ` Paolo Bonzini
2014-11-15 10:06 ` [Qemu-devel] [PATCH 7/9] qemu-char: fix MISSING_COMMA arei.gonglei
` (2 subsequent siblings)
8 siblings, 1 reply; 21+ messages in thread
From: arei.gonglei @ 2014-11-15 10:06 UTC (permalink / raw)
To: qemu-devel
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, mjt,
peter.huangpeng, Gonglei, pbonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 7/9] qemu-char: fix MISSING_COMMA
2014-11-15 10:06 [Qemu-devel] [PATCH 0/9] Fix Coverity warning reports arei.gonglei
` (5 preceding siblings ...)
2014-11-15 10:06 ` [Qemu-devel] [PATCH 6/9] acl: fix memory leak arei.gonglei
@ 2014-11-15 10:06 ` arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 8/9] shpc: fix dead code arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value arei.gonglei
8 siblings, 0 replies; 21+ messages in thread
From: arei.gonglei @ 2014-11-15 10:06 UTC (permalink / raw)
To: qemu-devel
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, mjt,
peter.huangpeng, Gonglei, pbonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 8/9] shpc: fix dead code
2014-11-15 10:06 [Qemu-devel] [PATCH 0/9] Fix Coverity warning reports arei.gonglei
` (6 preceding siblings ...)
2014-11-15 10:06 ` [Qemu-devel] [PATCH 7/9] qemu-char: fix MISSING_COMMA arei.gonglei
@ 2014-11-15 10:06 ` arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value arei.gonglei
8 siblings, 0 replies; 21+ messages in thread
From: arei.gonglei @ 2014-11-15 10:06 UTC (permalink / raw)
To: qemu-devel
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, mjt,
peter.huangpeng, Gonglei, pbonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value
2014-11-15 10:06 [Qemu-devel] [PATCH 0/9] Fix Coverity warning reports arei.gonglei
` (7 preceding siblings ...)
2014-11-15 10:06 ` [Qemu-devel] [PATCH 8/9] shpc: fix dead code arei.gonglei
@ 2014-11-15 10:06 ` arei.gonglei
2014-11-17 10:58 ` Paolo Bonzini
8 siblings, 1 reply; 21+ messages in thread
From: arei.gonglei @ 2014-11-15 10:06 UTC (permalink / raw)
To: qemu-devel
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, mjt,
peter.huangpeng, Gonglei, pbonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 6/9] acl: fix memory leak
2014-11-15 10:06 ` [Qemu-devel] [PATCH 6/9] acl: fix memory leak arei.gonglei
@ 2014-11-17 10:48 ` Paolo Bonzini
0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2014-11-17 10:48 UTC (permalink / raw)
To: arei.gonglei, qemu-devel
Cc: qemu-trivial, mjt, weidong.huang, peter.huangpeng, zhang.zhanghailiang
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value
2014-11-15 10:06 ` [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value arei.gonglei
@ 2014-11-17 10:58 ` Paolo Bonzini
2014-11-17 11:18 ` Gonglei
0 siblings, 1 reply; 21+ messages in thread
From: Paolo Bonzini @ 2014-11-17 10:58 UTC (permalink / raw)
To: arei.gonglei, qemu-devel
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, mjt,
peter.huangpeng, Gerd Hoffmann
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;
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value
2014-11-17 10:58 ` Paolo Bonzini
@ 2014-11-17 11:18 ` Gonglei
2014-11-17 12:55 ` Gonglei
0 siblings, 1 reply; 21+ messages in thread
From: Gonglei @ 2014-11-17 11:18 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Huangweidong (C),
Zhanghailiang, qemu-trivial, mjt, qemu-devel, Huangpeng (Peter),
Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value
2014-11-17 11:18 ` Gonglei
@ 2014-11-17 12:55 ` Gonglei
2014-11-17 13:36 ` Gerd Hoffmann
0 siblings, 1 reply; 21+ messages in thread
From: Gonglei @ 2014-11-17 12:55 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Huangweidong (C),
Zhanghailiang, qemu-trivial, mjt, qemu-devel, Huangpeng (Peter),
Gerd Hoffmann
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value
2014-11-17 12:55 ` Gonglei
@ 2014-11-17 13:36 ` Gerd Hoffmann
2014-11-17 13:39 ` Paolo Bonzini
0 siblings, 1 reply; 21+ messages in thread
From: Gerd Hoffmann @ 2014-11-17 13:36 UTC (permalink / raw)
To: Gonglei
Cc: Huangweidong (C),
Zhanghailiang, qemu-trivial, mjt, qemu-devel, Huangpeng (Peter),
Paolo Bonzini
> >> @@ -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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value
2014-11-17 13:36 ` Gerd Hoffmann
@ 2014-11-17 13:39 ` Paolo Bonzini
0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2014-11-17 13:39 UTC (permalink / raw)
To: Gerd Hoffmann, Gonglei
Cc: Huangweidong (C),
Zhanghailiang, qemu-trivial, mjt, qemu-devel, Huangpeng (Peter)
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak
2014-11-15 10:06 ` [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak arei.gonglei
@ 2014-11-17 16:51 ` Stefan Hajnoczi
2014-11-17 16:58 ` Michael Tokarev
1 sibling, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2014-11-17 16:51 UTC (permalink / raw)
To: arei.gonglei
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, mjt,
peter.huangpeng, qemu-devel, pbonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 5/9] nvme: remove superfluous check
2014-11-15 10:06 ` [Qemu-devel] [PATCH 5/9] nvme: remove superfluous check arei.gonglei
@ 2014-11-17 16:55 ` Stefan Hajnoczi
0 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2014-11-17 16:55 UTC (permalink / raw)
To: arei.gonglei
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, mjt,
peter.huangpeng, qemu-devel, pbonzini
[-- 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 --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak
2014-11-15 10:06 ` [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak arei.gonglei
2014-11-17 16:51 ` Stefan Hajnoczi
@ 2014-11-17 16:58 ` Michael Tokarev
2014-11-18 7:50 ` Markus Armbruster
1 sibling, 1 reply; 21+ messages in thread
From: Michael Tokarev @ 2014-11-17 16:58 UTC (permalink / raw)
To: arei.gonglei, qemu-devel
Cc: qemu-trivial, pbonzini, weidong.huang, peter.huangpeng,
zhang.zhanghailiang
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) {
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak
2014-11-17 16:58 ` Michael Tokarev
@ 2014-11-18 7:50 ` Markus Armbruster
2014-11-18 8:07 ` Gonglei
0 siblings, 1 reply; 21+ messages in thread
From: Markus Armbruster @ 2014-11-18 7:50 UTC (permalink / raw)
To: Michael Tokarev
Cc: weidong.huang, zhang.zhanghailiang, qemu-trivial, qemu-devel,
peter.huangpeng, arei.gonglei, pbonzini
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak
2014-11-18 7:50 ` Markus Armbruster
@ 2014-11-18 8:07 ` Gonglei
0 siblings, 0 replies; 21+ messages in thread
From: Gonglei @ 2014-11-18 8:07 UTC (permalink / raw)
To: Markus Armbruster
Cc: Huangweidong (C),
Zhanghailiang, qemu-trivial, Michael Tokarev, qemu-devel,
Huangpeng (Peter),
pbonzini
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
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2014-11-18 8:10 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-15 10:06 [Qemu-devel] [PATCH 0/9] Fix Coverity warning reports arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 1/9] l2tpv3: fix fd leak arei.gonglei
2014-11-17 16:51 ` Stefan Hajnoczi
2014-11-17 16:58 ` Michael Tokarev
2014-11-18 7:50 ` Markus Armbruster
2014-11-18 8:07 ` Gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 2/9] mips_mipssim: fix use-after-free for filename arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 3/9] qga: fix false negative argument passing arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 4/9] loader: fix NEGATIVE_RETURNS arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 5/9] nvme: remove superfluous check arei.gonglei
2014-11-17 16:55 ` Stefan Hajnoczi
2014-11-15 10:06 ` [Qemu-devel] [PATCH 6/9] acl: fix memory leak arei.gonglei
2014-11-17 10:48 ` Paolo Bonzini
2014-11-15 10:06 ` [Qemu-devel] [PATCH 7/9] qemu-char: fix MISSING_COMMA arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 8/9] shpc: fix dead code arei.gonglei
2014-11-15 10:06 ` [Qemu-devel] [PATCH 9/9] hcd-musb: fix dereference null return value arei.gonglei
2014-11-17 10:58 ` Paolo Bonzini
2014-11-17 11:18 ` Gonglei
2014-11-17 12:55 ` Gonglei
2014-11-17 13:36 ` Gerd Hoffmann
2014-11-17 13:39 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.