On Thu, Jan 16, 2020 at 2:00 AM zhiting zhu <[email protected]> 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) > On Wed, Jan 15, 2020 at 5:37 PM Nadav Har'El <[email protected]> wrote: > >> 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 >> [email protected] >> >> >> On Wed, Jan 15, 2020 at 7:39 PM Zhiting Zhu <[email protected]> >> wrote: >> >>> 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 <[email protected]> >>> Signed-off-by: Zhiting Zhu <[email protected]> >>> --- >>> 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 >>> >>> -- >>> 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/20200115173946.76109-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/CANEVyjuF-6X%2BOi1pdkNzJupkVQunaH7-FuEZhr85JNUgjKFvrA%40mail.gmail.com >> <https://groups.google.com/d/msgid/osv-dev/CANEVyjuF-6X%2BOi1pdkNzJupkVQunaH7-FuEZhr85JNUgjKFvrA%40mail.gmail.com?utm_medium=email&utm_source=footer> >> . >> > -- 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/CANEVyjstX%2BtnkoDges5T_oH4uN34KXiFR0Azejjbbfi90WVt8w%40mail.gmail.com.
