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

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

[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().

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. 

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             | 9 +++++++--
 tcg/tcg.c                | 4 ++++
 tests/bios-tables-test.c | 2 ++
 util/path.c              | 9 ++++++---
 10 files changed, 39 insertions(+), 11 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v4 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 v4 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 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

[Qemu-devel] [PATCH v4 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 v4 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 v4 05/10] util/path: check return value of malloc()

[zhanghailiang]

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

diff --git a/util/path.c b/util/path.c
index 5c59d9f..df1653f 100644
--- a/util/path.c
+++ b/util/path.c
@@ -46,9 +46,12 @@ static struct pathelem *new_entry(const char *root,
                                   const char *name)
 {
     struct pathelem *new = malloc(sizeof(*new));
-    new->name = strdup(name);
-    new->pathname = g_strdup_printf("%s/%s", root, name);
-    new->num_entries = 0;
+
+    if (new) {
+        new->name = strdup(name);
+        new->pathname = g_strdup_printf("%s/%s", root, name);
+        new->num_entries = 0;
+    }
     return new;
 }
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v4 05/10] util/path: check return value of malloc()

[Alex Bennée]

zhanghailiang writes:

> Reviewed-by: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  util/path.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/util/path.c b/util/path.c
> index 5c59d9f..df1653f 100644
> --- a/util/path.c
> +++ b/util/path.c
> @@ -46,9 +46,12 @@ static struct pathelem *new_entry(const char *root,
>                                    const char *name)
>  {
>      struct pathelem *new = malloc(sizeof(*new));
> -    new->name = strdup(name);
> -    new->pathname = g_strdup_printf("%s/%s", root, name);
> -    new->num_entries = 0;

Erm... isn't that malloc wrong as sizeof(*new) would be the size of a
pointer?

> +
> +    if (new) {
> +        new->name = strdup(name);
> +        new->pathname = g_strdup_printf("%s/%s", root, name);
> +        new->num_entries = 0;
> +    }
>      return new;
>  }

A better approach may be to just g_malloc which would abort on failure
(which would be fine for setup).

static struct pathelem *new_entry(const char *root,
                                  struct pathelem *parent,
                                  const char *name)
{
    struct pathelem *new = g_malloc(sizeof(pathelem));
    new->name = g_strdup(name);
    new->pathname = g_strdup_printf("%s/%s", root, name);
    new->num_entries = 0;
    return new;
}



-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v4 05/10] util/path: check return value of malloc()

[zhanghailiang]

On 2014/8/8 17:36, Alex Bennée wrote:
>
> zhanghailiang writes:
>
>> Reviewed-by: Gonglei<arei.gonglei@huawei.com>
>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>> ---
>>   util/path.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/util/path.c b/util/path.c
>> index 5c59d9f..df1653f 100644
>> --- a/util/path.c
>> +++ b/util/path.c
>> @@ -46,9 +46,12 @@ static struct pathelem *new_entry(const char *root,
>>                                     const char *name)
>>   {
>>       struct pathelem *new = malloc(sizeof(*new));
>> -    new->name = strdup(name);
>> -    new->pathname = g_strdup_printf("%s/%s", root, name);
>> -    new->num_entries = 0;
>
> Erm... isn't that malloc wrong as sizeof(*new) would be the size of a
> pointer?
>

No, this is OK! It is equal to sizeof(struct pathelem).

>> +
>> +    if (new) {
>> +        new->name = strdup(name);
>> +        new->pathname = g_strdup_printf("%s/%s", root, name);
>> +        new->num_entries = 0;
>> +    }
>>       return new;
>>   }
>
> A better approach may be to just g_malloc which would abort on failure
> (which would be fine for setup).
>

Hmm, Good idea! It is more quickly to know what happen when it fails.
I will change to it, Thanks, Alex.

> static struct pathelem *new_entry(const char *root,
>                                    struct pathelem *parent,
>                                    const char *name)
> {
>      struct pathelem *new = g_malloc(sizeof(pathelem));
>      new->name = g_strdup(name);
>      new->pathname = g_strdup_printf("%s/%s", root, name);
>      new->num_entries = 0;
>      return new;
> }
>
>
>

[Qemu-devel] [PATCH v4 06/10] slirp/misc: check return value of malloc()

[zhanghailiang]

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

diff --git a/slirp/misc.c b/slirp/misc.c
index b8eb74c..9b457ad 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -55,6 +55,10 @@ int add_exec(struct ex_list **ex_ptr, int do_pty, char *exec,
 
 	tmp_ptr = *ex_ptr;
 	*ex_ptr = (struct ex_list *)malloc(sizeof(struct ex_list));
+    if (*ex_ptr == NULL) {
+        fprintf(stderr, "Error: malloc failed\n");
+        return -1;
+    }
 	(*ex_ptr)->ex_fport = port;
 	(*ex_ptr)->ex_addr = addr;
 	(*ex_ptr)->ex_pty = do_pty;
@@ -236,8 +240,9 @@ strdup(str)
 	char *bptr;
 
 	bptr = (char *)malloc(strlen(str)+1);
-	strcpy(bptr, str);
-
+    if (bptr) {
+        strcpy(bptr, str);
+    }
 	return bptr;
 }
 #endif
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v4 06/10] slirp/misc: check return value of malloc()

[Alex Bennée]

zhanghailiang writes:

> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  slirp/misc.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/slirp/misc.c b/slirp/misc.c
> index b8eb74c..9b457ad 100644
> --- a/slirp/misc.c
> +++ b/slirp/misc.c
> @@ -55,6 +55,10 @@ int add_exec(struct ex_list **ex_ptr, int do_pty, char *exec,
>  
>  	tmp_ptr = *ex_ptr;
>  	*ex_ptr = (struct ex_list *)malloc(sizeof(struct ex_list));
> +    if (*ex_ptr == NULL) {
> +        fprintf(stderr, "Error: malloc failed\n");
> +        return -1;
> +    }

Your indenting has gone a bit weird there.

>  	(*ex_ptr)->ex_fport = port;
>  	(*ex_ptr)->ex_addr = addr;
>  	(*ex_ptr)->ex_pty = do_pty;
> @@ -236,8 +240,9 @@ strdup(str)
>  	char *bptr;
>  
>  	bptr = (char *)malloc(strlen(str)+1);
> -	strcpy(bptr, str);
> -
> +    if (bptr) {
> +        strcpy(bptr, str);
> +    }
>  	return bptr;
>  }
>  #endif

Again use of g_malloc would remove the need for this. HACKING section 3
says:

3. Low level memory management

Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign
APIs is not allowed in the QEMU codebase. Instead of these routines,
use the GLib memory allocation routines g_malloc/g_malloc0/g_new/
g_new0/g_realloc/g_free or QEMU's qemu_memalign/qemu_blockalign/qemu_vfree
APIs.

Please note that g_malloc will exit on allocation failure, so there
is no need to test for failure (as you would have to with malloc).
Calling g_malloc with a zero size is valid and will return NULL.


-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v4 06/10] slirp/misc: check return value of malloc()

[zhanghailiang]

On 2014/8/8 17:43, Alex Bennée wrote:
>
> zhanghailiang writes:
>
>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>> ---
>>   slirp/misc.c | 9 +++++++--
>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/slirp/misc.c b/slirp/misc.c
>> index b8eb74c..9b457ad 100644
>> --- a/slirp/misc.c
>> +++ b/slirp/misc.c
>> @@ -55,6 +55,10 @@ int add_exec(struct ex_list **ex_ptr, int do_pty, char *exec,
>>
>>   	tmp_ptr = *ex_ptr;
>>   	*ex_ptr = (struct ex_list *)malloc(sizeof(struct ex_list));
>> +    if (*ex_ptr == NULL) {
>> +        fprintf(stderr, "Error: malloc failed\n");
>> +        return -1;
>> +    }
>
> Your indenting has gone a bit weird there.

Hmm, this file has some places that use tab key as indent.
Here i used spaces as indent, otherwise the patch can not pass the check 
of '/scripts/checkpatch.pl'.

What's your opinion? Use tab as what it does? Thanks!

>
>>   	(*ex_ptr)->ex_fport = port;
>>   	(*ex_ptr)->ex_addr = addr;
>>   	(*ex_ptr)->ex_pty = do_pty;
>> @@ -236,8 +240,9 @@ strdup(str)
>>   	char *bptr;
>>
>>   	bptr = (char *)malloc(strlen(str)+1);
>> -	strcpy(bptr, str);
>> -
>> +    if (bptr) {
>> +        strcpy(bptr, str);
>> +    }
>>   	return bptr;
>>   }
>>   #endif
>
> Again use of g_malloc would remove the need for this. HACKING section 3
> says:
>

OK, Thanks!

> 3. Low level memory management
>
> Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign
> APIs is not allowed in the QEMU codebase. Instead of these routines,
> use the GLib memory allocation routines g_malloc/g_malloc0/g_new/
> g_new0/g_realloc/g_free or QEMU's qemu_memalign/qemu_blockalign/qemu_vfree
> APIs.
>
> Please note that g_malloc will exit on allocation failure, so there
> is no need to test for failure (as you would have to with malloc).
> Calling g_malloc with a zero size is valid and will return NULL.
>
>

Re: [Qemu-devel] [PATCH v4 06/10] slirp/misc: check return value of malloc()

[Alex Bennée]

zhanghailiang writes:

> On 2014/8/8 17:43, Alex Bennée wrote:
>>
>> zhanghailiang writes:
>>
>>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>>> ---
>>>   slirp/misc.c | 9 +++++++--
>>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>>
<snip>
>>
>> Your indenting has gone a bit weird there.
>
> Hmm, this file has some places that use tab key as indent.
> Here i used spaces as indent, otherwise the patch can not pass the check 
> of '/scripts/checkpatch.pl'.
>
> What's your opinion? Use tab as what it does? Thanks!

Welcome to the world of QEMU's inconsistent whitespace ;-)

You have two choices:

  * two patches: 1st to clean up whitespace for that function, 2nd to
    fix
  * keep to using tabs for that particular fix

Eventually the code base will get to a consistent state we hope...

>>>   	(*ex_ptr)->ex_fport = port;
>>>   	(*ex_ptr)->ex_addr = addr;
>>>   	(*ex_ptr)->ex_pty = do_pty;
>>> @@ -236,8 +240,9 @@ strdup(str)
>>>   	char *bptr;
>>>
>>>   	bptr = (char *)malloc(strlen(str)+1);
>>> -	strcpy(bptr, str);
>>> -
>>> +    if (bptr) {
>>> +        strcpy(bptr, str);
>>> +    }
>>>   	return bptr;
>>>   }
>>>   #endif
>>
>> Again use of g_malloc would remove the need for this. HACKING section 3
>> says:
>>
>
> OK, Thanks!
>
>> 3. Low level memory management
>>
>> Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign
>> APIs is not allowed in the QEMU codebase. Instead of these routines,
>> use the GLib memory allocation routines g_malloc/g_malloc0/g_new/
>> g_new0/g_realloc/g_free or QEMU's qemu_memalign/qemu_blockalign/qemu_vfree
>> APIs.
>>
>> Please note that g_malloc will exit on allocation failure, so there
>> is no need to test for failure (as you would have to with malloc).
>> Calling g_malloc with a zero size is valid and will return NULL.
>>
>>

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH v4 06/10] slirp/misc: check return value of malloc()

[zhanghailiang]

On 2014/8/8 21:24, Alex Bennée wrote:
>
> zhanghailiang writes:
>
>> On 2014/8/8 17:43, Alex Bennée wrote:
>>>
>>> zhanghailiang writes:
>>>
>>>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>>>> ---
>>>>    slirp/misc.c | 9 +++++++--
>>>>    1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
> <snip>
>>>
>>> Your indenting has gone a bit weird there.
>>
>> Hmm, this file has some places that use tab key as indent.
>> Here i used spaces as indent, otherwise the patch can not pass the check
>> of '/scripts/checkpatch.pl'.
>>
>> What's your opinion? Use tab as what it does? Thanks!
>
> Welcome to the world of QEMU's inconsistent whitespace ;-)
>
> You have two choices:
>
>    * two patches: 1st to clean up whitespace for that function, 2nd to
>      fix
>    * keep to using tabs for that particular fix
>
> Eventually the code base will get to a consistent state we hope...
>

OK, I will choose the second way! Thanks, Alex.

>>>>    	(*ex_ptr)->ex_fport = port;
>>>>    	(*ex_ptr)->ex_addr = addr;
>>>>    	(*ex_ptr)->ex_pty = do_pty;
>>>> @@ -236,8 +240,9 @@ strdup(str)
>>>>    	char *bptr;
>>>>
>>>>    	bptr = (char *)malloc(strlen(str)+1);
>>>> -	strcpy(bptr, str);
>>>> -
>>>> +    if (bptr) {
>>>> +        strcpy(bptr, str);
>>>> +    }
>>>>    	return bptr;
>>>>    }
>>>>    #endif
>>>
>>> Again use of g_malloc would remove the need for this. HACKING section 3
>>> says:
>>>
>>
>> OK, Thanks!
>>
>>> 3. Low level memory management
>>>
>>> Use of the malloc/free/realloc/calloc/valloc/memalign/posix_memalign
>>> APIs is not allowed in the QEMU codebase. Instead of these routines,
>>> use the GLib memory allocation routines g_malloc/g_malloc0/g_new/
>>> g_new0/g_realloc/g_free or QEMU's qemu_memalign/qemu_blockalign/qemu_vfree
>>> APIs.
>>>
>>> Please note that g_malloc will exit on allocation failure, so there
>>> is no need to test for failure (as you would have to with malloc).
>>> Calling g_malloc with a zero size is valid and will return NULL.
>>>
>>>
>

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

[zhanghailiang]

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

[Qemu-devel] [PATCH v4 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>
---
 tests/bios-tables-test.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 045eb27..6a357c0 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -790,6 +790,8 @@ int main(int argc, char *argv[])
     const char *arch = qtest_get_arch();
     FILE *f = fopen(disk, "w");
     int ret;
+
+    g_assert(f != NULL);
     fwrite(boot_sector, 1, sizeof boot_sector, f);
     fclose(f);
 
-- 
1.7.12.4

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

[Alex Bennée]

zhanghailiang writes:

> The function fopen() may fail, so check its return value.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Li Liu <john.liuli@huawei.com>
> ---
>  tests/bios-tables-test.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 045eb27..6a357c0 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -790,6 +790,8 @@ int main(int argc, char *argv[])
>      const char *arch = qtest_get_arch();
>      FILE *f = fopen(disk, "w");
>      int ret;
> +
> +    g_assert(f != NULL);
>      fwrite(boot_sector, 1, sizeof boot_sector, f);
>      fclose(f);

Incorrect use of g_assert. An assertion is a test for things that
shouldn't happen so in this case it's saying:

 * this function assumes files will always successfully open

Which is not the case. It's quite possible that a fopen will fail and
the correct behaviour is to handle the error, not assert out.



-- 
Alex Bennée

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

[zhanghailiang]

On 2014/8/8 17:51, Alex Bennée wrote:
>
> zhanghailiang writes:
>
>> The function fopen() may fail, so check its return value.
>>
>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Li Liu<john.liuli@huawei.com>
>> ---
>>   tests/bios-tables-test.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
>> index 045eb27..6a357c0 100644
>> --- a/tests/bios-tables-test.c
>> +++ b/tests/bios-tables-test.c
>> @@ -790,6 +790,8 @@ int main(int argc, char *argv[])
>>       const char *arch = qtest_get_arch();
>>       FILE *f = fopen(disk, "w");
>>       int ret;
>> +
>> +    g_assert(f != NULL);
>>       fwrite(boot_sector, 1, sizeof boot_sector, f);
>>       fclose(f);
>
> Incorrect use of g_assert. An assertion is a test for things that
> shouldn't happen so in this case it's saying:
>
>   * this function assumes files will always successfully open
>
> Which is not the case. It's quite possible that a fopen will fail and
> the correct behaviour is to handle the error, not assert out.
>
>

Good catch! That is a low grade fault, i will correct it! Thanks, Alex

Best regards,
zhanghailiang

[Qemu-devel] [PATCH v4 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.

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