Libguestfs - Jan 2018 - [PATCH nbdkit] locks: Cache the plugin thread model.

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

On 01/19/2018 09:06 AM, Richard W.M. Jones wrote:
> Commit 876d715e17a59bd25df53be34b942afd96e52da9
> ("Refactor plugin_* functions into a backend struct.") causes
hidden
> crashes in the test suite (which unfortunately don't cause tests to
> fail).
Bummer about the non-failure in the testsuite making it harder to see
the problem.
> 
> 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:
Yep, makes sense.  Patch looks good to go.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org