Re: [PATCH] dlfcn: Implement the RTLD_DI_PHDR request type for dlinfo

[Carlos O'Donell <carlos@redhat.com>]

On 4/20/22 14:53, Florian Weimer via Libc-alpha wrote:
> The information is theoretically available via dl_iterate_phdr as
> well, but that approach is very slow if there are many shared
> objects.
> 
> Tested on i686-linux-gnu and x86_64-linux-gnu.  Built with
> build-many-glibcs.py.
> 
> This patch depends on the previous dlinfo documentation patch.

Pending the manual v3 and commit.

LGTM. 

I agree that a dlinfo API for this is useful, and I saw that this helps 
resolve a real user request:

https://sourceware.org/pipermail/libc-alpha/2022-March/137096.html

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@rehdat.com>
 
> ---
>  dlfcn/Makefile          |   4 ++
>  dlfcn/dlfcn.h           |   7 ++-
>  dlfcn/dlinfo.c          |  13 ++++-
>  dlfcn/tst-dlinfo-phdr.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++
>  manual/dynlink.texi     |  15 ++++--
>  5 files changed, 159 insertions(+), 5 deletions(-)
> 
> diff --git a/dlfcn/Makefile b/dlfcn/Makefile
> index 6e0a014d99..3255ba02c5 100644
> --- a/dlfcn/Makefile
> +++ b/dlfcn/Makefile
> @@ -73,6 +73,10 @@ tststatic3-ENV = $(tststatic-ENV)
>  tststatic4-ENV = $(tststatic-ENV)
>  tststatic5-ENV = $(tststatic-ENV)
>  
> +tests-internal += \
> +  tst-dlinfo-phdr \
> +  # tests-internal

OK.

> +
>  ifneq (,$(CXX))
>  modules-names += bug-atexit3-lib
>  else
> diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
> index b5cd5c5238..a3af6051d4 100644
> --- a/dlfcn/dlfcn.h
> +++ b/dlfcn/dlfcn.h
> @@ -164,7 +164,12 @@ enum
>         segment, or if the calling thread has not allocated a block for it.  */
>      RTLD_DI_TLS_DATA = 10,
>  
> -    RTLD_DI_MAX = 10
> +    /* Treat ARG as const ElfW(Phdr) **, and store the address of the
> +       program header array at that location.  The dlinfo call returns
> +       the number of program headers in the array.  */
> +    RTLD_DI_PHDR = 11,
> +
> +    RTLD_DI_MAX = 11

OK. I like that the return is the number of program headers in the array.

>    };
>  
>  
> diff --git a/dlfcn/dlinfo.c b/dlfcn/dlinfo.c
> index fc63c02681..f64ecf59cb 100644
> --- a/dlfcn/dlinfo.c
> +++ b/dlfcn/dlinfo.c
> @@ -28,6 +28,10 @@ struct dlinfo_args
>    void *handle;
>    int request;
>    void *arg;
> +
> +  /* This is the value that is returned from dlinfo if no error is
> +     signaled.  */
> +  int result;

OK.

>  };
>  
>  static void
> @@ -40,6 +44,7 @@ dlinfo_doit (void *argsblock)
>      {
>      case RTLD_DI_CONFIGADDR:
>      default:
> +      args->result = -1;

OK.

>        _dl_signal_error (0, NULL, NULL, N_("unsupported dlinfo request"));
>        break;
>  
> @@ -75,6 +80,11 @@ dlinfo_doit (void *argsblock)
>  	*(void **) args->arg = data;
>  	break;
>        }
> +
> +    case RTLD_DI_PHDR:
> +      *(const ElfW(Phdr) **) args->arg = l->l_phdr;
> +      args->result = l->l_phnum;

OK. Straight forward copy of data we already have.

> +      break;
>      }
>  }
>  
> @@ -82,7 +92,8 @@ static int
>  dlinfo_implementation (void *handle, int request, void *arg)
>  {
>    struct dlinfo_args args = { handle, request, arg };
> -  return _dlerror_run (&dlinfo_doit, &args) ? -1 : 0;
> +  _dlerror_run (&dlinfo_doit, &args);
> +  return args.result;

OK. We need this because result is now returned which is not just -1 or 0.

>  }
>  
>  #ifdef SHARED
> diff --git a/dlfcn/tst-dlinfo-phdr.c b/dlfcn/tst-dlinfo-phdr.c
> new file mode 100644
> index 0000000000..a15a7d48eb
> --- /dev/null
> +++ b/dlfcn/tst-dlinfo-phdr.c
> @@ -0,0 +1,125 @@
> +/* Test for dlinfo (RTLD_DI_PHDR).

OK.

> +   Copyright (C) 2022 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <dlfcn.h>
> +#include <link.h>
> +#include <stdbool.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/auxv.h>
> +
> +#include <support/check.h>
> +#include <support/xdlfcn.h>
> +
> +/* Used to verify that the program header array appears as expected
> +   among the dl_iterate_phdr callback invocations.  */
> +
> +struct dlip_callback_args
> +{
> +  struct link_map *l;           /* l->l_addr is used to find the object.  */
> +  const ElfW(Phdr) *phdr;       /* Expected program header pointed.  */
> +  int phnum;                    /* Expected program header count.  */
> +  bool found;                   /* True if l->l_addr has been found.  */
> +};
> +
> +static int
> +dlip_callback (struct dl_phdr_info *dlpi, size_t size, void *closure)
> +{
> +  TEST_COMPARE (sizeof (*dlpi), size);
> +  struct dlip_callback_args *args = closure;
> +
> +  if (dlpi->dlpi_addr == args->l->l_addr)
> +    {
> +      TEST_VERIFY (!args->found);
> +      args->found = true;
> +      TEST_VERIFY (args->phdr == dlpi->dlpi_phdr);
> +      TEST_COMPARE (args->phnum, dlpi->dlpi_phnum);
> +    }
> +
> +  return 0;
> +}
> +
> +static int
> +do_test (void)
> +{
> +  /* Avoid a copy relocation.  */
> +  struct r_debug *debug = xdlsym (RTLD_DEFAULT, "_r_debug");
> +  struct link_map *l = (struct link_map *) debug->r_map;

OK. Using tests-internal view of struct link_map.

> +  TEST_VERIFY_EXIT (l != NULL);
> +
> +  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);

OK. Call dlinfo with new request.

> +      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);

OK. Compare.

> +
> +      /* 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);

OK. Compare.

> +            }
> +        TEST_VERIFY (dynamic_found);
> +      }
> +
> +      /* Check that dl_iterate_phdr finds the link map with the same
> +         program headers.  */
> +      {
> +        struct dlip_callback_args args =
> +          {
> +            .l =  l,
> +            .phdr = phdr,
> +            .phnum = phnum,
> +            .found = false,
> +          };
> +        TEST_COMPARE (dl_iterate_phdr (dlip_callback, &args), 0);

OK. Run comparison now with dl_iterate_phdr (after comparison via raw _r_debug values).

> +        TEST_VERIFY (args.found);
> +      }
> +
> +      if (l->l_prev == NULL)
> +        {
> +          /* This is the executable, so the information is also
> +             available via getauxval.  */
> +          TEST_COMPARE_STRING (l->l_name, "");
> +          TEST_VERIFY (phdr == (const ElfW(Phdr) *) getauxval (AT_PHDR));
> +          TEST_COMPARE (phnum, getauxval (AT_PHNUM));

OK. Compare now via getauxval.

> +        }
> +
> +      l = l->l_next;
> +    }
> +  while (l != NULL);
> +
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/manual/dynlink.texi b/manual/dynlink.texi
> index 6d74fbe2ef..18f78ee002 100644
> --- a/manual/dynlink.texi
> +++ b/manual/dynlink.texi
> @@ -28,9 +28,9 @@ location @var{arg}, based on @var{request}.  The @var{handle} argument
>  must be a pointer returned by @code{dlopen} or @code{dlmopen}; it must
>  not have been closed by @code{dlclose}.
>  
> -On success, @code{dlinfo} returns 0.  If there is an error, the function
> -returns @math{-1}, and @code{dlerror} can be used to obtain a
> -corresponding error message.
> +On success, @code{dlinfo} returns 0 for most request types; exceptions
> +are noted below.  If there is an error, the function returns @math{-1},
> +and @code{dlerror} can be used to obtain a corresponding error message.

OK.

>  
>  The following operations are defined for use with @var{request}:
>  
> @@ -82,6 +82,15 @@ This request writes the TLS module ID for the shared object @var{handle}
>  to @code{*@var{arg}}.  The argument @var{arg} must be the address of an
>  object of type @code{size_t}.  The module ID is zero if the object
>  does not have an associated TLS block.
> +
> +@item RTLD_DI_PHDR
> +This request writes the address of the program header array to
> +@code{*@var{arg}}.  The argument @var{arg} must be the address of an
> +object of type @code{const ElfW(Phdr) *} (that is,
> +@code{const Elf32_Phdr *} or @code{const Elf64_Phdr *}, as appropriate
> +for the current architecture).  For this request, the value returned by
> +@code{dlinfo} is the number of program headers in the program header
> +array.

OK.

>  @end vtable
>  @end deftypefun
>  
> 


-- 
Cheers,
Carlos.