[Libguestfs] [PATCH 2/3] Avoid race conditions when nbdkit exits.

[Richard W.M. Jones]

There are several race conditions where nbdkit exits (calling
plugin_cleanup() which unloads the plugin), while the plugin is either
in a callback or in plugin.close().

Avoid these by:

(1) Taking a shared lock around all plugin callbacks.

(2) Taking the same as an exclusive lock in plugin_cleanup.

This delays plugin_cleanup until all callbacks have finished.

(3) Removing a few unnecessary ‘assert (dl)’.

This is necessary because the plugin can now be unloaded while holding
locks.  ‘dl’ is not needed to lock or unlock the plugin so asserting
it is useless.

(4) Don't call plugin.close on the quit path.

Another thread could be unloading the plugin so we cannot call
the .close callback.
---
 src/connections.c | 10 ++++++++--
 src/plugins.c     | 20 +++++++++++++-------
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/src/connections.c b/src/connections.c
index 46609f0..0cbf54a 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -224,8 +224,14 @@ free_connection (struct connection *conn)
 
   pthread_mutex_destroy (&conn->request_lock);
 
-  if (conn->handle)
-    plugin_close (conn);
+  /* Don't call the plugin again if quit has been set because the main
+   * thread will be in the process of unloading it.  The plugin.unload
+   * callback should always be called.
+   */
+  if (!quit) {
+    if (conn->handle)
+      plugin_close (conn);
+  }
 
   free (conn);
 }
diff --git a/src/plugins.c b/src/plugins.c
index d2d0b39..f5056d9 100644
--- a/src/plugins.c
+++ b/src/plugins.c
@@ -48,6 +48,7 @@
 
 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;
 
 /* Maximum read or write request that we will handle. */
 #define MAX_REQUEST_SIZE (64 * 1024 * 1024)
@@ -155,6 +156,11 @@ void
 plugin_cleanup (void)
 {
   if (dl) {
+    /* Acquiring this lock prevents any plugin callbacks from running
+     * simultaneously.
+     */
+    pthread_rwlock_wrlock (&unload_prevention_lock);
+
     debug ("%s: unload", filename);
     if (plugin.unload)
       plugin.unload ();
@@ -163,6 +169,8 @@ plugin_cleanup (void)
     dl = NULL;
     free (filename);
     filename = NULL;
+
+    pthread_rwlock_unlock (&unload_prevention_lock);
   }
 }
 
@@ -299,8 +307,6 @@ plugin_config_complete (void)
 void
 plugin_lock_connection (void)
 {
-  assert (dl);
-
   if (plugin._thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS) {
     debug ("%s: acquire connection lock", filename);
     pthread_mutex_lock (&connection_lock);
@@ -310,8 +316,6 @@ plugin_lock_connection (void)
 void
 plugin_unlock_connection (void)
 {
-  assert (dl);
-
   if (plugin._thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS) {
     debug ("%s: release connection lock", filename);
     pthread_mutex_unlock (&connection_lock);
@@ -321,8 +325,6 @@ plugin_unlock_connection (void)
 void
 plugin_lock_request (struct connection *conn)
 {
-  assert (dl);
-
   if (plugin._thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS) {
     debug ("acquire global request lock");
     pthread_mutex_lock (&all_requests_lock);
@@ -332,12 +334,16 @@ plugin_lock_request (struct connection *conn)
     debug ("acquire per-connection request lock");
     pthread_mutex_lock (connection_get_request_lock (conn));
   }
+
+  debug ("acquire unload prevention lock");
+  pthread_rwlock_rdlock (&unload_prevention_lock);
 }
 
 void
 plugin_unlock_request (struct connection *conn)
 {
-  assert (dl);
+  debug ("release unload prevention lock");
+  pthread_rwlock_unlock (&unload_prevention_lock);
 
   if (plugin._thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS) {
     debug ("release per-connection request lock");
-- 
2.13.2