On 28 September 2011 11:37, Alex Jia <a...@redhat.com> wrote:
> On 09/28/2011 05:43 PM, Peter Maydell wrote:
>>
>> On 28 September 2011 09:24,<a...@redhat.com> wrote:
>>>
>>> From: Alex Jia<a...@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<a...@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
