Hi Justin, I hate this patch, but can you please check if it fixes your crash-during-exit problem that you reported yesterday?
-- Nadav Har'El [email protected] On Sun, Aug 28, 2016 at 3:29 PM, Nadav Har'El <[email protected]> wrote: > The change to osv::run() (and friends) to reuse the calling thread rather > than start a new one to run a new shared-object introduced a new problem: > > The new shared object may use pthread_key_create() to add destructors to > run when closing threads. There is (currently) a global list of these > destructors and once added they never get removed (even if the object is > unloaded). The danger is that osv::run() runs code which adds such a > destructors and allocates data (with pthread_setspecific()) and then the > program is unloaded - but the thread will only exit (and the destructors > run) later - and crash when trying to run the destructor pointing to > already-deleted code. > > Previously we didn't have this problem, because osv::run() had a new > thread, > which exited (and ran the destructors) *before* the reference counter on > the application dropped to 0 and the shared object was unloaded. > > A hacky workaround added in this patch is to run in > application::start_and_join - used by osv::run, application::run_and_join > and osv_execve - a new function which runs all these destructors. This > assumes the thread starting these run functions did not have any thread- > specific data (it would be deleted too!). It's a hack, but should be good > enough for use cases like osv_execve() which anyway uses a fresh thread > for running the application. > > Signed-off-by: Nadav Har'El <[email protected]> > --- > libc/pthread.hh | 4 ++++ > core/app.cc | 9 +++++++++ > libc/pthread.cc | 38 ++++++++++++++++++++++++++------------ > 3 files changed, 39 insertions(+), 12 deletions(-) > > diff --git a/libc/pthread.hh b/libc/pthread.hh > index 53e9db9..1a4d02c 100644 > --- a/libc/pthread.hh > +++ b/libc/pthread.hh > @@ -20,6 +20,10 @@ extern "C" { > > #ifdef __cplusplus > } > + > +namespace pthread_private { > +void run_tsd_dtors(); > +} > #endif > > #endif /* PTHREAD_HH_ */ > diff --git a/core/app.cc b/core/app.cc > index 56fd18b..ad3145f 100644 > --- a/core/app.cc > +++ b/core/app.cc > @@ -17,6 +17,7 @@ > #include <algorithm> > #include <boost/range/algorithm/transform.hpp> > #include <osv/wait_record.hh> > +#include "libc/pthread.hh" > > using namespace boost::range; > > @@ -251,6 +252,14 @@ void application::start_and_join(waiter* > setup_waiter) > } > _thread = pthread_self(); // may be null if the caller is not a > pthread. > main(); > + // FIXME: run_tsd_dtors() is a hack - If the new program registered a > + // destructor via pthread_key_create() and this thread keeps living > with > + // data for this key, the destructor function, part of the new > program, > + // may be called after the program is unloaded - and crash. Let's run > + // the destructors now. This is wrong if the calling program has its > own > + // thread-local data. It is fine if the thread was created specially > for > + // running start_and_join (or run_and_join() or osv::run()). > + pthread_private::run_tsd_dtors(); > sched::thread::current()->set_name(original_name); > sched::thread::current()->set_app_runtime(original_app); > original_app.reset(); > diff --git a/libc/pthread.cc b/libc/pthread.cc > index ac7b3f0..62db224 100644 > --- a/libc/pthread.cc > +++ b/libc/pthread.cc > @@ -41,27 +41,41 @@ namespace pthread_private { > __thread pthread_t current_pthread; > __thread int cancel_state = PTHREAD_CANCEL_ENABLE; > > + // NOTE: currently, the list of keys and destructor for each is > global, > + // not per shared object or ELF namespace. So if a shared object uses > + // pthread_key_create() but doesn't call pthread_key_delete() before > + // exiting, the key will be leaked. This is relatively harmless > (beyond > + // running out of keys) unless the shared object is unloaded before > the > + // thread exits and the destructors are run. > + // As a *hack* you can call run_tsd_dtors() before unloading the > object, > + // but this will run all dtors, not just those belonging to the > unloaded > + // object, so this is only useful on a thread especially created for > + // running the object. > __attribute__ ((init_priority ((int)init_prio::pthread))) mutex > tsd_key_mutex; > __attribute__ ((init_priority ((int)init_prio::pthread))) > std::vector<bool> > tsd_used_keys(tsd_nkeys); > __attribute__ ((init_priority ((int)init_prio::pthread))) > std::vector<void (*)(void*)> tsd_dtor(tsd_nkeys); > > + void run_tsd_dtors() { > + bool done = false; > + for (unsigned iter = 0; !done && iter < > PTHREAD_DESTRUCTOR_ITERATIONS; ++iter) { > + done = true; > + for (unsigned i = 0; i < tsd_nkeys; ++i) { > + if (tsd[i] && tsd_dtor[i]) { > + void *val = tsd[i]; > + tsd[i] = nullptr; > + tsd_dtor[i](val); > + done = false; > + } > + } > + } > + } > + > void __attribute__((constructor)) pthread_register_tsd_dtor_ > notifier() > { > sched::thread::register_exit_notifier([] { > - bool done = false; > - for (unsigned iter = 0; !done && iter < > PTHREAD_DESTRUCTOR_ITERATIONS; ++iter) { > - done = true; > - for (unsigned i = 0; i < tsd_nkeys; ++i) { > - if (tsd[i] && tsd_dtor[i]) { > - void *val = tsd[i]; > - tsd[i] = nullptr; > - tsd_dtor[i](val); > - done = false; > - } > - } > - } > + run_tsd_dtors(); > }); > } > > -- > 2.7.4 > > -- 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]. For more options, visit https://groups.google.com/d/optout.
