Keep the bank lock until plugins are loaded.

This is a bit ugly but it fixes two race conditions:
 - loading plugins while another thread is initializing,
 - using the bank when the first thread has not completed loading plugins.

Unfortunately, there is still a small race when module_need() calls
AllocatePlugin(). It really should not -need to- do that, but the fix would
be quite invasive. We would basically need to store plugin callbacks by names
rather than function pointers. Then the module descriptors would be fully
serializable, so we would not need to re-describe plugins when loading their
shared object. That would also fix the last known corruption bug in the plugins
cache.

src/libvlc.c

@@ -329,7 +329,7 @@ int libvlc_InternalInit( libvlc_int_t *p_libvlc, int i_argc,
 
     if( config_LoadCmdLine( p_libvlc, &i_argc, ppsz_argv, true ) )
     {
-        module_EndBank( p_libvlc );
+        module_EndBank( p_libvlc, false );
         return VLC_EGENERIC;
     }
 
@@ -429,7 +429,7 @@ int libvlc_InternalInit( libvlc_int_t *p_libvlc, int i_argc,
     if( b_exit )
     {
         free( priv->psz_configfile );
-        module_EndBank( p_libvlc );
+        module_EndBank( p_libvlc, false );
         return i_ret;
     }
 
@@ -455,7 +455,7 @@ int libvlc_InternalInit( libvlc_int_t *p_libvlc, int i_argc,
         /* Translate "C" to the language code: "fr", "en_GB", "nl", "ru"... */
         msg_Dbg( p_libvlc, "translation test: code is \"%s\"", _("C") );
 
-        module_EndBank( p_libvlc );
+        module_EndBank( p_libvlc, false );
         module_InitBank( p_libvlc );
         if( !config_GetInt( p_libvlc, "ignore-config" ) )
             config_LoadConfigFile( p_libvlc, "main" );
@@ -541,7 +541,7 @@ int libvlc_InternalInit( libvlc_int_t *p_libvlc, int i_argc,
     if( b_exit )
     {
         free( priv->psz_configfile );
-        module_EndBank( p_libvlc );
+        module_EndBank( p_libvlc, true );
         return i_ret;
     }
 
@@ -569,7 +569,7 @@ int libvlc_InternalInit( libvlc_int_t *p_libvlc, int i_argc,
         PauseConsole();
 #endif
         free( priv->psz_configfile );
-        module_EndBank( p_libvlc );
+        module_EndBank( p_libvlc, true );
         return VLC_EGENERIC;
     }
     priv->i_verbose = config_GetInt( p_libvlc, "verbose" );
@@ -826,7 +826,7 @@ int libvlc_InternalInit( libvlc_int_t *p_libvlc, int i_argc,
         {
             module_unneed( p_libvlc, priv->p_memcpy_module );
         }
-        module_EndBank( p_libvlc );
+        module_EndBank( p_libvlc, true );
         free( priv->psz_configfile );
         return VLC_EGENERIC;
     }
@@ -1109,7 +1109,7 @@ void libvlc_InternalCleanup( libvlc_int_t *p_libvlc )
     }
 
     /* Free module bank. It is refcounted, so we call this each time  */
-    module_EndBank( p_libvlc );
+    module_EndBank( p_libvlc, true );
 
     FREENULL( priv->psz_configfile );
     var_DelCallback( p_libvlc, "key-pressed", vlc_key_to_action,

src/modules/modules.c

@@ -148,28 +148,39 @@ void __module_InitBank( vlc_object_t *p_this )
     else
         p_module_bank->i_usage++;
 
-    vlc_mutex_unlock( &module_lock );
+    /* We do retain the module bank lock until the plugins are loaded as well.
+     * This is ugly, this staged loading approach is needed: LibVLC gets
+     * some configuration parameters relevant to loading the plugins from
+     * the main (builtin) module. The module bank becomes shared read-only data
+     * once it is ready, so we need to fully serialize initialization.
+     * DO NOT UNCOMMENT the following line unless you managed to squeeze
+     * module_LoadPlugins() before you unlock the mutex. */
+    /*vlc_mutex_unlock( &module_lock );*/
 }
 
-
+#undef module_EndBank
 /**
- * End bank
- *
  * Unloads all unused plugin modules and empties the module
  * bank in case of success.
  * \param p_this vlc object structure
  * \return nothing
  */
-void __module_EndBank( vlc_object_t *p_this )
+void module_EndBank( vlc_object_t *p_this, bool b_plugins )
 {
-    module_bank_t *p_bank;
+    module_bank_t *p_bank = p_module_bank;
+
+    assert (p_bank != NULL);
 
     /* Save the configuration */
     config_AutoSaveConfigFile( p_this );
 
-    vlc_mutex_lock( &module_lock );
-    p_bank = p_module_bank;
-    assert (p_bank != NULL);
+    /* If plugins were _not_ loaded, then the caller still has the bank lock
+     * from module_InitBank(). */
+    if( b_plugins )
+        vlc_mutex_lock( &module_lock );
+    /*else
+        vlc_assert_locked( &module_lock ); not for static mutexes :( */
+
     if( --p_bank->i_usage > 0 )
     {
         vlc_mutex_unlock( &module_lock );
@@ -218,34 +229,32 @@ void __module_EndBank( vlc_object_t *p_this )
 
 #undef module_LoadPlugins
 /**
- * Load all plugins
- *
- * Load all plugin modules we can find.
+ * Loads module descriptions for all available plugins.
  * Fills the module bank structure with the plugin modules.
+ *
  * \param p_this vlc object structure
  * \return nothing
  */
 void module_LoadPlugins( vlc_object_t * p_this, bool b_cache_delete )
 {
+    module_bank_t *p_bank = p_module_bank;
+
+    assert( p_bank );
+    /*vlc_assert_locked( &module_lock ); not for static mutexes :( */
+
 #ifdef HAVE_DYNAMIC_PLUGINS
-    vlc_mutex_lock( &module_lock );
-    if( p_module_bank->b_plugins )
+    if( p_bank->i_usage == 1 )
     {
-        vlc_mutex_unlock( &module_lock );
-        return;
+        msg_Dbg( p_this, "checking plugin modules" );
+        p_module_bank->b_cache = config_GetInt( p_this, "plugins-cache" ) > 0;
+
+        if( p_module_bank->b_cache || b_cache_delete )
+            CacheLoad( p_this, p_module_bank, b_cache_delete );
+        AllocateAllPlugins( p_this, p_module_bank );
     }
+#endif
     p_module_bank->b_plugins = true;
     vlc_mutex_unlock( &module_lock );
-
-    msg_Dbg( p_this, "checking plugin modules" );
-    p_module_bank->b_cache = config_GetInt( p_this, "plugins-cache" ) > 0;
-
-    if( p_module_bank->b_cache || b_cache_delete )
-        CacheLoad( p_this, p_module_bank, b_cache_delete );
-
-    /* FIXME: race - do this under the lock */
-    AllocateAllPlugins( p_this, p_module_bank );
-#endif
 }
 
 /**

src/modules/modules.h

@@ -152,8 +152,8 @@ module_t *vlc_submodule_create (module_t *module);
 void  __module_InitBank        ( vlc_object_t * );
 void module_LoadPlugins( vlc_object_t *, bool );
 #define module_LoadPlugins(a,b) module_LoadPlugins(VLC_OBJECT(a),b)
-#define module_EndBank(a)      __module_EndBank(VLC_OBJECT(a))
-void  __module_EndBank         ( vlc_object_t * );
+void module_EndBank( vlc_object_t *, bool );
+#define module_EndBank(a,b) module_EndBank(VLC_OBJECT(a), b)
 
 /* Low-level OS-dependent handler */
 int  module_Load   (vlc_object_t *, const char *, module_handle_t *);