[Ocfs2-tools-devel] [PATCH 1/1] ocfs2-tools: fix wrong pointer to pointer in mount.ocfs2

[Sunil Mushran]

On 03/31/2011 06:42 PM, Tiger Yang wrote:
> On 04/01/2011 12:59 AM, Sunil Mushran wrote:
>> On 03/30/2011 07:12 PM, Tiger Yang wrote:
>>> On 03/31/2011 08:16 AM, Sunil Mushran wrote:
>>>> On 03/29/2011 08:26 PM, Tiger Yang wrote:
>>>>> commit 52bae5e7a358e927a1e841ead2c6a95cf68c5db1 use the wrong
>>>>> pointer to pointer in if statement. This patch fixes this problem
>>>>> and clean the codes to create option string.
>>>>>
>>>>> Signed-off-by: Tiger Yang<tiger.yang at oracle.com>
>>>>> ---
>>>>>   mount.ocfs2/mount.ocfs2.c |    8 +++-----
>>>>>   1 files changed, 3 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/mount.ocfs2/mount.ocfs2.c b/mount.ocfs2/mount.ocfs2.c
>>>>> index a5e117d..4925f20 100644
>>>>> --- a/mount.ocfs2/mount.ocfs2.c
>>>>> +++ b/mount.ocfs2/mount.ocfs2.c
>>>>> @@ -133,11 +133,9 @@ static errcode_t add_mount_options(ocfs2_filesys *fs,
>>>>>       add = OCFS2_HB_LOCAL;
>>>>>
>>>>>   addit:
>>>>> -    if (optstr&&  *optstr) {
>>>>> -        extra = xstrndup(*optstr, strlen(*optstr) + strlen(add) + 1);
>>>>> -        if (extra)
>>>>> -            extra = xstrconcat3(extra, ",", add);
>>>>> -    } else
>>>>> +    if (*optstr&&  *(*optstr))
>>>>> +        extra = xstrconcat3(*optstr, ",", add);
>>>>
>>>> This function calls free(*optstr). Probably not what we want as
>>>> it could lead to a double free.
>>>  I know it will free the old optstr, and that is I want. Because the next line is
>>>  *optstr = extra;  then in main(), will free the extra.
>>>         if (mo.xtra_opts)
>>>                 free(mo.xtra_opts);
>>> But the old optstr will never get free, will cause memory leak.
>>
>> Fair enough.
>>
>>>>
>>>> This code has been there forever. Why is this problem happening now?
>>> I read the codes carefully, I can not understand why we copy string length is (strlen(*optstr) + strlen(add) + 1) from optstr,
>>> and then free that extra in xstrconcat3(extra, ",", add).
>>> Actually, these three lines do not raise the bug 11929515, it only cause memory leak.
>>> The wrong use of pointer to pointer in if statement will cause the condition true and then add a comma in option string.
>>
>> xstrconcat3() also allocates a new buffer.
>>
>> This problem is new. And this code is fairly old. What changed?
> the original code in if statement is mo.xtra_opts, but commit 52bae5e7a358e927a1e841ead2c6a95cf68c5db1 use &mo.xtra_opts in
> ret = add_mount_options(fs, &cluster, &mo.xtra_opts);
>
> Thanks,
> Tiger

ok. makes sense. add my ack and check in the fix.