[PATCH] elf: fix thread visiblity issue

[Waldemar Kozaczuk]

The commit fe37650fbbcd352806bab3e9e70949a65fadf29b to address
the issue #1067 relaxed the elf visibility rules while being loaded
to child threads so that they can resolve symbols if any threads
are created by the INIT functions.

Unfortunately inadvertently that patch allowed access to elf
ebjects too early in some cases and would lead to a crash. Here is a
concrete scenario when this would happen with the 'cli' app:

1. Thread A resolves objects of the httpserver app.
2. Thread A starts the httpserver app on new thread B.
3. Thread A continues to load objects of the cli app.
4. Thread B tries to resolve some symbols through elf_resolve_pltgot() for example
and ends up attempting to look up symbols of cli object that is just being loaded
by thread A and is not "ready" yet. However because thread B is a child of A
it can do so.

Clearly the example above illustrates a hole in the original patch.
This patch fills this hole by restricting back the access to the symbols
of the object being loaded to the thread doing it only as it used to be be.
And it only relaxes the access to child threads for the brief time while object
is being initialized.

It does so by introducing the elf access visibility levels:
- Public
- ThreadOnly
- ThreadAndItsChildren

The Public and ThreadOnly and equivalent to the old - nullptr, non-nullptr visibility attribute.
The ThreadAndItsChildren is only set to for the period the INIT functions are being called.

References #1067

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---
core/elf.cc | 31 ++++++++++++++++++++++---------
include/osv/elf.hh | 11 +++++++++--
2 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/core/elf.cc b/core/elf.cc
index 36466807..a25fb3c1 100644
--- a/core/elf.cc
+++ b/core/elf.cc
@@ -122,7 +122,8 @@ object::object(program& prog, std::string pathname)
, _dynamic_table(nullptr)
, _module_index(_prog.register_dtv(this))
, _is_executable(false)
- , _visibility(nullptr)
+ , _visibility_thread(nullptr)
+ , _visibility_level(VisibilityLevel::Public)
{
elf_debug("Instantiated\n");
}
@@ -140,22 +141,34 @@ ulong object::module_index() const

bool object::visible(void) const
{
- auto v = _visibility.load(std::memory_order_acquire);
- if (v == nullptr) {
+ auto level = _visibility_level.load(std::memory_order_acquire);
+ if (level == VisibilityLevel::Public) {
return true;
}
+
+ auto thread = _visibility_thread.load(std::memory_order_acquire);
+ if (!thread) { //Unlikely, but ...
+ return true;
+ }
+
+ if (level == VisibilityLevel::ThreadOnly) {
+ return thread == sched::thread::current();
+ }
+
+ // Is current thread the same as "thread" or is it a child ?`
for (auto t = sched::thread::current(); t != nullptr; t = t->parent()) {
- if (t == v) {
+ if (t == thread) {
return true;
}
}
return false;
}

-void object::setprivate(bool priv)
+void object::set_visibility(elf::VisibilityLevel visibilityLevel)
{
- _visibility.store(priv ? sched::thread::current() : nullptr,
+ _visibility_thread.store(visibilityLevel == VisibilityLevel::Public ? nullptr : sched::thread::current(),
std::memory_order_release);
+ _visibility_level.store(visibilityLevel, std::memory_order_release);
}


@@ -1323,7 +1336,7 @@ program::load_object(std::string name, std::vector<std::string> extra_path,
auto ef = std::shared_ptr<object>(new file(*this, f, name),
[=](object *obj) { remove_object(obj); });
ef->set_base(_next_alloc);
- ef->setprivate(true);
+ ef->set_visibility(ThreadOnly);
// We need to push the object at the end of the list (so that the main
// shared object gets searched before the shared libraries it uses),
// with one exception: the kernel needs to remain at the end of the
@@ -1395,13 +1408,13 @@ void program::init_library(int argc, char** argv)
// first) and finally make the loaded objects visible in search order.
auto size = loaded_objects.size();
for (unsigned i = 0; i < size; i++) {
- loaded_objects[i]->setprivate(true);
+ loaded_objects[i]->set_visibility(ThreadAndItsChildren);
}
for (int i = size - 1; i >= 0; i--) {
loaded_objects[i]->run_init_funcs(argc, argv);
}
for (unsigned i = 0; i < size; i++) {
- loaded_objects[i]->setprivate(false);
+ loaded_objects[i]->set_visibility(Public);
}
_loaded_objects_stack.pop();
}
diff --git a/include/osv/elf.hh b/include/osv/elf.hh
index 8e37aebb..32f3eeaa 100644
--- a/include/osv/elf.hh
+++ b/include/osv/elf.hh
@@ -329,6 +329,12 @@ struct [[gnu::packed]] Elf64_Shdr {
Elf64_Xword sh_entsize; /* Size of entries, if section has table */
};

+enum VisibilityLevel {
+ Public,
+ ThreadOnly,
+ ThreadAndItsChildren
+};
+
class object: public std::enable_shared_from_this<elf::object> {
public:
explicit object(program& prog, std::string pathname);
@@ -452,10 +458,11 @@ protected:
return _static_tls_offset + get_tls_size();
}
private:
- std::atomic<void*> _visibility;
+ std::atomic<void*> _visibility_thread;
+ std::atomic<VisibilityLevel> _visibility_level;
bool visible(void) const;
public:
- void setprivate(bool);
+ void set_visibility(VisibilityLevel);
};

class file : public object {
--
2.20.1

[Nadav Har'El]

On Sat, Jan 11, 2020 at 8:00 PM Waldemar Kozaczuk <jwkoz...@gmail.com> wrote:

The commit fe37650fbbcd352806bab3e9e70949a65fadf29b to address

I didn't notice you committed it :-( Sorry about the slow reviews....

I thought you said it wasn't possible to keep a parent thread pointer - because a parent thread pointer can go away at any time! I'll go review that patch next (belatedly).

Anyway, I'll commit this patch, and if we need more fixes later we can do them later.

Nitpick (but we can fix this later, if at all) - I don't think you need to use release order (and in the read - acquire) for both

fields. I think it's enough to do it for one. But since this is happens rarely (usually you just read the level), I'll let this slide.

--
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/20200111180020.22335-1-jwkozaczuk%40gmail.com.

[Commit Bot]

From: Waldemar Kozaczuk <jwkoz...@gmail.com>
Committer: Nadav Har'El <n...@scylladb.com>
Branch: master

elf: fix thread visiblity issue

Message-Id: <20200111180020.2...@gmail.com>

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