qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v5 00/10] fix three bugs about use-after-free and several api abuse
@ 2014-08-11  8:52 zhanghailiang
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 01/10] l2cap: fix access freed memory zhanghailiang
                   ` (9 more replies)
  0 siblings, 10 replies; 18+ messages in thread
From: zhanghailiang @ 2014-08-11  8:52 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

This patch set fix three bugs about accessing freed memory and several api abuse.

In qemu, there are serveral places that do not check 
the return value of fstat()/fopen()/malloc(),etc.

Though it is a small probability for the these functions to fail,
but it is better to fix them, Or there may be a serious segmentfault. 

v4 -> v5:
util/path:
 * Use the GLib memory APIs g_malloc/g_strdup/g_realloc 
which would abort on failure (Thanks for the suggestion of Alex Bennée)

slirp:
 * Again use of g_malloc to replace malloc(based on the review of Alex Bennée)

bios-tables-test:
 * Correct the wrong use of g_assert

v3 -> v4:
slirp: 
 * Check return value of '*ex_ptr', not 'ex_ptr',also add error message
(basedon the review of GongLei)

linux-user:
 * It should call unlock_user_struct() before return
(based on the review of Richard Henderson)
  
tests/bios-tables-test: 
 * Remove unnecessary check then return value of fopen() in qtest_init()

v2 -> v3:
ivshmem: 
 * Change the error message which advised by Levente Kurusa 

others: 
 * Add six new patches which check the return value of malloc() and fopen(),
  which may be failed.

v1 -> v2:
ivshmem: 
 * Modified the log message according to reviewing suggestion of Michael

Li Liu (3):
  tcg: check return value of fopen()
  block/vvfat: fix setbuf stream parameter may be NULL
  qtest: check the value returned by fopen()

zhanghailiang (7):
  l2cap: fix access freed memory
  monitor: fix access freed memory
  virtio-blk: fix reference a pointer which might be freed
  ivshmem: check the value returned by fstat()
  util/path: check return value of malloc()
  slirp: check return value of malloc()
  linux-user: check return value of malloc()

 block/vvfat.c            | 5 ++++-
 hw/block/virtio-blk.c    | 5 +++--
 hw/bt/l2cap.c            | 2 +-
 hw/misc/ivshmem.c        | 6 +++++-
 linux-user/syscall.c     | 4 ++++
 monitor.c                | 4 +++-
 slirp/misc.c             | 4 ++--
 tcg/tcg.c                | 4 ++++
 tests/bios-tables-test.c | 5 +++++
 util/path.c              | 6 +++---
 10 files changed, 34 insertions(+), 11 deletions(-)

-- 
1.7.12.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH v5 01/10] l2cap: fix access freed memory
  2014-08-11  8:52 [Qemu-devel] [PATCH v5 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
@ 2014-08-11  8:53 ` zhanghailiang
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 02/10] monitor: " zhanghailiang
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: zhanghailiang @ 2014-08-11  8:53 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] 18+ messages in thread

* [Qemu-devel] [PATCH v5 02/10] monitor: fix access freed memory
  2014-08-11  8:52 [Qemu-devel] [PATCH v5 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 01/10] l2cap: fix access freed memory zhanghailiang
@ 2014-08-11  8:53 ` zhanghailiang
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 03/10] virtio-blk: fix reference a pointer which might be freed zhanghailiang
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: zhanghailiang @ 2014-08-11  8:53 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] 18+ messages in thread

* [Qemu-devel] [PATCH v5 03/10] virtio-blk: fix reference a pointer which might be freed
  2014-08-11  8:52 [Qemu-devel] [PATCH v5 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 01/10] l2cap: fix access freed memory zhanghailiang
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 02/10] monitor: " zhanghailiang
@ 2014-08-11  8:53 ` zhanghailiang
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 04/10] ivshmem: check the value returned by fstat() zhanghailiang
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: zhanghailiang @ 2014-08-11  8:53 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] 18+ messages in thread

* [Qemu-devel] [PATCH v5 04/10] ivshmem: check the value returned by fstat()
  2014-08-11  8:52 [Qemu-devel] [PATCH v5 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
                   ` (2 preceding siblings ...)
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 03/10] virtio-blk: fix reference a pointer which might be freed zhanghailiang
@ 2014-08-11  8:53 ` zhanghailiang
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 05/10] util/path: Use the GLib memory allocation routines zhanghailiang
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 18+ messages in thread
From: zhanghailiang @ 2014-08-11  8:53 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] 18+ messages in thread

* [Qemu-devel] [PATCH v5 05/10] util/path: Use the GLib memory allocation routines
  2014-08-11  8:52 [Qemu-devel] [PATCH v5 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
                   ` (3 preceding siblings ...)
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 04/10] ivshmem: check the value returned by fstat() zhanghailiang
@ 2014-08-11  8:53 ` zhanghailiang
  2014-08-11  9:32   ` Alex Bennée
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 06/10] slirp/misc: Use g_malloc() instead of malloc() zhanghailiang
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: zhanghailiang @ 2014-08-11  8:53 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 this file, we don't check the return value of malloc/strdup/realloc which may fail.
Instead of using these APIs, we use the GLib memory APIs g_malloc/g_strdup/g_realloc.
They will exit on allocation failure, so there is no need to test for failure,
which would be fine for setup.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 util/path.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/util/path.c b/util/path.c
index 5c59d9f..e152f2a 100644
--- a/util/path.c
+++ b/util/path.c
@@ -45,8 +45,8 @@ static struct pathelem *new_entry(const char *root,
                                   struct pathelem *parent,
                                   const char *name)
 {
-    struct pathelem *new = malloc(sizeof(*new));
-    new->name = strdup(name);
+    struct pathelem *new = g_malloc(sizeof(*new));
+    new->name = g_strdup(name);
     new->pathname = g_strdup_printf("%s/%s", root, name);
     new->num_entries = 0;
     return new;
@@ -88,7 +88,7 @@ static struct pathelem *add_entry(struct pathelem *root, const char *name,
 
     root->num_entries++;
 
-    root = realloc(root, sizeof(*root)
+    root = g_realloc(root, sizeof(*root)
                    + sizeof(root->entries[0])*root->num_entries);
     e = &root->entries[root->num_entries-1];
 
-- 
1.7.12.4

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH v5 06/10] slirp/misc: Use g_malloc() instead of malloc()
  2014-08-11  8:52 [Qemu-devel] [PATCH v5 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
                   ` (4 preceding siblings ...)
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 05/10] util/path: Use the GLib memory allocation routines zhanghailiang
@ 2014-08-11  8:53 ` zhanghailiang
  2014-08-11  9:33   ` Alex Bennée
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 07/10] linux-user: check return value " zhanghailiang
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: zhanghailiang @ 2014-08-11  8:53 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

Here we don't check the return value of malloc() which may fail.
Use the g_malloc() instead, which will abort the program when
there is not enough memory.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 slirp/misc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/slirp/misc.c b/slirp/misc.c
index b8eb74c..f7fe497 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -54,7 +54,7 @@ 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));
+	*ex_ptr = (struct ex_list *)g_malloc(sizeof(struct ex_list));
 	(*ex_ptr)->ex_fport = port;
 	(*ex_ptr)->ex_addr = addr;
 	(*ex_ptr)->ex_pty = do_pty;
@@ -235,7 +235,7 @@ strdup(str)
 {
 	char *bptr;
 
-	bptr = (char *)malloc(strlen(str)+1);
+	bptr = (char *)g_malloc(strlen(str)+1);
 	strcpy(bptr, str);
 
 	return bptr;
-- 
1.7.12.4

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH v5 07/10] linux-user: check return value of malloc()
  2014-08-11  8:52 [Qemu-devel] [PATCH v5 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
                   ` (5 preceding siblings ...)
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 06/10] slirp/misc: Use g_malloc() instead of malloc() zhanghailiang
@ 2014-08-11  8:53 ` zhanghailiang
  2014-08-12  7:34   ` Riku Voipio
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 08/10] tests/bios-tables-test: check the value returned by fopen() zhanghailiang
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 18+ messages in thread
From: zhanghailiang @ 2014-08-11  8:53 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] 18+ messages in thread

* [Qemu-devel] [PATCH v5 08/10] tests/bios-tables-test: check the value returned by fopen()
  2014-08-11  8:52 [Qemu-devel] [PATCH v5 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
                   ` (6 preceding siblings ...)
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 07/10] linux-user: check return value " zhanghailiang
@ 2014-08-11  8:53 ` zhanghailiang
  2014-08-11  9:33   ` Alex Bennée
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 09/10] tcg: check return value of fopen() zhanghailiang
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 10/10] block/vvfat: fix setbuf stream parameter may be NULL zhanghailiang
  9 siblings, 1 reply; 18+ messages in thread
From: zhanghailiang @ 2014-08-11  8:53 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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 045eb27..28ec28d 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -790,6 +790,11 @@ int main(int argc, char *argv[])
     const char *arch = qtest_get_arch();
     FILE *f = fopen(disk, "w");
     int ret;
+
+    if (f == NULL) {
+        fprintf(stderr, "Couldn't open \"%s\": %s", disk, strerror(errno));
+        return -1;
+    }
     fwrite(boot_sector, 1, sizeof boot_sector, f);
     fclose(f);
 
-- 
1.7.12.4

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [Qemu-devel] [PATCH v5 09/10] tcg: check return value of fopen()
  2014-08-11  8:52 [Qemu-devel] [PATCH v5 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
                   ` (7 preceding siblings ...)
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 08/10] tests/bios-tables-test: check the value returned by fopen() zhanghailiang
@ 2014-08-11  8:53 ` zhanghailiang
  2014-08-11  9:34   ` Alex Bennée
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 10/10] block/vvfat: fix setbuf stream parameter may be NULL zhanghailiang
  9 siblings, 1 reply; 18+ messages in thread
From: zhanghailiang @ 2014-08-11  8:53 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] 18+ messages in thread

* [Qemu-devel] [PATCH v5 10/10] block/vvfat: fix setbuf stream parameter may be NULL
  2014-08-11  8:52 [Qemu-devel] [PATCH v5 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
                   ` (8 preceding siblings ...)
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 09/10] tcg: check return value of fopen() zhanghailiang
@ 2014-08-11  8:53 ` zhanghailiang
  2014-09-08  8:37   ` Stefan Hajnoczi
  9 siblings, 1 reply; 18+ messages in thread
From: zhanghailiang @ 2014-08-11  8:53 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] 18+ messages in thread

* Re: [Qemu-devel] [PATCH v5 05/10] util/path: Use the GLib memory allocation routines
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 05/10] util/path: Use the GLib memory allocation routines zhanghailiang
@ 2014-08-11  9:32   ` Alex Bennée
  0 siblings, 0 replies; 18+ messages in thread
From: Alex Bennée @ 2014-08-11  9:32 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:

> In this file, we don't check the return value of malloc/strdup/realloc which may fail.
> Instead of using these APIs, we use the GLib memory APIs g_malloc/g_strdup/g_realloc.
> They will exit on allocation failure, so there is no need to test for failure,
> which would be fine for setup.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  util/path.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/util/path.c b/util/path.c
> index 5c59d9f..e152f2a 100644
> --- a/util/path.c
> +++ b/util/path.c
> @@ -45,8 +45,8 @@ static struct pathelem *new_entry(const char *root,
>                                    struct pathelem *parent,
>                                    const char *name)
>  {
> -    struct pathelem *new = malloc(sizeof(*new));
> -    new->name = strdup(name);
> +    struct pathelem *new = g_malloc(sizeof(*new));
> +    new->name = g_strdup(name);
>      new->pathname = g_strdup_printf("%s/%s", root, name);
>      new->num_entries = 0;
>      return new;
> @@ -88,7 +88,7 @@ static struct pathelem *add_entry(struct pathelem *root, const char *name,
>  
>      root->num_entries++;
>  
> -    root = realloc(root, sizeof(*root)
> +    root = g_realloc(root, sizeof(*root)
>                     + sizeof(root->entries[0])*root->num_entries);
>      e = &root->entries[root->num_entries-1];

-- 
Alex Bennée

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH v5 06/10] slirp/misc: Use g_malloc() instead of malloc()
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 06/10] slirp/misc: Use g_malloc() instead of malloc() zhanghailiang
@ 2014-08-11  9:33   ` Alex Bennée
  0 siblings, 0 replies; 18+ messages in thread
From: Alex Bennée @ 2014-08-11  9:33 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:

> Here we don't check the return value of malloc() which may fail.
> Use the g_malloc() instead, which will abort the program when
> there is not enough memory.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  slirp/misc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/slirp/misc.c b/slirp/misc.c
> index b8eb74c..f7fe497 100644
> --- a/slirp/misc.c
> +++ b/slirp/misc.c
> @@ -54,7 +54,7 @@ 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));
> +	*ex_ptr = (struct ex_list *)g_malloc(sizeof(struct ex_list));
>  	(*ex_ptr)->ex_fport = port;
>  	(*ex_ptr)->ex_addr = addr;
>  	(*ex_ptr)->ex_pty = do_pty;
> @@ -235,7 +235,7 @@ strdup(str)
>  {
>  	char *bptr;
>  
> -	bptr = (char *)malloc(strlen(str)+1);
> +	bptr = (char *)g_malloc(strlen(str)+1);
>  	strcpy(bptr, str);
>  
>  	return bptr;

-- 
Alex Bennée

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH v5 08/10] tests/bios-tables-test: check the value returned by fopen()
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 08/10] tests/bios-tables-test: check the value returned by fopen() zhanghailiang
@ 2014-08-11  9:33   ` Alex Bennée
  0 siblings, 0 replies; 18+ messages in thread
From: Alex Bennée @ 2014-08-11  9:33 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 | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 045eb27..28ec28d 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -790,6 +790,11 @@ int main(int argc, char *argv[])
>      const char *arch = qtest_get_arch();
>      FILE *f = fopen(disk, "w");
>      int ret;
> +
> +    if (f == NULL) {
> +        fprintf(stderr, "Couldn't open \"%s\": %s", disk, strerror(errno));
> +        return -1;
> +    }

if (!f) would also be acceptable short-hand.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

>      fwrite(boot_sector, 1, sizeof boot_sector, f);
>      fclose(f);

-- 
Alex Bennée

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH v5 09/10] tcg: check return value of fopen()
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 09/10] tcg: check return value of fopen() zhanghailiang
@ 2014-08-11  9:34   ` Alex Bennée
  0 siblings, 0 replies; 18+ messages in thread
From: Alex Bennée @ 2014-08-11  9:34 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:

> 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>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  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]);
>      }

-- 
Alex Bennée

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH v5 07/10] linux-user: check return value of malloc()
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 07/10] linux-user: check return value " zhanghailiang
@ 2014-08-12  7:34   ` Riku Voipio
  0 siblings, 0 replies; 18+ messages in thread
From: Riku Voipio @ 2014-08-12  7:34 UTC (permalink / raw)
  To: zhanghailiang
  Cc: kwolf, lkurusa, mst, jan.kiszka, riku.voipio, mjt, qemu-devel,
	peter.huangpeng, stefanha, luonengjun, pbonzini, lcapitulino,
	alex.bennee, rth

On Mon, Aug 11, 2014 at 04:53:06PM +0800, zhanghailiang wrote:
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

Acked-by: Riku Voipio <riku.voipio@linaro.org>
> ---
>  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	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH v5 10/10] block/vvfat: fix setbuf stream parameter may be NULL
  2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 10/10] block/vvfat: fix setbuf stream parameter may be NULL zhanghailiang
@ 2014-09-08  8:37   ` Stefan Hajnoczi
  2014-09-08 10:57     ` Michael Tokarev
  0 siblings, 1 reply; 18+ messages in thread
From: Stefan Hajnoczi @ 2014-09-08  8:37 UTC (permalink / raw)
  To: zhanghailiang
  Cc: kwolf, lkurusa, mst, jan.kiszka, riku.voipio, mjt, qemu-devel,
	peter.huangpeng, stefanha, Li Liu, luonengjun, pbonzini,
	lcapitulino, alex.bennee, rth

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

On Mon, Aug 11, 2014 at 04:53:09PM +0800, zhanghailiang wrote:
> 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);

This no longer exists:

commit 13b552c2f43298a42e26d7aec7b58a5c205b75a0
Author: Michael Tokarev <mjt@tls.msk.ru>
Date:   Wed Aug 20 19:02:38 2014 +0400

    block/vvfat.c: remove debugging code to reinit stderr if NULL
    
    Just log to stderr unconditionally, like other similar code does.
    
    Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
    Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
    Signed-off-by: Kevin Wolf <kwolf@redhat.com>

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Qemu-devel] [PATCH v5 10/10] block/vvfat: fix setbuf stream parameter may be NULL
  2014-09-08  8:37   ` Stefan Hajnoczi
@ 2014-09-08 10:57     ` Michael Tokarev
  0 siblings, 0 replies; 18+ messages in thread
From: Michael Tokarev @ 2014-09-08 10:57 UTC (permalink / raw)
  To: Stefan Hajnoczi, zhanghailiang
  Cc: kwolf, lkurusa, mst, jan.kiszka, riku.voipio, luonengjun,
	qemu-devel, peter.huangpeng, stefanha, Li Liu, pbonzini,
	lcapitulino, alex.bennee, rth

08.09.2014 12:37, Stefan Hajnoczi wrote:
> On Mon, Aug 11, 2014 at 04:53:09PM +0800, zhanghailiang wrote:
>> From: Li Liu <john.liuli@huawei.com>
>>
>> fopen() may return NULL which will cause setbuf() segmentfault
[]
> This no longer exists:
> 
> commit 13b552c2f43298a42e26d7aec7b58a5c205b75a0
> Author: Michael Tokarev <mjt@tls.msk.ru>
> Date:   Wed Aug 20 19:02:38 2014 +0400
> 
>     block/vvfat.c: remove debugging code to reinit stderr if NULL

The second patch is a result of discussion emerged after first patch,
consider the commit to be a follow-up for the initial patch :)

Thanks,

/mjt

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2014-09-08 10:57 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-11  8:52 [Qemu-devel] [PATCH v5 00/10] fix three bugs about use-after-free and several api abuse zhanghailiang
2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 01/10] l2cap: fix access freed memory zhanghailiang
2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 02/10] monitor: " zhanghailiang
2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 03/10] virtio-blk: fix reference a pointer which might be freed zhanghailiang
2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 04/10] ivshmem: check the value returned by fstat() zhanghailiang
2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 05/10] util/path: Use the GLib memory allocation routines zhanghailiang
2014-08-11  9:32   ` Alex Bennée
2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 06/10] slirp/misc: Use g_malloc() instead of malloc() zhanghailiang
2014-08-11  9:33   ` Alex Bennée
2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 07/10] linux-user: check return value " zhanghailiang
2014-08-12  7:34   ` Riku Voipio
2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 08/10] tests/bios-tables-test: check the value returned by fopen() zhanghailiang
2014-08-11  9:33   ` Alex Bennée
2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 09/10] tcg: check return value of fopen() zhanghailiang
2014-08-11  9:34   ` Alex Bennée
2014-08-11  8:53 ` [Qemu-devel] [PATCH v5 10/10] block/vvfat: fix setbuf stream parameter may be NULL zhanghailiang
2014-09-08  8:37   ` Stefan Hajnoczi
2014-09-08 10:57     ` Michael Tokarev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).