From 80cf47d906a7757130c771a05f7b983ca1662318 Mon Sep 17 00:00:00 2001 From: Sebastian Sumpf Date: Tue, 13 Apr 2021 17:48:43 +0200 Subject: [PATCH] ldso: protect object list with mutex When we allowed symbol resolution during exceptions, we used the shared object lock to protect ELF object list manipulation (e.g., dlopen, dclose) when executing exception unwinding code in the linker. Unfortunately, sometimes libraries that are loaded by 'dlopen' may raise exceptions in the process, leading to a deadlock within the unwind code. In order to resolve this, we now protect the object list operations (i.e., enqueue, removal, iteration) by a separate mutex. This allows the shared object interface to throw exceptions. issue #4071 --- repos/base/src/lib/ldso/exception.cc | 43 +++++----- repos/base/src/lib/ldso/include/linker.h | 62 +++++++++++--- repos/base/src/lib/ldso/main.cc | 103 +++++++++++------------ repos/base/src/lib/ldso/shared_object.cc | 13 ++- 4 files changed, 134 insertions(+), 87 deletions(-) diff --git a/repos/base/src/lib/ldso/exception.cc b/repos/base/src/lib/ldso/exception.cc index 4941ca89cc..049253f5a3 100644 --- a/repos/base/src/lib/ldso/exception.cc +++ b/repos/base/src/lib/ldso/exception.cc @@ -38,22 +38,22 @@ extern "C" int dl_iterate_phdr(int (*callback) (Phdr_info *info, size_t size, vo int err = 0; Phdr_info info; - /* forbid object list manipulation during traversal */ - Mutex::Guard guard(Linker::shared_object_mutex()); + bool last = false; + Linker::for_each_object([&] (Linker::Object &obj) { - for (Object *e = obj_list_head();e; e = e->next_obj()) { + if (last) return; - info.addr = e->reloc_base(); - info.name = e->name(); - info.phdr = e->file()->phdr.phdr; - info.phnum = e->file()->phdr.count; + info.addr = obj.reloc_base(); + info.name = obj.name(); + info.phdr = obj.file()->phdr.phdr; + info.phnum = obj.file()->phdr.count; if (verbose_exception) - log(e->name(), " reloc ", Hex(e->reloc_base())); + log(obj.name(), " reloc ", Hex(obj.reloc_base())); if ((err = callback(&info, sizeof(Phdr_info), data))) - break; - } + last = true; + }); return err; } @@ -89,27 +89,28 @@ extern "C" unsigned long dl_unwind_find_exidx(unsigned long pc, int *pcount) enum { EXIDX_ENTRY_SIZE = 8 }; *pcount = 0; - /* forbid object list manipulation during traversal */ - Mutex::Guard guard(Linker::shared_object_mutex()); + unsigned long value = 0; - for (Object *e = obj_list_head(); e; e = e->next_obj()) { + Linker::for_each_object([&] (Object &obj) { + + if (value) return; /* address of first PT_LOAD header */ - addr_t base = e->reloc_base() + e->file()->start; + addr_t base = obj.reloc_base() + obj.file()->start; /* is the 'pc' somewhere within this ELF image */ - if ((pc < base) || (pc >= base + e->file()->size)) - continue; + if ((pc < base) || (pc >= base + obj.file()->size)) + return; /* retrieve PHDR of exception-table segment */ - Elf::Phdr const *exidx = phdr_exidx(e->file()); + Elf::Phdr const *exidx = phdr_exidx(obj.file()); if (!exidx) - continue; + return; *pcount = exidx->p_memsz / EXIDX_ENTRY_SIZE; - return exidx->p_vaddr + e->reloc_base(); - } + value = exidx->p_vaddr + obj.reloc_base(); + }); - return 0; + return value; } diff --git a/repos/base/src/lib/ldso/include/linker.h b/repos/base/src/lib/ldso/include/linker.h index 937f113f20..ea95c7c11c 100644 --- a/repos/base/src/lib/ldso/include/linker.h +++ b/repos/base/src/lib/ldso/include/linker.h @@ -115,11 +115,6 @@ namespace Linker { Object &load(Env &, Allocator &md_alloc, char const *path, Dependency &dep, Keep keep); - /** - * Returns the head of the global object list - */ - Object *obj_list_head(); - /** * Returns the root-dependeny of the dynamic binary */ @@ -151,6 +146,7 @@ class Linker::Object : private Fifo::Element, public: typedef String<128> Name; + class Object_list; protected: @@ -162,6 +158,8 @@ class Linker::Object : private Fifo::Element, File const *_file { nullptr }; Elf::Addr _reloc_base { 0 }; + static Object_list &_object_list(); + public: void init(Name const &name, Elf::Addr reloc_base) @@ -177,6 +175,41 @@ class Linker::Object : private Fifo::Element, _reloc_base = file.reloc_base; } + class Object_list + { + private: + + Fifo _fifo { }; + Mutex _mutex { }; + + public: + + void enqueue(Object &obj) + { + Mutex::Guard guard(_mutex); + _fifo.enqueue(obj); + } + + void remove(Object &obj) + { + Mutex::Guard guard(_mutex); + _fifo.remove(obj); + } + + template + void for_each(FUNC const &func) + { + Mutex::Guard guard(_mutex); + _fifo.for_each(func); + } + }; + + template + static void with_object_list(FUNC const func) + { + func(_object_list()); + } + virtual ~Object() { } Elf::Addr reloc_base() const { return _reloc_base; } @@ -190,15 +223,11 @@ class Linker::Object : private Fifo::Element, virtual void relocate(Bind) = 0; virtual bool keep() const = 0; + virtual void force_keep() = 0; virtual void load() = 0; virtual bool unload() { return false;} - /** - * Next object in global object list - */ - virtual Object *next_obj() const = 0; - /** * Next object in initialization list */ @@ -228,6 +257,19 @@ class Linker::Object : private Fifo::Element, }; +namespace Linker { + /** + * Apply func to each object + */ + template + void for_each_object(FUNC const &func) + { + Object::with_object_list([&] (Object::Object_list &list) { + list.for_each(func); }); + } +} + + /** * Dependency of object */ diff --git a/repos/base/src/lib/ldso/main.cc b/repos/base/src/lib/ldso/main.cc index 752d793203..6a8d156c36 100644 --- a/repos/base/src/lib/ldso/main.cc +++ b/repos/base/src/lib/ldso/main.cc @@ -75,10 +75,18 @@ Genode::Mutex &Linker::shared_object_mutex() return _mutex; } + /************************************************************** ** ELF object types (shared object, dynamic binaries, ldso ** **************************************************************/ +Object::Object_list &Object::_object_list() +{ + static Object_list _list; + return _list; +} + + /** * The actual ELF object, one per file */ @@ -134,7 +142,8 @@ class Linker::Elf_object : public Object, private Fifo::Element { /* register for static construction and relocation */ Init::list()->insert(this); - obj_list()->enqueue(*this); + with_object_list([&] (Object_list &list) { + list.enqueue(*this); }); /* add to link map */ Debug::state_change(Debug::ADD, nullptr); @@ -156,7 +165,8 @@ class Linker::Elf_object : public Object, private Fifo::Element Debug::state_change(Debug::CONSISTENT, nullptr); /* remove from loaded objects list */ - obj_list()->remove(*this); + with_object_list([&] (Object_list &list) { + list.remove(*this); }); Init::list()->remove(this); } @@ -201,7 +211,7 @@ class Linker::Elf_object : public Object, private Fifo::Element Link_map::make_first(&_map); } - void force_keep() { _keep = KEEP; } + void force_keep() override { _keep = KEEP; } Link_map const &link_map() const override { return _map; } Dynamic const &dynamic() const override { return _dyn; } @@ -234,22 +244,6 @@ class Linker::Elf_object : public Object, private Fifo::Element */ Object *next_init() const override { return _next_object(); } - /** - * Next in object list - */ - Object *next_obj() const override { - return Fifo::Element::next(); - } - - /** - * Object list - */ - static Fifo *obj_list() - { - static Fifo _list; - return &_list; - } - void load() override { _ref_count++; } bool unload() override { return (_keep == DONT_KEEP) && !(--_ref_count); } @@ -331,7 +325,8 @@ Linker::Ld &Linker::Ld::linker() { Ld_vtable() { - Elf_object::obj_list()->enqueue(*this); + with_object_list([&] (Object_list &list) { + list.enqueue(*this); }); plt_setup(); } }; @@ -543,32 +538,28 @@ Object &Linker::load(Env &env, Allocator &md_alloc, char const *path, Dependency &dep, Keep keep) { Object *result = nullptr; - Elf_object::obj_list()->for_each([&] (Object &e) { - if (result == nullptr) { - if (verbose_loading) - log("LOAD: ", Linker::file(path), " == ", e.name()); + Object::with_object_list([&] (Object::Object_list &list) { + list.for_each([&] (Object &obj) { - if (!strcmp(Linker::file(path), e.name())) { - e.load(); - result = &e; + if (result == nullptr) { + if (verbose_loading) + log("LOAD: ", Linker::file(path), " == ", obj.name()); + + if (!strcmp(Linker::file(path), obj.name())) { + obj.load(); + result = &obj; + } } - } + }); }); + if (result == nullptr) result = new (md_alloc) Elf_object(env, md_alloc, path, dep, keep); + return *result; } -Object *Linker::obj_list_head() -{ - Object *result = nullptr; - Elf_object::obj_list()->head([&result] (Object &obj) { - result = &obj; }); - return result; -} - - Elf::Sym const *Linker::lookup_symbol(unsigned sym_index, Dependency const &dep, Elf::Addr *base, bool undef, bool other) { @@ -748,22 +739,24 @@ void Genode::exec_static_constructors() void Genode::Dynamic_linker::_for_each_loaded_object(Env &, For_each_fn const &fn) { - Elf_object::obj_list()->for_each([&] (Object const &obj) { + Object::with_object_list([&] (Object::Object_list &list) { + list.for_each([&] (Object const &obj) { - Elf_file const *elf_file_ptr = - obj.file() ? dynamic_cast(obj.file()) : nullptr; + Elf_file const *elf_file_ptr = + obj.file() ? dynamic_cast(obj.file()) : nullptr; - if (!elf_file_ptr) - return; + if (!elf_file_ptr) + return; - elf_file_ptr->with_rw_phdr([&] (Elf::Phdr const &phdr) { + elf_file_ptr->with_rw_phdr([&] (Elf::Phdr const &phdr) { - Object_info info { .name = obj.name(), - .ds_cap = elf_file_ptr->rom_cap, - .rw_start = (void *)(obj.reloc_base() + phdr.p_vaddr), - .rw_size = phdr.p_memsz }; + Object_info info { .name = obj.name(), + .ds_cap = elf_file_ptr->rom_cap, + .rw_start = (void *)(obj.reloc_base() + phdr.p_vaddr), + .rw_size = phdr.p_memsz }; - fn.supply_object_info(info); + fn.supply_object_info(info); + }); }); }); } @@ -771,9 +764,11 @@ void Genode::Dynamic_linker::_for_each_loaded_object(Env &, For_each_fn const &f void Dynamic_linker::keep(Env &, char const *binary) { - Elf_object::obj_list()->for_each([&] (Elf_object &obj) { - if (Object::Name(binary) == obj.name()) - obj.force_keep(); }); + Object::with_object_list([&] (Object::Object_list &list) { + list.for_each([&] (Object &obj) { + if (Object::Name(binary) == obj.name()) + obj.force_keep(); }); + }); } @@ -829,8 +824,10 @@ void Component::construct(Genode::Env &env) " .. ", Hex(Thread::stack_area_virtual_base() + Thread::stack_area_virtual_size() - 1), ": stack area"); - Elf_object::obj_list()->for_each([] (Object const &obj) { - dump_link_map(obj); }); + Object::with_object_list([] (Object::Object_list &list) { + list.for_each([] (Object const &obj) { + dump_link_map(obj); }); + }); } } catch (...) { } diff --git a/repos/base/src/lib/ldso/shared_object.cc b/repos/base/src/lib/ldso/shared_object.cc index 6c6798a3ef..9ded9a2afc 100644 --- a/repos/base/src/lib/ldso/shared_object.cc +++ b/repos/base/src/lib/ldso/shared_object.cc @@ -27,9 +27,16 @@ static Linker::Root_object const &to_root(void *h) static Linker::Object *find_obj(Genode::addr_t addr) { - for (Linker::Object *e = Linker::obj_list_head(); e; e = e->next_obj()) - if (addr >= e->link_map().addr && addr < e->link_map().addr + e->size()) - return e; + Linker::Object *elf = 0; + Linker::for_each_object([&] (Linker::Object &e) { + if (elf) return; + + if (addr >= e.link_map().addr && addr < e.link_map().addr + e.size()) + elf = &e; + }); + + if (elf) + return elf; throw Genode::Address_info::Invalid_address(); }