[v6,15/20] elf: Run constructors if executable has a soname of a dependency
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
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
* 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
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.
* 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
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).
@@ -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))
{
@@ -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