dlfcn: dlinfo (RTLD_DI_PHDR) should work for proxy link maps (bug 32060)

Previously, 0 was returned as the program header address.

---
 dlfcn/dlinfo.c          |  3 +++
 dlfcn/tst-dlinfo-phdr.c | 72 ++++++++++++++++++++++++++++++-------------------
 2 files changed, 48 insertions(+), 27 deletions(-)


base-commit: a0ecbb45969e93ec5eb6ba0d1f0a5578bdb2e54c

On 08/08/24 13:40, Florian Weimer wrote:
> Previously, 0 was returned as the program header address.

Looks good, but I think we should define if we can really cast
the dlopen/dlmopen return (below).

> 
> ---
>  dlfcn/dlinfo.c          |  3 +++
>  dlfcn/tst-dlinfo-phdr.c | 72 ++++++++++++++++++++++++++++++-------------------
>  2 files changed, 48 insertions(+), 27 deletions(-)
> 
> diff --git a/dlfcn/dlinfo.c b/dlfcn/dlinfo.c
> index b0feb9362d..1979ba3356 100644
> --- a/dlfcn/dlinfo.c
> +++ b/dlfcn/dlinfo.c
> @@ -40,6 +40,9 @@ dlinfo_doit (void *argsblock)
>    struct dlinfo_args *const args = argsblock;
>    struct link_map *l = args->handle;
>  
> +  /* Support proxy maps (although they are never returned by dlopen).  */
> +  l = l->l_real;
> +
>    switch (args->request)
>      {
>      case RTLD_DI_CONFIGADDR:

Ok.

> diff --git a/dlfcn/tst-dlinfo-phdr.c b/dlfcn/tst-dlinfo-phdr.c
> index fdffb17724..43deb4c6d5 100644
> --- a/dlfcn/tst-dlinfo-phdr.c
> +++ b/dlfcn/tst-dlinfo-phdr.c
> @@ -17,6 +17,7 @@
>     <https://www.gnu.org/licenses/>.  */
>  
>  #include <dlfcn.h>
> +#include <gnu/lib-names.h>
>  #include <link.h>
>  #include <stdbool.h>
>  #include <stdio.h>
> @@ -54,6 +55,42 @@ dlip_callback (struct dl_phdr_info *dlpi, size_t size, void *closure)
>    return 0;
>  }
>  
> +/* Set by basic_checks.   */
> +static const ElfW(Phdr) *phdr;
> +static int phnum;
> +
> +/* Basic checks that can be used regardless of namespace.  */
> +static void
> +basic_checks (struct link_map *l)
> +{
> +  printf ("info: checking link map %p (%p) for \"%s\"\n",
> +          l, l->l_phdr, l->l_name);
> +
> +  /* Cause dlerror () to return an error message.  */
> +  dlsym (RTLD_DEFAULT, "does-not-exist");
> +
> +  /* Use the extension that link maps are valid dlopen handles.  */
> +  phnum = dlinfo (l, RTLD_DI_PHDR, &phdr);
> +  TEST_VERIFY (phnum >= 0);
> +  /* Verify that the error message has been cleared.  */
> +  TEST_COMPARE_STRING (dlerror (), NULL);
> +
> +  TEST_VERIFY (phdr == l->l_real->l_phdr);
> +  TEST_COMPARE (phnum, l->l_real->l_phnum);
> +
> +  /* Check that we can find PT_DYNAMIC among the array.  */
> +  {
> +    bool dynamic_found = false;
> +    for (int i = 0; i < phnum; ++i)
> +      if (phdr[i].p_type == PT_DYNAMIC)
> +        {
> +          dynamic_found = true;
> +          TEST_COMPARE ((ElfW(Addr)) l->l_ld, l->l_addr + phdr[i].p_vaddr);
> +        }
> +    TEST_VERIFY (dynamic_found);
> +  }
> +}
> +
>  static int
>  do_test (void)
>  {
> @@ -64,33 +101,7 @@ do_test (void)
>  
>    do
>      {
> -      printf ("info: checking link map %p (%p) for \"%s\"\n",
> -              l, l->l_phdr, l->l_name);
> -
> -      /* Cause dlerror () to return an error message.  */
> -      dlsym (RTLD_DEFAULT, "does-not-exist");
> -
> -      /* Use the extension that link maps are valid dlopen handles.  */
> -      const ElfW(Phdr) *phdr;
> -      int phnum = dlinfo (l, RTLD_DI_PHDR, &phdr);
> -      TEST_VERIFY (phnum >= 0);
> -      /* Verify that the error message has been cleared.  */
> -      TEST_COMPARE_STRING (dlerror (), NULL);
> -
> -      TEST_VERIFY (phdr == l->l_phdr);
> -      TEST_COMPARE (phnum, l->l_phnum);
> -
> -      /* Check that we can find PT_DYNAMIC among the array.  */
> -      {
> -        bool dynamic_found = false;
> -        for (int i = 0; i < phnum; ++i)
> -          if (phdr[i].p_type == PT_DYNAMIC)
> -            {
> -              dynamic_found = true;
> -              TEST_COMPARE ((ElfW(Addr)) l->l_ld, l->l_addr + phdr[i].p_vaddr);
> -            }
> -        TEST_VERIFY (dynamic_found);
> -      }
> +      basic_checks (l);
>  
>        /* Check that dl_iterate_phdr finds the link map with the same
>           program headers.  */
> @@ -119,6 +130,13 @@ do_test (void)
>      }
>    while (l != NULL);
>  
> +  /* The secondary namespace does not contain the main executable, and
> +     dl_iterate_phdr does not cover it.  */
> +  struct link_map *secondary_libc = xdlmopen (LM_ID_NEWLM, LIBC_SO, RTLD_NOW);
> +  for (l = secondary_libc; l != NULL; l = l->l_next)

I am not sure about this implication, it implicit states that the
return opaque handler of dlopen/dlmopen can be cast to a link_map. 
This is true for current implementation, but it is not stated in our
manual nor defined in any standard.

Should we proper document it or avoid the test on the rest of the
link_maps?

> +    basic_checks (l);
> +  xdlclose (secondary_libc);
> +
>    return 0;
>  }
>  
> 
> base-commit: a0ecbb45969e93ec5eb6ba0d1f0a5578bdb2e54c
>

* Adhemerval Zanella Netto:

>> +  /* The secondary namespace does not contain the main executable, and
>> +     dl_iterate_phdr does not cover it.  */
>> +  struct link_map *secondary_libc = xdlmopen (LM_ID_NEWLM, LIBC_SO, RTLD_NOW);
>> +  for (l = secondary_libc; l != NULL; l = l->l_next)
>
> I am not sure about this implication, it implicit states that the
> return opaque handler of dlopen/dlmopen can be cast to a link_map. 
> This is true for current implementation, but it is not stated in our
> manual nor defined in any standard.

It's *not* true for the current implementation in general.  It works for
dlinfo (and my recent documentation update blesses this use), but it can
fail for dlsym if the link map was not returned by dlopen.  In this
case, there is no search scope allocated for it, and dlsym will crash.

The proposed manual update is here:

  manual: Describe struct link_map, support link maps with dlinfo
  <https://inbox.sourceware.org/libc-alpha/875xsagf4f.fsf@oldenburg.str.redhat.com/>

Thanks,
Florian

On 15/08/24 06:39, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>>> +  /* The secondary namespace does not contain the main executable, and
>>> +     dl_iterate_phdr does not cover it.  */
>>> +  struct link_map *secondary_libc = xdlmopen (LM_ID_NEWLM, LIBC_SO, RTLD_NOW);
>>> +  for (l = secondary_libc; l != NULL; l = l->l_next)
>>
>> I am not sure about this implication, it implicit states that the
>> return opaque handler of dlopen/dlmopen can be cast to a link_map. 
>> This is true for current implementation, but it is not stated in our
>> manual nor defined in any standard.
> 
> It's *not* true for the current implementation in general.  It works for
> dlinfo (and my recent documentation update blesses this use), but it can
> fail for dlsym if the link map was not returned by dlopen.  In this
> case, there is no search scope allocated for it, and dlsym will crash.
> 
> The proposed manual update is here:
> 
>   manual: Describe struct link_map, support link maps with dlinfo
>   <https://inbox.sourceware.org/libc-alpha/875xsagf4f.fsf@oldenburg.str.redhat.com/>

Yes, but my reservation is the cast of dlmopen return to 'struct linkmap'.
You patch itself states:

  Pointers to link maps can be obtained from the @code{_r_debug} variable,
  from the @code{RTLD_DI_LINKMAP} request for @code{dlinfo}, and from the
  @code{_dl_find_object} function.  See below for details.

So I am not following why this cast is strictly supported.

* Adhemerval Zanella Netto:

> On 15/08/24 06:39, Florian Weimer wrote:
>> * Adhemerval Zanella Netto:
>> 
>>>> +  /* The secondary namespace does not contain the main executable, and
>>>> +     dl_iterate_phdr does not cover it.  */
>>>> +  struct link_map *secondary_libc = xdlmopen (LM_ID_NEWLM, LIBC_SO, RTLD_NOW);
>>>> +  for (l = secondary_libc; l != NULL; l = l->l_next)
>>>
>>> I am not sure about this implication, it implicit states that the
>>> return opaque handler of dlopen/dlmopen can be cast to a link_map. 
>>> This is true for current implementation, but it is not stated in our
>>> manual nor defined in any standard.
>> 
>> It's *not* true for the current implementation in general.  It works for
>> dlinfo (and my recent documentation update blesses this use), but it can
>> fail for dlsym if the link map was not returned by dlopen.  In this
>> case, there is no search scope allocated for it, and dlsym will crash.
>> 
>> The proposed manual update is here:
>> 
>>   manual: Describe struct link_map, support link maps with dlinfo
>>   <https://inbox.sourceware.org/libc-alpha/875xsagf4f.fsf@oldenburg.str.redhat.com/>
>
> Yes, but my reservation is the cast of dlmopen return to 'struct linkmap'.
> You patch itself states:
>
>   Pointers to link maps can be obtained from the @code{_r_debug} variable,
>   from the @code{RTLD_DI_LINKMAP} request for @code{dlinfo}, and from the
>   @code{_dl_find_object} function.  See below for details.
>
> So I am not following why this cast is strictly supported.

It's already an internal test:

dlfcn/Makefile-tests-internal += \
dlfcn/Makefile:  tst-dlinfo-phdr \
dlfcn/Makefile-  # tests-internal

I think this kind of usage of dlopen handles is not uncommon in such
tests.  I can perhaps add a comment.

Thanks,
Florian