Re: [PATCH] Free errstring if _dl_addr returns 0

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Sat, Sep 8, 2012 at 7:24 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Sep 7, 2012 at 4:13 PM, Roland McGrath <roland@hack.frob.com> wrote:
>>> --    * dlfcn/dlerror.c (check_free): Free errstring if _dl_addr
>>>       returns 0.  Don't check if map is NULL.
>>
>> MAP in caps.
>>
>>> -      always the C library in the base namespave.  */
>>> +      always the C library in the base namespace.  _dl_addr retuns 0
>>
>> Typo: "returns"
>>
>>> +      when check_free isn't called from any loaded object.  This
>>> +      indicates static executable and we can free the string.  */
>>
>> "This indicates we're in a static executable, in which case it's always
>> safe to free the string."
>>
>> However, it occurs to me that we probably want a sanity check here.  That
>> is, if _dl_addr returns 0 for some unexpected reason, we should not call
>> free.  It should never be possible #ifdef SHARED, right?
>>
>> Hmm.  Why do this test dynamically rather than simply use #ifndef SHARED?
>> i.e.:
>>       /* We can free the string only if the allocation happened in the
>>          C library used by the dynamic linker.  This means, it is
>>          always the C library in the base namespace.  When we're statically
>>          linked, the dynamic linker is part of the program and so always
>>          uses the same C library we use here.  */
>> #ifndef SHARED
> ^^^^^^^^^^^ A typo.
>
>>       struct link_map *map = NULL;
>>       Dl_info info;
>>       if (_dl_addr (check_free, &info, &map, NULL) != 0 && map->l_ns == 0)
>> #endif
>>         free ((char *) rec->errstring);
>>
>> Is there any case where that would be wrong?
>>
>>
>
> This works for me.  Is this OK?
>
> Thanks.
>
> --
> H.J.
> ----
> 012-09-05  Roland McGrath  <roland@hack.frob.com>
>
>         * dlfcn/dlerror.c (check_free): Call _dl_addr only if SHARED is
>         defined.  Don't check if MAP is NULL.
>
> diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c
> index 8138cc2..7597703 100644
> --- a/dlfcn/dlerror.c
> +++ b/dlfcn/dlerror.c
> @@ -190,11 +190,14 @@ check_free (struct dl_action_result *rec)
>      {
>        /* We can free the string only if the allocation happened in the
>          C library used by the dynamic linker.  This means, it is
> -        always the C library in the base namespave.  */
> +        always the C library in the base namespace.  When we're statically
> +         linked, the dynamic linker is part of the program and so always
> +        uses the same C library we use here.  */
> +#ifdef SHARED
>        struct link_map *map = NULL;
>        Dl_info info;
> -      if (_dl_addr (check_free, &info, &map, NULL) != 0
> -         && map != NULL && map->l_ns == 0)
> +      if (_dl_addr (check_free, &info, &map, NULL) != 0 && map->l_ns == 0)
> +#endif
>         free ((char *) rec->errstring);
>      }
>  }


PING.

-- 
H.J.