* [Qemu-devel] [PATCH v4 01/10] l2cap: fix access freed memory
2014-08-08 9:21 [Qemu-devel] [PATCH v4 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
@ 2014-08-08 9:21 ` zhanghailiang
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 02/10] monitor: " zhanghailiang
` (8 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: zhanghailiang @ 2014-08-08 9:21 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, lkurusa, zhanghailiang, mst, jan.kiszka, riku.voipio, mjt,
peter.huangpeng, lcapitulino, stefanha, luonengjun, pbonzini,
alex.bennee, rth
Pointer 'ch' will be used in function 'l2cap_channel_open_req_msg' after
it was previously freed in 'l2cap_channel_open'.
Assigned it to NULL after it is freed.
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
hw/bt/l2cap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/bt/l2cap.c b/hw/bt/l2cap.c
index 2301d6f..591e047 100644
--- a/hw/bt/l2cap.c
+++ b/hw/bt/l2cap.c
@@ -429,7 +429,7 @@ static struct l2cap_chan_s *l2cap_channel_open(struct l2cap_instance_s *l2cap,
status = L2CAP_CS_NO_INFO;
} else {
g_free(ch);
-
+ ch = NULL;
result = L2CAP_CR_NO_MEM;
status = L2CAP_CS_NO_INFO;
}
--
1.7.12.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 02/10] monitor: fix access freed memory
2014-08-08 9:21 [Qemu-devel] [PATCH v4 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 01/10] l2cap: fix access freed memory zhanghailiang
@ 2014-08-08 9:21 ` zhanghailiang
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 03/10] virtio-blk: fix reference a pointer which might be freed zhanghailiang
` (7 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: zhanghailiang @ 2014-08-08 9:21 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, lkurusa, zhanghailiang, mst, jan.kiszka, riku.voipio, mjt,
peter.huangpeng, lcapitulino, stefanha, luonengjun, pbonzini,
alex.bennee, rth
The function monitor_fdset_dup_fd_find_remove() references member of 'mon_fdset'
which may be freed in function monitor_fdset_cleanup()
Reviewed-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
monitor.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/monitor.c b/monitor.c
index 5bc70a6..41e46a6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2532,8 +2532,10 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
{
MonFdset *mon_fdset;
MonFdsetFd *mon_fdset_fd_dup;
+ int64_t id = -1;
QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
+ id = mon_fdset->id;
QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) {
if (mon_fdset_fd_dup->fd == dup_fd) {
if (remove) {
@@ -2542,7 +2544,7 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
monitor_fdset_cleanup(mon_fdset);
}
}
- return mon_fdset->id;
+ return id;
}
}
}
--
1.7.12.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 03/10] virtio-blk: fix reference a pointer which might be freed
2014-08-08 9:21 [Qemu-devel] [PATCH v4 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 01/10] l2cap: fix access freed memory zhanghailiang
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 02/10] monitor: " zhanghailiang
@ 2014-08-08 9:21 ` zhanghailiang
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 04/10] ivshmem: check the value returned by fstat() zhanghailiang
` (6 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: zhanghailiang @ 2014-08-08 9:21 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, lkurusa, zhanghailiang, mst, jan.kiszka, riku.voipio, mjt,
peter.huangpeng, lcapitulino, stefanha, luonengjun, pbonzini,
alex.bennee, rth
In function virtio_blk_handle_request, it may freed memory pointed by req,
So do not access member of req after calling this function.
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
hw/block/virtio-blk.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index c241c50..54a853a 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -458,7 +458,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
static void virtio_blk_dma_restart_bh(void *opaque)
{
VirtIOBlock *s = opaque;
- VirtIOBlockReq *req = s->rq;
+ VirtIOBlockReq *req = s->rq, *next = NULL;
MultiReqBuffer mrb = {
.num_writes = 0,
};
@@ -469,8 +469,9 @@ static void virtio_blk_dma_restart_bh(void *opaque)
s->rq = NULL;
while (req) {
+ next = req->next;
virtio_blk_handle_request(req, &mrb);
- req = req->next;
+ req = next;
}
virtio_submit_multiwrite(s->bs, &mrb);
--
1.7.12.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 04/10] ivshmem: check the value returned by fstat()
2014-08-08 9:21 [Qemu-devel] [PATCH v4 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
` (2 preceding siblings ...)
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 03/10] virtio-blk: fix reference a pointer which might be freed zhanghailiang
@ 2014-08-08 9:21 ` zhanghailiang
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 05/10] util/path: check return value of malloc() zhanghailiang
` (5 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: zhanghailiang @ 2014-08-08 9:21 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, lkurusa, zhanghailiang, mst, jan.kiszka, riku.voipio, mjt,
peter.huangpeng, lcapitulino, stefanha, luonengjun, pbonzini,
alex.bennee, rth
The function fstat() may fail, so check its return value.
Acked-by: Levente Kurusa <lkurusa@redhat.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
hw/misc/ivshmem.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 768e528..2be4b86 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -324,7 +324,11 @@ static int check_shm_size(IVShmemState *s, int fd) {
struct stat buf;
- fstat(fd, &buf);
+ if (fstat(fd, &buf) < 0) {
+ fprintf(stderr, "ivshmem: exiting: fstat on fd %d failed: %s\n",
+ fd, strerror(errno));
+ return -1;
+ }
if (s->ivshmem_size > buf.st_size) {
fprintf(stderr,
--
1.7.12.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 05/10] util/path: check return value of malloc()
2014-08-08 9:21 [Qemu-devel] [PATCH v4 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
` (3 preceding siblings ...)
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 04/10] ivshmem: check the value returned by fstat() zhanghailiang
@ 2014-08-08 9:21 ` zhanghailiang
2014-08-08 9:36 ` Alex Bennée
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 06/10] slirp/misc: " zhanghailiang
` (4 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: zhanghailiang @ 2014-08-08 9:21 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, lkurusa, zhanghailiang, mst, jan.kiszka, riku.voipio, mjt,
peter.huangpeng, lcapitulino, stefanha, luonengjun, pbonzini,
alex.bennee, rth
Reviewed-by: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
util/path.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/util/path.c b/util/path.c
index 5c59d9f..df1653f 100644
--- a/util/path.c
+++ b/util/path.c
@@ -46,9 +46,12 @@ static struct pathelem *new_entry(const char *root,
const char *name)
{
struct pathelem *new = malloc(sizeof(*new));
- new->name = strdup(name);
- new->pathname = g_strdup_printf("%s/%s", root, name);
- new->num_entries = 0;
+
+ if (new) {
+ new->name = strdup(name);
+ new->pathname = g_strdup_printf("%s/%s", root, name);
+ new->num_entries = 0;
+ }
return new;
}
--
1.7.12.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 05/10] util/path: check return value of malloc()
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 05/10] util/path: check return value of malloc() zhanghailiang
@ 2014-08-08 9:36 ` Alex Bennée
2014-08-08 10:35 ` zhanghailiang
0 siblings, 1 reply; 19+ messages in thread
From: Alex Bennée @ 2014-08-08 9:36 UTC (permalink / raw)
To: zhanghailiang
Cc: kwolf, lkurusa, mst, jan.kiszka, riku.voipio, mjt, qemu-devel,
lcapitulino, stefanha, luonengjun, pbonzini, peter.huangpeng,
rth
zhanghailiang writes:
> Reviewed-by: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
> util/path.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/util/path.c b/util/path.c
> index 5c59d9f..df1653f 100644
> --- a/util/path.c
> +++ b/util/path.c
> @@ -46,9 +46,12 @@ static struct pathelem *new_entry(const char *root,
> const char *name)
> {
> struct pathelem *new = malloc(sizeof(*new));
> - new->name = strdup(name);
> - new->pathname = g_strdup_printf("%s/%s", root, name);
> - new->num_entries = 0;
Erm... isn't that malloc wrong as sizeof(*new) would be the size of a
pointer?
> +
> + if (new) {
> + new->name = strdup(name);
> + new->pathname = g_strdup_printf("%s/%s", root, name);
> + new->num_entries = 0;
> + }
> return new;
> }
A better approach may be to just g_malloc which would abort on failure
(which would be fine for setup).
static struct pathelem *new_entry(const char *root,
struct pathelem *parent,
const char *name)
{
struct pathelem *new = g_malloc(sizeof(pathelem));
new->name = g_strdup(name);
new->pathname = g_strdup_printf("%s/%s", root, name);
new->num_entries = 0;
return new;
}
--
Alex Bennée
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 05/10] util/path: check return value of malloc()
2014-08-08 9:36 ` Alex Bennée
@ 2014-08-08 10:35 ` zhanghailiang
0 siblings, 0 replies; 19+ messages in thread
From: zhanghailiang @ 2014-08-08 10:35 UTC (permalink / raw)
To: Alex Bennée
Cc: kwolf, lkurusa, mst, jan.kiszka, riku.voipio, mjt, qemu-devel,
lcapitulino, stefanha, luonengjun, pbonzini, peter.huangpeng,
rth
On 2014/8/8 17:36, Alex Bennée wrote:
>
> zhanghailiang writes:
>
>> Reviewed-by: Gonglei<arei.gonglei@huawei.com>
>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>> ---
>> util/path.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/util/path.c b/util/path.c
>> index 5c59d9f..df1653f 100644
>> --- a/util/path.c
>> +++ b/util/path.c
>> @@ -46,9 +46,12 @@ static struct pathelem *new_entry(const char *root,
>> const char *name)
>> {
>> struct pathelem *new = malloc(sizeof(*new));
>> - new->name = strdup(name);
>> - new->pathname = g_strdup_printf("%s/%s", root, name);
>> - new->num_entries = 0;
>
> Erm... isn't that malloc wrong as sizeof(*new) would be the size of a
> pointer?
>
No, this is OK! It is equal to sizeof(struct pathelem).
>> +
>> + if (new) {
>> + new->name = strdup(name);
>> + new->pathname = g_strdup_printf("%s/%s", root, name);
>> + new->num_entries = 0;
>> + }
>> return new;
>> }
>
> A better approach may be to just g_malloc which would abort on failure
> (which would be fine for setup).
>
Hmm, Good idea! It is more quickly to know what happen when it fails.
I will change to it, Thanks, Alex.
> static struct pathelem *new_entry(const char *root,
> struct pathelem *parent,
> const char *name)
> {
> struct pathelem *new = g_malloc(sizeof(pathelem));
> new->name = g_strdup(name);
> new->pathname = g_strdup_printf("%s/%s", root, name);
> new->num_entries = 0;
> return new;
> }
>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 06/10] slirp/misc: check return value of malloc()
2014-08-08 9:21 [Qemu-devel] [PATCH v4 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
` (4 preceding siblings ...)
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 05/10] util/path: check return value of malloc() zhanghailiang
@ 2014-08-08 9:21 ` zhanghailiang
2014-08-08 9:43 ` Alex Bennée
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 07/10] linux-user: " zhanghailiang
` (3 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: zhanghailiang @ 2014-08-08 9:21 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, lkurusa, zhanghailiang, mst, jan.kiszka, riku.voipio, mjt,
peter.huangpeng, lcapitulino, stefanha, luonengjun, pbonzini,
alex.bennee, rth
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
slirp/misc.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/slirp/misc.c b/slirp/misc.c
index b8eb74c..9b457ad 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -55,6 +55,10 @@ int add_exec(struct ex_list **ex_ptr, int do_pty, char *exec,
tmp_ptr = *ex_ptr;
*ex_ptr = (struct ex_list *)malloc(sizeof(struct ex_list));
+ if (*ex_ptr == NULL) {
+ fprintf(stderr, "Error: malloc failed\n");
+ return -1;
+ }
(*ex_ptr)->ex_fport = port;
(*ex_ptr)->ex_addr = addr;
(*ex_ptr)->ex_pty = do_pty;
@@ -236,8 +240,9 @@ strdup(str)
char *bptr;
bptr = (char *)malloc(strlen(str)+1);
- strcpy(bptr, str);
-
+ if (bptr) {
+ strcpy(bptr, str);
+ }
return bptr;
}
#endif
--
1.7.12.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 06/10] slirp/misc: check return value of malloc()
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 06/10] slirp/misc: " zhanghailiang
@ 2014-08-08 9:43 ` Alex Bennée
2014-08-08 10:44 ` zhanghailiang
0 siblings, 1 reply; 19+ messages in thread
From: Alex Bennée @ 2014-08-08 9:43 UTC (permalink / raw)
To: zhanghailiang
Cc: kwolf, lkurusa, mst, jan.kiszka, riku.voipio, mjt, qemu-devel,
lcapitulino, stefanha, luonengjun, pbonzini, peter.huangpeng,
rth
zhanghailiang writes:
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
> slirp/misc.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/slirp/misc.c b/slirp/misc.c
> index b8eb74c..9b457ad 100644
> --- a/slirp/misc.c
> +++ b/slirp/misc.c
> @@ -55,6 +55,10 @@ int add_exec(struct ex_list **ex_ptr, int do_pty, char *exec,
>
> tmp_ptr = *ex_ptr;
> *ex_ptr = (struct ex_list *)malloc(sizeof(struct ex_list));
> + if (*ex_ptr == NULL) {
> + fprintf(stderr, "Error: malloc failed\n");
> + return -1;
> + }
Your indenting has gone a bit weird there.
> (*ex_ptr)->ex_fport = port;
> (*ex_ptr)->ex_addr = addr;
> (*ex_ptr)->ex_pty = do_pty;
> @@ -236,8 +240,9 @@ strdup(str)
> char *bptr;
>
> bptr = (char *)malloc(strlen(str)+1);
> - strcpy(bptr, str);
> -
> + if (bptr) {
> + strcpy(bptr, str);
> + }
> return bptr;
> }
> #endif
Again use of g_malloc would remove the need for this. HACKING section 3
says:
3. Low level memory management
Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign
APIs is not allowed in the QEMU codebase. Instead of these routines,
use the GLib memory allocation routines g_malloc/g_malloc0/g_new/
g_new0/g_realloc/g_free or QEMU's qemu_memalign/qemu_blockalign/qemu_vfree
APIs.
Please note that g_malloc will exit on allocation failure, so there
is no need to test for failure (as you would have to with malloc).
Calling g_malloc with a zero size is valid and will return NULL.
--
Alex Bennée
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 06/10] slirp/misc: check return value of malloc()
2014-08-08 9:43 ` Alex Bennée
@ 2014-08-08 10:44 ` zhanghailiang
2014-08-08 13:24 ` Alex Bennée
0 siblings, 1 reply; 19+ messages in thread
From: zhanghailiang @ 2014-08-08 10:44 UTC (permalink / raw)
To: Alex Bennée
Cc: kwolf, lkurusa, mst, jan.kiszka, riku.voipio, mjt, qemu-devel,
lcapitulino, stefanha, luonengjun, pbonzini, peter.huangpeng,
rth
On 2014/8/8 17:43, Alex Bennée wrote:
>
> zhanghailiang writes:
>
>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>> ---
>> slirp/misc.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/slirp/misc.c b/slirp/misc.c
>> index b8eb74c..9b457ad 100644
>> --- a/slirp/misc.c
>> +++ b/slirp/misc.c
>> @@ -55,6 +55,10 @@ int add_exec(struct ex_list **ex_ptr, int do_pty, char *exec,
>>
>> tmp_ptr = *ex_ptr;
>> *ex_ptr = (struct ex_list *)malloc(sizeof(struct ex_list));
>> + if (*ex_ptr == NULL) {
>> + fprintf(stderr, "Error: malloc failed\n");
>> + return -1;
>> + }
>
> Your indenting has gone a bit weird there.
Hmm, this file has some places that use tab key as indent.
Here i used spaces as indent, otherwise the patch can not pass the check
of '/scripts/checkpatch.pl'.
What's your opinion? Use tab as what it does? Thanks!
>
>> (*ex_ptr)->ex_fport = port;
>> (*ex_ptr)->ex_addr = addr;
>> (*ex_ptr)->ex_pty = do_pty;
>> @@ -236,8 +240,9 @@ strdup(str)
>> char *bptr;
>>
>> bptr = (char *)malloc(strlen(str)+1);
>> - strcpy(bptr, str);
>> -
>> + if (bptr) {
>> + strcpy(bptr, str);
>> + }
>> return bptr;
>> }
>> #endif
>
> Again use of g_malloc would remove the need for this. HACKING section 3
> says:
>
OK, Thanks!
> 3. Low level memory management
>
> Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign
> APIs is not allowed in the QEMU codebase. Instead of these routines,
> use the GLib memory allocation routines g_malloc/g_malloc0/g_new/
> g_new0/g_realloc/g_free or QEMU's qemu_memalign/qemu_blockalign/qemu_vfree
> APIs.
>
> Please note that g_malloc will exit on allocation failure, so there
> is no need to test for failure (as you would have to with malloc).
> Calling g_malloc with a zero size is valid and will return NULL.
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 06/10] slirp/misc: check return value of malloc()
2014-08-08 10:44 ` zhanghailiang
@ 2014-08-08 13:24 ` Alex Bennée
2014-08-11 7:18 ` zhanghailiang
0 siblings, 1 reply; 19+ messages in thread
From: Alex Bennée @ 2014-08-08 13:24 UTC (permalink / raw)
To: zhanghailiang
Cc: kwolf, lkurusa, mst, jan.kiszka, riku.voipio, mjt, qemu-devel,
lcapitulino, stefanha, luonengjun, pbonzini, peter.huangpeng,
rth
zhanghailiang writes:
> On 2014/8/8 17:43, Alex Bennée wrote:
>>
>> zhanghailiang writes:
>>
>>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>>> ---
>>> slirp/misc.c | 9 +++++++--
>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>
<snip>
>>
>> Your indenting has gone a bit weird there.
>
> Hmm, this file has some places that use tab key as indent.
> Here i used spaces as indent, otherwise the patch can not pass the check
> of '/scripts/checkpatch.pl'.
>
> What's your opinion? Use tab as what it does? Thanks!
Welcome to the world of QEMU's inconsistent whitespace ;-)
You have two choices:
* two patches: 1st to clean up whitespace for that function, 2nd to
fix
* keep to using tabs for that particular fix
Eventually the code base will get to a consistent state we hope...
>>> (*ex_ptr)->ex_fport = port;
>>> (*ex_ptr)->ex_addr = addr;
>>> (*ex_ptr)->ex_pty = do_pty;
>>> @@ -236,8 +240,9 @@ strdup(str)
>>> char *bptr;
>>>
>>> bptr = (char *)malloc(strlen(str)+1);
>>> - strcpy(bptr, str);
>>> -
>>> + if (bptr) {
>>> + strcpy(bptr, str);
>>> + }
>>> return bptr;
>>> }
>>> #endif
>>
>> Again use of g_malloc would remove the need for this. HACKING section 3
>> says:
>>
>
> OK, Thanks!
>
>> 3. Low level memory management
>>
>> Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign
>> APIs is not allowed in the QEMU codebase. Instead of these routines,
>> use the GLib memory allocation routines g_malloc/g_malloc0/g_new/
>> g_new0/g_realloc/g_free or QEMU's qemu_memalign/qemu_blockalign/qemu_vfree
>> APIs.
>>
>> Please note that g_malloc will exit on allocation failure, so there
>> is no need to test for failure (as you would have to with malloc).
>> Calling g_malloc with a zero size is valid and will return NULL.
>>
>>
--
Alex Bennée
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 06/10] slirp/misc: check return value of malloc()
2014-08-08 13:24 ` Alex Bennée
@ 2014-08-11 7:18 ` zhanghailiang
0 siblings, 0 replies; 19+ messages in thread
From: zhanghailiang @ 2014-08-11 7:18 UTC (permalink / raw)
To: Alex Bennée
Cc: kwolf, lkurusa, mst, jan.kiszka, riku.voipio, mjt, qemu-devel,
lcapitulino, stefanha, luonengjun, pbonzini, peter.huangpeng,
rth
On 2014/8/8 21:24, Alex Bennée wrote:
>
> zhanghailiang writes:
>
>> On 2014/8/8 17:43, Alex Bennée wrote:
>>>
>>> zhanghailiang writes:
>>>
>>>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>>>> ---
>>>> slirp/misc.c | 9 +++++++--
>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
> <snip>
>>>
>>> Your indenting has gone a bit weird there.
>>
>> Hmm, this file has some places that use tab key as indent.
>> Here i used spaces as indent, otherwise the patch can not pass the check
>> of '/scripts/checkpatch.pl'.
>>
>> What's your opinion? Use tab as what it does? Thanks!
>
> Welcome to the world of QEMU's inconsistent whitespace ;-)
>
> You have two choices:
>
> * two patches: 1st to clean up whitespace for that function, 2nd to
> fix
> * keep to using tabs for that particular fix
>
> Eventually the code base will get to a consistent state we hope...
>
OK, I will choose the second way! Thanks, Alex.
>>>> (*ex_ptr)->ex_fport = port;
>>>> (*ex_ptr)->ex_addr = addr;
>>>> (*ex_ptr)->ex_pty = do_pty;
>>>> @@ -236,8 +240,9 @@ strdup(str)
>>>> char *bptr;
>>>>
>>>> bptr = (char *)malloc(strlen(str)+1);
>>>> - strcpy(bptr, str);
>>>> -
>>>> + if (bptr) {
>>>> + strcpy(bptr, str);
>>>> + }
>>>> return bptr;
>>>> }
>>>> #endif
>>>
>>> Again use of g_malloc would remove the need for this. HACKING section 3
>>> says:
>>>
>>
>> OK, Thanks!
>>
>>> 3. Low level memory management
>>>
>>> Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign
>>> APIs is not allowed in the QEMU codebase. Instead of these routines,
>>> use the GLib memory allocation routines g_malloc/g_malloc0/g_new/
>>> g_new0/g_realloc/g_free or QEMU's qemu_memalign/qemu_blockalign/qemu_vfree
>>> APIs.
>>>
>>> Please note that g_malloc will exit on allocation failure, so there
>>> is no need to test for failure (as you would have to with malloc).
>>> Calling g_malloc with a zero size is valid and will return NULL.
>>>
>>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 07/10] linux-user: check return value of malloc()
2014-08-08 9:21 [Qemu-devel] [PATCH v4 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
` (5 preceding siblings ...)
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 06/10] slirp/misc: " zhanghailiang
@ 2014-08-08 9:21 ` zhanghailiang
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 08/10] tests/bios-tables-test: check the value returned by fopen() zhanghailiang
` (2 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: zhanghailiang @ 2014-08-08 9:21 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, lkurusa, zhanghailiang, mst, jan.kiszka, riku.voipio, mjt,
peter.huangpeng, lcapitulino, stefanha, luonengjun, pbonzini,
alex.bennee, rth
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
linux-user/syscall.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index a50229d..8e5ccf1 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2870,6 +2870,10 @@ static inline abi_long do_msgsnd(int msqid, abi_long msgp,
if (!lock_user_struct(VERIFY_READ, target_mb, msgp, 0))
return -TARGET_EFAULT;
host_mb = malloc(msgsz+sizeof(long));
+ if (!host_mb) {
+ unlock_user_struct(target_mb, msgp, 0);
+ return -TARGET_ENOMEM;
+ }
host_mb->mtype = (abi_long) tswapal(target_mb->mtype);
memcpy(host_mb->mtext, target_mb->mtext, msgsz);
ret = get_errno(msgsnd(msqid, host_mb, msgsz, msgflg));
--
1.7.12.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 08/10] tests/bios-tables-test: check the value returned by fopen()
2014-08-08 9:21 [Qemu-devel] [PATCH v4 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
` (6 preceding siblings ...)
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 07/10] linux-user: " zhanghailiang
@ 2014-08-08 9:21 ` zhanghailiang
2014-08-08 9:51 ` Alex Bennée
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 09/10] tcg: check return value of fopen() zhanghailiang
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 10/10] block/vvfat: fix setbuf stream parameter may be NULL zhanghailiang
9 siblings, 1 reply; 19+ messages in thread
From: zhanghailiang @ 2014-08-08 9:21 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, lkurusa, zhanghailiang, mst, jan.kiszka, riku.voipio, mjt,
peter.huangpeng, lcapitulino, stefanha, Li Liu, luonengjun,
pbonzini, alex.bennee, rth
The function fopen() may fail, so check its return value.
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Li Liu <john.liuli@huawei.com>
---
tests/bios-tables-test.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 045eb27..6a357c0 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -790,6 +790,8 @@ int main(int argc, char *argv[])
const char *arch = qtest_get_arch();
FILE *f = fopen(disk, "w");
int ret;
+
+ g_assert(f != NULL);
fwrite(boot_sector, 1, sizeof boot_sector, f);
fclose(f);
--
1.7.12.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 08/10] tests/bios-tables-test: check the value returned by fopen()
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 08/10] tests/bios-tables-test: check the value returned by fopen() zhanghailiang
@ 2014-08-08 9:51 ` Alex Bennée
2014-08-08 10:46 ` zhanghailiang
0 siblings, 1 reply; 19+ messages in thread
From: Alex Bennée @ 2014-08-08 9:51 UTC (permalink / raw)
To: zhanghailiang
Cc: kwolf, lkurusa, mst, jan.kiszka, riku.voipio, mjt, qemu-devel,
lcapitulino, stefanha, Li Liu, luonengjun, pbonzini,
peter.huangpeng, rth
zhanghailiang writes:
> The function fopen() may fail, so check its return value.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Li Liu <john.liuli@huawei.com>
> ---
> tests/bios-tables-test.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 045eb27..6a357c0 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -790,6 +790,8 @@ int main(int argc, char *argv[])
> const char *arch = qtest_get_arch();
> FILE *f = fopen(disk, "w");
> int ret;
> +
> + g_assert(f != NULL);
> fwrite(boot_sector, 1, sizeof boot_sector, f);
> fclose(f);
Incorrect use of g_assert. An assertion is a test for things that
shouldn't happen so in this case it's saying:
* this function assumes files will always successfully open
Which is not the case. It's quite possible that a fopen will fail and
the correct behaviour is to handle the error, not assert out.
--
Alex Bennée
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH v4 08/10] tests/bios-tables-test: check the value returned by fopen()
2014-08-08 9:51 ` Alex Bennée
@ 2014-08-08 10:46 ` zhanghailiang
0 siblings, 0 replies; 19+ messages in thread
From: zhanghailiang @ 2014-08-08 10:46 UTC (permalink / raw)
To: Alex Bennée
Cc: kwolf, lkurusa, mst, jan.kiszka, riku.voipio, mjt, qemu-devel,
lcapitulino, stefanha, Li Liu, luonengjun, pbonzini,
peter.huangpeng, rth
On 2014/8/8 17:51, Alex Bennée wrote:
>
> zhanghailiang writes:
>
>> The function fopen() may fail, so check its return value.
>>
>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Li Liu<john.liuli@huawei.com>
>> ---
>> tests/bios-tables-test.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
>> index 045eb27..6a357c0 100644
>> --- a/tests/bios-tables-test.c
>> +++ b/tests/bios-tables-test.c
>> @@ -790,6 +790,8 @@ int main(int argc, char *argv[])
>> const char *arch = qtest_get_arch();
>> FILE *f = fopen(disk, "w");
>> int ret;
>> +
>> + g_assert(f != NULL);
>> fwrite(boot_sector, 1, sizeof boot_sector, f);
>> fclose(f);
>
> Incorrect use of g_assert. An assertion is a test for things that
> shouldn't happen so in this case it's saying:
>
> * this function assumes files will always successfully open
>
> Which is not the case. It's quite possible that a fopen will fail and
> the correct behaviour is to handle the error, not assert out.
>
>
Good catch! That is a low grade fault, i will correct it! Thanks, Alex
Best regards,
zhanghailiang
^ permalink raw reply [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 09/10] tcg: check return value of fopen()
2014-08-08 9:21 [Qemu-devel] [PATCH v4 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
` (7 preceding siblings ...)
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 08/10] tests/bios-tables-test: check the value returned by fopen() zhanghailiang
@ 2014-08-08 9:21 ` zhanghailiang
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 10/10] block/vvfat: fix setbuf stream parameter may be NULL zhanghailiang
9 siblings, 0 replies; 19+ messages in thread
From: zhanghailiang @ 2014-08-08 9:21 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, lkurusa, zhanghailiang, mst, jan.kiszka, riku.voipio, mjt,
peter.huangpeng, lcapitulino, stefanha, Li Liu, luonengjun,
pbonzini, alex.bennee, rth
From: Li Liu <john.liuli@huawei.com>
Give a warning message if fopen() failed to open the log file.
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Li Liu <john.liuli@huawei.com>
---
tcg/tcg.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tcg/tcg.c b/tcg/tcg.c
index c068990..8f50d2a 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2406,6 +2406,10 @@ static void dump_op_count(void)
int i;
FILE *f;
f = fopen("/tmp/op.log", "w");
+ if (f == NULL) {
+ fprintf(stderr, "Failed to open /tmp/op.log\n");
+ return;
+ }
for(i = INDEX_op_end; i < NB_OPS; i++) {
fprintf(f, "%s %" PRId64 "\n", tcg_op_defs[i].name, tcg_table_op_count[i]);
}
--
1.7.12.4
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH v4 10/10] block/vvfat: fix setbuf stream parameter may be NULL
2014-08-08 9:21 [Qemu-devel] [PATCH v4 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
` (8 preceding siblings ...)
2014-08-08 9:21 ` [Qemu-devel] [PATCH v4 09/10] tcg: check return value of fopen() zhanghailiang
@ 2014-08-08 9:21 ` zhanghailiang
9 siblings, 0 replies; 19+ messages in thread
From: zhanghailiang @ 2014-08-08 9:21 UTC (permalink / raw)
To: qemu-devel
Cc: kwolf, lkurusa, zhanghailiang, mst, jan.kiszka, riku.voipio, mjt,
peter.huangpeng, lcapitulino, stefanha, Li Liu, luonengjun,
pbonzini, alex.bennee, rth
From: Li Liu <john.liuli@huawei.com>
fopen() may return NULL which will cause setbuf() segmentfault
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Li Liu <john.liuli@huawei.com>
---
block/vvfat.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/block/vvfat.c b/block/vvfat.c
index 70176b1..6889ea9 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1084,7 +1084,10 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
DLOG(if (stderr == NULL) {
stderr = fopen("vvfat.log", "a");
- setbuf(stderr, NULL);
+
+ if (stderr) {
+ setbuf(stderr, NULL);
+ }
})
opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
--
1.7.12.4
^ permalink raw reply related [flat|nested] 19+ messages in thread