From cea6ffbd06a6ebb5c21cb51e7775b8ebb5e34f38 Mon Sep 17 00:00:00 2001 From: Cary Coutant Date: Thu, 21 Jun 2018 13:51:16 -0700 Subject: [PATCH] Fix treatment of symbol versions with unused as-needed libraries. 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 | 19 ++++++++ gold/resolve.cc | 37 +++++++++++---- gold/symtab.cc | 33 ++++---------- gold/testsuite/Makefile.am | 17 +++++++ gold/testsuite/Makefile.in | 28 +++++++++++- gold/testsuite/weak_as_needed.sh | 62 ++++++++++++++++++++++++++ gold/testsuite/weak_as_needed_a.c | 10 +++++ gold/testsuite/weak_as_needed_b.c | 23 ++++++++++ gold/testsuite/weak_as_needed_b.script | 11 +++++ gold/testsuite/weak_as_needed_c.c | 29 ++++++++++++ gold/testsuite/weak_as_needed_c.script | 12 +++++ 11 files changed, 246 insertions(+), 35 deletions(-) create mode 100755 gold/testsuite/weak_as_needed.sh create mode 100644 gold/testsuite/weak_as_needed_a.c create mode 100644 gold/testsuite/weak_as_needed_b.c create mode 100644 gold/testsuite/weak_as_needed_b.script create mode 100644 gold/testsuite/weak_as_needed_c.c create mode 100644 gold/testsuite/weak_as_needed_c.script diff --git a/gold/ChangeLog b/gold/ChangeLog index 626bcdba830..9297e6d249b 100644 --- a/gold/ChangeLog +++ b/gold/ChangeLog @@ -1,3 +1,22 @@ +2018-06-21 Cary Coutant + + * 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 PR gold/23268 diff --git a/gold/resolve.cc b/gold/resolve.cc index 4a5784cf8bd..ddd2bacc10a 100644 --- a/gold/resolve.cc +++ b/gold/resolve.cc @@ -394,7 +394,7 @@ Symbol_table::resolve(Sized_symbol* to, object, &adjust_common_sizes, &adjust_dyndef, is_default_version)) { - elfcpp::STB tobinding = to->binding(); + elfcpp::STB orig_tobinding = to->binding(); typename Sized_symbol::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* 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* 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: diff --git a/gold/symtab.cc b/gold/symtab.cc index 238834dce88..c43d127507e 100644 --- a/gold/symtab.cc +++ b/gold/symtab.cc @@ -2543,8 +2543,6 @@ Symbol_table::set_dynsym_indexes(unsigned int index, Stringpool* dynpool, Versions* versions) { - std::vector 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::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); diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am index b88b02f17d2..ab3b25809a9 100644 --- a/gold/testsuite/Makefile.am +++ b/gold/testsuite/Makefile.am @@ -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 diff --git a/gold/testsuite/Makefile.in b/gold/testsuite/Makefile.in index e1b1762ecb4..14e4ce5c2c7 100644 --- a/gold/testsuite/Makefile.in +++ b/gold/testsuite/Makefile.in @@ -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 index 00000000000..05fdc75b454 --- /dev/null +++ b/gold/testsuite/weak_as_needed.sh @@ -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 . + +# 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 index 00000000000..2e4ff8f2280 --- /dev/null +++ b/gold/testsuite/weak_as_needed_a.c @@ -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 index 00000000000..6a1fbf8f854 --- /dev/null +++ b/gold/testsuite/weak_as_needed_b.c @@ -0,0 +1,23 @@ +#include + +__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 index 00000000000..23ce2fab9a5 --- /dev/null +++ b/gold/testsuite/weak_as_needed_b.script @@ -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 index 00000000000..4c16bacf129 --- /dev/null +++ b/gold/testsuite/weak_as_needed_c.c @@ -0,0 +1,29 @@ +#include + +__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 index 00000000000..206d40758b1 --- /dev/null +++ b/gold/testsuite/weak_as_needed_c.script @@ -0,0 +1,12 @@ +v1 { + global: + bar; +}; +v2 { + global: + bar; + baz; + t4; + local: + *; +} v1; -- 2.30.2