elf: Fix handling of symbol versions which hash to zero (bug 29190)

  This was found through code inspection.  No application impact is
known.

---
 elf/Makefile                          | 26 +++++++++++++++++
 elf/dl-lookup.c                       | 22 ++++++++++----
 elf/dl-version.c                      |  7 +++++
 elf/tst-version-hash-zero-linkmod.c   | 22 ++++++++++++++
 elf/tst-version-hash-zero-linkmod.map |  7 +++++
 elf/tst-version-hash-zero-mod.c       | 20 +++++++++++++
 elf/tst-version-hash-zero-mod.map     | 13 +++++++++
 elf/tst-version-hash-zero-refmod.c    | 23 +++++++++++++++
 elf/tst-version-hash-zero.c           | 55 +++++++++++++++++++++++++++++++++++
 9 files changed, 189 insertions(+), 6 deletions(-)
  

On 2022-05-27, Florian Weimer via Libc-alpha wrote:
>This was found through code inspection.  No application impact is
>known.
>
>---
> elf/Makefile                          | 26 +++++++++++++++++
> elf/dl-lookup.c                       | 22 ++++++++++----
> elf/dl-version.c                      |  7 +++++
> elf/tst-version-hash-zero-linkmod.c   | 22 ++++++++++++++
> elf/tst-version-hash-zero-linkmod.map |  7 +++++
> elf/tst-version-hash-zero-mod.c       | 20 +++++++++++++
> elf/tst-version-hash-zero-mod.map     | 13 +++++++++
> elf/tst-version-hash-zero-refmod.c    | 23 +++++++++++++++
> elf/tst-version-hash-zero.c           | 55 +++++++++++++++++++++++++++++++++++
> 9 files changed, 189 insertions(+), 6 deletions(-)
>
>diff --git a/elf/Makefile b/elf/Makefile
>index 838fe39afb..f141eedfb0 100644
>--- a/elf/Makefile
>+++ b/elf/Makefile
>@@ -466,6 +466,7 @@ tests += \
>   tst-unique2 \
>   tst-unwind-ctor \
>   tst-unwind-main \
>+  tst-version-hash-zero \
>   unload3 \
>   unload4 \
>   unload5 \
>@@ -913,6 +914,9 @@ modules-names += \
>   tst-unique2mod1 \
>   tst-unique2mod2 \
>   tst-unwind-ctor-lib \
>+  tst-version-hash-zero-linkmod \
>+  tst-version-hash-zero-mod \
>+  tst-version-hash-zero-refmod \
>   unload2dep \
>   unload2mod \
>   unload3mod1 \
>@@ -2960,3 +2964,25 @@ $(objpfx)tst-tls-allocation-failure-static-patched.out: \
> 	grep -q '^Fatal glibc error: Cannot allocate TLS block$$' $@ \
> 	  && grep -q '^status: 127$$' $@; \
> 	  $(evaluate-test)
>+
>+$(objpfx)tst-version-hash-zero.out: \
>+  $(objpfx)tst-version-hash-zero-mod.so \
>+  $(objpfx)tst-version-hash-zero-refmod.so
>+$(objpfx)tst-version-hash-zero-mod.so: $(objpfx)tst-version-hash-zero-mod.os \
>+  tst-version-hash-zero-mod.map
>+	$(LINK.o) -shared -o $@ $(LDFLAGS.so) $< \
>+	  -Wl,--version-script=tst-version-hash-zero-mod.map
>+# The run-time test module tst-version-hash-zero-refmod.so is linked
>+# to a stub module, tst-version-hash-zero-linkmod.so, to produce an
>+# expected relocation error.
>+$(objpfx)tst-version-hash-zero-refmod.so: \
>+  $(objpfx)tst-version-hash-zero-linkmod.so
>+$(objpfx)tst-version-hash-zero-linkmod.so:\
>+  $(objpfx)tst-version-hash-zero-linkmod.os \
>+  tst-version-hash-zero-linkmod.map
>+	$(LINK.o) -shared -o $@ $(LDFLAGS.so) $< \
>+	  -Wl,--version-script=tst-version-hash-zero-linkmod.map \
>+	  -Wl,--soname=tst-version-hash-zero-mod.so
>+$(objpfx)tst-version-hash-zero-refmod.so: \
>+  $(objpfx)tst-version-hash-zero-linkmod.so
>+tst-version-hash-zero-refmod.so-no-z-defs = yes
>diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
>index a42f6d5390..9abe96fd92 100644
>--- a/elf/dl-lookup.c
>+++ b/elf/dl-lookup.c
>@@ -114,12 +114,22 @@ check_match (const char *const undef_name,
> 	  /* We can match the version information or use the
> 	     default one if it is not hidden.  */
> 	  ElfW(Half) ndx = verstab[symidx] & 0x7fff;
>-	  if ((map->l_versions[ndx].hash != version->hash
>-	       || strcmp (map->l_versions[ndx].name, version->name))
>-	      && (version->hidden || map->l_versions[ndx].hash
>-		  || (verstab[symidx] & 0x8000)))
>-	    /* It's not the version we want.  */
>-	    return NULL;
>+	  if (map->l_versions[ndx].hash == version->hash
>+	      && strcmp (map->l_versions[ndx].name, version->name) == 0)
>+	    /* This is an exact version match.  Return the symbol below.  */
>+	    ;
>+	  else
>+	    {
>+	      if (!version->hidden
>+		  && map->l_versions[ndx].name[0] == '\0'
>+		  && (verstab[symidx] & 0x8000) == 0
>+		  && (*num_versions)++ == 0)

(*num_versions)++ == 0 can be removed.

map->l_versions[ndx].hash => map->l_versions[ndx].name[0] == '\0'
looks good to me.

I think version->hidden is to differentiate relocation resolving and
dlvsym. Since dlsym and dlvsym are differentiated by `version`,
we could introduce a flag to differentiate relocation resolving from
dlsym/dlvsym. Then version->hidden can be made clearer.

>+		/* This is the global default version.  Store it as a
>+		   fallback match.  */
>+		*versioned_sym = sym;
>+
>+	      return NULL;
>+	    }
> 	}
>     }
>   else
>diff --git a/elf/dl-version.c b/elf/dl-version.c
>index cda0889209..c10ead82fe 100644
>--- a/elf/dl-version.c
>+++ b/elf/dl-version.c
>@@ -357,6 +357,13 @@ _dl_check_map_versions (struct link_map *map, int verbose, int trace_mode)
> 	      ent = (ElfW(Verdef) *) ((char *) ent + ent->vd_next);
> 	    }
> 	}
>+
>+      /* The empty string has ELF hash zero.  This avoids a NULL check
>+	 before the version string comparison in check_match in
>+	 dl-lookup.c.  */
>+      for (unsigned int i = 0; i < map->l_nversions; ++i)
>+	if (map->l_versions[i].name == NULL)
>+	  map->l_versions[i].name = "";
>     }
>
>   /* When there is a DT_VERNEED entry with libc.so on DT_NEEDED, issue
>diff --git a/elf/tst-version-hash-zero-linkmod.c b/elf/tst-version-hash-zero-linkmod.c
>new file mode 100644
>index 0000000000..f71574bd2d
>--- /dev/null
>+++ b/elf/tst-version-hash-zero-linkmod.c
>@@ -0,0 +1,22 @@
>+/* Stub module for linking tst-version-hash-zero-refmod.so.
>+   Copyright (C) 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; see the file COPYING.LIB.  If
>+   not, see <https://www.gnu.org/licenses/>.  */
>+
>+/* The version script assigns a different symbol version for the stub
>+   module.  Loading the module with the incorrect version is expected
>+   to fail.  */
>+#include "tst-version-hash-zero-mod.c"
>diff --git a/elf/tst-version-hash-zero-linkmod.map b/elf/tst-version-hash-zero-linkmod.map
>new file mode 100644
>index 0000000000..2dba7c22d7
>--- /dev/null
>+++ b/elf/tst-version-hash-zero-linkmod.map
>@@ -0,0 +1,7 @@
>+Base {
>+  local: *;
>+};
>+
>+OTHER_VERSION {
>+  global: global_variable;
>+} Base;
>diff --git a/elf/tst-version-hash-zero-mod.c b/elf/tst-version-hash-zero-mod.c
>new file mode 100644
>index 0000000000..f99c020746
>--- /dev/null
>+++ b/elf/tst-version-hash-zero-mod.c
>@@ -0,0 +1,20 @@
>+/* Test module with a zero version symbol hash.
>+   Copyright (C) 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; see the file COPYING.LIB.  If
>+   not, see <https://www.gnu.org/licenses/>.  */
>+
>+/* The symbol version is assigned by version script.  */
>+int global_variable;
>diff --git a/elf/tst-version-hash-zero-mod.map b/elf/tst-version-hash-zero-mod.map
>new file mode 100644
>index 0000000000..41eaff7914
>--- /dev/null
>+++ b/elf/tst-version-hash-zero-mod.map
>@@ -0,0 +1,13 @@
>+Base {
>+  local: *;
>+};
>+
>+/* Define the version so that tst-version-hash-zero-refmod.so passes
>+   the initial symbol version check.  */
>+OTHER_VERSION {
>+} Base;
>+
>+/* This version string hashes to zero.  */
>+PPPPPPPPPPPP {
>+  global: global_variable;
>+} Base;
>diff --git a/elf/tst-version-hash-zero-refmod.c b/elf/tst-version-hash-zero-refmod.c
>new file mode 100644
>index 0000000000..29a9caaa40
>--- /dev/null
>+++ b/elf/tst-version-hash-zero-refmod.c
>@@ -0,0 +1,23 @@
>+/* Test module that triggers a relocation failure in tst-version-hash-zero.
>+   Copyright (C) 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; see the file COPYING.LIB.  If
>+   not, see <https://www.gnu.org/licenses/>.  */
>+
>+/* This is bound to global_variable@@OTHER_VERSION via
>+   tst-version-hash-zero-linkmod.so, but at run time, only
>+   global_variable@PPPPPPPPPPPP exists.  */
>+extern int global_variable;
>+int *pointer_variable = &global_variable;
>diff --git a/elf/tst-version-hash-zero.c b/elf/tst-version-hash-zero.c
>new file mode 100644
>index 0000000000..ad391bf202
>--- /dev/null
>+++ b/elf/tst-version-hash-zero.c
>@@ -0,0 +1,55 @@
>+/* Symbols with version hash zero should not match any version (bug 29190).
>+   Copyright (C) 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; see the file COPYING.LIB.  If
>+   not, see <https://www.gnu.org/licenses/>.  */
>+
>+#include <support/check.h>
>+#include <support/xdlfcn.h>
>+#include <stddef.h>
>+#include <string.h>
>+
>+static int
>+do_test (void)
>+{
>+  void *handle = xdlopen ("tst-version-hash-zero-mod.so", RTLD_NOW);
>+
>+  /* This used to crash because some struct r_found_version entries
>+     with hash zero did not have valid version strings.  */
>+  TEST_VERIFY (xdlvsym (handle, "global_variable", "PPPPPPPPPPPP") != NULL);
>+
>+  /* Consistency check.  */
>+  TEST_VERIFY (xdlsym (handle, "global_variable")
>+               == xdlvsym (handle, "global_variable", "PPPPPPPPPPPP"));
>+
>+  /* This symbol version is supposed to be missing.  */
>+  TEST_VERIFY (dlvsym (handle, "global_variable", "OTHER_VERSION") == NULL);
>+
>+  /* tst-version-hash-zero-refmod.so references
>+     global_variable@@OTHER_VERSION and is expected to fail to load.
>+     dlvsym sets the hidden flag during lookup.  Relocation does not,
>+     so this exercises a different failure case.  */
>+  TEST_VERIFY (dlopen ("tst-version-hash-zero-refmod.so", RTLD_NOW) == NULL);
>+  const char *message = dlerror ();
>+  if (strstr (message,
>+              ": undefined symbol: global_variable, version OTHER_VERSION")
>+      == NULL)
>+    FAIL_EXIT1 ("unexpected dlopen failure: %s", message);
>+
>+  xdlclose (handle);
>+  return 0;
>+}
>+
>+#include <support/test-driver.c>
>
  

* Fangrui Song:

>>diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
>>index a42f6d5390..9abe96fd92 100644
>>--- a/elf/dl-lookup.c
>>+++ b/elf/dl-lookup.c
>>@@ -114,12 +114,22 @@ check_match (const char *const undef_name,
>> 	  /* We can match the version information or use the
>> 	     default one if it is not hidden.  */
>> 	  ElfW(Half) ndx = verstab[symidx] & 0x7fff;
>>-	  if ((map->l_versions[ndx].hash != version->hash
>>-	       || strcmp (map->l_versions[ndx].name, version->name))
>>-	      && (version->hidden || map->l_versions[ndx].hash
>>-		  || (verstab[symidx] & 0x8000)))
>>-	    /* It's not the version we want.  */
>>-	    return NULL;
>>+	  if (map->l_versions[ndx].hash == version->hash
>>+	      && strcmp (map->l_versions[ndx].name, version->name) == 0)
>>+	    /* This is an exact version match.  Return the symbol below.  */
>>+	    ;
>>+	  else
>>+	    {
>>+	      if (!version->hidden
>>+		  && map->l_versions[ndx].name[0] == '\0'
>>+		  && (verstab[symidx] & 0x8000) == 0
>>+		  && (*num_versions)++ == 0)
>
> (*num_versions)++ == 0 can be removed.

That's not quite clear to me.  I kept it for consistency with the code
below.  But I can remove it.

> map->l_versions[ndx].hash => map->l_versions[ndx].name[0] == '\0'
> looks good to me.
>
> I think version->hidden is to differentiate relocation resolving and
> dlvsym. Since dlsym and dlvsym are differentiated by `version`,
> we could introduce a flag to differentiate relocation resolving from
> dlsym/dlvsym. Then version->hidden can be made clearer.

Given this is a versioned lookup, I'm not sure we want to different
behavior for dlvsym and dlsym.  So we should probably drop the condition
altogether.

Should I send a v2 with these changes?

Thanks,
Florian
  

On Wed, Jun 1, 2022 at 12:28 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Fangrui Song:
>
> >>diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
> >>index a42f6d5390..9abe96fd92 100644
> >>--- a/elf/dl-lookup.c
> >>+++ b/elf/dl-lookup.c
> >>@@ -114,12 +114,22 @@ check_match (const char *const undef_name,
> >>        /* We can match the version information or use the
> >>           default one if it is not hidden.  */
> >>        ElfW(Half) ndx = verstab[symidx] & 0x7fff;
> >>-       if ((map->l_versions[ndx].hash != version->hash
> >>-            || strcmp (map->l_versions[ndx].name, version->name))
> >>-           && (version->hidden || map->l_versions[ndx].hash
> >>-               || (verstab[symidx] & 0x8000)))
> >>-         /* It's not the version we want.  */
> >>-         return NULL;
> >>+       if (map->l_versions[ndx].hash == version->hash
> >>+           && strcmp (map->l_versions[ndx].name, version->name) == 0)
> >>+         /* This is an exact version match.  Return the symbol below.  */
> >>+         ;
> >>+       else
> >>+         {
> >>+           if (!version->hidden
> >>+               && map->l_versions[ndx].name[0] == '\0'
> >>+               && (verstab[symidx] & 0x8000) == 0
> >>+               && (*num_versions)++ == 0)
> >
> > (*num_versions)++ == 0 can be removed.
>
> That's not quite clear to me.  I kept it for consistency with the code
> below.  But I can remove it.
>
> > map->l_versions[ndx].hash => map->l_versions[ndx].name[0] == '\0'
> > looks good to me.
> >
> > I think version->hidden is to differentiate relocation resolving and
> > dlvsym. Since dlsym and dlvsym are differentiated by `version`,
> > we could introduce a flag to differentiate relocation resolving from
> > dlsym/dlvsym. Then version->hidden can be made clearer.
>
> Given this is a versioned lookup, I'm not sure we want to different
> behavior for dlvsym and dlsym.  So we should probably drop the condition
> altogether.

See my reply on
https://sourceware.org/pipermail/libc-alpha/2022-June/139283.html .
I think we should keep distinct behaviors for dlvsym and dlsym.

> Should I send a v2 with these changes?

If you drop   (*num_versions)++ == 0  , this can be committed as-is.

Reviewed-by: Fangrui Song <maskray@google.com>

> Thanks,
> Florian
>
  

* Fāng-ruì Sòng:

>> Should I send a v2 with these changes?
>
> If you drop   (*num_versions)++ == 0  , this can be committed as-is.
>
> Reviewed-by: Fangrui Song <maskray@google.com>

It doesn't work in the current code because

      /* If we have seen exactly one versioned symbol while we are
	 looking for an unversioned symbol and the version is not the
	 default version we still accept this symbol since there are
	 no possible ambiguities.  */
      sym = num_versions == 1 ? versioned_sym : NULL;

discards the found versioned_sym if num_versions is not incremented.

libio/tst-vtables-interposed and other tests fail as a result.

Thanks,
Florian
  

On 2022-06-02, Florian Weimer wrote:
>* Fāng-ruì Sòng:
>
>>> Should I send a v2 with these changes?
>>
>> If you drop   (*num_versions)++ == 0  , this can be committed as-is.
>>
>> Reviewed-by: Fangrui Song <maskray@google.com>

Sorry, undo.

>It doesn't work in the current code because
>
>      /* If we have seen exactly one versioned symbol while we are
>	 looking for an unversioned symbol and the version is not the
>	 default version we still accept this symbol since there are
>	 no possible ambiguities.  */
>      sym = num_versions == 1 ? versioned_sym : NULL;
>
>discards the found versioned_sym if num_versions is not incremented.
>
>libio/tst-vtables-interposed and other tests fail as a result.

I think this code should be fixed this way (splitting the complex ||):

          /* We can match the version information or use the
              default one if it is not hidden.  */
           ElfW(Half) ndx = verstab[symidx] & 0x7fff;
           if (map->l_versions[ndx].hash == version->hash
               && strcmp (map->l_versions[ndx].name, version->name) == 0)
             /* This is an exact version match.  Return the symbol below.  */
             ;
           else if (version->hidden || ndx > VER_NDX_GLOBAL)
             return NULL;
         }

dlvsym (version->hidden == true) requires exact match.

Relocation binding (version->hidden == false) uses exact match or can bind to VER_NDX_GLOBAL.

---

There should be no num_versions or versioned_sym in this code block.

VER_NDX_GLOBAL can't be a non-default version in ld, so 0x8000 test is unneeded.