diff mbox series

[v6,15/20] elf: Run constructors if executable has a soname of a dependency

Message ID 20211115183734.531155-16-adhemerval.zanella@linaro.org
State Superseded
Headers show
Series Multiple rtld-audit fixes | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Adhemerval Zanella Nov. 15, 2021, 6:37 p.m. UTC
The DSO constructor should not be ignored if the main executable
has the SONAME set to a dependency.  It fixes the case where
(using the scripts/dso-ordering-test.py definition):

  {}->a->b->c;soname({})=c

Where the constructors should return

  c>b>a>{}<a<b<c

Checked on x86_64-linux-gnu.
---
 elf/dl-load.c            | 11 +++++++++--
 elf/dso-sort-tests-1.def |  5 ++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

Comments

Florian Weimer Dec. 18, 2021, 8:08 p.m. UTC | #1
* Adhemerval Zanella:

> The DSO constructor should not be ignored if the main executable
> has the SONAME set to a dependency.  It fixes the case where
> (using the scripts/dso-ordering-test.py definition):
>
>   {}->a->b->c;soname({})=c
>
> Where the constructors should return
>
>   c>b>a>{}<a<b<c

I'm a bit unclear what is this trying to fix.

It reminds me of this change:

commit 9ffa50b26b0cb5d3043adf6d3d0b1ea735acc147
Author: Florian Weimer <fweimer@redhat.com>
Date:   Fri Dec 11 17:30:03 2020 +0100

    elf: Include libc.so.6 as main program in dependency sort (bug 20972)
    
    _dl_map_object_deps always sorts the initially loaded object first
    during dependency sorting.  This means it is relocated last in
    dl_open_worker.  This results in crashes in IFUNC resolvers without
    lazy bindings if libraries are preloaded that refer to IFUNCs in
    libc.so.6: the resolvers are called when libc.so.6 has not been
    relocated yet, so references to _rtld_global_ro etc. crash.
    
    The fix is to check against the libc.so.6 link map recorded by the
    __libc_early_init framework, and let it participate in the dependency
    sort.
    
    This fixes bug 20972.
    
    Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Is it a more general/correct version of this commit?

But if the main program has a soname, wouldn't we stop loading it as a
dependency, too?  So that the expected ouptut turns into:

  b>a>{}<a<b

because the separate c is not actually loaded.

Thanks,
Florian
Adhemerval Zanella Dec. 20, 2021, 4:49 p.m. UTC | #2
On 18/12/2021 17:08, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> The DSO constructor should not be ignored if the main executable
>> has the SONAME set to a dependency.  It fixes the case where
>> (using the scripts/dso-ordering-test.py definition):
>>
>>   {}->a->b->c;soname({})=c
>>
>> Where the constructors should return
>>
>>   c>b>a>{}<a<b<c
> 
> I'm a bit unclear what is this trying to fix.

The issue is tst-dso-ordering11 sets its SONAME as one of its NEEDED objects:

$ readelf -d t
st-dso-ordering11 | grep 'SONAME\|NEEDED'
 0x0000000000000001 (NEEDED)             Shared library: [[...]/tst-dso-ordering11-dir/tst-dso-ordering11-a.so]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x000000000000000e (SONAME)             Library soname: [[...]/tst-dso-ordering11-dir/tst-dso-ordering11-a.so]

And then the tst-dso-ordering11-a.so constructor is skipped.

> 
> It reminds me of this change:
> 
> commit 9ffa50b26b0cb5d3043adf6d3d0b1ea735acc147
> Author: Florian Weimer <fweimer@redhat.com>
> Date:   Fri Dec 11 17:30:03 2020 +0100
> 
>     elf: Include libc.so.6 as main program in dependency sort (bug 20972)
>     
>     _dl_map_object_deps always sorts the initially loaded object first
>     during dependency sorting.  This means it is relocated last in
>     dl_open_worker.  This results in crashes in IFUNC resolvers without
>     lazy bindings if libraries are preloaded that refer to IFUNCs in
>     libc.so.6: the resolvers are called when libc.so.6 has not been
>     relocated yet, so references to _rtld_global_ro etc. crash.
>     
>     The fix is to check against the libc.so.6 link map recorded by the
>     __libc_early_init framework, and let it participate in the dependency
>     sort.
>     
>     This fixes bug 20972.
>     
>     Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 
> Is it a more general/correct version of this commit?

I think it is unrelated.

> 
> But if the main program has a soname, wouldn't we stop loading it as a
> dependency, too?  So that the expected ouptut turns into:
> 
>   b>a>{}<a<b
> 
> because the separate c is not actually loaded.

Right, so if the idea is indeed to not load an dependency that matches the
binary SONAME current behavior is indeed what is expected.  Not sure if
this the expected behavior, if so I will drop this patch.
Florian Weimer Dec. 20, 2021, 4:52 p.m. UTC | #3
* Adhemerval Zanella:

>> But if the main program has a soname, wouldn't we stop loading it as a
>> dependency, too?  So that the expected ouptut turns into:
>> 
>>   b>a>{}<a<b
>> 
>> because the separate c is not actually loaded.
>
> Right, so if the idea is indeed to not load an dependency that matches the
> binary SONAME current behavior is indeed what is expected.  Not sure if
> this the expected behavior, if so I will drop this patch.

Is it the current behavior?

I can see some value in preserving that, e.g. for scripting language
interpreters which historically avoided DSOs for the core interpreter,
preferring position-dependent code for it (but still offering a DSO for
embedding).  They may even set DT_SONAME on the main program; I haven't
checked.

Thanks,
Florian
Adhemerval Zanella Dec. 20, 2021, 4:55 p.m. UTC | #4
On 20/12/2021 13:52, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>> But if the main program has a soname, wouldn't we stop loading it as a
>>> dependency, too?  So that the expected ouptut turns into:
>>>
>>>   b>a>{}<a<b
>>>
>>> because the separate c is not actually loaded.
>>
>> Right, so if the idea is indeed to not load an dependency that matches the
>> binary SONAME current behavior is indeed what is expected.  Not sure if
>> this the expected behavior, if so I will drop this patch.
> 
> Is it the current behavior?

Yes.

> 
> I can see some value in preserving that, e.g. for scripting language
> interpreters which historically avoided DSOs for the core interpreter,
> preferring position-dependent code for it (but still offering a DSO for
> embedding).  They may even set DT_SONAME on the main program; I haven't
> checked.

Right, this does make sense (although accomplished in a kinda hackish
way).
diff mbox series

Patch

diff --git a/elf/dl-load.c b/elf/dl-load.c
index e28893b779..6bf1eabfe5 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1993,13 +1993,20 @@  _dl_map_object (struct link_map *loader, const char *name,
   assert (nsid >= 0);
   assert (nsid < GL(dl_nns));
 
+  /* Special case: trying to map itself.  An empty name correspond to
+     a NULL or "" argument for dlopen(), and it returns the head object
+     of the namespace.  */
+  if (name[0] == '\0')
+    return GL(dl_ns)[nsid]._ns_loaded;
+
   /* Look for this name among those already loaded.  */
   for (l = GL(dl_ns)[nsid]._ns_loaded; l; l = l->l_next)
     {
       /* If the requested name matches the soname of a loaded object,
 	 use that object.  Elide this check for names that have not
-	 yet been opened.  */
-      if (__glibc_unlikely ((l->l_faked | l->l_removed) != 0))
+	 yet been opened or the executable itself.  */
+      if (__glibc_unlikely ((l->l_faked | l->l_removed) != 0
+			    || l->l_type == lt_executable))
 	continue;
       if (!_dl_name_match_p (name, l))
 	{
diff --git a/elf/dso-sort-tests-1.def b/elf/dso-sort-tests-1.def
index 5f7f18ef27..2228910c0d 100644
--- a/elf/dso-sort-tests-1.def
+++ b/elf/dso-sort-tests-1.def
@@ -50,7 +50,10 @@  output: e>d>c>b>a>{}<a<b<c<d<e
 # Test if init/fini ordering behavior is proper, despite main program with
 # an soname that may cause confusion
 tst-dso-ordering10: {}->a->b->c;soname({})=c
-output: b>a>{}<a<b
+output: c>b>a>{}<a<b<c
+# Same as before, but setting the soname as a direct dependency.
+tst-dso-ordering11: {}->a->b->c;soname({})=a
+output: c>b>a>{}<a<b<c
 
 # Complex example from Bugzilla #15311, under-linked and with circular
 # relocation(dynamic) dependencies. While this is technically unspecified, the