Message ID | 20211115183734.531155-16-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | Multiple rtld-audit fixes | expand |
* 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).
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