On Mon, Jan 13, 2020 at 7:14 AM Waldemar Kozaczuk <[email protected]> wrote:
> The commit > https://github.com/cloudius-systems/osv/commit/ed1eed7a567ec17138c65f0a5628c2311603c712 > enhanced dynamic linker to skip old symbols per versions table. > Unfortunately > the relevant code did not take into account whether the old symbol is being > looked up by the object itself during relocation. > > This sentence from https://www.akkadia.org/drepper/symbol-versioning - > "If the highest bit (bit 15) is set this is a hidden symbol which cannot > be referenced from outside the object." - SEEMS to indicate the old > symbols should be visible to the object itself only. > > This patch enhances lookup method to help track "who" is looking > and if it is the object itself, then the versions table is ignored. > > Here is some background information: > > The main (and so far the only one) motivation for this patch is to allow > upgrading libgcc_s.so to the newest version from host instead of relying > on the "antique" version from externals (see usr.manifest.skel). It turns > out that at some point (starting with version 6) GCC team moved the > __cpu_model > and __cpu_indicator_init (constructor function) symbols from the shared > library > libgcc_s.so to the static one - libgcc.a (please see this commit for some > details - > > https://github.com/gcc-mirror/gcc/commit/4b5fb32aba30762e0d8c9e75d1b46b47bee5eeb4#diff-3592c95126806aad11bb0f09e182f2f4 > ) > and marked them as "compat" in libgcc_s.so (shown with single @). > > This makes those symbols invisible to OSv linker because per symbols > version > table they are effectively "old" and should be ignored during lookup > but not to the object itself when it tries to relocate them. > This patch actually makes the change to the version symbol handling > logic to achieve exactly that. > > The version symbol handling logic in OSv is based on musl, but musl does > not try to expose > such "old" symbols to the object itself like this patch does. It turns out > that Musl team complained > about original change to libgcc_s.so and GCC team changed how libgcc_s.so > for musl is built. > This commit - > https://github.com/gcc-mirror/gcc/commit/6e6c7fc1e15525a10f48d4f5ac2edd853e2f5cb7 > and this discussion - https://patchwork.ozlabs.org/patch/693782/ - sheds > some light. > Unfortunately I have to admit that I didn't understand any of the details on these commits and discussions :-( It seems glibc deliberately wanted to hide these symbols. Can you please remind me what application or user uses them? Or does the C compiler generate their uses? Other than this, I don't have any specific objections or comments about this patch. One thing I'm not sure about is your call in object::symbol() to _prog.lookup(name, this). This supposedly means object::symbol() is only used "from" the object itself. Is that the case? > Signed-off-by: Waldemar Kozaczuk <[email protected]> > --- > core/elf.cc | 20 ++++++++++---------- > include/osv/elf.hh | 6 +++--- > libc/dlfcn.cc | 4 ++-- > 3 files changed, 15 insertions(+), 15 deletions(-) > > diff --git a/core/elf.cc b/core/elf.cc > index 24dfe914..fa63c9aa 100644 > --- a/core/elf.cc > +++ b/core/elf.cc > @@ -154,7 +154,7 @@ void object::setprivate(bool priv) > template <> > void* object::lookup(const char* symbol) > { > - symbol_module sm{lookup_symbol(symbol), this}; > + symbol_module sm{lookup_symbol(symbol, false), this}; > if (!sm.symbol || sm.symbol->st_shndx == SHN_UNDEF) { > return nullptr; > } > @@ -648,7 +648,7 @@ symbol_module object::symbol(unsigned idx, bool > ignore_missing) > } > auto nameidx = sym->st_name; > auto name = dynamic_ptr<const char>(DT_STRTAB) + nameidx; > - auto ret = _prog.lookup(name); > + auto ret = _prog.lookup(name, this); > if (!ret.symbol && binding == STB_WEAK) { > return symbol_module(sym, this); > } > @@ -676,7 +676,7 @@ symbol_module object::symbol_other(unsigned idx) > for (auto module : ml.objects) { > if (module == this) > continue; // do not match this module > - if (auto sym = module->lookup_symbol(name)) { > + if (auto sym = module->lookup_symbol(name, false)) { > ret = symbol_module(sym, module); > break; > } > @@ -852,7 +852,7 @@ dl_new_hash(const char *s) > return h & 0xffffffff; > } > > -Elf64_Sym* object::lookup_symbol_gnu(const char* name) > +Elf64_Sym* object::lookup_symbol_gnu(const char* name, bool self_lookup) > { > auto symtab = dynamic_ptr<Elf64_Sym>(DT_SYMTAB); > auto strtab = dynamic_ptr<char>(DT_STRTAB); > @@ -876,7 +876,7 @@ Elf64_Sym* object::lookup_symbol_gnu(const char* name) > if (idx == 0) { > return nullptr; > } > - auto version_symtab = dynamic_exists(DT_VERSYM) ? > dynamic_ptr<Elf64_Versym>(DT_VERSYM) : nullptr; > + auto version_symtab = (!self_lookup && dynamic_exists(DT_VERSYM)) ? > dynamic_ptr<Elf64_Versym>(DT_VERSYM) : nullptr; > do { > if ((chains[idx] & ~1) != (hashval & ~1)) { > continue; > @@ -891,14 +891,14 @@ Elf64_Sym* object::lookup_symbol_gnu(const char* > name) > return nullptr; > } > > -Elf64_Sym* object::lookup_symbol(const char* name) > +Elf64_Sym* object::lookup_symbol(const char* name, bool self_lookup) > { > if (!visible()) { > return nullptr; > } > Elf64_Sym* sym; > if (dynamic_exists(DT_GNU_HASH)) { > - sym = lookup_symbol_gnu(name); > + sym = lookup_symbol_gnu(name, self_lookup); > } else { > sym = lookup_symbol_old(name); > } > @@ -1457,14 +1457,14 @@ void program::del_debugger_obj(object* obj) > } > } > > -symbol_module program::lookup(const char* name) > +symbol_module program::lookup(const char* name, object* seeker) > { > trace_elf_lookup(name); > symbol_module ret(nullptr,nullptr); > with_modules([&](const elf::program::modules_list &ml) > { > for (auto module : ml.objects) { > - if (auto sym = module->lookup_symbol(name)) { > + if (auto sym = module->lookup_symbol(name, seeker == module)) > { > ret = symbol_module(sym, module); > return; > } > @@ -1475,7 +1475,7 @@ symbol_module program::lookup(const char* name) > > void* program::do_lookup_function(const char* name) > { > - auto sym = lookup(name); > + auto sym = lookup(name, nullptr); > if (!sym.symbol) { > throw std::runtime_error("symbol not found " + demangle(name)); > } > diff --git a/include/osv/elf.hh b/include/osv/elf.hh > index 4466b2ab..fce71ba5 100644 > --- a/include/osv/elf.hh > +++ b/include/osv/elf.hh > @@ -340,7 +340,7 @@ public: > void set_dynamic_table(Elf64_Dyn* dynamic_table); > void* base() const; > void* end() const; > - Elf64_Sym* lookup_symbol(const char* name); > + Elf64_Sym* lookup_symbol(const char* name, bool self_lookup); > void load_segments(); > void unload_segments(); > void fix_permissions(); > @@ -382,7 +382,7 @@ protected: > unsigned get_segment_mmap_permissions(const Elf64_Phdr& phdr); > private: > Elf64_Sym* lookup_symbol_old(const char* name); > - Elf64_Sym* lookup_symbol_gnu(const char* name); > + Elf64_Sym* lookup_symbol_gnu(const char* name, bool self_lookup); > template <typename T> > T* dynamic_ptr(unsigned tag); > Elf64_Xword dynamic_val(unsigned tag); > @@ -574,7 +574,7 @@ public: > */ > void set_search_path(std::initializer_list<std::string> path); > > - symbol_module lookup(const char* symbol); > + symbol_module lookup(const char* symbol, object* seeker); > template <typename T> > T* lookup_function(const char* symbol); > > diff --git a/libc/dlfcn.cc b/libc/dlfcn.cc > index 8cfb7b2a..346cf195 100644 > --- a/libc/dlfcn.cc > +++ b/libc/dlfcn.cc > @@ -68,13 +68,13 @@ void* dlsym(void* handle, const char* name) > elf::symbol_module sym; > auto program = elf::get_program(); > if ((program == handle) || (handle == RTLD_DEFAULT)) { > - sym = program->lookup(name); > + sym = program->lookup(name, nullptr); > } else if (handle == RTLD_NEXT) { > // FIXME: implement > abort(); > } else { > auto obj = > *reinterpret_cast<std::shared_ptr<elf::object>*>(handle); > - sym = { obj->lookup_symbol(name), obj.get() }; > + sym = { obj->lookup_symbol(name, false), obj.get() }; > } > if (!sym.obj || !sym.symbol) { > dlerror_fmt("dlsym: symbol %s not found", name); > -- > 2.20.1 > > -- > You received this message because you are subscribed to the Google Groups > "OSv Development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/osv-dev/20200113051347.8583-1-jwkozaczuk%40gmail.com > . > -- You received this message because you are subscribed to the Google Groups "OSv Development" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/CANEVyjszcSUYoxMYuXfKLg4zTPjx3vJ%2BaoptExURbTJyuwcREA%40mail.gmail.com.
