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

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

[ajia]

From: Alex Jia <ajia@redhat.com>

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 <ajia@redhat.com>
---
 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;
@@ -2779,9 +2780,9 @@ static inline abi_long do_msgrcv(int msqid, abi_long msgp,
     }
 
     target_mb->mtype = tswapl(host_mb->mtype);
-    free(host_mb);
 
 end:
+    free(host_mb);
     if (target_mb)
         unlock_user_struct(target_mb, msgp, 1);
     return ret;
-- 
1.7.1

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

[Peter Maydell]

On 28 September 2011 09:24,  <ajia@redhat.com> wrote:
> From: Alex Jia <ajia@redhat.com>
>
> 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 <ajia@redhat.com>
> ---
>  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

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

[Alex Jia]

On 09/28/2011 05:43 PM, Peter Maydell wrote:
> On 28 September 2011 09:24,<ajia@redhat.com>  wrote:
>> From: Alex Jia<ajia@redhat.com>
>>
>> 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<ajia@redhat.com>
>> ---
>>   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"
Hi Peter,
You mean caller should free these allocated memory, right?
if so, is v1 patch okay?

Thanks for your nice comment.

Regards,
Alex
> 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
>

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

[Peter Maydell]

On 28 September 2011 11:37, Alex Jia <ajia@redhat.com> wrote:
> On 09/28/2011 05:43 PM, Peter Maydell wrote:
>>
>> On 28 September 2011 09:24,<ajia@redhat.com>  wrote:
>>>
>>> From: Alex Jia<ajia@redhat.com>
>>>
>>> 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<ajia@redhat.com>
>>> ---
>>>  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"
>
> Hi Peter,
> You mean caller should free these allocated memory, right?
> if so, is v1 patch okay?

No, I mean the caller should be doing both the allocation
and the freeing, which means restucturing the code a bit.

-- PMM