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

[Fangrui Song <maskray@google.com>]

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