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

This was underspecified before. --- docs/nbdkit-plugin.pod | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index ceaef1a..47a46da 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -240,6 +240,7 @@ This may be called once just before the plugin is unloaded from memory. Note that it's not guaranteed that C<.unload> will always be called (eg. the server might be killed or segfault), so you should try to make the plugin as robust as possible by not requiring cleanup. +See also L</SHUTDOWN> below. =head2 C<.config> @@ -528,6 +529,16 @@ If none of the above thread models are suitable, then use C<NBDKIT_THREAD_MODEL_PARALLEL> and implement your own locking using C<pthread_mutex_t> etc. +=head1 SHUTDOWN + +When nbdkit receives certain signals it will shut down (see +L<nbdkit(1)/SIGNALS>). The server will wait for any currently running +plugin callbacks to finish and also call the C<.unload> callback +before unloading the plugin. + +Note that it's not guaranteed this can always happen (eg. the server +might be killed by C<SIGKILL> or segfault). + =head1 PARSING SIZE PARAMETERS Use the C<nbdkit_parse_size> utility function to parse human-readable -- 2.13.2