[Ocfs2-tools-devel] [PATCH] Fix memleak while calculating number of extents

[Goldwyn Rodrigues]

Hi Tao,

On Thu, Aug 19, 2010 at 9:24 AM, Tao Ma <tao.ma at oracle.com> wrote:
> Hi Goldwyn,
> Goldwyn Rodrigues wrote:
>>
>> On Wed, Aug 18, 2010 at 5:25 PM, Sunil Mushran <sunil.mushran at oracle.com>
>> wrote:
>>
>>>
>>> On 08/18/2010 10:09 AM, Goldwyn Rodrigues wrote:
>>>
>>>>
>>>> While calculating number of extents, the buffers are allocated in a loop
>>>> but only the last buffer is freed in the end. Fix the memleak by freeing
>>>> the buffer inside the loop.
>>>>
>>>> Found by Joel Becker while reviewing defrag.ocfs2
>>>>
>>>> Signed-off-by: Goldwyn Rodrigues<rgoldwyn at suse.de>
>>>> ---
>>>> diff --git a/debugfs.ocfs2/commands.c b/debugfs.ocfs2/commands.c
>>>> index 1f60049..ae12479 100644
>>>> --- a/debugfs.ocfs2/commands.c
>>>> +++ b/debugfs.ocfs2/commands.c
>>>> @@ -2092,6 +2092,7 @@ static errcode_t calc_num_extents(ocfs2_filesys
>>>> *fs,
>>>>                       if (ret)
>>>>                               goto bail;
>>>>
>>>> +                       ocfs2_free(&buf);
>>>>               }
>>>>
>>>>               *ne = *ne + extents;
>>>>
>>>>
>>>>
>>>
>>> Correct. But the ocfs2_free() after bail needs to be removed.
>>>
>>>
>>
>> No, what if ocfs2_read_extent_block fails? You would still get a leak.
>> The ocfs2_free after bail is conditional with respect to buf, so it seems
>> fine.
>>
>
> I think you need to reset 'buf = NULL' after you call ocfs2_free otherwise
> if the following ocfs2_malloc_block fails, you
> will double free the same buf.

ocfs2_free()  sets it to NULL. No point doing the same operation twice.

>
> But I think the right solution is lift the ocfs2_malloc_block and ocfs2_free
> out of the loop.

Yes that would be a better solution.

> I see you just use it iterately in the loop, so why not allocating it at the
> beginning of calc_num_extents
> and free in the end?

Just go ahead and fix it.


-- 
Goldwyn