Re: [PATCH][BZ #15859] Memory leak detected in _dl_map_object_deps()

[Vinitha Vijayan <vinitha dot vijayann at gmail dot com>]

Thank you for the comments.
I have gone through the link
https://sourceware.org/glibc/wiki/Style_and_Conventions .
I understand and will be  taking care of this from next time.

Regards,
Vinitha

On Sun, Sep 8, 2013 at 11:46 PM, OndÅej BÃlka <neleai@seznam.cz> wrote:
> On Thu, Sep 05, 2013 at 12:02:29PM +0530, Vinitha Vijayan wrote:
>> Hi,
>>  Memory leak observed in the _dl_map_object_deps().
>>  The following malloc() causes the leak.
>>
> Change looks ok, if you want to submit more patches you should learn
> conventions, see below.
>
>> ...
>>             l_reldeps = malloc (sizeof (*l_reldeps)
>>                                 + map->l_reldepsmax
>>                                   * sizeof (struct link_map *));
>> ...
>>
>> The issue can be reproduced using a minimal test scenario which is shown below
>> ...
>>    * libX.so
>>        \--------> libA.so
>>        \--------> libB.so
>>        \--------> libC.so
>>
>>    * libA.so
>>        \-------> libB.so
>>        \........................> libC.so ( relocation dependency)
>>
>>    * libB.so
>>        \-------> libC.so
>>  * main application
>>   {
>>    ...
>>    dlopen(libX.so);
>>    ...
>>    dlopen(libA.so)
>>    }
>> ...
>> This happens because of duplicate declaration of pointer l_reldeps as
>> shown below
>> ...
>>   struct link_map_reldeps *l_reldeps = NULL;       ===> HERE
>>   if (map->l_reldeps != NULL)
>>     {
>>       for (i = 1; i < nlist; ++i)
>>         map->l_searchlist.r_list[i]->
>>
>> l_reserved = 1;
>>       struct link_map **list = &map->l_reldeps->list[0];
>>       for (i = 0; i < map->l_reldeps->act; ++i)
>>         if (list[i]->l_reserved)
>>           {
>>             /* Need to allocate new array of relocation dependencies.  */
>>             struct link_map_reldeps *l_reldeps;  ===>  HERE
>>             l_reldeps = malloc (sizeof (*l_reldeps)
>>                                 + map->l_reldepsmax
>>                                   * sizeof (struct link_map *));
>> ...
>>
>> The fix is to remove the duplicate declaration inside the if loop.
>>
>> Patch:
>>
>> Index: b/elf/dl-deps.c
>> ===================================================================
>> --- a/elf/dl-deps.c
>> +++ b/elf/dl-deps.c
>> @@ -595,7 +595,6 @@ Filters not supported with LD_TRACE_PREL
>>
>>   if (list[i]->l_reserved)
>>    {
>>      /* Need to allocate new array of relocation dependencies.  */
>> -    struct link_map_reldeps *l_reldeps;
>>      l_reldeps = malloc (sizeof (*l_reldeps)
>>   + map->l_reldepsmax
>>
>>    * sizeof (struct link_map *));
>>
>> Regards,
>> Vinitha Vijayan
>> Sony India Software Centre Pvt Ltd.
>
> A formatting of patch looks off, also you need to write changelog entry.
> see http://sourceware.org/glibc/wiki/Style_and_Conventions
>
> For this change correct format is following:
>
> 2013-09-08  Vinitha Vijayan <vinitha.vijayann@gmail.com>
>
>         [BZ #15859]
>         * elf/dl-deps.c (_dl_map_object_deps): Remove duplicate declaration.
>
> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> index 1c36f50..6652f6d 100644
> --- a/elf/dl-deps.c
> +++ b/elf/dl-deps.c
> @@ -595,7 +595,6 @@ Filters not supported with LD_TRACE_PRELINKING"));
>         if (list[i]->l_reserved)
>           {
>             /* Need to allocate new array of relocation dependencies.  */
> -           struct link_map_reldeps *l_reldeps;
>             l_reldeps = malloc (sizeof (*l_reldeps)
>                                 + map->l_reldepsmax
>                                   * sizeof (struct link_map *));