Re: [Qemu-devel] [PATCH v2] linux-user: fix memory leak in failure path

[Peter Maydell]

On 28 September 2011 09:24,  <address@hidden> wrote:
> From: Alex Jia <address@hidden>
>
> Haven't released memory of 'host_mb' in failure path, and calling malloc 
> allocate
> memory to 'host_array', however, memory hasn't been freed before the function
> target_to_host_semarray returns.
>
> Signed-off-by: Alex Jia <address@hidden>
> ---
>  linux-user/syscall.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 7735008..22d4fcc 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2466,6 +2466,7 @@ static inline abi_long target_to_host_semarray(int 
> semid, unsigned short **host_
>     for(i=0; i<nsems; i++) {
>         __get_user((*host_array)[i], &array[i]);
>     }
> +    free(*host_array);
>     unlock_user(array, target_addr, 0);
>
>     return 0;

This is wrong -- you're freeing the array in the exit-success
path, not the error path. And you're still not checking the
return value from malloc().

In fact, on closer examination, this code is pretty nasty:
we allocate the array in target_to_host_semarray and then
free it in host_to_target_semarray, and in both functions
we make a syscall purely to figure out the length of the
array -- so we end up doing that twice when we only need
do it once. And the error exit cases in host_to_target_semarray
don't free the host array either. It could probably be
refactored to make it less ugly: have the code at the
top level do the "find out size of array, malloc it, free"
work, and have host_to_target_semarray and
target_to_host_semarray only do the copying work (this
would match other target_to_host* helpers.)

-- PMM