Fix treatment of symbol versions with unused as-needed libraries.
authorCary Coutant <ccoutant@gmail.com>
Thu, 21 Jun 2018 20:51:16 +0000 (13:51 -0700)
committerCary Coutant <ccoutant@gmail.com>
Thu, 21 Jun 2018 20:54:16 +0000 (13:54 -0700)
When we have a weak reference to a symbol defined in an
as-needed library, and that library ends up not-needed, gold
simply clears the version information in the symbol table, even
if the symbol could have been resolved by a needed library later
in the link order. This results in a loss of version information,
which can cause the program to bind to the wrong version at run
time.

This patch lets a dynamic definition override an earlier one if
the earlier one is from a not-needed library, so that we can
retain the version information from the binding to the needed
library. In order to do that, the tracking of needed/not-needed
had to be moved up to symbol resolution time, instead of during
Symbol_table::set_dynsym_indexes().

In cases where we still end up discarding version information,
I've added a warning.

For the original problem report and discussion, see:

https://stackoverflow.com/questions/50751421/undefined-behavior-in-shared-lib-using-libpthread-but-not-having-it-in-elf-as-d

gold/
* resolve.cc (Symbol_table::resolve): Rename tobinding to
orig_tobinding.  Call set_is_needed() for objects that resolve
non-weak references.
(Symbol_table::should_override): Allow a dynamic definition to
override an earlier one in a not-needed library.
* symtab.cc (Symbol_table::set_dynsym_indexes): Remove separate
processing for as-needed symbols.  Add warning when discarding
version informatin.
* testsuite/Makefile.am (weak_as_needed): New test case.
* testsuite/Makefile.in: Regenerate.
* testsuite/weak_as_needed.sh: New test script.
* testsuite/weak_as_needed_a.c: New source file.
* testsuite/weak_as_needed_b.c: New source file.
* testsuite/weak_as_needed_b.script: New version script.
* testsuite/weak_as_needed_c.c: New source file.
* testsuite/weak_as_needed_c.script: New version script.

gold/ChangeLog
gold/resolve.cc
gold/symtab.cc
gold/testsuite/Makefile.am
gold/testsuite/Makefile.in
gold/testsuite/weak_as_needed.sh [new file with mode: 0755]
gold/testsuite/weak_as_needed_a.c [new file with mode: 0644]
gold/testsuite/weak_as_needed_b.c [new file with mode: 0644]
gold/testsuite/weak_as_needed_b.script [new file with mode: 0644]
gold/testsuite/weak_as_needed_c.c [new file with mode: 0644]
gold/testsuite/weak_as_needed_c.script [new file with mode: 0644]

index 626bcdba830f549accf6ed4493252dc841f2f296..9297e6d249b78abd5272c4d2333fa9fbfe5d5682 100644 (file)
@@ -1,3 +1,22 @@
+2018-06-21  Cary Coutant  <ccoutant@gmail.com>
+
+       * resolve.cc (Symbol_table::resolve): Rename tobinding to
+       orig_tobinding.  Call set_is_needed() for objects that resolve
+       non-weak references.
+       (Symbol_table::should_override): Allow a dynamic definition to
+       override an earlier one in a not-needed library.
+       * symtab.cc (Symbol_table::set_dynsym_indexes): Remove separate
+       processing for as-needed symbols.  Add warning when discarding
+       version informatin.
+       * testsuite/Makefile.am (weak_as_needed): New test case.
+       * testsuite/Makefile.in: Regenerate.
+       * testsuite/weak_as_needed.sh: New test script.
+       * testsuite/weak_as_needed_a.c: New source file.
+       * testsuite/weak_as_needed_b.c: New source file.
+       * testsuite/weak_as_needed_b.script: New version script.
+       * testsuite/weak_as_needed_c.c: New source file.
+       * testsuite/weak_as_needed_c.script: New version script.
+
 2018-06-20  Cary Coutant  <ccoutant@gmail.com>
 
        PR gold/23268
index 4a5784cf8bd22a42835bd6d61ab80e39303ea1dc..ddd2bacc10a6b7b26efffb5de0fe4425061da615 100644 (file)
@@ -394,7 +394,7 @@ Symbol_table::resolve(Sized_symbol<size>* to,
                                    object, &adjust_common_sizes,
                                    &adjust_dyndef, is_default_version))
     {
-      elfcpp::STB tobinding = to->binding();
+      elfcpp::STB orig_tobinding = to->binding();
       typename Sized_symbol<size>::Value_type tovalue = to->value();
       this->override(to, sym, st_shndx, is_ordinary, object, version);
       if (adjust_common_sizes)
@@ -408,7 +408,7 @@ Symbol_table::resolve(Sized_symbol<size>* to,
        {
          // We are overriding an UNDEF or WEAK UNDEF with a DYN DEF.
          // Remember which kind of UNDEF it was for future reference.
-         to->set_undef_binding(tobinding);
+         to->set_undef_binding(orig_tobinding);
        }
     }
   else
@@ -431,6 +431,11 @@ Symbol_table::resolve(Sized_symbol<size>* to,
       to->override_visibility(sym.get_st_visibility());
     }
 
+  // If we have a non-WEAK reference from a regular object to a
+  // dynamic object, mark the dynamic object as needed.
+  if (to->is_from_dynobj() && to->in_reg() && !to->is_undef_binding_weak())
+    to->object()->set_is_needed();
+
   if (adjust_common_sizes && parameters->options().warn_common())
     {
       if (tosize > sym.get_st_size())
@@ -621,6 +626,13 @@ Symbol_table::should_override(const Symbol* to, unsigned int frombits,
           && to->version() == NULL
           && is_default_version)
         return true;
+      // Or, if the existing definition is in an unused --as-needed library,
+      // and the reference is weak, let the new definition override.
+      if (to->in_reg()
+         && to->is_undef_binding_weak()
+         && to->object()->as_needed()
+         && !to->object()->is_needed())
+       return true;
       return false;
 
     case UNDEF * 16 + DYN_DEF:
@@ -637,16 +649,12 @@ Symbol_table::should_override(const Symbol* to, unsigned int frombits,
 
     case COMMON * 16 + DYN_DEF:
     case WEAK_COMMON * 16 + DYN_DEF:
-    case DYN_COMMON * 16 + DYN_DEF:
-    case DYN_WEAK_COMMON * 16 + DYN_DEF:
       // Ignore a dynamic definition if we already have a common
       // definition.
       return false;
 
     case DEF * 16 + DYN_WEAK_DEF:
     case WEAK_DEF * 16 + DYN_WEAK_DEF:
-    case DYN_DEF * 16 + DYN_WEAK_DEF:
-    case DYN_WEAK_DEF * 16 + DYN_WEAK_DEF:
       // Ignore a weak dynamic definition if we already have a
       // definition.
       return false;
@@ -670,12 +678,25 @@ Symbol_table::should_override(const Symbol* to, unsigned int frombits,
 
     case COMMON * 16 + DYN_WEAK_DEF:
     case WEAK_COMMON * 16 + DYN_WEAK_DEF:
-    case DYN_COMMON * 16 + DYN_WEAK_DEF:
-    case DYN_WEAK_COMMON * 16 + DYN_WEAK_DEF:
       // Ignore a weak dynamic definition if we already have a common
       // definition.
       return false;
 
+    case DYN_COMMON * 16 + DYN_DEF:
+    case DYN_WEAK_COMMON * 16 + DYN_DEF:
+    case DYN_DEF * 16 + DYN_WEAK_DEF:
+    case DYN_WEAK_DEF * 16 + DYN_WEAK_DEF:
+    case DYN_COMMON * 16 + DYN_WEAK_DEF:
+    case DYN_WEAK_COMMON * 16 + DYN_WEAK_DEF:
+      // If the existing definition is in an unused --as-needed library,
+      // and the reference is weak, let a new dynamic definition override.
+      if (to->in_reg()
+         && to->is_undef_binding_weak()
+         && to->object()->as_needed()
+         && !to->object()->is_needed())
+       return true;
+      return false;
+
     case DEF * 16 + UNDEF:
     case WEAK_DEF * 16 + UNDEF:
     case UNDEF * 16 + UNDEF:
index 238834dce8899eaad3662070b6956b63a67f1839..c43d127507e6dc7b7f96ef37970e444e4ae617b0 100644 (file)
@@ -2543,8 +2543,6 @@ Symbol_table::set_dynsym_indexes(unsigned int index,
                                 Stringpool* dynpool,
                                 Versions* versions)
 {
-  std::vector<Symbol*> as_needed_sym;
-
   // First process all the symbols which have been forced to be local,
   // as they must appear before all global symbols.
   unsigned int forced_local_count = 0;
@@ -2611,15 +2609,6 @@ Symbol_table::set_dynsym_indexes(unsigned int index,
          syms->push_back(sym);
          dynpool->add(sym->name(), false, NULL);
 
-         // If the symbol is defined in a dynamic object and is
-         // referenced strongly in a regular object, then mark the
-         // dynamic object as needed.  This is used to implement
-         // --as-needed.
-         if (sym->is_from_dynobj()
-             && sym->in_reg()
-             && !sym->is_undef_binding_weak())
-           sym->object()->set_is_needed();
-
          // Record any version information, except those from
          // as-needed libraries not seen to be needed.  Note that the
          // is_needed state for such libraries can change in this loop.
@@ -2630,24 +2619,18 @@ Symbol_table::set_dynsym_indexes(unsigned int index,
                  || sym->object()->is_needed())
                versions->record_version(this, dynpool, sym);
              else
-               as_needed_sym.push_back(sym);
+               {
+                 gold_warning(_("discarding version information for "
+                                "%s@%s, defined in unused shared library %s "
+                                "(linked with --as-needed)"),
+                              sym->name(), sym->version(),
+                              sym->object()->name().c_str());
+                 sym->clear_version();
+               }
            }
        }
     }
 
-  // Process version information for symbols from as-needed libraries.
-  for (std::vector<Symbol*>::iterator p = as_needed_sym.begin();
-       p != as_needed_sym.end();
-       ++p)
-    {
-      Symbol* sym = *p;
-
-      if (sym->object()->is_needed())
-       versions->record_version(this, dynpool, sym);
-      else
-       sym->clear_version();
-    }
-
   // Finish up the versions.  In some cases this may add new dynamic
   // symbols.
   index = versions->finalize(this, index, syms);
index b88b02f17d26310f620dba09cefe9ee15572778c..ab3b25809a9b02cb40284f5f8854c98a0ec3adad 100644 (file)
@@ -1880,6 +1880,23 @@ ver_test_14.syms: ver_test_14
 ver_test_14: gcctestdir/ld ver_test_main.o ver_test_1.so ver_test_2.so ver_test_4.so ver_test_14.script
        $(CXXLINK) -Bgcctestdir/ -Wl,--version-script,$(srcdir)/ver_test_14.script -Wl,-E -Wl,-R,. ver_test_main.o ver_test_1.so ver_test_2.so ver_test_4.so
 
+check_SCRIPTS += weak_as_needed.sh
+check_DATA += weak_as_needed.stdout
+weak_as_needed.stdout: weak_as_needed_a.so
+       $(TEST_READELF) -dW --dyn-syms $< >$@
+weak_as_needed_a.so: gcctestdir/ld weak_as_needed_a.o weak_as_needed_b.so weak_as_needed_c.so
+       gcctestdir/ld -shared -rpath . -o $@ weak_as_needed_a.o --as-needed weak_as_needed_b.so weak_as_needed_c.so
+weak_as_needed_b.so: gcctestdir/ld weak_as_needed_b.o weak_as_needed_b.script
+       gcctestdir/ld -shared -rpath . -o $@ --version-script $(srcdir)/weak_as_needed_b.script weak_as_needed_b.o
+weak_as_needed_c.so: gcctestdir/ld weak_as_needed_c.o weak_as_needed_c.script
+       gcctestdir/ld -shared -rpath . -o $@ --version-script $(srcdir)/weak_as_needed_c.script weak_as_needed_c.o
+weak_as_needed_a.o: weak_as_needed_a.c
+       $(COMPILE) -c -fpic -o $@ $<
+weak_as_needed_b.o: weak_as_needed_b.c
+       $(COMPILE) -c -fpic -o $@ $<
+weak_as_needed_c.o: weak_as_needed_c.c
+       $(COMPILE) -c -fpic -o $@ $<
+
 check_PROGRAMS += protected_1
 protected_1_SOURCES = \
        protected_main_1.cc protected_main_2.cc protected_main_3.cc
index e1b1762ecb4242c64638b2ccbe27d665b64d7f12..14e4ce5c2c71bdae137db7f1f2fca573216184a2 100644 (file)
@@ -479,7 +479,8 @@ check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ ver_test_4.sh ver_test_5.sh \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ ver_test_7.sh ver_test_8.sh \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ ver_test_10.sh ver_test_13.sh \
-@GCC_TRUE@@NATIVE_LINKER_TRUE@ ver_test_14.sh relro_test.sh \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ ver_test_14.sh weak_as_needed.sh \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ relro_test.sh \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ ver_matching_test.sh \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ script_test_3.sh \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ script_test_4.sh \
@@ -534,7 +535,9 @@ check_PROGRAMS = $(am__EXEEXT_1) $(am__EXEEXT_2) $(am__EXEEXT_3) \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ ver_test_8_2.so.syms \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ ver_test_10.syms \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ ver_test_13.syms \
-@GCC_TRUE@@NATIVE_LINKER_TRUE@ ver_test_14.syms protected_3.err \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ ver_test_14.syms \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ weak_as_needed.stdout \
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ protected_3.err \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ relro_test.stdout \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ ver_matching_test.stdout \
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ script_test_3.stdout \
@@ -5802,6 +5805,13 @@ ver_test_14.sh.log: ver_test_14.sh
        --log-file $$b.log --trs-file $$b.trs \
        $(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
        "$$tst" $(AM_TESTS_FD_REDIRECT)
+weak_as_needed.sh.log: weak_as_needed.sh
+       @p='weak_as_needed.sh'; \
+       b='weak_as_needed.sh'; \
+       $(am__check_pre) $(LOG_DRIVER) --test-name "$$f" \
+       --log-file $$b.log --trs-file $$b.trs \
+       $(am__common_driver_flags) $(AM_LOG_DRIVER_FLAGS) $(LOG_DRIVER_FLAGS) -- $(LOG_COMPILE) \
+       "$$tst" $(AM_TESTS_FD_REDIRECT)
 relro_test.sh.log: relro_test.sh
        @p='relro_test.sh'; \
        b='relro_test.sh'; \
@@ -8712,6 +8722,20 @@ uninstall-am:
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ $(TEST_OBJDUMP) -T $< | $(TEST_CXXFILT) >$@
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ver_test_14: gcctestdir/ld ver_test_main.o ver_test_1.so ver_test_2.so ver_test_4.so ver_test_14.script
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ $(CXXLINK) -Bgcctestdir/ -Wl,--version-script,$(srcdir)/ver_test_14.script -Wl,-E -Wl,-R,. ver_test_main.o ver_test_1.so ver_test_2.so ver_test_4.so
+@GCC_TRUE@@NATIVE_LINKER_TRUE@weak_as_needed.stdout: weak_as_needed_a.so
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(TEST_READELF) -dW --dyn-syms $< >$@
+@GCC_TRUE@@NATIVE_LINKER_TRUE@weak_as_needed_a.so: gcctestdir/ld weak_as_needed_a.o weak_as_needed_b.so weak_as_needed_c.so
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ gcctestdir/ld -shared -rpath . -o $@ weak_as_needed_a.o --as-needed weak_as_needed_b.so weak_as_needed_c.so
+@GCC_TRUE@@NATIVE_LINKER_TRUE@weak_as_needed_b.so: gcctestdir/ld weak_as_needed_b.o weak_as_needed_b.script
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ gcctestdir/ld -shared -rpath . -o $@ --version-script $(srcdir)/weak_as_needed_b.script weak_as_needed_b.o
+@GCC_TRUE@@NATIVE_LINKER_TRUE@weak_as_needed_c.so: gcctestdir/ld weak_as_needed_c.o weak_as_needed_c.script
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ gcctestdir/ld -shared -rpath . -o $@ --version-script $(srcdir)/weak_as_needed_c.script weak_as_needed_c.o
+@GCC_TRUE@@NATIVE_LINKER_TRUE@weak_as_needed_a.o: weak_as_needed_a.c
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(COMPILE) -c -fpic -o $@ $<
+@GCC_TRUE@@NATIVE_LINKER_TRUE@weak_as_needed_b.o: weak_as_needed_b.c
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(COMPILE) -c -fpic -o $@ $<
+@GCC_TRUE@@NATIVE_LINKER_TRUE@weak_as_needed_c.o: weak_as_needed_c.c
+@GCC_TRUE@@NATIVE_LINKER_TRUE@ $(COMPILE) -c -fpic -o $@ $<
 
 @GCC_TRUE@@NATIVE_LINKER_TRUE@protected_1.so: gcctestdir/ld protected_1_pic.o protected_2_pic.o protected_3_pic.o
 @GCC_TRUE@@NATIVE_LINKER_TRUE@ $(CXXLINK) -Bgcctestdir/ -shared protected_1_pic.o protected_2_pic.o protected_3_pic.o
diff --git a/gold/testsuite/weak_as_needed.sh b/gold/testsuite/weak_as_needed.sh
new file mode 100755 (executable)
index 0000000..05fdc75
--- /dev/null
@@ -0,0 +1,62 @@
+#!/bin/sh
+
+# weak_as_needed.sh -- a test case for version handling with weak symbols
+# and --as-needed libraries.
+
+# Copyright (C) 2018 Free Software Foundation, Inc.
+# Written by Cary Coutant <ccoutant@gmail.com>.
+
+# This file is part of gold.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+
+# This program 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 General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston,
+# MA 02110-1301, USA.
+
+# This test verifies that a weak reference is properly bound to
+# an as-needed library, when it is first resolved to a symbol in
+# a library that ends up being not needed.
+
+# Ref: https://stackoverflow.com/questions/50751421/undefined-behavior-in-shared-lib-using-libpthread-but-not-having-it-in-elf-as-d
+
+check()
+{
+    if ! grep -q "$2" "$1"
+    then
+       echo "Did not find expected output in $1:"
+       echo "   $2"
+       echo ""
+       echo "Actual output below:"
+       cat "$1"
+       exit 1
+    fi
+}
+
+check_missing()
+{
+    if grep -q "$2" "$1"
+    then
+       echo "Found unexpected output in $1:"
+       echo "   $2"
+       echo ""
+       echo "Actual output below:"
+       cat "$1"
+       exit 1
+    fi
+}
+
+check weak_as_needed.stdout "WEAK .* UND  *bar@v2"
+check weak_as_needed.stdout "NEEDED.*weak_as_needed_c\.so"
+check_missing weak_as_needed.stdout "NEEDED.*weak_as_needed_b\.so"
+
+exit 0
diff --git a/gold/testsuite/weak_as_needed_a.c b/gold/testsuite/weak_as_needed_a.c
new file mode 100644 (file)
index 0000000..2e4ff8f
--- /dev/null
@@ -0,0 +1,10 @@
+extern void bar(void) __attribute__ (( weak ));
+extern void t4(void);
+
+void foo(void);
+
+void foo(void)
+{
+  bar();
+  t4();
+}
diff --git a/gold/testsuite/weak_as_needed_b.c b/gold/testsuite/weak_as_needed_b.c
new file mode 100644 (file)
index 0000000..6a1fbf8
--- /dev/null
@@ -0,0 +1,23 @@
+#include <stdio.h>
+
+__asm__ (".symver bar_v1, bar@v1");
+__asm__ (".symver bar_v2, bar@@v2");
+
+void bar_v1(void);
+void bar_v2(void);
+void baz(void);
+
+void bar_v1(void)
+{
+  printf("weak_as_needed_b: bar_v1\n");
+}
+
+void bar_v2(void)
+{
+  printf("weak_as_needed_b: bar_v2\n");
+}
+
+void baz(void)
+{
+  printf("weak_as_needed_b: baz\n");
+}
diff --git a/gold/testsuite/weak_as_needed_b.script b/gold/testsuite/weak_as_needed_b.script
new file mode 100644 (file)
index 0000000..23ce2fa
--- /dev/null
@@ -0,0 +1,11 @@
+v1 {
+  global:
+   bar;
+};
+v2 {
+  global:
+   bar;
+   baz;
+  local:
+   *;
+} v1;
diff --git a/gold/testsuite/weak_as_needed_c.c b/gold/testsuite/weak_as_needed_c.c
new file mode 100644 (file)
index 0000000..4c16bac
--- /dev/null
@@ -0,0 +1,29 @@
+#include <stdio.h>
+
+__asm__ (".symver bar_v1, bar@v1");
+__asm__ (".symver bar_v2, bar@@v2");
+
+void bar_v1(void);
+void bar_v2(void);
+void baz(void);
+void t4(void);
+
+void bar_v1(void)
+{
+  printf("weak_as_needed_c: bar_v1\n");
+}
+
+void bar_v2(void)
+{
+  printf("weak_as_needed_c: bar_v2\n");
+}
+
+void baz(void)
+{
+  printf("weak_as_needed_c: baz\n");
+}
+
+void t4(void)
+{
+  printf("weak_as_needed_c: t4\n");
+}
diff --git a/gold/testsuite/weak_as_needed_c.script b/gold/testsuite/weak_as_needed_c.script
new file mode 100644 (file)
index 0000000..206d407
--- /dev/null
@@ -0,0 +1,12 @@
+v1 {
+  global:
+   bar;
+};
+v2 {
+  global:
+   bar;
+   baz;
+   t4;
+  local:
+   *;
+} v1;