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.
On Thu, Jan 16, 2020 at 8:31 PM Zhiting Zhu <[email protected]> wrote: > 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 <[email protected]> > Signed-off-by: Zhiting Zhu <[email protected]> > --- > 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) > 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). > + 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(); > 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 > > -- > 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/20200116183055.100586-1-zhitingz%40cs.utexas.edu > . > -- 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/CANEVyju7rQkSckT5kHE2v%2BOS%2BFr5DDQtGVL71qhUHRpoG8qfEQ%40mail.gmail.com.
