| Message ID | 20220520083507.2368165-1-maskray@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932] | expand |
On 20/05/2022 05:35, Fangrui Song via Libc-alpha wrote: > When the first object providing foo defines both foo@v1 and foo@@v2, > dlsym(RTLD_NEXT, "foo") returns foo@v1 while dlsym(RTLD_DEFAULT, "foo") > returns foo@@v2. The issue is that RTLD_DEFAULT use the > DL_LOOKUP_RETURN_NEWEST flag while RTLD_NEXT doesn't. Fix the RTLD_NEXT > branch to use DL_LOOKUP_RETURN_NEWEST. I am afraid we will need to add a compat dlsym for this change. > --- > elf/Makefile | 7 +++++++ > elf/dl-sym.c | 2 +- > elf/nextmod3.c | 19 +++++++++++++++++++ > elf/nextmod3.map | 3 +++ > elf/tst-next-ver.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 76 insertions(+), 1 deletion(-) > create mode 100644 elf/nextmod3.c > create mode 100644 elf/nextmod3.map > create mode 100644 elf/tst-next-ver.c > > diff --git a/elf/Makefile b/elf/Makefile > index 6f4ea78007..6cc0023364 100644 > --- a/elf/Makefile > +++ b/elf/Makefile > @@ -425,6 +425,7 @@ tests += \ > tst-initorder2 \ > tst-latepthread \ > tst-main1 \ > + tst-next-ver \ > tst-nodelete2 \ > tst-nodelete-dlclose \ > tst-nodelete-opened \ > @@ -711,6 +712,7 @@ modules-names += \ > neededobj6 \ > nextmod1 \ > nextmod2 \ > + nextmod3 \ > nodel2mod1 \ > nodel2mod2 \ > nodel2mod3 \ > @@ -1689,6 +1691,9 @@ $(objpfx)reldep4.out: $(objpfx)reldep4mod1.so $(objpfx)reldep4mod2.so > $(objpfx)next: $(objpfx)nextmod1.so $(objpfx)nextmod2.so > LDFLAGS-next = -Wl,--no-as-needed > > +$(objpfx)tst-next-ver: $(objpfx)nextmod3.so > +LDFLAGS-tst-next-ver = -Wl,--no-as-needed > + > $(objpfx)unload2.out: $(objpfx)unload2mod.so $(objpfx)unload2dep.so > > $(objpfx)lateglobal.out: $(objpfx)ltglobmod1.so $(objpfx)ltglobmod2.so > @@ -2436,6 +2441,8 @@ $(objpfx)tst-linkall-static: \ > endif > endif > > +LDFLAGS-nextmod3.so = -Wl,--version-script=nextmod3.map > + > # The application depends on the DSO, and the DSO loads the plugin. > # The plugin also depends on the DSO. This creates the circular > # dependency via dlopen that we're testing to make sure works. > diff --git a/elf/dl-sym.c b/elf/dl-sym.c > index aa993942df..b1cf42f36d 100644 > --- a/elf/dl-sym.c > +++ b/elf/dl-sym.c > @@ -144,7 +144,7 @@ RTLD_NEXT used in code not dynamically loaded")); > l = l->l_loader; > > result = GLRO(dl_lookup_symbol_x) (name, match, &ref, l->l_local_scope, > - vers, 0, 0, match); > + vers, 0, flags, match); > } > else > { > diff --git a/elf/nextmod3.c b/elf/nextmod3.c > new file mode 100644 > index 0000000000..96608a65c0 > --- /dev/null > +++ b/elf/nextmod3.c > @@ -0,0 +1,19 @@ > +int > +foo_v1 (int a) > +{ > + return 1; > +} > +asm (".symver foo_v1, foo@v1"); > + > +int > +foo_v2 (int a) > +{ > + return 2; > +} > +asm (".symver foo_v2, foo@v2"); > + > +int > +foo (int a) > +{ > + return 3; > +} > diff --git a/elf/nextmod3.map b/elf/nextmod3.map > new file mode 100644 > index 0000000000..0a8e4e4ee3 > --- /dev/null > +++ b/elf/nextmod3.map > @@ -0,0 +1,3 @@ > +v1 { }; > +v2 { }; > +v3 { foo; }; > diff --git a/elf/tst-next-ver.c b/elf/tst-next-ver.c > new file mode 100644 > index 0000000000..7241f9038b > --- /dev/null > +++ b/elf/tst-next-ver.c > @@ -0,0 +1,46 @@ > +/* Test RTLD_DEFAULT/RTLD_NEXT when the definition has multiple versions. > + Copyright (C) 2018-2022 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <dlfcn.h> > +#include <stdio.h> > + > +#include "testobj.h" > + > +static int > +do_test (void) > +{ > + /* Resolve to foo@@v3 in nextmod3.so, instead of > + foo@v1 or foo@v2. */ > + int (*fp) (int) = dlsym (RTLD_DEFAULT, "foo"); > + int res = fp (0); > + printf ("preload (0) = %d, %s\n", res, res == 3 ? "ok" : "wrong"); > + if (res != 3) > + return 1; > + > + /* Resolve to foo@@v3 in nextmod3.so, instead of > + foo@v1 or foo@v2. */ > + fp = dlsym (RTLD_NEXT, "foo"); > + res = fp (0); > + printf ("preload (0) = %d, %s\n", res, res == 3 ? "ok" : "wrong"); > + if (res != 3) > + return 1; > + > + return 0; > +} > + > +#include <support/test-driver.c>
* Adhemerval Zanella via Libc-alpha: > On 20/05/2022 05:35, Fangrui Song via Libc-alpha wrote: >> When the first object providing foo defines both foo@v1 and foo@@v2, >> dlsym(RTLD_NEXT, "foo") returns foo@v1 while dlsym(RTLD_DEFAULT, "foo") >> returns foo@@v2. The issue is that RTLD_DEFAULT use the >> DL_LOOKUP_RETURN_NEWEST flag while RTLD_NEXT doesn't. Fix the RTLD_NEXT >> branch to use DL_LOOKUP_RETURN_NEWEST. > > I am afraid we will need to add a compat dlsym for this change. My working theory is dlsym with RTLD_NEXT with a versioned symbol is so buggy that this is not needed. I want to understand the nature of the bug, and plan to post a write-up.
On Fri, May 20, 2022 at 7:22 AM Florian Weimer <fw@deneb.enyo.de> wrote: > > * Adhemerval Zanella via Libc-alpha: > > > On 20/05/2022 05:35, Fangrui Song via Libc-alpha wrote: > >> When the first object providing foo defines both foo@v1 and foo@@v2, > >> dlsym(RTLD_NEXT, "foo") returns foo@v1 while dlsym(RTLD_DEFAULT, "foo") > >> returns foo@@v2. The issue is that RTLD_DEFAULT use the > >> DL_LOOKUP_RETURN_NEWEST flag while RTLD_NEXT doesn't. Fix the RTLD_NEXT > >> branch to use DL_LOOKUP_RETURN_NEWEST. > > > > I am afraid we will need to add a compat dlsym for this change. > > My working theory is dlsym with RTLD_NEXT with a versioned symbol is > so buggy that this is not needed. I want to understand the nature of > the bug, and plan to post a write-up. Ok, thanks. FWIW: I have verified that on FreeBSD 13.1, both dlsym(RTLD_NEXT, "foo") and dlsym(RTLD_DEFAULT, "foo") resolve to the default version definition, like what this patch does.
I've been looking at this for a while.
We currently have this code in elf/dl-lookup.c:
/* No specific version is selected. There are two ways we
can got here:
- a binary which does not include versioning information
is loaded
- dlsym() instead of dlvsym() is used to get a symbol which
might exist in more than one form
If the library does not provide symbol version information
there is no problem at all: we simply use the symbol if it
is defined.
These two lookups need to be handled differently if the
library defines versions. In the case of the old
unversioned application the oldest (default) version
should be used. In case of a dlsym() call the latest and
public interface should be returned. */
if (verstab != NULL)
{
if ((verstab[symidx] & 0x7fff)
>= ((flags & DL_LOOKUP_RETURN_NEWEST) ? 2 : 3))
{
/* Don't accept hidden symbols. */
if ((verstab[symidx] & 0x8000) == 0
&& (*num_versions)++ == 0)
/* No version so far. */
*versioned_sym = sym;
return NULL;
}
}
The numbers 2 and 3 look suspicious. The condition involving
num_versions ensures that we only store the first matching symbol in
*versioned_sym. But the skipped version indices are not special.
Indices are not specific to individual symbols, so it is no clear that
index 2 is special and contains the oldest possible version for that
particular symbol, and the next index will have the latest version.
Aligning dlvsym with dlsym still makes sense, so I suggest to proceed
with this patch. But I think there are more bugs in this area.
> diff --git a/elf/nextmod3.c b/elf/nextmod3.c
> new file mode 100644
> index 0000000000..96608a65c0
> --- /dev/null
> +++ b/elf/nextmod3.c
> @@ -0,0 +1,19 @@
> +int
> +foo_v1 (int a)
> +{
> + return 1;
> +}
> +asm (".symver foo_v1, foo@v1");
> +
> +int
> +foo_v2 (int a)
> +{
> + return 2;
> +}
> +asm (".symver foo_v2, foo@v2");
> +
> +int
> +foo (int a)
> +{
> + return 3;
> +}
Please set foo@@v3 explicitly.
> diff --git a/elf/nextmod3.map b/elf/nextmod3.map
> new file mode 100644
> index 0000000000..0a8e4e4ee3
> --- /dev/null
> +++ b/elf/nextmod3.map
> @@ -0,0 +1,3 @@
> +v1 { };
> +v2 { };
> +v3 { foo; };
These versions are not ordered. Maybe use this instead?
v1 { };
v2 { } v1;
v3 { foo; } v2;
Thanks,
Florian
On 22/05/2022 15:54, Fāng-ruì Sòng wrote: > On Fri, May 20, 2022 at 7:22 AM Florian Weimer <fw@deneb.enyo.de> wrote: >> >> * Adhemerval Zanella via Libc-alpha: >> >>> On 20/05/2022 05:35, Fangrui Song via Libc-alpha wrote: >>>> When the first object providing foo defines both foo@v1 and foo@@v2, >>>> dlsym(RTLD_NEXT, "foo") returns foo@v1 while dlsym(RTLD_DEFAULT, "foo") >>>> returns foo@@v2. The issue is that RTLD_DEFAULT use the >>>> DL_LOOKUP_RETURN_NEWEST flag while RTLD_NEXT doesn't. Fix the RTLD_NEXT >>>> branch to use DL_LOOKUP_RETURN_NEWEST. >>> >>> I am afraid we will need to add a compat dlsym for this change. >> >> My working theory is dlsym with RTLD_NEXT with a versioned symbol is >> so buggy that this is not needed. I want to understand the nature of >> the bug, and plan to post a write-up. > > Ok, thanks. > > FWIW: I have verified that on FreeBSD 13.1, both dlsym(RTLD_NEXT, > "foo") and dlsym(RTLD_DEFAULT, "foo") resolve to the default version > definition, like what this patch does. Right, I agree this is the most reasonable way to handle RTLD_NEXT with versioned symbols specially because current semantic is to return the *oldest* version with not apparent reason. I guess that if caller wants an old version it can use dlvsym with the appropriated version. LGTM as well with Florian's remarks fixed. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
On 2022-05-27, Florian Weimer wrote: >I've been looking at this for a while. > >We currently have this code in elf/dl-lookup.c: > > /* No specific version is selected. There are two ways we > can got here: > > - a binary which does not include versioning information > is loaded > > - dlsym() instead of dlvsym() is used to get a symbol which > might exist in more than one form > > If the library does not provide symbol version information > there is no problem at all: we simply use the symbol if it > is defined. > > These two lookups need to be handled differently if the > library defines versions. In the case of the old > unversioned application the oldest (default) version > should be used. In case of a dlsym() call the latest and > public interface should be returned. */ > if (verstab != NULL) > { > if ((verstab[symidx] & 0x7fff) > >= ((flags & DL_LOOKUP_RETURN_NEWEST) ? 2 : 3)) > { > /* Don't accept hidden symbols. */ > if ((verstab[symidx] & 0x8000) == 0 > && (*num_versions)++ == 0) > /* No version so far. */ > *versioned_sym = sym; > > return NULL; > } > } > >The numbers 2 and 3 look suspicious. The condition involving >num_versions ensures that we only store the first matching symbol in >*versioned_sym. But the skipped version indices are not special. >Indices are not specific to individual symbols, so it is no clear that >index 2 is special and contains the oldest possible version for that >particular symbol, and the next index will have the latest version. > >Aligning dlvsym with dlsym still makes sense, so I suggest to proceed >with this patch. But I think there are more bugs in this area. > >> diff --git a/elf/nextmod3.c b/elf/nextmod3.c >> new file mode 100644 >> index 0000000000..96608a65c0 >> --- /dev/null >> +++ b/elf/nextmod3.c >> @@ -0,0 +1,19 @@ >> +int >> +foo_v1 (int a) >> +{ >> + return 1; >> +} >> +asm (".symver foo_v1, foo@v1"); >> + >> +int >> +foo_v2 (int a) >> +{ >> + return 2; >> +} >> +asm (".symver foo_v2, foo@v2"); >> + >> +int >> +foo (int a) >> +{ >> + return 3; >> +} > >Please set foo@@v3 explicitly. Ack. Having asm (".symver foo, foo@@@v3") is clearer, though it is redundant since the version script specifies foo in the v3 version node. >> diff --git a/elf/nextmod3.map b/elf/nextmod3.map >> new file mode 100644 >> index 0000000000..0a8e4e4ee3 >> --- /dev/null >> +++ b/elf/nextmod3.map >> @@ -0,0 +1,3 @@ >> +v1 { }; >> +v2 { }; >> +v3 { foo; }; > >These versions are not ordered. Maybe use this instead? > >v1 { }; >v2 { } v1; >v3 { foo; } v2; The dependencies are a no-op in lld but tracks version definition dependencies in GNU ld and gold (see readelf -V output: `Parent 1`). The vd_cnt member is ignored in glibc and FreeBSD rtld, so omitting it should be fine. That said, I'll add it...
On 2022-05-27, Fangrui Song wrote: >On 2022-05-27, Florian Weimer wrote: >>I've been looking at this for a while. >> >>We currently have this code in elf/dl-lookup.c: >> >> /* No specific version is selected. There are two ways we >> can got here: >> >> - a binary which does not include versioning information >> is loaded >> >> - dlsym() instead of dlvsym() is used to get a symbol which >> might exist in more than one form >> >> If the library does not provide symbol version information >> there is no problem at all: we simply use the symbol if it >> is defined. >> >> These two lookups need to be handled differently if the >> library defines versions. In the case of the old >> unversioned application the oldest (default) version >> should be used. In case of a dlsym() call the latest and >> public interface should be returned. */ >> if (verstab != NULL) >> { >> if ((verstab[symidx] & 0x7fff) >> >= ((flags & DL_LOOKUP_RETURN_NEWEST) ? 2 : 3)) >> { >> /* Don't accept hidden symbols. */ >> if ((verstab[symidx] & 0x8000) == 0 >> && (*num_versions)++ == 0) >> /* No version so far. */ >> *versioned_sym = sym; >> >> return NULL; >> } >> } >> >>The numbers 2 and 3 look suspicious. The condition involving >>num_versions ensures that we only store the first matching symbol in >>*versioned_sym. But the skipped version indices are not special. >>Indices are not specific to individual symbols, so it is no clear that >>index 2 is special and contains the oldest possible version for that >>particular symbol, and the next index will have the latest version. >> >>Aligning dlvsym with dlsym still makes sense, so I suggest to proceed >>with this patch. But I think there are more bugs in this area. I suspect the code (from 199x) has a hidden assumption that for a stem symbol (say foo), the .dynsym entries (say foo@v1, foo@v2, foo@@v3) are ordered by the version node indices (in .gnu.version_d). If so: * (flags & DL_LOOKUP_RETURN_NEWEST) == 0 => the symbol with verstab[symidx]==2 will skip the check and be returned * (flags & DL_LOOKUP_RETURN_NEWEST) != 0 => only the default version symbol gets assigned in `*versioned_sym = sym` Unfortunately, this assumption hold for none of GNU ld, gold, and ld.lld (seems unrelated to --hash-style=): ``` % ld.bfd @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ 5: 0000000000001100 6 FUNC GLOBAL DEFAULT 13 foo@v1 8: 0000000000001120 6 FUNC GLOBAL DEFAULT 13 foo@@v3 9: 0000000000001110 6 FUNC GLOBAL DEFAULT 13 foo@v2 % gold @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ 7: 00000000000007c0 6 FUNC GLOBAL DEFAULT 14 foo@@v3 9: 00000000000007a0 6 FUNC GLOBAL DEFAULT 14 foo@v1 10: 00000000000007b0 6 FUNC GLOBAL DEFAULT 14 foo@v2 % ld.lld @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ 7: 0000000000001740 6 FUNC GLOBAL DEFAULT 13 foo@@v3 8: 0000000000001720 6 FUNC GLOBAL DEFAULT 13 foo@v1 9: 0000000000001730 6 FUNC GLOBAL DEFAULT 13 foo@v2 ``` I think the simplification https://sourceware.org/pipermail/libc-alpha/2022-May/138304.html ([PATCH] elf: Remove one-default-version check when searching an unversioned symbol) can proceed, but a bug fix will still be needed. >>>diff --git a/elf/nextmod3.c b/elf/nextmod3.c >>>new file mode 100644 >>>index 0000000000..96608a65c0 >>>--- /dev/null >>>+++ b/elf/nextmod3.c >>>@@ -0,0 +1,19 @@ >>>+int >>>+foo_v1 (int a) >>>+{ >>>+ return 1; >>>+} >>>+asm (".symver foo_v1, foo@v1"); >>>+ >>>+int >>>+foo_v2 (int a) >>>+{ >>>+ return 2; >>>+} >>>+asm (".symver foo_v2, foo@v2"); >>>+ >>>+int >>>+foo (int a) >>>+{ >>>+ return 3; >>>+} >> >>Please set foo@@v3 explicitly. > >Ack. Having asm (".symver foo, foo@@@v3") is clearer, though it is >redundant since the version script specifies foo in the v3 version node. > >>>diff --git a/elf/nextmod3.map b/elf/nextmod3.map >>>new file mode 100644 >>>index 0000000000..0a8e4e4ee3 >>>--- /dev/null >>>+++ b/elf/nextmod3.map >>>@@ -0,0 +1,3 @@ >>>+v1 { }; >>>+v2 { }; >>>+v3 { foo; }; >> >>These versions are not ordered. Maybe use this instead? >> >>v1 { }; >>v2 { } v1; >>v3 { foo; } v2; > >The dependencies are a no-op in lld but tracks version definition >dependencies in GNU ld and gold (see readelf -V output: `Parent 1`). > >The vd_cnt member is ignored in glibc and FreeBSD rtld, so omitting it >should be fine. That said, I'll add it...
* Fangrui Song: > I suspect the code (from 199x) has a hidden assumption that for a stem > symbol (say foo), the .dynsym entries (say foo@v1, foo@v2, foo@@v3) are > ordered by the version node indices (in .gnu.version_d). Right, the code clearly has this assumption. There has to be some ordering because we don't want to do a topological sort to find the lowest and highest versions. > If so: > > * (flags & DL_LOOKUP_RETURN_NEWEST) == 0 => the symbol with > verstab[symidx]==2 will skip the check and be returned > * (flags & DL_LOOKUP_RETURN_NEWEST) != 0 => only the default version > symbol gets assigned in `*versioned_sym = sym` > > Unfortunately, this assumption hold for none of GNU ld, gold, and > ld.lld (seems unrelated to --hash-style=): > > ``` > % ld.bfd @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ > 5: 0000000000001100 6 FUNC GLOBAL DEFAULT 13 foo@v1 > 8: 0000000000001120 6 FUNC GLOBAL DEFAULT 13 foo@@v3 > 9: 0000000000001110 6 FUNC GLOBAL DEFAULT 13 foo@v2 > % gold @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ > 7: 00000000000007c0 6 FUNC GLOBAL DEFAULT 14 foo@@v3 > 9: 00000000000007a0 6 FUNC GLOBAL DEFAULT 14 foo@v1 > 10: 00000000000007b0 6 FUNC GLOBAL DEFAULT 14 foo@v2 > % ld.lld @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ > 7: 0000000000001740 6 FUNC GLOBAL DEFAULT 13 foo@@v3 > 8: 0000000000001720 6 FUNC GLOBAL DEFAULT 13 foo@v1 > 9: 0000000000001730 6 FUNC GLOBAL DEFAULT 13 foo@v2 The order of symbols in the symbol table is not what counts here, it's the order in the list for the hash bucket. I'm not sure if it's possible to dump this with readelf. I'm writing a custom dumper for this. Thanks, Florian
On 2022-05-27, Florian Weimer wrote: >* Fangrui Song: > >> I suspect the code (from 199x) has a hidden assumption that for a stem >> symbol (say foo), the .dynsym entries (say foo@v1, foo@v2, foo@@v3) are >> ordered by the version node indices (in .gnu.version_d). > >Right, the code clearly has this assumption. There has to be some >ordering because we don't want to do a topological sort to find the >lowest and highest versions. > >> If so: >> >> * (flags & DL_LOOKUP_RETURN_NEWEST) == 0 => the symbol with >> verstab[symidx]==2 will skip the check and be returned >> * (flags & DL_LOOKUP_RETURN_NEWEST) != 0 => only the default version >> symbol gets assigned in `*versioned_sym = sym` >> >> Unfortunately, this assumption hold for none of GNU ld, gold, and >> ld.lld (seems unrelated to --hash-style=): >> >> ``` >> % ld.bfd @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ >> 5: 0000000000001100 6 FUNC GLOBAL DEFAULT 13 foo@v1 >> 8: 0000000000001120 6 FUNC GLOBAL DEFAULT 13 foo@@v3 >> 9: 0000000000001110 6 FUNC GLOBAL DEFAULT 13 foo@v2 >> % gold @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ >> 7: 00000000000007c0 6 FUNC GLOBAL DEFAULT 14 foo@@v3 >> 9: 00000000000007a0 6 FUNC GLOBAL DEFAULT 14 foo@v1 >> 10: 00000000000007b0 6 FUNC GLOBAL DEFAULT 14 foo@v2 >> % ld.lld @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ >> 7: 0000000000001740 6 FUNC GLOBAL DEFAULT 13 foo@@v3 >> 8: 0000000000001720 6 FUNC GLOBAL DEFAULT 13 foo@v1 >> 9: 0000000000001730 6 FUNC GLOBAL DEFAULT 13 foo@v2 > >The order of symbols in the symbol table is not what counts here, it's >the order in the list for the hash bucket. > >I'm not sure if it's possible to dump this with readelf. I'm writing a >custom dumper for this. > >Thanks, >Florian > llvm-readelf --gnu-hash-table can dump the chain array (`Values:`), with the size equal to the number of defined symbols. The odd numbers indicate the end of a chain. I believe the .gnu.hash lookup order is in increasing symbol index (in all of musl, glibc, FreeBSD rtld). So the glibc (flags & DL_LOOKUP_RETURN_NEWEST) == 0 code path (for relocations) is wrong...
On 27/05/2022 18:03, Fangrui Song via Libc-alpha wrote: > On 2022-05-27, Florian Weimer wrote: >> * Fangrui Song: >> >>> I suspect the code (from 199x) has a hidden assumption that for a stem >>> symbol (say foo), the .dynsym entries (say foo@v1, foo@v2, foo@@v3) are >>> ordered by the version node indices (in .gnu.version_d). >> >> Right, the code clearly has this assumption. There has to be some >> ordering because we don't want to do a topological sort to find the >> lowest and highest versions. >> >>> If so: >>> >>> * (flags & DL_LOOKUP_RETURN_NEWEST) == 0 => the symbol with >>> verstab[symidx]==2 will skip the check and be returned >>> * (flags & DL_LOOKUP_RETURN_NEWEST) != 0 => only the default version >>> symbol gets assigned in `*versioned_sym = sym` >>> >>> Unfortunately, this assumption hold for none of GNU ld, gold, and >>> ld.lld (seems unrelated to --hash-style=): >>> >>> ``` >>> % ld.bfd @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ >>> 5: 0000000000001100 6 FUNC GLOBAL DEFAULT 13 foo@v1 >>> 8: 0000000000001120 6 FUNC GLOBAL DEFAULT 13 foo@@v3 >>> 9: 0000000000001110 6 FUNC GLOBAL DEFAULT 13 foo@v2 >>> % gold @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ >>> 7: 00000000000007c0 6 FUNC GLOBAL DEFAULT 14 foo@@v3 >>> 9: 00000000000007a0 6 FUNC GLOBAL DEFAULT 14 foo@v1 >>> 10: 00000000000007b0 6 FUNC GLOBAL DEFAULT 14 foo@v2 >>> % ld.lld @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ >>> 7: 0000000000001740 6 FUNC GLOBAL DEFAULT 13 foo@@v3 >>> 8: 0000000000001720 6 FUNC GLOBAL DEFAULT 13 foo@v1 >>> 9: 0000000000001730 6 FUNC GLOBAL DEFAULT 13 foo@v2 >> >> The order of symbols in the symbol table is not what counts here, it's >> the order in the list for the hash bucket. >> >> I'm not sure if it's possible to dump this with readelf. I'm writing a >> custom dumper for this. >> >> Thanks, >> Florian >> > llvm-readelf --gnu-hash-table can dump the chain array (`Values:`), with > the size equal to the number of defined symbols. The odd numbers > indicate the end of a chain. > > I believe the .gnu.hash lookup order is in increasing symbol index (in > all of musl, glibc, FreeBSD rtld). > So the glibc (flags & DL_LOOKUP_RETURN_NEWEST) == 0 code path (for > relocations) is wrong... My understanding is (flags & DL_LOOKUP_RETURN_NEWEST) == 0 is only used for dlvsym and I think we never documented properly hwo dlvsym (handle, symbol, "") should behave. Currently it is different than dlsym (handle, symbol) (it will return the a versioned symbol), while with your patch it will be similar to dlsym.
On 2022-05-27, Adhemerval Zanella wrote: > > >On 27/05/2022 18:03, Fangrui Song via Libc-alpha wrote: >> On 2022-05-27, Florian Weimer wrote: >>> * Fangrui Song: >>> >>>> I suspect the code (from 199x) has a hidden assumption that for a stem >>>> symbol (say foo), the .dynsym entries (say foo@v1, foo@v2, foo@@v3) are >>>> ordered by the version node indices (in .gnu.version_d). >>> >>> Right, the code clearly has this assumption. There has to be some >>> ordering because we don't want to do a topological sort to find the >>> lowest and highest versions. >>> >>>> If so: >>>> >>>> * (flags & DL_LOOKUP_RETURN_NEWEST) == 0 => the symbol with >>>> verstab[symidx]==2 will skip the check and be returned >>>> * (flags & DL_LOOKUP_RETURN_NEWEST) != 0 => only the default version >>>> symbol gets assigned in `*versioned_sym = sym` >>>> >>>> Unfortunately, this assumption hold for none of GNU ld, gold, and >>>> ld.lld (seems unrelated to --hash-style=): >>>> >>>> ``` >>>> % ld.bfd @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ >>>> 5: 0000000000001100 6 FUNC GLOBAL DEFAULT 13 foo@v1 >>>> 8: 0000000000001120 6 FUNC GLOBAL DEFAULT 13 foo@@v3 >>>> 9: 0000000000001110 6 FUNC GLOBAL DEFAULT 13 foo@v2 >>>> % gold @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ >>>> 7: 00000000000007c0 6 FUNC GLOBAL DEFAULT 14 foo@@v3 >>>> 9: 00000000000007a0 6 FUNC GLOBAL DEFAULT 14 foo@v1 >>>> 10: 00000000000007b0 6 FUNC GLOBAL DEFAULT 14 foo@v2 >>>> % ld.lld @response.txt && readelf -W --dyn-syms nextmod3.so | grep foo@ >>>> 7: 0000000000001740 6 FUNC GLOBAL DEFAULT 13 foo@@v3 >>>> 8: 0000000000001720 6 FUNC GLOBAL DEFAULT 13 foo@v1 >>>> 9: 0000000000001730 6 FUNC GLOBAL DEFAULT 13 foo@v2 >>> >>> The order of symbols in the symbol table is not what counts here, it's >>> the order in the list for the hash bucket. >>> >>> I'm not sure if it's possible to dump this with readelf. I'm writing a >>> custom dumper for this. >>> >>> Thanks, >>> Florian >>> >> llvm-readelf --gnu-hash-table can dump the chain array (`Values:`), with >> the size equal to the number of defined symbols. The odd numbers >> indicate the end of a chain. >> >> I believe the .gnu.hash lookup order is in increasing symbol index (in >> all of musl, glibc, FreeBSD rtld). >> So the glibc (flags & DL_LOOKUP_RETURN_NEWEST) == 0 code path (for >> relocations) is wrong... OK, I was wrong. Re-reading, I think the code is correct but confusing. The following example tests a relocation (flags = DL_LOOKUP_ADD_DEPENDENCY | DL_LOOKUP_FOR_RELOCATE; note there is no DL_LOOKUP_RETURN_NEWEST): printf > ./a.c %s ' #define _GNU_SOURCE // workaround before 748df8126ac69e68e0b94e236ea3c2e11b1176cb #include <dlfcn.h> #include <stdio.h> int foo(int a); int main(void) { int res = foo (0); printf ("foo (0) = %d, %s\n", res, res == 0 ? "ok" : "wrong"); /* Resolve to foo@@v3 in nextmod3.so, instead of foo@v1 or foo@v2. */ int (*fp) (int) = dlsym (RTLD_DEFAULT, "foo"); res = fp (0); printf ("foo (0) = %d, %s\n", res, res == 3 ? "ok" : "wrong"); /* Resolve to foo@@v3 in nextmod3.so, instead of foo@v1 or foo@v2. */ fp = dlsym (RTLD_NEXT, "foo"); res = fp (0); printf ("foo (0) = %d, %s\n", res, res == 3 ? "ok" : "wrong"); } ' echo 'int foo(int a) { return -1; }' > ./b0.c printf > ./b.c %s ' int foo_va(int a) { return 0; } asm(".symver foo_va, foo@va, remove"); int foo_v1(int a) { return 1; } asm(".symver foo_v1, foo@v1, remove"); int foo_v2(int a) { return 2; } asm(".symver foo_v2, foo@v2, remove"); int foo(int a) { return 3; } asm(".symver foo, foo@@@v3"); ' echo 'va {}; v1 {} va; v2 {} v1; v3 {} v2;' > ./b.ver sed 's/^ /\t/g' > ./Makefile <<'eof' .MAKE.MODE := meta curdirOk=1 CFLAGS = -fpic -g a: a.o b0.so b.so $(CC) -Wl,--no-as-needed a.o b0.so -ldl -Wl,-rpath=$$PWD -o $@ b0.so: b0.o $(CC) $> -shared -Wl,--soname=b.so -o $@ b.so: b.o $(CC) $> -shared -Wl,--soname=b.so,--version-script=b.ver -o $@ clean: rm -f a *.so *.o *.meta eof Use `bmake` to build (meta mode ensures changing the command line will cause a re-build). It will be nice the enhance tst-next-ver.c with the `foo` relocation test but my glibc-Makefile-fu isn't great... > >My understanding is (flags & DL_LOOKUP_RETURN_NEWEST) == 0 is only used for >dlvsym and I think we never documented properly hwo dlvsym (handle, symbol, "") >should behave. Currently it is different than dlsym (handle, symbol) (it will >return the a versioned symbol), while with your patch it will be similar to dlsym. It's for both dlvsym and relocation resolving. https://refspecs.linuxfoundation.org/LSB_5.0.0/LSB-Core-generic/LSB-Core-generic.html "10.7.6 Symbol Resolution" says: > The object with the reference does not use versioning, while the object > with the defin- > itions does. In this instance, only the definitions with index numbers 1 > and 2 will be > used in the reference match, the same identified by the static linker as > the base defini- > tion. In cases where the static linker was not used, such as in calls to > dlopen(), a > version that does not have the base definition index shall be acceptable > if it is the only > version for which the symbol is defined. % readelf -WV b.so ... Version definition section '.gnu.version_d' contains 5 entries: Addr: 0x0000000000000374 Offset: 0x000374 Link: 6 (.dynstr) 000000: Rev: 1 Flags: BASE Index: 1 Cnt: 1 Name: b.so 0x001c: Rev: 1 Flags: none Index: 2 Cnt: 1 Name: va 0x0038: Rev: 1 Flags: none Index: 3 Cnt: 1 Name: v1 0x0054: Rev: 1 Flags: none Index: 4 Cnt: 1 Name: v2 0x0070: Rev: 1 Flags: none Index: 5 Cnt: 1 Name: v3 https://maskray.me/blog/2020-11-26-all-about-symbol-versioning#rtld-behavior has my updated understanding of the behavior.
diff --git a/elf/Makefile b/elf/Makefile index 6f4ea78007..6cc0023364 100644 --- a/elf/Makefile +++ b/elf/Makefile @@ -425,6 +425,7 @@ tests += \ tst-initorder2 \ tst-latepthread \ tst-main1 \ + tst-next-ver \ tst-nodelete2 \ tst-nodelete-dlclose \ tst-nodelete-opened \ @@ -711,6 +712,7 @@ modules-names += \ neededobj6 \ nextmod1 \ nextmod2 \ + nextmod3 \ nodel2mod1 \ nodel2mod2 \ nodel2mod3 \ @@ -1689,6 +1691,9 @@ $(objpfx)reldep4.out: $(objpfx)reldep4mod1.so $(objpfx)reldep4mod2.so $(objpfx)next: $(objpfx)nextmod1.so $(objpfx)nextmod2.so LDFLAGS-next = -Wl,--no-as-needed +$(objpfx)tst-next-ver: $(objpfx)nextmod3.so +LDFLAGS-tst-next-ver = -Wl,--no-as-needed + $(objpfx)unload2.out: $(objpfx)unload2mod.so $(objpfx)unload2dep.so $(objpfx)lateglobal.out: $(objpfx)ltglobmod1.so $(objpfx)ltglobmod2.so @@ -2436,6 +2441,8 @@ $(objpfx)tst-linkall-static: \ endif endif +LDFLAGS-nextmod3.so = -Wl,--version-script=nextmod3.map + # The application depends on the DSO, and the DSO loads the plugin. # The plugin also depends on the DSO. This creates the circular # dependency via dlopen that we're testing to make sure works. diff --git a/elf/dl-sym.c b/elf/dl-sym.c index aa993942df..b1cf42f36d 100644 --- a/elf/dl-sym.c +++ b/elf/dl-sym.c @@ -144,7 +144,7 @@ RTLD_NEXT used in code not dynamically loaded")); l = l->l_loader; result = GLRO(dl_lookup_symbol_x) (name, match, &ref, l->l_local_scope, - vers, 0, 0, match); + vers, 0, flags, match); } else { diff --git a/elf/nextmod3.c b/elf/nextmod3.c new file mode 100644 index 0000000000..96608a65c0 --- /dev/null +++ b/elf/nextmod3.c @@ -0,0 +1,19 @@ +int +foo_v1 (int a) +{ + return 1; +} +asm (".symver foo_v1, foo@v1"); + +int +foo_v2 (int a) +{ + return 2; +} +asm (".symver foo_v2, foo@v2"); + +int +foo (int a) +{ + return 3; +} diff --git a/elf/nextmod3.map b/elf/nextmod3.map new file mode 100644 index 0000000000..0a8e4e4ee3 --- /dev/null +++ b/elf/nextmod3.map @@ -0,0 +1,3 @@ +v1 { }; +v2 { }; +v3 { foo; }; diff --git a/elf/tst-next-ver.c b/elf/tst-next-ver.c new file mode 100644 index 0000000000..7241f9038b --- /dev/null +++ b/elf/tst-next-ver.c @@ -0,0 +1,46 @@ +/* Test RTLD_DEFAULT/RTLD_NEXT when the definition has multiple versions. + Copyright (C) 2018-2022 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <dlfcn.h> +#include <stdio.h> + +#include "testobj.h" + +static int +do_test (void) +{ + /* Resolve to foo@@v3 in nextmod3.so, instead of + foo@v1 or foo@v2. */ + int (*fp) (int) = dlsym (RTLD_DEFAULT, "foo"); + int res = fp (0); + printf ("preload (0) = %d, %s\n", res, res == 3 ? "ok" : "wrong"); + if (res != 3) + return 1; + + /* Resolve to foo@@v3 in nextmod3.so, instead of + foo@v1 or foo@v2. */ + fp = dlsym (RTLD_NEXT, "foo"); + res = fp (0); + printf ("preload (0) = %d, %s\n", res, res == 3 ? "ok" : "wrong"); + if (res != 3) + return 1; + + return 0; +} + +#include <support/test-driver.c>