[Qemu-devel] [PATCH v6 00/10] fix three bugs about use-after-free and several api abuse

[Qemu-devel] [PATCH v6 00/10] fix three bugs about use-after-free and several api abuse

[zhanghailiang]

Hi,

Ping...

Nine patches of this patch series has been reviewd, and the last patch is not reviewed.

Are these patches accepted? 

Though this series contains ten patches, but most of them only touch a few lines of code,
and it involves several modules. 

So, should these patched be applied to qemu trivial branch?

Any help will be greatly appreciated.

Thanks,
zhanghailiang

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. 

v5 -> v6:
 * Add reviewd-by info

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

[Qemu-devel] [PATCH v6 01/10] l2cap: fix access freed memory

[zhanghailiang]

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

[Qemu-devel] [PATCH v6 02/10] monitor: fix access freed memory

[zhanghailiang]

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 cdbaa60..42ba1b6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2533,8 +2533,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) {
@@ -2543,7 +2545,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

[Qemu-devel] [PATCH v6 03/10] virtio-blk: fix reference a pointer which might be freed

[zhanghailiang]

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

[Qemu-devel] [PATCH v6 04/10] ivshmem: check the value returned by fstat()

[zhanghailiang]

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

[Qemu-devel] [PATCH v6 05/10] util/path: Use the GLib memory allocation routines

[zhanghailiang]

In this file, we don't check the return value of malloc/strdup/realloc which may fail.
Instead of using these routines, 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];
 
-- 
1.7.12.4

[Qemu-devel] [PATCH v6 06/10] slirp/misc: Use g_malloc() instead of malloc()

[zhanghailiang]

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

[Qemu-devel] [PATCH v6 07/10] linux-user: check return value of malloc()

[zhanghailiang]

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

[Qemu-devel] [PATCH v6 08/10] tests/bios-tables-test: check the value returned by fopen()

[zhanghailiang]

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>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
---
 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

[Qemu-devel] [PATCH v6 09/10] tcg: check return value of fopen()

[zhanghailiang]

From: Li Liu <john.liuli@huawei.com>

Give a warning message if fopen() failed to open the log file.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
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

[Qemu-devel] [PATCH v6 10/10] block/vvfat: fix setbuf stream parameter may be NULL

[zhanghailiang]

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

Re: [Qemu-devel] [PATCH v6 04/10] ivshmem: check the value returned by fstat()

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 03:29:15PM +0800, zhanghailiang wrote:
> 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>

Reviewed-by: Michael S. Tsirkin <mst@redhat.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
> 

Re: [Qemu-devel] [PATCH v6 05/10] util/path: Use the GLib memory allocation routines

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 03:29:16PM +0800, zhanghailiang wrote:
> In this file, we don't check the return value of malloc/strdup/realloc which may fail.
> Instead of using these routines, 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;

Would not we have to free name using g_free as well?

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

Re: [Qemu-devel] [PATCH v6 00/10] fix three bugs about use-after-free and several api abuse

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 03:29:11PM +0800, zhanghailiang wrote:
> Hi,
> 
> Ping...
> 
> Nine patches of this patch series has been reviewd, and the last patch is not reviewed.
> 
> Are these patches accepted? 
> Though this series contains ten patches, but most of them only touch a few lines of code,
> and it involves several modules. 
> 
> So, should these patched be applied to qemu trivial branch?
> Any help will be greatly appreciated.

Why post them as a series? Make each one a separate patch,
will be easier to merge, each in the correct tree.

> 
> Thanks,
> zhanghailiang
> 
> 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. 
> 
> v5 -> v6:
>  * Add reviewd-by info
> 
> 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
> 

Re: [Qemu-devel] [PATCH v6 01/10] l2cap: fix access freed memory

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 03:29:12PM +0800, zhanghailiang wrote:
> 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: Michael S. Tsirkin <mst@redhat.com>


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

Re: [Qemu-devel] [PATCH v6 02/10] monitor: fix access freed memory

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 03:29:13PM +0800, zhanghailiang wrote:
> 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>

Id doesn't make sense after fdset is gone.
A cleaner way would be return -1 within that remove
clause.

--->

monitor: fix use after free

The function monitor_fdset_dup_fd_find_remove() references member of
'mon_fdset' which - when remove flag is set - may be freed in function
monitor_fdset_cleanup().
remove is set by monitor_fdset_dup_fd_remove which in practice
does not need the returned value, so make it void,
and return -1 from monitor_fdset_dup_fd_find_remove.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 3d6929d..78a5fc8 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -64,7 +64,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
                                 Error **errp);
 int monitor_fdset_get_fd(int64_t fdset_id, int flags);
 int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
-int monitor_fdset_dup_fd_remove(int dup_fd);
+void monitor_fdset_dup_fd_remove(int dup_fd);
 int monitor_fdset_dup_fd_find(int dup_fd);
 
 #endif /* !MONITOR_H */
diff --git a/monitor.c b/monitor.c
index 5bc70a6..ef28328 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2541,8 +2541,10 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
                     if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
                         monitor_fdset_cleanup(mon_fdset);
                     }
+                    return -1;
+                } else {
+                    return mon_fdset->id;
                 }
-                return mon_fdset->id;
             }
         }
     }
@@ -2554,9 +2556,9 @@ int monitor_fdset_dup_fd_find(int dup_fd)
     return monitor_fdset_dup_fd_find_remove(dup_fd, false);
 }
 
-int monitor_fdset_dup_fd_remove(int dup_fd)
+void monitor_fdset_dup_fd_remove(int dup_fd)
 {
-    return monitor_fdset_dup_fd_find_remove(dup_fd, true);
+    monitor_fdset_dup_fd_find_remove(dup_fd, true);
 }
 
 int monitor_handle_fd_param(Monitor *mon, const char *fdname)
diff --git a/stubs/fdset-remove-fd.c b/stubs/fdset-remove-fd.c
index b3886d9..7f6d61e 100644
--- a/stubs/fdset-remove-fd.c
+++ b/stubs/fdset-remove-fd.c
@@ -1,7 +1,6 @@
 #include "qemu-common.h"
 #include "monitor/monitor.h"
 
-int monitor_fdset_dup_fd_remove(int dupfd)
+void monitor_fdset_dup_fd_remove(int dupfd)
 {
-    return -1;
 }

Re: [Qemu-devel] [PATCH v6 06/10] slirp/misc: Use g_malloc() instead of malloc()

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 03:29:17PM +0800, zhanghailiang wrote:
> 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>

OK, but then we need to find and replace free calls
as well.

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

Re: [Qemu-devel] [PATCH v6 08/10] tests/bios-tables-test: check the value returned by fopen()

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 03:29:19PM +0800, zhanghailiang wrote:
> 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>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  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) {

if (!f) please

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

Re: [Qemu-devel] [PATCH v6 09/10] tcg: check return value of fopen()

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 03:29:20PM +0800, zhanghailiang wrote:
> From: Li Liu <john.liuli@huawei.com>
> 
> Give a warning message if fopen() failed to open the log file.
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 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) {

if (!f) please.

> +        fprintf(stderr, "Failed to open /tmp/op.log\n");

Maybe add "for writing. Logging op count will be disabled.".

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

Re: [Qemu-devel] [PATCH v6 10/10] block/vvfat: fix setbuf stream parameter may be NULL

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 03:29:21PM +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);

I would say assert on failure here.
If one is trying to debug, seeing no output will just confuse
matters more.

> -- 
> 1.7.12.4
> 

Re: [Qemu-devel] [PATCH v6 03/10] virtio-blk: fix reference a pointer which might be freed

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 03:29:14PM +0800, zhanghailiang wrote:
> 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>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Stefan want to pick up this one?

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

Re: [Qemu-devel] [PATCH v6 00/10] fix three bugs about use-after-free and several api abuse

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 03:29:11PM +0800, zhanghailiang wrote:
> Hi,
> 
> Ping...
> 
> Nine patches of this patch series has been reviewd, and the last patch is not reviewed.
> 
> Are these patches accepted? 
> 
> Though this series contains ten patches, but most of them only touch a few lines of code,
> and it involves several modules. 
> 
> So, should these patched be applied to qemu trivial branch?


To me, most of these patches don't seem appropriate for the trivial tree.
Pls work to merge each one through the correct tree.

> Any help will be greatly appreciated.
> 
> Thanks,
> zhanghailiang
> 
> 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. 
> 
> v5 -> v6:
>  * Add reviewd-by info
> 
> 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
> 

Re: [Qemu-devel] [PATCH v6 03/10] virtio-blk: fix reference a pointer which might be freed

[Michael Tokarev]

14.08.2014 14:37, Michael S. Tsirkin wrote:
> On Thu, Aug 14, 2014 at 03:29:14PM +0800, zhanghailiang wrote:
>> 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>
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Just a small nitpick...

> Stefan want to pick up this one?
> 
>> ---
>>  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;

There's no need to initialize next here.
With init like this I for one instinctively start searching below
how this `NULL' is used (which it isn't).

I'd declare it inside the loop too, since it is a local-to-loop
var, but that doesn't matter much.

/mjt

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

Re: [Qemu-devel] [PATCH v6 03/10] virtio-blk: fix reference a pointer which might be freed

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 02:39:22PM +0400, Michael Tokarev wrote:
> 14.08.2014 14:37, Michael S. Tsirkin wrote:
> > On Thu, Aug 14, 2014 at 03:29:14PM +0800, zhanghailiang wrote:
> >> 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>
> > 
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Just a small nitpick...
> 
> > Stefan want to pick up this one?
> > 
> >> ---
> >>  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;
> 
> There's no need to initialize next here.
> With init like this I for one instinctively start searching below
> how this `NULL' is used (which it isn't).
> 
> I'd declare it inside the loop too, since it is a local-to-loop
> var, but that doesn't matter much.
> 
> /mjt

Good point, I agree.

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

Re: [Qemu-devel] [PATCH v6 07/10] linux-user: check return value of malloc()

[Riku Voipio]

On Thu, Aug 14, 2014 at 03:29:18PM +0800, zhanghailiang wrote:
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Acked-by: Riku Voipio <riku.voipio@linaro.org>

Applied to linux-user as Michael seemed wary of passing these via
trivial.

Riku

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

Re: [Qemu-devel] [PATCH v6 07/10] linux-user: check return value of malloc()

[Michael Tokarev]

14.08.2014 17:31, Riku Voipio wrote:
> On Thu, Aug 14, 2014 at 03:29:18PM +0800, zhanghailiang wrote:
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Acked-by: Riku Voipio <riku.voipio@linaro.org>
> 
> Applied to linux-user as Michael seemed wary of passing these via
> trivial.

I'm not.  It is just that mst said it shoudn't go to trivial ;)

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v6 01/10] l2cap: fix access freed memory

[Michael Tokarev]

Applied to -trivial, thanks!

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v6 04/10] ivshmem: check the value returned by fstat()

[Michael Tokarev]

Applied to -trivial, thanks!

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v6 09/10] tcg: check return value of fopen()

[Michael Tokarev]

14.08.2014 14:33, Michael S. Tsirkin wrote:
> On Thu, Aug 14, 2014 at 03:29:20PM +0800, zhanghailiang wrote:
>> From: Li Liu <john.liuli@huawei.com>
>>
>> Give a warning message if fopen() failed to open the log file.
>>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> 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");

Gosh.  So why are we still use fixed filenames in /tmp?????
Every such use is a potential security holw... :(  Ughm.

Can't we get rid of this somehow, by requiring a filename
parameter for example?

Thanks,

/mjt


>> +    if (f == NULL) {
> 
> if (!f) please.
> 
>> +        fprintf(stderr, "Failed to open /tmp/op.log\n");

> Maybe add "for writing. Logging op count will be disabled.".
> 
>> +        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
>>
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH v6 09/10] tcg: check return value of fopen()

[Richard Henderson]

On 08/15/2014 05:03 AM, Michael Tokarev wrote:
>>>      f = fopen("/tmp/op.log", "w");
> 
> Gosh.  So why are we still use fixed filenames in /tmp?????
> Every such use is a potential security holw... :(  Ughm.
> 
> Can't we get rid of this somehow, by requiring a filename
> parameter for example?

It's in code that isn't compiled in by default.

Better than taking a parameter, or doing something else one-off, I think it'd
be best to dump this to the normal log file.  I.e. use qemu_log instead of fprintf.


r~

Re: [Qemu-devel] [PATCH v6 02/10] monitor: fix access freed memory

[Luiz Capitulino]

On Thu, 14 Aug 2014 12:30:10 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Aug 14, 2014 at 03:29:13PM +0800, zhanghailiang wrote:
> > 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>
> 
> Id doesn't make sense after fdset is gone.
> A cleaner way would be return -1 within that remove
> clause.
> 
> --->
> 
> monitor: fix use after free
> 
> The function monitor_fdset_dup_fd_find_remove() references member of
> 'mon_fdset' which - when remove flag is set - may be freed in function
> monitor_fdset_cleanup().
> remove is set by monitor_fdset_dup_fd_remove which in practice
> does not need the returned value, so make it void,
> and return -1 from monitor_fdset_dup_fd_find_remove.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Looks correct to me, can you post as a toplevel patch please?

> 
> ---
> 
> diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> index 3d6929d..78a5fc8 100644
> --- a/include/monitor/monitor.h
> +++ b/include/monitor/monitor.h
> @@ -64,7 +64,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
>                                  Error **errp);
>  int monitor_fdset_get_fd(int64_t fdset_id, int flags);
>  int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
> -int monitor_fdset_dup_fd_remove(int dup_fd);
> +void monitor_fdset_dup_fd_remove(int dup_fd);
>  int monitor_fdset_dup_fd_find(int dup_fd);
>  
>  #endif /* !MONITOR_H */
> diff --git a/monitor.c b/monitor.c
> index 5bc70a6..ef28328 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2541,8 +2541,10 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>                      if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
>                          monitor_fdset_cleanup(mon_fdset);
>                      }
> +                    return -1;
> +                } else {
> +                    return mon_fdset->id;
>                  }
> -                return mon_fdset->id;
>              }
>          }
>      }
> @@ -2554,9 +2556,9 @@ int monitor_fdset_dup_fd_find(int dup_fd)
>      return monitor_fdset_dup_fd_find_remove(dup_fd, false);
>  }
>  
> -int monitor_fdset_dup_fd_remove(int dup_fd)
> +void monitor_fdset_dup_fd_remove(int dup_fd)
>  {
> -    return monitor_fdset_dup_fd_find_remove(dup_fd, true);
> +    monitor_fdset_dup_fd_find_remove(dup_fd, true);
>  }
>  
>  int monitor_handle_fd_param(Monitor *mon, const char *fdname)
> diff --git a/stubs/fdset-remove-fd.c b/stubs/fdset-remove-fd.c
> index b3886d9..7f6d61e 100644
> --- a/stubs/fdset-remove-fd.c
> +++ b/stubs/fdset-remove-fd.c
> @@ -1,7 +1,6 @@
>  #include "qemu-common.h"
>  #include "monitor/monitor.h"
>  
> -int monitor_fdset_dup_fd_remove(int dupfd)
> +void monitor_fdset_dup_fd_remove(int dupfd)
>  {
> -    return -1;
>  }
> 

Re: [Qemu-devel] [PATCH v6 02/10] monitor: fix access freed memory

[Michael S. Tsirkin]

On Fri, Aug 15, 2014 at 02:25:51PM -0400, Luiz Capitulino wrote:
> On Thu, 14 Aug 2014 12:30:10 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Aug 14, 2014 at 03:29:13PM +0800, zhanghailiang wrote:
> > > 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>
> > 
> > Id doesn't make sense after fdset is gone.
> > A cleaner way would be return -1 within that remove
> > clause.
> > 
> > --->
> > 
> > monitor: fix use after free
> > 
> > The function monitor_fdset_dup_fd_find_remove() references member of
> > 'mon_fdset' which - when remove flag is set - may be freed in function
> > monitor_fdset_cleanup().
> > remove is set by monitor_fdset_dup_fd_remove which in practice
> > does not need the returned value, so make it void,
> > and return -1 from monitor_fdset_dup_fd_find_remove.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Looks correct to me, can you post as a toplevel patch please?

Done.

> > 
> > ---
> > 
> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > index 3d6929d..78a5fc8 100644
> > --- a/include/monitor/monitor.h
> > +++ b/include/monitor/monitor.h
> > @@ -64,7 +64,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
> >                                  Error **errp);
> >  int monitor_fdset_get_fd(int64_t fdset_id, int flags);
> >  int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
> > -int monitor_fdset_dup_fd_remove(int dup_fd);
> > +void monitor_fdset_dup_fd_remove(int dup_fd);
> >  int monitor_fdset_dup_fd_find(int dup_fd);
> >  
> >  #endif /* !MONITOR_H */
> > diff --git a/monitor.c b/monitor.c
> > index 5bc70a6..ef28328 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -2541,8 +2541,10 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
> >                      if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
> >                          monitor_fdset_cleanup(mon_fdset);
> >                      }
> > +                    return -1;
> > +                } else {
> > +                    return mon_fdset->id;
> >                  }
> > -                return mon_fdset->id;
> >              }
> >          }
> >      }
> > @@ -2554,9 +2556,9 @@ int monitor_fdset_dup_fd_find(int dup_fd)
> >      return monitor_fdset_dup_fd_find_remove(dup_fd, false);
> >  }
> >  
> > -int monitor_fdset_dup_fd_remove(int dup_fd)
> > +void monitor_fdset_dup_fd_remove(int dup_fd)
> >  {
> > -    return monitor_fdset_dup_fd_find_remove(dup_fd, true);
> > +    monitor_fdset_dup_fd_find_remove(dup_fd, true);
> >  }
> >  
> >  int monitor_handle_fd_param(Monitor *mon, const char *fdname)
> > diff --git a/stubs/fdset-remove-fd.c b/stubs/fdset-remove-fd.c
> > index b3886d9..7f6d61e 100644
> > --- a/stubs/fdset-remove-fd.c
> > +++ b/stubs/fdset-remove-fd.c
> > @@ -1,7 +1,6 @@
> >  #include "qemu-common.h"
> >  #include "monitor/monitor.h"
> >  
> > -int monitor_fdset_dup_fd_remove(int dupfd)
> > +void monitor_fdset_dup_fd_remove(int dupfd)
> >  {
> > -    return -1;
> >  }
> > 

Re: [Qemu-devel] [PATCH v6 02/10] monitor: fix access freed memory

[Michael S. Tsirkin]

On Fri, Aug 15, 2014 at 02:25:51PM -0400, Luiz Capitulino wrote:
> On Thu, 14 Aug 2014 12:30:10 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Thu, Aug 14, 2014 at 03:29:13PM +0800, zhanghailiang wrote:
> > > 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>
> > 
> > Id doesn't make sense after fdset is gone.
> > A cleaner way would be return -1 within that remove
> > clause.
> > 
> > --->
> > 
> > monitor: fix use after free
> > 
> > The function monitor_fdset_dup_fd_find_remove() references member of
> > 'mon_fdset' which - when remove flag is set - may be freed in function
> > monitor_fdset_cleanup().
> > remove is set by monitor_fdset_dup_fd_remove which in practice
> > does not need the returned value, so make it void,
> > and return -1 from monitor_fdset_dup_fd_find_remove.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Looks correct to me, can you post as a toplevel patch please?

Done.

> > 
> > ---
> > 
> > diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
> > index 3d6929d..78a5fc8 100644
> > --- a/include/monitor/monitor.h
> > +++ b/include/monitor/monitor.h
> > @@ -64,7 +64,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id,
> >                                  Error **errp);
> >  int monitor_fdset_get_fd(int64_t fdset_id, int flags);
> >  int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd);
> > -int monitor_fdset_dup_fd_remove(int dup_fd);
> > +void monitor_fdset_dup_fd_remove(int dup_fd);
> >  int monitor_fdset_dup_fd_find(int dup_fd);
> >  
> >  #endif /* !MONITOR_H */
> > diff --git a/monitor.c b/monitor.c
> > index 5bc70a6..ef28328 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -2541,8 +2541,10 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
> >                      if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
> >                          monitor_fdset_cleanup(mon_fdset);
> >                      }
> > +                    return -1;
> > +                } else {
> > +                    return mon_fdset->id;
> >                  }
> > -                return mon_fdset->id;
> >              }
> >          }
> >      }
> > @@ -2554,9 +2556,9 @@ int monitor_fdset_dup_fd_find(int dup_fd)
> >      return monitor_fdset_dup_fd_find_remove(dup_fd, false);
> >  }
> >  
> > -int monitor_fdset_dup_fd_remove(int dup_fd)
> > +void monitor_fdset_dup_fd_remove(int dup_fd)
> >  {
> > -    return monitor_fdset_dup_fd_find_remove(dup_fd, true);
> > +    monitor_fdset_dup_fd_find_remove(dup_fd, true);
> >  }
> >  
> >  int monitor_handle_fd_param(Monitor *mon, const char *fdname)
> > diff --git a/stubs/fdset-remove-fd.c b/stubs/fdset-remove-fd.c
> > index b3886d9..7f6d61e 100644
> > --- a/stubs/fdset-remove-fd.c
> > +++ b/stubs/fdset-remove-fd.c
> > @@ -1,7 +1,6 @@
> >  #include "qemu-common.h"
> >  #include "monitor/monitor.h"
> >  
> > -int monitor_fdset_dup_fd_remove(int dupfd)
> > +void monitor_fdset_dup_fd_remove(int dupfd)
> >  {
> > -    return -1;
> >  }
> > 

Re: [Qemu-devel] [PATCH v6 06/10] slirp/misc: Use g_malloc() instead of malloc()

[zhanghailiang]

On 2014/8/14 18:31, Michael S. Tsirkin wrote:
> On Thu, Aug 14, 2014 at 03:29:17PM +0800, zhanghailiang wrote:
>> 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>
>
> OK, but then we need to find and replace free calls
> as well.
>

OK, i will do this, thanks. :)

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

Re: [Qemu-devel] [PATCH v6 08/10] tests/bios-tables-test: check the value returned by fopen()

[zhanghailiang]

On 2014/8/14 18:32, Michael S. Tsirkin wrote:
> On Thu, Aug 14, 2014 at 03:29:19PM +0800, zhanghailiang wrote:
>> 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>
>> Reviewed-by: Alex Bennée<alex.bennee@linaro.org>
>> ---
>>   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) {
>
> if (!f) please
>

OK, i will modify it, thanks.

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

Re: [Qemu-devel] [PATCH v6 10/10] block/vvfat: fix setbuf stream parameter may be NULL

[zhanghailiang]

On 2014/8/14 18:36, Michael S. Tsirkin wrote:
> On Thu, Aug 14, 2014 at 03:29:21PM +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);
>
> I would say assert on failure here.
> If one is trying to debug, seeing no output will just confuse
> matters more.
>

OK, i will change it to use assert here, thanks.

Re: [Qemu-devel] [PATCH v6 06/10] slirp/misc: Use g_malloc() instead of malloc()

[zhanghailiang]

On 2014/8/14 18:31, Michael S. Tsirkin wrote:
> On Thu, Aug 14, 2014 at 03:29:17PM +0800, zhanghailiang wrote:
>> 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>
>
> OK, but then we need to find and replace free calls
> as well.
>

Hmm, actually, i can't find any places that free the *ex_ptr and
the ptr allocated by strdup, so i think here, it does not need to call
g_free for them, may be they should be exist in all qemu's lifecycle,

Thanks,
zhanghailiang

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

Re: [Qemu-devel] [PATCH v6 05/10] util/path: Use the GLib memory allocation routines

[zhanghailiang]

On 2014/8/14 18:15, Michael S. Tsirkin wrote:
> On Thu, Aug 14, 2014 at 03:29:16PM +0800, zhanghailiang wrote:
>> In this file, we don't check the return value of malloc/strdup/realloc which may fail.
>> Instead of using these routines, 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;
>
> Would not we have to free name using g_free as well?
>

Yes, Good catch, i will do this, Thanks.

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

Re: [Qemu-devel] [Qemu-trivial] [PATCH v6 09/10] tcg: check return value of fopen()

[zhanghailiang]

On 2014/8/16 0:53, Richard Henderson wrote:
> On 08/15/2014 05:03 AM, Michael Tokarev wrote:
>>>>       f = fopen("/tmp/op.log", "w");
>>
>> Gosh.  So why are we still use fixed filenames in /tmp?????
>> Every such use is a potential security holw... :(  Ughm.
>>
>> Can't we get rid of this somehow, by requiring a filename
>> parameter for example?
>
> It's in code that isn't compiled in by default.
>
> Better than taking a parameter, or doing something else one-off, I think it'd
> be best to dump this to the normal log file.  I.e. use qemu_log instead of fprintf.
>

Hmm, i agreed. I will get rid of this, and use qemu_log instead.
Thanks.:)

Re: [Qemu-devel] [Qemu-trivial] [PATCH v6 03/10] virtio-blk: fix reference a pointer which might be freed

[Michael Tokarev]

14.08.2014 11:29, zhanghailiang wrote:
> 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);

So, finally, I've applied this patch:

--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -469,8 +469,9 @@ static void virtio_blk_dma_restart_bh(void *opaque)
     s->rq = NULL;

     while (req) {
+        VirtIOBlockReq *next = req->next;
         virtio_blk_handle_request(req, &mrb);
-        req = req->next;
+        req = next;
     }

     virtio_submit_multiwrite(s->bs, &mrb);

and dropped Stefan's Reviewed-by on the way ;)

This is a bugfix after all ;)

Thanks,

/mjt

Re: [Qemu-devel] [Qemu-trivial] [PATCH v6 03/10] virtio-blk: fix reference a pointer which might be freed

[Michael S. Tsirkin]

On Mon, Aug 18, 2014 at 03:49:22PM +0400, Michael Tokarev wrote:
> 14.08.2014 11:29, zhanghailiang wrote:
> > 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);
> 
> So, finally, I've applied this patch:
> 
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -469,8 +469,9 @@ static void virtio_blk_dma_restart_bh(void *opaque)
>      s->rq = NULL;
> 
>      while (req) {
> +        VirtIOBlockReq *next = req->next;
>          virtio_blk_handle_request(req, &mrb);
> -        req = req->next;
> +        req = next;
>      }
> 
>      virtio_submit_multiwrite(s->bs, &mrb);
> 
> and dropped Stefan's Reviewed-by on the way ;)
> 
> This is a bugfix after all ;)
> 
> Thanks,
> 
> /mjt


By the way, could you please add Cc qemu-stable on bugfixes
you have queued?
These are likely appopriate for 2.1.1.

-- 
MST

Re: [Qemu-devel] [PATCH v6 07/10] linux-user: check return value of malloc()

[Michael S. Tsirkin]

On Thu, Aug 14, 2014 at 04:31:35PM +0300, Riku Voipio wrote:
> On Thu, Aug 14, 2014 at 03:29:18PM +0800, zhanghailiang wrote:
> > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> > Acked-by: Riku Voipio <riku.voipio@linaro.org>
> 
> Applied to linux-user as Michael seemed wary of passing these via
> trivial.
> 
> Riku

Pls remember to add Cc qemu-trivial on bugfixes in your tree.


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

Re: [Qemu-devel] [Qemu-trivial] [PATCH v6 03/10] virtio-blk: fix reference a pointer which might be freed

[Michael Tokarev]

19.08.2014 00:17, Michael S. Tsirkin wrote:
[]
> By the way, could you please add Cc qemu-stable on bugfixes
> you have queued?
> These are likely appopriate for 2.1.1.

Actually I've added Cc: qemu-stable@ in the commit message.
So it will go to stable (or should) once I'll send a pull
request.

Thanks,

/mjt