[PATCH 1/2] Lookup symbol deeper

[Zhiting Zhu]

dlsym function doesn't seem to lookup symbol deep enough. This
patch fixes test_dlsym_from_sofile_with_preload and
dlsym_with_dependencies

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
Signed-off-by: Zhiting Zhu <zhit...@cs.utexas.edu>
---
core/elf.cc | 14 ++++++++++++++
include/osv/elf.hh | 1 +
libc/dlfcn.cc | 2 +-
tests/tst-dlfcn.cc | 2 +-
4 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/core/elf.cc b/core/elf.cc
index 73834b19..26a25c43 100644
--- a/core/elf.cc
+++ b/core/elf.cc
@@ -950,6 +950,20 @@ Elf64_Sym* object::lookup_symbol(const char* name, bool self_lookup)
return sym;
}

+symbol_module object::lookup_symbol_deep(const char* name)
+{
+ symbol_module sym = { lookup_symbol(name, false), this };
+ if (!sym.symbol) {
+ for (auto&& _obj : _needed) {
+ sym = _obj->lookup_symbol_deep(name);
+ if (sym.symbol) {
+ break;
+ }
+ }
+ }
+ return sym;
+}
+
unsigned object::symtab_len()
{
if (dynamic_exists(DT_HASH)) {
diff --git a/include/osv/elf.hh b/include/osv/elf.hh
index 1dac6c08..8cd7b37a 100644
--- a/include/osv/elf.hh
+++ b/include/osv/elf.hh
@@ -347,6 +347,7 @@ public:
void* base() const;
void* end() const;
Elf64_Sym* lookup_symbol(const char* name, bool self_lookup);
+ symbol_module lookup_symbol_deep(const char* name);
void load_segments();
void process_headers();
void unload_segments();
diff --git a/libc/dlfcn.cc b/libc/dlfcn.cc
index 346cf195..e3a4a577 100644
--- a/libc/dlfcn.cc
+++ b/libc/dlfcn.cc
@@ -74,7 +74,7 @@ void* dlsym(void* handle, const char* name)
abort();
} else {
auto obj = *reinterpret_cast<std::shared_ptr<elf::object>*>(handle);
- sym = { obj->lookup_symbol(name, false), obj.get() };
+ sym = obj->lookup_symbol_deep(name);
}
if (!sym.obj || !sym.symbol) {
dlerror_fmt("dlsym: symbol %s not found", name);
diff --git a/tests/tst-dlfcn.cc b/tests/tst-dlfcn.cc
index 5258256a..6d91fb93 100644
--- a/tests/tst-dlfcn.cc
+++ b/tests/tst-dlfcn.cc
@@ -13,7 +13,7 @@
namespace utf = boost::unit_test;

const bool rtld_next = false;
-const bool deep_lookup = false;
+const bool deep_lookup = true;

static bool called = false;
extern "C" void DlSymTestFunction()
--
2.17.1

[Zhiting Zhu]

Signed-off-by: Zhiting Zhu <zhit...@cs.utexas.edu>
---

core/elf.cc | 33 +++++++++++++++++++++++++++++++++
include/osv/elf.hh | 1 +
libc/dlfcn.cc | 4 ++--
tests/tst-dlfcn.cc | 2 +-
4 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/core/elf.cc b/core/elf.cc
index 26a25c43..69dbd7ed 100644
--- a/core/elf.cc
+++ b/core/elf.cc
@@ -41,6 +41,7 @@
TRACEPOINT(trace_elf_load, "%s", const char *);
TRACEPOINT(trace_elf_unload, "%s", const char *);
TRACEPOINT(trace_elf_lookup, "%s", const char *);
+TRACEPOINT(trace_elf_lookup_next, "%s", const char *);
TRACEPOINT(trace_elf_lookup_addr, "%p", const void *);

extern void* elf_start;
@@ -1532,6 +1533,38 @@ symbol_module program::lookup(const char* name, object* seeker)
return ret;
}

+symbol_module program::lookup_next(const char* name, const void* retaddr)
+{
+ trace_elf_lookup_next(name);
+ symbol_module ret(nullptr,nullptr);
+ if (retaddr == nullptr) {
+ return ret;
+ }
+ with_modules([&](const elf::program::modules_list &ml)
+ {
+ auto start = ml.objects.end();
+ for (auto it = ml.objects.begin(), end = ml.objects.end(); it != end; ++it) {
+ auto module = *it;
+ if (module->contains_addr(retaddr)) {
+ start = it;
+ break;
+ }
+ }
+ if (start == ml.objects.end()) {
+ return;
+ }
+ start = ++start;
+ for (auto it = start, end = ml.objects.end(); it != end; ++it) {
+ auto module = *it;
+ if (auto sym = module->lookup_symbol(name, false)) {
+ ret = symbol_module(sym, module);
+ break;
+ }
+ }
+ });
+ return ret;
+}
+
void* program::do_lookup_function(const char* name)
{
auto sym = lookup(name, nullptr);
diff --git a/include/osv/elf.hh b/include/osv/elf.hh
index 8cd7b37a..edcec7ff 100644
--- a/include/osv/elf.hh
+++ b/include/osv/elf.hh
@@ -584,6 +584,7 @@ public:
void set_search_path(std::initializer_list<std::string> path);

symbol_module lookup(const char* symbol, object* seeker);
+ symbol_module lookup_next(const char* name, const void* retaddr);
template <typename T>
T* lookup_function(const char* symbol);

diff --git a/libc/dlfcn.cc b/libc/dlfcn.cc
index e3a4a577..108a78ed 100644
--- a/libc/dlfcn.cc
+++ b/libc/dlfcn.cc
@@ -70,8 +70,8 @@ void* dlsym(void* handle, const char* name)
if ((program == handle) || (handle == RTLD_DEFAULT)) {
sym = program->lookup(name, nullptr);
} else if (handle == RTLD_NEXT) {
- // FIXME: implement
- abort();
+ auto retaddr = __builtin_extract_return_addr(__builtin_return_address(0));
+ sym = program->lookup_next(name, retaddr);

} else {
auto obj = *reinterpret_cast<std::shared_ptr<elf::object>*>(handle);

sym = obj->lookup_symbol_deep(name);
diff --git a/tests/tst-dlfcn.cc b/tests/tst-dlfcn.cc
index 6d91fb93..6188236f 100644
--- a/tests/tst-dlfcn.cc
+++ b/tests/tst-dlfcn.cc
@@ -12,7 +12,7 @@
#include <boost/test/unit_test.hpp>

namespace utf = boost::unit_test;

-const bool rtld_next = false;
+const bool rtld_next = true;

const bool deep_lookup = true;

static bool called = false;

--
2.17.1

[Nadav Har'El]

When you send a second version of a patch, please label it with "v2" and so on (if you use git format-patch, you can do "git format-patch -v2" to do this automatically) and write below the fold (the "--" line) some comments about what changed. Because it's hard for me to keep track what are new patches and what are modified versions and what was modified.

In the present case, this appears to be a second version of your earlier patch "[PATCH 2/3] look up symbol deeper".

I had some review comments in that version, that I think you forgot to address. In that version, I was worried that the "DT_NEEDED" graph may have cycles, and cause your code to go into an infinite loop. I asked if you believe (and why) it can't have cycles. If it can, I suggested you can use object::collect_dependencies(), which builds a list of all modules you need to look at (dependencies and dependencies of dependencies and ...) - without a risk of cycles because the DFS walk uses a set to avoid walking the same node more than once.

(by the way, the dlsym() manual says that it does a BFS walk over the dependencies. object::collect_dependencies() which I suggested above does DFS, not BFS. We need to consider if this makes any difference and if we need to do a BFS version of collect_dependencies()... I hope not, but I don't know).

Thanks,

Nadav.

--
Nadav Har'El

n...@scylladb.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 osv-dev+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20200115173946.76109-1-zhitingz%40cs.utexas.edu.

[zhiting zhu]

Sorry, I might miss your reply. Thanks for the tip on format-patch. I will do it in the future. This patch is written by Waldemar for a fix on the test errors. I'm not familiar with the elf spec. I don't know whether DT_NEEDED would have a cycle.

I feel the order of the objects matter, especially when the man page said about it.

To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/CANEVyjuF-6X%2BOi1pdkNzJupkVQunaH7-FuEZhr85JNUgjKFvrA%40mail.gmail.com.

[Nadav Har'El]

On Thu, Jan 16, 2020 at 2:00 AM zhiting zhu <zhit...@cs.utexas.edu> wrote:

Sorry, I might miss your reply. Thanks for the tip on format-patch. I will do it in the future. This patch is written by Waldemar for a fix on the test errors. I'm not familiar with the elf spec. I don't know whether DT_NEEDED would have a cycle.

Unfortunately I also don't know the answer. My gut feeling tells me there is no compelling reason why cycles are impossible, and it's safer not to assume they can't happen.

I feel the order of the objects matter, especially when the man page said about it.

I think you're right. I think it's fairly straightforward to write a variant object::collect_dependencies() which does BFS instead of the DFS which collect_dependencies() uses (and even worse - collect_dependencies() returns an unordered_set so the order of modules it returns is completely arbitrary)

[Zhiting Zhu]

dlsym function doesn't seem to lookup symbol deep enough. This

patch includes a bfs version of collect dependencies. This
patch fixes test_dlsym_from_sofile_with_preload and
dlsym_with_dependencies.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
Signed-off-by: Zhiting Zhu <zhit...@cs.utexas.edu>
---

core/elf.cc | 38 ++++++++++++++++++++++++++++++++++++++
include/osv/elf.hh | 2 ++

libc/dlfcn.cc | 2 +-
tests/tst-dlfcn.cc | 2 +-

4 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/core/elf.cc b/core/elf.cc
index 73834b19..fec2b590 100644
--- a/core/elf.cc
+++ b/core/elf.cc
@@ -27,6 +27,7 @@
#include <sys/utsname.h>
#include <osv/demangle.hh>
#include <boost/version.hpp>
+#include <deque>

#include "arch.hh"

@@ -950,6 +951,23 @@ Elf64_Sym* object::lookup_symbol(const char* name, bool self_lookup)

return sym;
}

+symbol_module object::lookup_symbol_deep(const char* name)
+{
+ symbol_module sym = { lookup_symbol(name, false), this };
+ if (!sym.symbol) {

+ std::deque<object*> deps;
+ this->collect_dependencies_bfs(deps);
+ for (auto&& _obj : deps) {
+ auto symbol = _obj->lookup_symbol(name, false);
+ if (symbol) {
+ sym = { symbol, _obj };

+ break;
+ }
+ }
+ }
+ return sym;
+}
+
unsigned object::symtab_len()
{
if (dynamic_exists(DT_HASH)) {

@@ -1089,6 +1107,26 @@ void object::collect_dependencies(std::unordered_set<elf::object*>& ds)
}
}

+void object::collect_dependencies_bfs(std::deque<elf::object*>& deps)
+{
+ std::unordered_set<object*> deps_set;
+ std::deque<object*> operate_queue;
+ operate_queue.push_back(this);
+
+ while (!operate_queue.empty()) {
+ object* obj = operate_queue.front();
+ operate_queue.pop_front();
+ deps.push_back(obj);
+ deps_set.insert(obj);
+ for (auto&& d : obj->_needed) {
+ if (!deps_set.count(d.get())) {
+ deps_set.insert(d.get());
+ operate_queue.push_back(d.get());
+ }
+ }
+ }
+}
+
std::string object::soname()
{
return dynamic_exists(DT_SONAME) ? dynamic_str(DT_SONAME) : std::string();
diff --git a/include/osv/elf.hh b/include/osv/elf.hh
index 1dac6c08..398afd00 100644

--- a/include/osv/elf.hh
+++ b/include/osv/elf.hh
@@ -347,6 +347,7 @@ public:
void* base() const;
void* end() const;
Elf64_Sym* lookup_symbol(const char* name, bool self_lookup);
+ symbol_module lookup_symbol_deep(const char* name);
void load_segments();
void process_headers();
void unload_segments();

@@ -405,6 +406,7 @@ private:
void relocate_pltgot();
unsigned symtab_len();
void collect_dependencies(std::unordered_set<elf::object*>& ds);
+ void collect_dependencies_bfs(std::deque<elf::object*>& deps);
void prepare_initial_tls(void* buffer, size_t size, std::vector<ptrdiff_t>& offsets);
void prepare_local_tls(std::vector<ptrdiff_t>& offsets);
void alloc_static_tls();

[Zhiting Zhu]

Signed-off-by: Zhiting Zhu <zhit...@cs.utexas.edu>
---

core/elf.cc | 33 +++++++++++++++++++++++++++++++++
include/osv/elf.hh | 1 +
libc/dlfcn.cc | 4 ++--

tests/tst-dlfcn.cc | 2 +-
4 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/core/elf.cc b/core/elf.cc
index fec2b590..290b263f 100644
--- a/core/elf.cc
+++ b/core/elf.cc
@@ -42,6 +42,7 @@

TRACEPOINT(trace_elf_load, "%s", const char *);
TRACEPOINT(trace_elf_unload, "%s", const char *);
TRACEPOINT(trace_elf_lookup, "%s", const char *);
+TRACEPOINT(trace_elf_lookup_next, "%s", const char *);
TRACEPOINT(trace_elf_lookup_addr, "%p", const void *);

extern void* elf_start;

@@ -1556,6 +1557,38 @@ symbol_module program::lookup(const char* name, object* seeker)

return ret;
}

+symbol_module program::lookup_next(const char* name, const void* retaddr)
+{
+ trace_elf_lookup_next(name);
+ symbol_module ret(nullptr,nullptr);
+ if (retaddr == nullptr) {
+ return ret;
+ }
+ with_modules([&](const elf::program::modules_list &ml)
+ {
+ auto start = ml.objects.end();
+ for (auto it = ml.objects.begin(), end = ml.objects.end(); it != end; ++it) {
+ auto module = *it;
+ if (module->contains_addr(retaddr)) {
+ start = it;

+ break;
+ }
+ }

+ if (start == ml.objects.end()) {
+ return;
+ }
+ start = ++start;
+ for (auto it = start, end = ml.objects.end(); it != end; ++it) {
+ auto module = *it;
+ if (auto sym = module->lookup_symbol(name, false)) {
+ ret = symbol_module(sym, module);

+ break;
+ }
+ }

+ });
+ return ret;
+}
+
void* program::do_lookup_function(const char* name)
{
auto sym = lookup(name, nullptr);

diff --git a/include/osv/elf.hh b/include/osv/elf.hh
index 398afd00..ab7eaca3 100644
--- a/include/osv/elf.hh
+++ b/include/osv/elf.hh
@@ -585,6 +585,7 @@ public:

void set_search_path(std::initializer_list<std::string> path);

symbol_module lookup(const char* symbol, object* seeker);
+ symbol_module lookup_next(const char* name, const void* retaddr);
template <typename T>
T* lookup_function(const char* symbol);

diff --git a/libc/dlfcn.cc b/libc/dlfcn.cc
index e3a4a577..108a78ed 100644
--- a/libc/dlfcn.cc
+++ b/libc/dlfcn.cc

@@ -70,8 +70,8 @@ void* dlsym(void* handle, const char* name)
if ((program == handle) || (handle == RTLD_DEFAULT)) {
sym = program->lookup(name, nullptr);
} else if (handle == RTLD_NEXT) {
- // FIXME: implement
- abort();
+ auto retaddr = __builtin_extract_return_addr(__builtin_return_address(0));
+ sym = program->lookup_next(name, retaddr);

} else {
auto obj = *reinterpret_cast<std::shared_ptr<elf::object>*>(handle);

sym = obj->lookup_symbol_deep(name);
diff --git a/tests/tst-dlfcn.cc b/tests/tst-dlfcn.cc
index 6d91fb93..6188236f 100644
--- a/tests/tst-dlfcn.cc
+++ b/tests/tst-dlfcn.cc

@@ -12,7 +12,7 @@
#include <boost/test/unit_test.hpp>

namespace utf = boost::unit_test;

-const bool rtld_next = false;
+const bool rtld_next = true;

const bool deep_lookup = true;

static bool called = false;

--
2.17.1

[Nadav Har'El]

Looks mostly good, sorry for having so many iterations... Just one tiny issue below about setting each item in the set twice,

and one style comment.

Because this function is no longer recursive, it doesn't need to take "deps" as a parameter -

it can just create a local object, and then return it at the end. It's more C++ style.

+{
+    std::unordered_set<object*> deps_set;
+    std::deque<object*> operate_queue;
+    operate_queue.push_back(this);

You can add deps_set.insert(this) here - more on why, below:

+
+    while (!operate_queue.empty()) {
+        object* obj = operate_queue.front();
+        operate_queue.pop_front();
+        deps.push_back(obj);
+        deps_set.insert(obj);

You're inserting each object into deps_set twice - here, and below when you first inserted obj into operate_queue.

It's not wrong (because, after all, it's a set), but also not necessary.

You can drop this deps_set.insert(obj) call if you make sure to do deps_set.insert(this) once above (see my previous comment).

 

--
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/20200116183055.100586-1-zhitingz%40cs.utexas.edu.

[Nadav Har'El]

Reviewed-by: Nadav Har'El <n...@scylladb.com>

--
Nadav Har'El

n...@scylladb.com

On Thu, Jan 16, 2020 at 8:31 PM Zhiting Zhu <zhit...@cs.utexas.edu> wrote:

--
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/20200116183055.100586-2-zhitingz%40cs.utexas.edu.

[Zhiting Zhu]

dlsym function doesn't seem to lookup symbol deep enough. This

patch includes a bfs version of collect dependencies. It

fixes test_dlsym_from_sofile_with_preload and
dlsym_with_dependencies.

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

Signed-off-by: Zhiting Zhu <zhit...@cs.utexas.edu>
---

core/elf.cc | 39 +++++++++++++++++++++++++++++++++++++++

include/osv/elf.hh | 2 ++
libc/dlfcn.cc | 2 +-

tests/tst-dlfcn.cc | 2 +-
4 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/core/elf.cc b/core/elf.cc
index 73834b19..a419ea6a 100644
--- a/core/elf.cc
+++ b/core/elf.cc

@@ -27,6 +27,7 @@
#include <sys/utsname.h>
#include <osv/demangle.hh>
#include <boost/version.hpp>
+#include <deque>

#include "arch.hh"

@@ -950,6 +951,22 @@ Elf64_Sym* object::lookup_symbol(const char* name, bool self_lookup)

return sym;
}

+symbol_module object::lookup_symbol_deep(const char* name)
+{
+ symbol_module sym = { lookup_symbol(name, false), this };
+ if (!sym.symbol) {

+ auto deps = this->collect_dependencies_bfs();

+ for (auto&& _obj : deps) {
+ auto symbol = _obj->lookup_symbol(name, false);
+ if (symbol) {
+ sym = { symbol, _obj };

+ break;
+ }
+ }
+ }

+ return sym;
+}
+
unsigned object::symtab_len()
{
if (dynamic_exists(DT_HASH)) {

@@ -1089,6 +1106,28 @@ void object::collect_dependencies(std::unordered_set<elf::object*>& ds)
}
}

+std::deque<elf::object*> object::collect_dependencies_bfs()

+{
+ std::unordered_set<object*> deps_set;
+ std::deque<object*> operate_queue;

+ std::deque<object*> deps;
+ operate_queue.push_back(this);
+ deps_set.insert(this);

+
+ while (!operate_queue.empty()) {
+ object* obj = operate_queue.front();
+ operate_queue.pop_front();
+ deps.push_back(obj);

+ for (auto&& d : obj->_needed) {

+ if (!deps_set.count(d.get())) {
+ deps_set.insert(d.get());
+ operate_queue.push_back(d.get());
+ }
+ }
+ }

+ return deps;

+}
+
std::string object::soname()
{
return dynamic_exists(DT_SONAME) ? dynamic_str(DT_SONAME) : std::string();

diff --git a/include/osv/elf.hh b/include/osv/elf.hh
index 1dac6c08..c942ac61 100644
--- a/include/osv/elf.hh
+++ b/include/osv/elf.hh

@@ -347,6 +347,7 @@ public:
void* base() const;
void* end() const;
Elf64_Sym* lookup_symbol(const char* name, bool self_lookup);
+ symbol_module lookup_symbol_deep(const char* name);
void load_segments();
void process_headers();
void unload_segments();
@@ -405,6 +406,7 @@ private:
void relocate_pltgot();
unsigned symtab_len();
void collect_dependencies(std::unordered_set<elf::object*>& ds);

+ std::deque<elf::object*> collect_dependencies_bfs();

void prepare_initial_tls(void* buffer, size_t size, std::vector<ptrdiff_t>& offsets);
void prepare_local_tls(std::vector<ptrdiff_t>& offsets);
void alloc_static_tls();

diff --git a/libc/dlfcn.cc b/libc/dlfcn.cc
index 346cf195..e3a4a577 100644
--- a/libc/dlfcn.cc
+++ b/libc/dlfcn.cc
@@ -74,7 +74,7 @@ void* dlsym(void* handle, const char* name)
abort();

} else {
auto obj = *reinterpret_cast<std::shared_ptr<elf::object>*>(handle);

- sym = { obj->lookup_symbol(name, false), obj.get() };
+ sym = obj->lookup_symbol_deep(name);
}
if (!sym.obj || !sym.symbol) {
dlerror_fmt("dlsym: symbol %s not found", name);

diff --git a/tests/tst-dlfcn.cc b/tests/tst-dlfcn.cc
index 5258256a..6d91fb93 100644
--- a/tests/tst-dlfcn.cc
+++ b/tests/tst-dlfcn.cc
@@ -13,7 +13,7 @@

namespace utf = boost::unit_test;

const bool rtld_next = false;
-const bool deep_lookup = false;

+const bool deep_lookup = true;

static bool called = false;

[Zhiting Zhu]

Signed-off-by: Zhiting Zhu <zhit...@cs.utexas.edu>
---

core/elf.cc | 33 +++++++++++++++++++++++++++++++++

include/osv/elf.hh | 1 +
libc/dlfcn.cc | 4 ++--

tests/tst-dlfcn.cc | 2 +-
4 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/core/elf.cc b/core/elf.cc
index a419ea6a..ffb16004 100644
--- a/core/elf.cc
+++ b/core/elf.cc

@@ -42,6 +42,7 @@
TRACEPOINT(trace_elf_load, "%s", const char *);
TRACEPOINT(trace_elf_unload, "%s", const char *);
TRACEPOINT(trace_elf_lookup, "%s", const char *);
+TRACEPOINT(trace_elf_lookup_next, "%s", const char *);
TRACEPOINT(trace_elf_lookup_addr, "%p", const void *);

extern void* elf_start;

@@ -1557,6 +1558,38 @@ symbol_module program::lookup(const char* name, object* seeker)

return ret;
}

+symbol_module program::lookup_next(const char* name, const void* retaddr)
+{
+ trace_elf_lookup_next(name);
+ symbol_module ret(nullptr,nullptr);
+ if (retaddr == nullptr) {
+ return ret;
+ }
+ with_modules([&](const elf::program::modules_list &ml)
+ {
+ auto start = ml.objects.end();
+ for (auto it = ml.objects.begin(), end = ml.objects.end(); it != end; ++it) {
+ auto module = *it;
+ if (module->contains_addr(retaddr)) {
+ start = it;

+ break;
+ }
+ }
+ if (start == ml.objects.end()) {

+ return;
+ }
+ start = ++start;
+ for (auto it = start, end = ml.objects.end(); it != end; ++it) {
+ auto module = *it;
+ if (auto sym = module->lookup_symbol(name, false)) {
+ ret = symbol_module(sym, module);

+ break;
+ }
+ }
+ });

+ return ret;
+}
+
void* program::do_lookup_function(const char* name)
{
auto sym = lookup(name, nullptr);

diff --git a/include/osv/elf.hh b/include/osv/elf.hh
index c942ac61..8f8db68a 100644
--- a/include/osv/elf.hh
+++ b/include/osv/elf.hh

@@ -585,6 +585,7 @@ public:
void set_search_path(std::initializer_list<std::string> path);

symbol_module lookup(const char* symbol, object* seeker);
+ symbol_module lookup_next(const char* name, const void* retaddr);
template <typename T>
T* lookup_function(const char* symbol);

diff --git a/libc/dlfcn.cc b/libc/dlfcn.cc
index e3a4a577..108a78ed 100644
--- a/libc/dlfcn.cc
+++ b/libc/dlfcn.cc
@@ -70,8 +70,8 @@ void* dlsym(void* handle, const char* name)

if ((program == handle) || (handle == RTLD_DEFAULT)) {
sym = program->lookup(name, nullptr);
} else if (handle == RTLD_NEXT) {
- // FIXME: implement
- abort();
+ auto retaddr = __builtin_extract_return_addr(__builtin_return_address(0));
+ sym = program->lookup_next(name, retaddr);

} else {
auto obj = *reinterpret_cast<std::shared_ptr<elf::object>*>(handle);

sym = obj->lookup_symbol_deep(name);
diff --git a/tests/tst-dlfcn.cc b/tests/tst-dlfcn.cc
index 6d91fb93..6188236f 100644
--- a/tests/tst-dlfcn.cc
+++ b/tests/tst-dlfcn.cc

@@ -12,7 +12,7 @@
#include <boost/test/unit_test.hpp>

namespace utf = boost::unit_test;

-const bool rtld_next = false;
+const bool rtld_next = true;

const bool deep_lookup = true;

static bool called = false;

--
2.17.1

[Waldek Kozaczuk]

Shall we commit this and another V4 version of these patches?

[Nadav Har'El]

Sorry, I forgot to review this. I'll do it now.

--
Nadav Har'El

n...@scylladb.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 osv-dev+u...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/e4fd1c02-1111-49a1-9128-fe12e36a42ba%40googlegroups.com.

[Commit Bot]

From: Zhiting Zhu <zhit...@cs.utexas.edu>
Committer: Nadav Har'El <n...@scylladb.com>
Branch: master

Lookup symbol deeper

dlsym function doesn't seem to lookup symbol deep enough. This
patch includes a bfs version of collect dependencies. It
fixes test_dlsym_from_sofile_with_preload and
dlsym_with_dependencies.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
Signed-off-by: Zhiting Zhu <zhit...@cs.utexas.edu>

Message-Id: <20200120011727....@cs.utexas.edu>

---
diff --git a/core/elf.cc b/core/elf.cc

--- a/include/osv/elf.hh
+++ b/include/osv/elf.hh
@@ -347,6 +347,7 @@ public:
void* base() const;
void* end() const;
Elf64_Sym* lookup_symbol(const char* name, bool self_lookup);
+ symbol_module lookup_symbol_deep(const char* name);
void load_segments();
void process_headers();
void unload_segments();
@@ -405,6 +406,7 @@ private:
void relocate_pltgot();
unsigned symtab_len();
void collect_dependencies(std::unordered_set<elf::object*>& ds);
+ std::deque<elf::object*> collect_dependencies_bfs();
void prepare_initial_tls(void* buffer, size_t size, std::vector<ptrdiff_t>& offsets);
void prepare_local_tls(std::vector<ptrdiff_t>& offsets);
void alloc_static_tls();
diff --git a/libc/dlfcn.cc b/libc/dlfcn.cc

--- a/libc/dlfcn.cc
+++ b/libc/dlfcn.cc
@@ -74,7 +74,7 @@ void* dlsym(void* handle, const char* name)
abort();
} else {
auto obj = *reinterpret_cast<std::shared_ptr<elf::object>*>(handle);
- sym = { obj->lookup_symbol(name, false), obj.get() };
+ sym = obj->lookup_symbol_deep(name);
}
if (!sym.obj || !sym.symbol) {
dlerror_fmt("dlsym: symbol %s not found", name);
diff --git a/tests/tst-dlfcn.cc b/tests/tst-dlfcn.cc

[Commit Bot]

From: Zhiting Zhu <zhit...@cs.utexas.edu>
Committer: Nadav Har'El <n...@scylladb.com>
Branch: master

Implement RTLD_NEXT

Signed-off-by: Zhiting Zhu <zhit...@cs.utexas.edu>
Message-Id: <20200120011727....@cs.utexas.edu>

---
diff --git a/core/elf.cc b/core/elf.cc

--- a/include/osv/elf.hh
+++ b/include/osv/elf.hh
@@ -585,6 +585,7 @@ public:
void set_search_path(std::initializer_list<std::string> path);

symbol_module lookup(const char* symbol, object* seeker);
+ symbol_module lookup_next(const char* name, const void* retaddr);
template <typename T>
T* lookup_function(const char* symbol);

diff --git a/libc/dlfcn.cc b/libc/dlfcn.cc

--- a/libc/dlfcn.cc
+++ b/libc/dlfcn.cc
@@ -70,8 +70,8 @@ void* dlsym(void* handle, const char* name)
if ((program == handle) || (handle == RTLD_DEFAULT)) {
sym = program->lookup(name, nullptr);
} else if (handle == RTLD_NEXT) {
- // FIXME: implement
- abort();
+ auto retaddr = __builtin_extract_return_addr(__builtin_return_address(0));
+ sym = program->lookup_next(name, retaddr);
} else {
auto obj = *reinterpret_cast<std::shared_ptr<elf::object>*>(handle);
sym = obj->lookup_symbol_deep(name);
diff --git a/tests/tst-dlfcn.cc b/tests/tst-dlfcn.cc

[Waldek Kozaczuk]

I think you meant to commit the V4 version of these patches not these one?

On Wednesday, January 22, 2020 at 12:42:16 PM UTC-5, Bot Droid wrote:

From: Zhiting Zhu <zhit...@cs.utexas.edu>
Committer: Nadav Har'El <n...@scylladb.com>
Branch: master

Lookup symbol deeper

dlsym function doesn't seem to lookup symbol deep enough. This
patch includes a bfs version of collect dependencies. It
fixes test_dlsym_from_sofile_with_preload and
dlsym_with_dependencies.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
Signed-off-by: Zhiting Zhu <zhit...@cs.utexas.edu>

Message-Id: <20200120011727.1736-1-zhit...@cs.utexas.edu>

[Waldek Kozaczuk]

Sorry I think it was a false alarm on my side :-) I think you committed the right ones.