Re: [PATCH v4] elf: Add _dl_find_object function

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella:

>> +/* The ELF segment which contains the exception handling data.  */
>> +#define DLFO_EH_SEGMENT_TYPE PT_GNU_EH_FRAME
>
> Do we really need to export it?

Technically?  No, but it documents the ELF segment glibc is using for
the debugging information.  There is currently no other good way to get
this information.

>> diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
>> index 4a3b870a48..ed03e9e7ed 100644
>> --- a/dlfcn/dlfcn.h
>> +++ b/dlfcn/dlfcn.h
>> @@ -28,6 +28,8 @@
>>  
>>  
>>  #ifdef __USE_GNU
>> +#include <bits/dl_find_object.h>
>> +
>>  /* If the first argument of `dlsym' or `dlvsym' is set to RTLD_NEXT
>>     the run-time address of the symbol called NAME in the next shared
>>     object is returned.  The "next" relation is defined by the order
>> @@ -194,6 +196,31 @@ typedef struct
>>    Dl_serpath dls_serpath[1];	/* Actually longer, dls_cnt elements.  */
>>  # endif
>>  } Dl_serinfo;
>> +
>> +struct dl_find_object
>> +{
>> +  unsigned long long int dlfo_flags;
>
> Shouldn't we use __extension__ for 'long long' in exported headers?

I fixed both occurrences, and also changed the name of
__dlfo_eh_count_dbase to __dlfo_eh_dbase_pad.

>> @@ -1994,4 +2006,32 @@ $(objpfx)tst-ro-dynamic-mod.so: $(objpfx)tst-ro-dynamic-mod.os \
>>  		-Wl,--script=tst-ro-dynamic-mod.map \
>>  		$(objpfx)tst-ro-dynamic-mod.os
>>  
>> +
>
> Spurious new line?

Fixed.

>>  $(objpfx)tst-rtld-run-static.out: $(objpfx)/ldconfig
>> +
>> +$(objpfx)tst-dl_find_object.out: \
>> +  $(objpfx)tst-dl_find_object-mod1.so $(objpfx)tst-dl_find_object-mod2.so
>> +$(objpfx)tst-dl_find_object-static.out: \
>> +  $(objpfx)tst-dl_find_object-mod1.so $(objpfx)tst-dl_find_object-mod2.so
>> +tst-dl_find_object-static-ENV = $(static-dlopen-environment)
>> +CFLAGS-tst-dl_find_object.c += -funwind-tables
>> +CFLAGS-tst-dl_find_object-static.c += -funwind-tables
>> +LDFLAGS-tst-dl_find_object-static += -Wl,--eh-frame-hdr
>> +CFLAGS-tst-dl_find_object-mod1.c += -funwind-tables
>> +CFLAGS-tst-dl_find_object-mod2.c += -funwind-tables
>> +LDFLAGS-tst-dl_find_object-mod2.so += -Wl,--enable-new-dtags,-z,nodelete
>> +$(objpfx)tst-dl_find_object-threads: $(shared-thread-library)
>> +CFLAGS-tst-dl_find_object-threads.c += -funwind-tables
>> +$(objpfx)tst-dl_find_object-threads.out: \
>> +  $(objpfx)tst-dl_find_object-mod1.so $(objpfx)tst-dl_find_object-mod2.so \
>> +  $(objpfx)tst-dl_find_object-mod3.so $(objpfx)tst-dl_find_object-mod4.so \
>> +  $(objpfx)tst-dl_find_object-mod5.so $(objpfx)tst-dl_find_object-mod6.so \
>> +  $(objpfx)tst-dl_find_object-mod7.so $(objpfx)tst-dl_find_object-mod8.so \
>> +  $(objpfx)tst-dl_find_object-mod9.so
>
> Maybe add one object per line.

Also fixed.

>> +/* To achieve async-signal-safety, two copies of the data structure
>> +   are used, so that a signal handler can still use this data even if
>> +   dlopen or dlclose modify the other copy.  The the MSB in
>> +   _dlfo_loaded_mappings_version determins which array element is the
>
> s/determins/determines

Applied.

>> +   currently active region.  */
>> +static struct dlfo_mappings_segment *_dlfo_loaded_mappings[2];
>> +
>> +/* Returns the number of actually used elements in all segements
>
> s/segements/segments

Likewise.

>> +  /* We may have obtained slightly more space if malloc happened
>> +     to provide an over-aligned pointer.  */
>> +  result->allocated = (((uintptr_t) (end - ptr)
>
> I think it should ptrdiff_t here.

I believe uintptr_t results in more efficient code because it is an
unsigned type.  I would like to leave it as-is.

>> +int
>> +_dl_find_object (void *pc1, struct dl_find_object *result)
>> +{
>> +  uintptr_t pc = (uintptr_t) pc1;
>> +
>> +  if (_dlfo_main.map_end == 0)
>
> Maybe __glibc_unlikely here?

Hmm.  Looks like one of the rare occasions where it actually might be
helpful.

>> +  /* Other initially loaded objects.  */
>> +  if (pc >= _dlfo_nodelete_mappings->map_start
>> +      && pc < _dlfo_nodelete_mappings_end)
>> +    {
>> +      struct dl_find_object_internal *obj
>> +        = _dlfo_lookup (pc, _dlfo_nodelete_mappings,
>> +                        _dlfo_nodelete_mappings_size);
>> +      if (obj != NULL)
>> +        {
>> +          _dl_find_object_to_external (obj, result);
>> +          return 0;
>> +        }
>> +      /* Fall through to the full search.  The kernel may have mapped
>> +         the initial mappings with gaps that are later filled by
>> +         dlopen with other mappings.  */
>> +    }
>> +
>> +  /* Handle audit modules, dlopen, dlopen objects.  This uses software
>
> Won't audit modules fall in the 'initially loaded objects'?

Only initially loaded objects in the default namespace are treated as
such.  In audit namespaces, we don't know which objects are initally
loaded (and can't be unloaded), and which ones have been dlopen'ed and
could theoretically be dlclose'd by the auditor.  I couldn't find a
straightforward way to make the distinction, and deemed it not worth the
additional complexity.

>> diff --git a/elf/libc-dl_find_object.c b/elf/libc-dl_find_object.c
>> new file mode 100644
>> index 0000000000..38ea3bc949
>> --- /dev/null
>> +++ b/elf/libc-dl_find_object.c

>> +int
>> +_dl_find_object (void *address, struct dl_find_object *result)
>> +{
>> +  return GLRO (dl_find_object) (address, result);
>> +}
>
> Why this function is required? Can we use GLRO (dl_find_object) on
> libc code?

This function is needed to export _dl_find_object from libc.so.6.  This
approach has the advantage that we can patch GLRO (dl_find_object)
during static dlopen.  Placing this function in libc also avoids an
annoying linking issue during the glibc build: We do not link with the
libc.so linker script, so some links involving libgcc_s.so fail if
_dl_find_object is only available in ld.so.

>> +/* Compare _dl_find_object result at ADDRESS with *EXPECTED.  */
>> +static void
>> +check (void *address, struct dl_find_object *expected, int line)
>> +{
>> +  struct dl_find_object actual;
>> +  int ret = _dl_find_object (address, &actual);
>> +  if (expected == NULL)
>> +    {
>> +      if (ret != -1)
>> +        {
>> +          support_record_failure ();
>> +          printf ("%s:%d: unexpected success for %p\n",
>> +                  __FILE__, line, address);
>> +        }
>> +      return;
>> +    }
>
> Can't we use TEST_VERIFY here?
>
>> +  if (ret != 0)
>> +    {
>> +      support_record_failure ();
>> +      printf ("%s:%d: unexpected failure for %p\n",
>> +              __FILE__, line, address);
>> +      return;
>> +    }
>> +
>
> Same as before.
>
>> +  if (actual.dlfo_flags != expected->dlfo_flags)
>> +    {
>> +      support_record_failure ();
>> +      printf ("%s:%d: error: %p: flags is %llu, expected %llu\n",
>> +              __FILE__, line, address,
>> +              actual.dlfo_flags, expected->dlfo_flags);
>> +    }
>
> Can't we use TEST_COMPARE and in the following tests?

I wanted to add more diagnostics.

>> +/* Request process termination after 3 seconds.  */
>> +static bool exit_requested;
>> +static void *
>> +exit_thread (void *ignored)
>> +{
>> +  usleep (3 * 100 * 1000);
>
> This will add a 3s, do we really need this large timeout?

We can lower it, but it makes it less likely the test will find
concurrency issues.

>> +  struct drand48_data state;
>> +  srand48_r (1, &state);
>> +  while (!__atomic_load_n (&exit_requested, __ATOMIC_RELAXED))
>> +    {
>> +      long int idx;
>> +      lrand48_r (&state, &idx);
>> +      idx %= array_length (temp_objects);
>> +      if (temp_objects[idx].link_map == NULL)
>> +        {
>> +          temp_objects[idx].link_map = xdlopen (temp_objects[idx].soname,
>> +                                                RTLD_NOW);
>> +          temp_objects[idx].address = xdlsym (temp_objects[idx].link_map,
>> +                                              temp_objects[idx].symbol);
>> +        }
>> +      else
>> +        {
>> +          xdlclose (temp_objects[idx].link_map);
>> +          temp_objects[idx].link_map = NULL;
>> +          struct dl_find_object dlfo;
>> +          int ret = _dl_find_object (temp_objects[idx].address, &dlfo);
>> +          if (ret != -1)
>
> Can't you use use TEST_VERIFY_EXIT (ret == 0) here and add debug information
> only for '-d' option?

See above, I'd like to have the information in the logs in case of
spurious failures, so that we can investigate them.

>> +static void
>> +check_initial (void)
>> +{
>> +#ifndef FOR_STATIC
>> +  /* Avoid direct reference, which could lead to copy relocations.  */
>> +  struct r_debug *debug = xdlsym (NULL, "_r_debug");
>> +  TEST_VERIFY_EXIT (debug != NULL);
>> +  char **tzname = xdlsym (NULL, "tzname");
>> +
>> +  /* The main executable has an unnamed link map.  */
>> +  struct link_map *main_map = (struct link_map *) debug->r_map;
>> +  TEST_COMPARE_STRING (main_map->l_name, "");
>> +
>> +  /* The link map of the dynamic linker.  */
>> +  struct link_map *rtld_map = xdlopen (LD_SO, RTLD_LAZY | RTLD_NOLOAD);
>> +  TEST_VERIFY_EXIT (rtld_map != NULL);
>> +
>> +  /* The link map of libc.so.  */
>> +  struct link_map *libc_map = xdlopen (LIBC_SO, RTLD_LAZY | RTLD_NOLOAD);
>> +  TEST_VERIFY_EXIT (libc_map != NULL);
>
> I know this an internal test, but maybe use dlinfo or add a wrapper to
> return the link_map on libsupport instead?

Hmm.  Is this really necessary?  I can add a helper function to
support/.

But dlopen returns a void *, so this function will not add any
additional type safety, just a tiny bit of extra documentation
(but searching for “link_map” would already give you places to check if
we ever need to break the dlopen/struct link_map association).

>> +@deftypefun {void *} _dl_find_object (void *@var{address}, struct dl_find_object *@var{result})
>> +@standards{GNU, dlfcn.h}
>> +@safety{@mtsafe{}@assafe{}@acsafe{}}
>> +On success, this function returns 0 and writes about the object
>> +surrounding the address to @code{*@var{result}}.  On failure, -1 is
>> +returned.
>> +
>> +The @var{address} can be a code address or data address.  On
>> +architectures using function descriptors, not attempt is made to decode
>
> I think it should be 'no attempt'.

Right, fixed.

>> +the function descriptor.  Depending on how these descriptors are
>> +implemented, @code{_dl_find_object} may return the object that defines
>> +the function descriptor (and not the object that contains the code
>> +implementing the function), or fail to find any object at all.
>
> At least I did not see any failures on powerpc64.

The tests use object addresses to avoid this issue. 8-)

I'll test and repost a v5.

Thanks,
Florian