[PATCH] dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932]

[PATCH] dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932]

[Fangrui Song]

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.
---
 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>
-- 
2.36.1.124.g0e6072fb45-goog

Re: [PATCH] dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932]

[Adhemerval Zanella]

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>

Re: [PATCH] dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932]

[Florian Weimer]

* 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.

Re: [PATCH] dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932]

[Fāng-ruì Sòng]

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.

Re: [PATCH] dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932]

[Florian Weimer]

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

Re: [PATCH] dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932]

[Adhemerval Zanella]

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>

Re: [PATCH] dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932]

[Fangrui Song]

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...

Re: [PATCH] dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932]

[Fangrui Song]

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...

Re: [PATCH] dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932]

[Florian Weimer]

* 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

Re: [PATCH] dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932]

[Fangrui Song]

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...

Re: [PATCH] dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932]

[Adhemerval Zanella]

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.

Re: [PATCH] dlsym: Make RTLD_NEXT prefer default version definition [#BZ #14932]

[Fangrui Song]

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.