[Qemu-devel] [PATCH 0/4] linux-user: four patches for coverity errors

[Qemu-devel] [PATCH 0/4] linux-user: four patches for coverity errors

[Peter Maydell]

This patchset contains fixes for four bugs spotted by coverity;
mostly these are problems in error-handling codepaths.

Peter Maydell (4):
  linux-user/elfload.c: Avoid calling g_free() on uninitialized data
  linux-user/signal.c: Don't pass sigaction uninitialised sa_flags
  linux-user: Fix error handling in lock_iovec()
  linux-user: Fix error handling in target_to_host_semarray()

 linux-user/elfload.c | 16 ++++++++++++----
 linux-user/signal.c  |  1 +
 linux-user/syscall.c | 19 +++++++++++++------
 3 files changed, 26 insertions(+), 10 deletions(-)

-- 
1.8.5

[Qemu-devel] [PATCH 1/4] linux-user/elfload.c: Avoid calling g_free() on uninitialized data

[Peter Maydell]

Avoid calling g_free() on unintialized data in the error-handling
paths in elf_core_dump() by splitting the initialization of the
elf_note_info struct out of fill_note_info() so that it's always
valid to call free_note_info() whether we got to the point of
being able to fill_note_info() or not.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/elfload.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 5902f16..c0687e3 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2636,6 +2636,16 @@ static void fill_thread_info(struct elf_note_info *info, const CPUArchState *env
     info->notes_size += note_size(&ets->notes[0]);
 }
 
+static void init_note_info(struct elf_note_info *info)
+{
+    /* Initialize the elf_note_info structure so that it is at
+     * least safe to call free_note_info() on it. Must be
+     * called before calling fill_note_info().
+     */
+    memset(info, 0, sizeof (*info));
+    QTAILQ_INIT(&info->thread_list);
+}
+
 static int fill_note_info(struct elf_note_info *info,
                           long signr, const CPUArchState *env)
 {
@@ -2644,10 +2654,6 @@ static int fill_note_info(struct elf_note_info *info,
     TaskState *ts = (TaskState *)env->opaque;
     int i;
 
-    (void) memset(info, 0, sizeof (*info));
-
-    QTAILQ_INIT(&info->thread_list);
-
     info->notes = g_malloc0(NUMNOTES * sizeof (struct memelfnote));
     if (info->notes == NULL)
         return (-ENOMEM);
@@ -2781,6 +2787,8 @@ static int elf_core_dump(int signr, const CPUArchState *env)
     int segs = 0;
     int fd = -1;
 
+    init_note_info(&info);
+
     errno = 0;
     getrlimit(RLIMIT_CORE, &dumpsize);
     if (dumpsize.rlim_cur == 0)
-- 
1.8.5

[Qemu-devel] [PATCH 2/4] linux-user/signal.c: Don't pass sigaction uninitialised sa_flags

[Peter Maydell]

When forcing a fatal signal, we weren't initialising the sa_flags
field in the struct sigaction we used to reset the signal handler
to SIG_DFL.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/signal.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 82e8592..04638e2 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -420,6 +420,7 @@ static void QEMU_NORETURN force_sig(int target_sig)
      * it to arrive. */
     sigfillset(&act.sa_mask);
     act.sa_handler = SIG_DFL;
+    act.sa_flags = 0;
     sigaction(host_sig, &act, NULL);
 
     /* For some reason raise(host_sig) doesn't send the signal when
-- 
1.8.5

[Qemu-devel] [PATCH 3/4] linux-user: Fix error handling in lock_iovec()

[Peter Maydell]

In lock_iovec() if lock_user() failed we were doing an unlock_user
but not a free(vec), which is the wrong way round. We were also
assuming that free() and unlock_user() don't touch errno, which
is not guaranteed. Fix both these problems.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f370087..bb3e4b1 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1707,6 +1707,7 @@ static struct iovec *lock_iovec(int type, abi_ulong target_addr,
     struct iovec *vec;
     abi_ulong total_len, max_len;
     int i;
+    int err = 0;
 
     if (count == 0) {
         errno = 0;
@@ -1726,7 +1727,7 @@ static struct iovec *lock_iovec(int type, abi_ulong target_addr,
     target_vec = lock_user(VERIFY_READ, target_addr,
                            count * sizeof(struct target_iovec), 1);
     if (target_vec == NULL) {
-        errno = EFAULT;
+        err = EFAULT;
         goto fail2;
     }
 
@@ -1740,7 +1741,7 @@ static struct iovec *lock_iovec(int type, abi_ulong target_addr,
         abi_long len = tswapal(target_vec[i].iov_len);
 
         if (len < 0) {
-            errno = EINVAL;
+            err = EINVAL;
             goto fail;
         } else if (len == 0) {
             /* Zero length pointer is ignored.  */
@@ -1748,7 +1749,7 @@ static struct iovec *lock_iovec(int type, abi_ulong target_addr,
         } else {
             vec[i].iov_base = lock_user(type, base, len, copy);
             if (!vec[i].iov_base) {
-                errno = EFAULT;
+                err = EFAULT;
                 goto fail;
             }
             if (len > max_len - total_len) {
@@ -1763,9 +1764,10 @@ static struct iovec *lock_iovec(int type, abi_ulong target_addr,
     return vec;
 
  fail:
-    free(vec);
- fail2:
     unlock_user(target_vec, target_addr, 0);
+ fail2:
+    free(vec);
+    errno = err;
     return NULL;
 }
 
-- 
1.8.5

[Qemu-devel] [PATCH 4/4] linux-user: Fix error handling in target_to_host_semarray()

[Peter Maydell]

Fix two issues in error handling in target_to_host_semarray():
 * don't leak the host_array buffer if lock_user fails
 * return an error if malloc() fails

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index bb3e4b1..c92f026 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2429,10 +2429,15 @@ static inline abi_long target_to_host_semarray(int semid, unsigned short **host_
     nsems = semid_ds.sem_nsems;
 
     *host_array = malloc(nsems*sizeof(unsigned short));
+    if (!*host_array) {
+        return -TARGET_ENOMEM;
+    }
     array = lock_user(VERIFY_READ, target_addr,
                       nsems*sizeof(unsigned short), 1);
-    if (!array)
+    if (!array) {
+        free(host_array);
         return -TARGET_EFAULT;
+    }
 
     for(i=0; i<nsems; i++) {
         __get_user((*host_array)[i], &array[i]);
-- 
1.8.5

Re: [Qemu-devel] [PATCH 4/4] linux-user: Fix error handling in target_to_host_semarray()

[Riku Voipio]

On Mon, Feb 17, 2014 at 06:55:34PM +0000, Peter Maydell wrote:
> Fix two issues in error handling in target_to_host_semarray():
>  * don't leak the host_array buffer if lock_user fails
>  * return an error if malloc() fails

With this patch I get on ubuntu raring x86_64 (gcc 4.7):

In function ‘target_to_host_semarray’,
    inlined from ‘do_semctl’ at /home/voipio/linaro/qemu/linux-user/syscall.c:2502:17,
    inlined from ‘do_syscall’ at /home/voipio/linaro/qemu/linux-user/syscall.c:6917:13:
/home/voipio/linaro/qemu/linux-user/syscall.c:2439:13: error: attempt to free a non-heap object ‘array’ [-Werror=free-nonheap-object]
cc1: all warnings being treated as errors

Other patches in the set seem fine so far (still testing)

> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/syscall.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index bb3e4b1..c92f026 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2429,10 +2429,15 @@ static inline abi_long target_to_host_semarray(int semid, unsigned short **host_
>      nsems = semid_ds.sem_nsems;
>  
>      *host_array = malloc(nsems*sizeof(unsigned short));
> +    if (!*host_array) {
> +        return -TARGET_ENOMEM;
> +    }
>      array = lock_user(VERIFY_READ, target_addr,
>                        nsems*sizeof(unsigned short), 1);
> -    if (!array)
> +    if (!array) {
> +        free(host_array);
>          return -TARGET_EFAULT;
> +    }
>  
>      for(i=0; i<nsems; i++) {
>          __get_user((*host_array)[i], &array[i]);
> -- 
> 1.8.5
> 

Re: [Qemu-devel] [PATCH 4/4] linux-user: Fix error handling in target_to_host_semarray()

[Peter Maydell]

On 18 February 2014 15:10, Riku Voipio <riku.voipio@iki.fi> wrote:
> On Mon, Feb 17, 2014 at 06:55:34PM +0000, Peter Maydell wrote:
>> Fix two issues in error handling in target_to_host_semarray():
>>  * don't leak the host_array buffer if lock_user fails
>>  * return an error if malloc() fails
>
> With this patch I get on ubuntu raring x86_64 (gcc 4.7):
>
> In function ‘target_to_host_semarray’,
>     inlined from ‘do_semctl’ at /home/voipio/linaro/qemu/linux-user/syscall.c:2502:17,
>     inlined from ‘do_syscall’ at /home/voipio/linaro/qemu/linux-user/syscall.c:6917:13:
> /home/voipio/linaro/qemu/linux-user/syscall.c:2439:13: error: attempt to free a non-heap object ‘array’ [-Werror=free-nonheap-object]
> cc1: all warnings being treated as errors

Doh. Missing '*':

>>
>>      *host_array = malloc(nsems*sizeof(unsigned short));
>> +    if (!*host_array) {
>> +        return -TARGET_ENOMEM;
>> +    }
>>      array = lock_user(VERIFY_READ, target_addr,
>>                        nsems*sizeof(unsigned short), 1);
>> -    if (!array)
>> +    if (!array) {
>> +        free(host_array);

...should be
    free(*host_array);

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/4] linux-user: Fix error handling in target_to_host_semarray()

[Riku Voipio]

On Tue, Feb 18, 2014 at 03:11:37PM +0000, Peter Maydell wrote:
> On 18 February 2014 15:10, Riku Voipio <riku.voipio@iki.fi> wrote:
> > On Mon, Feb 17, 2014 at 06:55:34PM +0000, Peter Maydell wrote:
> >> Fix two issues in error handling in target_to_host_semarray():
> >>  * don't leak the host_array buffer if lock_user fails
> >>  * return an error if malloc() fails
> >
> > With this patch I get on ubuntu raring x86_64 (gcc 4.7):
> >
> > In function ‘target_to_host_semarray’,
> >     inlined from ‘do_semctl’ at /home/voipio/linaro/qemu/linux-user/syscall.c:2502:17,
> >     inlined from ‘do_syscall’ at /home/voipio/linaro/qemu/linux-user/syscall.c:6917:13:
> > /home/voipio/linaro/qemu/linux-user/syscall.c:2439:13: error: attempt to free a non-heap object ‘array’ [-Werror=free-nonheap-object]
> > cc1: all warnings being treated as errors
> 
> Doh. Missing '*':
> 
> >>
> >>      *host_array = malloc(nsems*sizeof(unsigned short));
> >> +    if (!*host_array) {
> >> +        return -TARGET_ENOMEM;
> >> +    }
> >>      array = lock_user(VERIFY_READ, target_addr,
> >>                        nsems*sizeof(unsigned short), 1);
> >> -    if (!array)
> >> +    if (!array) {
> >> +        free(host_array);
> 
> ...should be
>     free(*host_array);

Edited and updated patch in my linux-user updates branch.

Riku