Commit 876d715e17a59bd25df53be34b942afd96e52da9 ("Refactor plugin_* functions into a backend struct.") causes hidden crashes in the test suite (which unfortunately don't cause tests to fail). These all come from handle_single_connection where we may lock the connection, run a single connection, free the backend, and then try to unlock the connection. Unfortunately the unlock operation has to check the thread model again which fails because the backend has gone away: Program terminated with signal SIGSEGV, Segmentation fault. #0 0x00000000004070ce in unlock_connection () at locks.c:59 59 int thread_model = backend->thread_model (backend); [Current thread is 1 (Thread 0x7fab43243700 (LWP 6676))] (gdb) bt #0 0x00000000004070ce in unlock_connection () at locks.c:59 #1 0x00000000004061b1 in handle_single_connection (sockin=<optimized out>, sockout=<optimized out>) at connections.c:317 #2 0x00000000004087d3 in start_thread (datav=0x229fd00) at sockets.c:262 #3 0x00007fab44e525e1 in start_thread () from /lib64/libpthread.so.0 #4 0x00007fab44b8717f in clone () from /lib64/libc.so.6 Since the thread model cannot be changed after a plugin is loaded, just cache it in src/locks.c after the backend is created. Note that I don't believe commit 876d715e17a59bd25df53be34b942afd96e52da9 caused this breakage. I think it only worked by accident before because it would still have been possible that handle_single_connection was reading the plugin._thread_model field after dlclose. --- src/internal.h | 1 + src/locks.c | 19 +++++++++++-------- src/main.c | 1 + 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/internal.h b/src/internal.h index 28b1aaf..8047b3b 100644 --- a/src/internal.h +++ b/src/internal.h @@ -181,6 +181,7 @@ struct backend { extern struct backend *plugin_register (const char *_filename, void *_dl, struct nbdkit_plugin *(*plugin_init) (void)); /* locks.c */ +extern void lock_init_thread_model (void); extern void lock_connection (void); extern void unlock_connection (void); extern void lock_request (struct connection *conn); diff --git a/src/locks.c b/src/locks.c index 62b2dd0..bd8fd99 100644 --- a/src/locks.c +++ b/src/locks.c @@ -38,15 +38,24 @@ #include "internal.h" +/* Note that the plugin's thread model cannot change after being + * loaded, so caching it here is safe. + */ +static int thread_model; + static pthread_mutex_t connection_lock = PTHREAD_MUTEX_INITIALIZER; static pthread_mutex_t all_requests_lock = PTHREAD_MUTEX_INITIALIZER; static pthread_rwlock_t unload_prevention_lock = PTHREAD_RWLOCK_INITIALIZER; void -lock_connection (void) +lock_init_thread_model (void) { - int thread_model = backend->thread_model (backend); + thread_model = backend->thread_model (backend); +} +void +lock_connection (void) +{ if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS) { debug ("acquire connection lock"); pthread_mutex_lock (&connection_lock); @@ -56,8 +65,6 @@ lock_connection (void) void unlock_connection (void) { - int thread_model = backend->thread_model (backend); - if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS) { debug ("release connection lock"); pthread_mutex_unlock (&connection_lock); @@ -67,8 +74,6 @@ unlock_connection (void) void lock_request (struct connection *conn) { - int thread_model = backend->thread_model (backend); - if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS) { debug ("acquire global request lock"); pthread_mutex_lock (&all_requests_lock); @@ -86,8 +91,6 @@ lock_request (struct connection *conn) void unlock_request (struct connection *conn) { - int thread_model = backend->thread_model (backend); - debug ("release unload prevention lock"); pthread_rwlock_unlock (&unload_prevention_lock); diff --git a/src/main.c b/src/main.c index b3e6bad..90d464a 100644 --- a/src/main.c +++ b/src/main.c @@ -497,6 +497,7 @@ main (int argc, char *argv[]) } backend = open_plugin_so (filename, short_name); + lock_init_thread_model (); if (help) { usage (); -- 2.15.1