[Qemu-devel] [PATCH] linux-user/syscall.c: malloc()/calloc() to g_malloc()/g_try_malloc()/g_new0()

[Qemu-devel] [PATCH] linux-user/syscall.c: malloc()/calloc() to g_malloc()/g_try_malloc()/g_new0()

[Harmandeep Kaur]

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

Convert malloc()/calloc() calls to g_malloc()/g_try_malloc()/g_new0()
in linux-user/syscall.c file

Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
---
 linux-user/syscall.c | 36 ++++++++++--------------------------
 1 file changed, 10 insertions(+), 26 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index d1d3eb2..55a0a7a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1554,12 +1554,7 @@ set_timeout:
                 }

                 fprog.len = tswap16(tfprog->len);
-                filter = malloc(fprog.len * sizeof(*filter));
-                if (filter == NULL) {
-                    unlock_user_struct(tfilter, tfprog->filter, 1);
-                    unlock_user_struct(tfprog, optval_addr, 1);
-                    return -TARGET_ENOMEM;
-                }
+                filter = g_malloc(fprog.len * sizeof(*filter));
                 for (i = 0; i < fprog.len; i++) {
                     filter[i].code = tswap16(tfilter[i].code);
                     filter[i].jt = tfilter[i].jt;
@@ -1570,7 +1565,7 @@ set_timeout:

                 ret = get_errno(setsockopt(sockfd, SOL_SOCKET,
                                 SO_ATTACH_FILTER, &fprog, sizeof(fprog)));
-                free(filter);
+                g_free(filter);

                 unlock_user_struct(tfilter, tfprog->filter, 1);
                 unlock_user_struct(tfprog, optval_addr, 1);
@@ -1881,11 +1876,7 @@ static struct iovec *lock_iovec(int type, abi_ulong
target_addr,
         return NULL;
     }

-    vec = calloc(count, sizeof(struct iovec));
-    if (vec == NULL) {
-        errno = ENOMEM;
-        return NULL;
-    }
+    vec = g_new0(struct iovec, count);

     target_vec = lock_user(VERIFY_READ, target_addr,
                            count * sizeof(struct target_iovec), 1);
@@ -1945,7 +1936,7 @@ static struct iovec *lock_iovec(int type, abi_ulong
target_addr,
     }
     unlock_user(target_vec, target_addr, 0);
  fail2:
-    free(vec);
+    g_free(vec);
     errno = err;
     return NULL;
 }
@@ -2672,14 +2663,11 @@ 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;
-    }
+    *host_array = g_malloc(nsems*sizeof(unsigned short));
     array = lock_user(VERIFY_READ, target_addr,
                       nsems*sizeof(unsigned short), 1);
     if (!array) {
-        free(*host_array);
+        g_free(*host_array);
         return -TARGET_EFAULT;
     }

@@ -2975,15 +2963,11 @@ 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 = g_malloc(msgsz+sizeof(long));
     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));
-    free(host_mb);
+    g_free(host_mb);
     unlock_user_struct(target_mb, msgp, 0);

     return ret;
@@ -7625,7 +7609,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
arg1,
             struct linux_dirent *dirp;
             abi_long count = arg3;

-        dirp = malloc(count);
+        dirp = g_try_malloc(sizeof(count));
         if (!dirp) {
                 ret = -TARGET_ENOMEM;
                 goto fail;
@@ -7662,7 +7646,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
arg1,
         ret = count1;
                 unlock_user(target_dirp, arg2, ret);
             }
-        free(dirp);
+        g_free(dirp);
         }
 #else
         {
-- 
1.9.1

[-- Attachment #2: Type: text/html, Size: 5010 bytes --]

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: malloc()/calloc() to g_malloc()/g_try_malloc()/g_new0()

[Stefan Hajnoczi]

On Wed, Sep 30, 2015 at 4:32 PM, Harmandeep Kaur
<write.harmandeep@gmail.com> wrote:
> @@ -2672,14 +2663,11 @@ 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;
> -    }
> +    *host_array = g_malloc(nsems*sizeof(unsigned short));
>      array = lock_user(VERIFY_READ, target_addr,
>                        nsems*sizeof(unsigned short), 1);
>      if (!array) {
> -        free(*host_array);
> +        g_free(*host_array);
>          return -TARGET_EFAULT;
>      }

This free covers the error case, but in the success case the pointer
is available to the caller after this function returns.

You also need to convert the free() call in host_to_target_semarray()
to g_free().

Looking at host_to_target_semarray(), it seems to me there is a
separate issue in that function.  host_array is leaked if an error
occurs during host_to_target_semarray().  This would be a good bug to
fix in a separate patch.

> @@ -7625,7 +7609,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long
> arg1,
>              struct linux_dirent *dirp;
>              abi_long count = arg3;
>
> -        dirp = malloc(count);
> +        dirp = g_try_malloc(sizeof(count));
>          if (!dirp) {

It should be g_try_malloc(count) because we're trying to allocate
count bytes, not sizeof(abi_long) bytes.

Also, please check the other malloc/calloc calls you converted to see
if byte count is a value passed in from the program being emulated.
If yes, then we don't control the count and it could be huge.  In that
case g_try_malloc() needs to be used and the NULL return value must be
handled.

Stefan

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: malloc()/calloc() to g_malloc()/g_try_malloc()/g_new0()

[Harmandeep Kaur]

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

Thank you for the review. I will definitely make
the necessary changes.

Have a great day.

On Thu, Oct 1, 2015 at 12:49 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Wed, Sep 30, 2015 at 4:32 PM, Harmandeep Kaur
> <write.harmandeep@gmail.com> wrote:
> > @@ -2672,14 +2663,11 @@ 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;
> > -    }
> > +    *host_array = g_malloc(nsems*sizeof(unsigned short));
> >      array = lock_user(VERIFY_READ, target_addr,
> >                        nsems*sizeof(unsigned short), 1);
> >      if (!array) {
> > -        free(*host_array);
> > +        g_free(*host_array);
> >          return -TARGET_EFAULT;
> >      }
>
> This free covers the error case, but in the success case the pointer
> is available to the caller after this function returns.
>
> You also need to convert the free() call in host_to_target_semarray()
> to g_free().
>
> Looking at host_to_target_semarray(), it seems to me there is a
> separate issue in that function.  host_array is leaked if an error
> occurs during host_to_target_semarray().  This would be a good bug to
> fix in a separate patch.
>
> > @@ -7625,7 +7609,7 @@ abi_long do_syscall(void *cpu_env, int num,
> abi_long
> > arg1,
> >              struct linux_dirent *dirp;
> >              abi_long count = arg3;
> >
> > -        dirp = malloc(count);
> > +        dirp = g_try_malloc(sizeof(count));
> >          if (!dirp) {
>
> It should be g_try_malloc(count) because we're trying to allocate
> count bytes, not sizeof(abi_long) bytes.
>
> Also, please check the other malloc/calloc calls you converted to see
> if byte count is a value passed in from the program being emulated.
> If yes, then we don't control the count and it could be huge.  In that
> case g_try_malloc() needs to be used and the NULL return value must be
> handled.
>
> Stefan
>

[-- Attachment #2: Type: text/html, Size: 2839 bytes --]

Re: [Qemu-devel] [PATCH] linux-user/syscall.c: malloc()/calloc() to g_malloc()/g_try_malloc()/g_new0()

[Eric Blake]

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

On 09/30/2015 08:32 AM, Harmandeep Kaur wrote:
> Convert malloc()/calloc() calls to g_malloc()/g_try_malloc()/g_new0()
> in linux-user/syscall.c file
> 
> Signed-off-by: Harmandeep Kaur <write.harmandeep@gmail.com>
> ---
>  linux-user/syscall.c | 36 ++++++++++--------------------------
>  1 file changed, 10 insertions(+), 26 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d1d3eb2..55a0a7a 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1554,12 +1554,7 @@ set_timeout:
>                  }
> 
>                  fprog.len = tswap16(tfprog->len);
> -                filter = malloc(fprog.len * sizeof(*filter));
> -                if (filter == NULL) {
> -                    unlock_user_struct(tfilter, tfprog->filter, 1);
> -                    unlock_user_struct(tfprog, optval_addr, 1);
> -                    return -TARGET_ENOMEM;
> -                }
> +                filter = g_malloc(fprog.len * sizeof(*filter));

Please consider using g_new(TYPE, fprog.len) - see commit 97f3ad35 (and
lots others) for reasons why.

> @@ -2672,14 +2663,11 @@ 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;
> -    }
> +    *host_array = g_malloc(nsems*sizeof(unsigned short));

and again here (not to mention that it would fix the lack of spaces
around binary *)

> @@ -2975,15 +2963,11 @@ 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 = g_malloc(msgsz+sizeof(long));

As long as you are touching this, spaces around binary + would be nice.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]