[PATCH V2] elf: ignore versioning table if symbol is looked up by object itself

[Waldemar Kozaczuk]

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.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---
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

[Waldek Kozaczuk]

Is there anything wrong/deficient with this patch that prevents us from applying it?

[Waldek Kozaczuk]

I have felt a bit uneasy about applying this patch. Let me explain why.

The main (and so far 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. But OSv version symbol handling logic was based on what I found in musl AND I could not find any evidence that musl makes such "old" symbols visible to the object itself when relocating which is what this patch fixes. And this is what made me uneasy because then how come musl handles the exact same problem?

It turns out it does not. Musl team complained about original patch and GCC team changes 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. I did also verify that libgcc_s.so in alpine linux does not have these 2 symbols at all.

So given all that I wonder if this patch is still the best one. It seems to possibly be handling just this one "one-off". There are alternatives but I am not sure if not worse:

1. Ignore version table if soname is libgcc_s - much simples but super hacky plus exposes these 2 symbols to everyone.
2. When relocating detect that symbols are old per symver table and set them to 0; then when calling DT_INIT_ARRAY constructors skip them if 0 (feels worse than original patch).

What do you think? Any other ideas? Is this patch still the best compromise?

[Waldemar Kozaczuk]

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.

[Nadav Har'El]

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?

--
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 osv-dev+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20200113051347.8583-1-jwkozaczuk%40gmail.com.

[Waldek Kozaczuk]

I just wanted to say in general that I am not super happy we had to come up with this patch.  But it blocks us from getting rid of externals. 

I am not surprised.  I barely could follow them. 

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?

I honestly do not know. I think gnu FORTRAN might be using it. 

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?

Yeah the object::symbol() is only called by object relocation methods which fail without this patch because symbol is not found. 

I theory dlsym() could also be called by object itself but for now I am deliberately ignoring this potential case. 

[Commit Bot]

From: Waldemar Kozaczuk <jwkoz...@gmail.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

elf: ignore versioning table if symbol is looked up by object itself

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>

---

diff --git a/core/elf.cc b/core/elf.cc
--- a/core/elf.cc
+++ b/core/elf.cc
@@ -186,7 +186,7 @@ void object::set_visibility(elf::VisibilityLevel
visibilityLevel)

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;
}

@@ -680,7 +680,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);
}

@@ -708,7 +708,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;
}

@@ -885,7 +885,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);

@@ -909,7 +909,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;

@@ -924,14 +924,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);
}

@@ -1491,14 +1491,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;
}

@@ -1509,7 +1509,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

--- a/include/osv/elf.hh
+++ b/include/osv/elf.hh
@@ -346,7 +346,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();

@@ -388,7 +388,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);

@@ -581,7 +581,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