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