Re: [PATCH 1/3] elf: Allow dlopen of filter object to work [BZ #16272]

[David Kilroy <David dot Kilroy at arm dot com>]

Hi Florian,

Thanks for the detailed review. Apologies for the various formatting issues,
I haven't got used the GNU style yet. I'll attempt to correct this in the
next submission.

From: Florian Weimer <fweimer@redhat.com>
>* David Kilroy:
>
>> diff --git a/elf/Makefile b/elf/Makefile
>> index 8f96299..55bed87 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
>> @@ -279,7 +280,9 @@ modules-names = testobj1 testobj2 testobj3 testobj4 testobj5 testobj6 \
>>               tst-main1mod tst-libc_dlvsym-dso tst-absolute-sym-lib \
>>               tst-absolute-zero-lib tst-big-note-lib tst-unwind-ctor-lib \
>>               tst-audit13mod1 tst-sonamemove-linkmod1 \
>> -             tst-sonamemove-runmod1 tst-sonamemove-runmod2
>> +             tst-sonamemove-runmod1 tst-sonamemove-runmod2 \
>> +             tst-filterobj-flt tst-filterobj-lib
>
>This needs rebasing to current master.  Unfortunately, I've got various
>patches in flight which conflict.

Will do. I think the main conflicts are in the Makefile where we are both
adding tests. I've seen that you're also touching dl_open_worker, but
so far it looks like the changes are in different areas.

>> +LDFLAGS-tst-filterobj-flt.so=-Wl,--filter=$(objpfx)tst-filterobj-lib.so
>> +CFLAGS-tst-filterobj-dlopen.c += -DPFX=\"$(objpfx)\"
>> +$(objpfx)tst-filterobj: $(objpfx)tst-filterobj-flt.so | $(objpfx)tst-filterobj-lib.so
>> +$(objpfx)tst-filterobj-dlopen: $(libdl) | $(objpfx)tst-filterobj-lib.so
>
>Please use spaces around operators.  To add a run-time dependency to
>tst-filterobj-dlopen, use something like this, please:
>
>$(objpfx)tst-filterobj-dlopen.out : $(objpfx)tst-filterobj-lib.so
>
>This way, the test is re-run if tst-filterobj-lib.so changed.

Well, spotted! Will do.

>> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
>> index c29b988..c177cef 100644
>> --- a/elf/dl-deps.c
>> +++ b/elf/dl-deps.c

>These changes look reasonable to me.

That's good to know. I tried a few different things before I realised I needed
to fix up the relocations as well.

>But please use spaces around operators.
>
>(Not sure if you want to commit this yourself; if not, I can clean up
>such things for you prior to committing.)

I doubt I have commit access (this is my first submission), but I will attempt
to fix all formatting issues.

>> diff --git a/elf/dl-open.c b/elf/dl-open.c
>> index a9fd4cb..7fcfdc0 100644
>> --- a/elf/dl-open.c
>> +++ b/elf/dl-open.c
>> @@ -305,22 +305,25 @@ dl_open_worker (void *a)
>>       allows IFUNC relocations to work and it also means copy
>>       relocation of dependencies are if necessary overwritten.  */
>>    unsigned int nmaps = 0;
>> -  struct link_map *l = new;
>> +  unsigned int j = 0;
>> +  struct link_map *l =  new->l_initfini[0];
>>    do
>>      {
>>        if (! l->l_real->l_relocated)
>>       ++nmaps;
>> -      l = l->l_next;
>> +      l = new->l_initfini[++j];
>>      }
>>    while (l != NULL);
>> +  /* Stack allocation is limited by the number of loaded objects.  */
>>    struct link_map *maps[nmaps];
>>    nmaps = 0;
>> -  l = new;
>> +  j = 0;
>> +  l = new->l_initfini[0];
>>    do
>>      {
>>        if (! l->l_real->l_relocated)
>>       maps[nmaps++] = l;
>> -      l = l->l_next;
>> +      l = new->l_initfini[++j];
>>      }
>>    while (l != NULL);
>>    _dl_sort_maps (maps, nmaps, NULL, false);
>
>I have much more trouble ascertaining whether this change is correct.
>Are we certain that new->l_initfini is not a subset of the maps that
>have been loaded?

That is something I'm not sure about (yet). I'll look into this further. My
reasoning for switching to using l_initfini was that the filtee's constructors
were crashing, and clearly anything that needed initialisation ought to be
relocated first. If I can't confirm whether l_initfini is a subset, there are a
couple alternatives here:

* I can verify that the filter is in l->l_prev, and walk that before
  constructing map via l->l_next.

* after relocating via l->l_next, check l_initfini for any un-relocated
  objects.

Both of these would invalidate the following patches.

>> diff --git a/elf/tst-filterobj-dlopen.c b/elf/tst-filterobj-dlopen.c
>> new file mode 100644
>> index 0000000..7c24a1d
>> --- /dev/null
>> +++ b/elf/tst-filterobj-dlopen.c

>> +int main (int argc, const char**argv)
>> +{
>> +  void *lib = dlopen (PFX "tst-filterobj-flt.so", RTLD_LAZY);
>
>I think you should use xasprintf and support_objdir_root from
><support/support.h> to construct the path.  Alternatively, you can drop
>this completely because it will be on the library search path set up by
>the test framework.

So PFX ($(objpfx) in the makefile) is just a path that will be on the
library search path? I assumed it might be a file prefix too. This is
something I copied from an existing dlopen test (I don't have the code
to hand to double check this).

>> +       printf ("%s\n", text);
>> +
>> +       /* Verify the text matches what we expect from the filtee */
>> +       result = strcmp (text, "Hello from filtee (PASS)") != 0;
>> +        }
>> +      else
>> +        {
>> +       fprintf (stderr, "No function\n");
>
>Please use the test framework (<support/test-driver.c>,
><support/check.h>, <support/xdlfcn.h>) and TEST_COMPARE_STRING.
>Tests should write to standard output only.

Will do.



Regards,

Dave.