From 264189e7569ee7482eefd7e3c378adfe4ed76d4a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 29 Apr 2016 14:55:58 +0200 Subject: [PATCH 1/3] libnm/vpn: don't close the VPN plugin library on cleanup Closing the library will interfere badly as the glib types cannot be unregistered. We must leak the library handle. Switch to dlopen() instead of g_module_open(), because the former supports RTLD_NOLOAD. This is useful for two reasons: - checking the file prior loading only makes any sense when dlopen() would actually load a file anew. - if the library was loaded before, we want to return the handle. On the other hand, if the library was not loaded, we leak the handle. Thereby, refactor the code from if-else blocks to return-early, because the function nicely does individual steps and if one fails just error out. --- libnm-core/nm-vpn-editor-plugin.c | 145 +++++++++++++++++------------- 1 file changed, 82 insertions(+), 63 deletions(-) diff --git a/libnm-core/nm-vpn-editor-plugin.c b/libnm-core/nm-vpn-editor-plugin.c index 792b00f944..a92bbea986 100644 --- a/libnm-core/nm-vpn-editor-plugin.c +++ b/libnm-core/nm-vpn-editor-plugin.c @@ -24,6 +24,8 @@ #include "nm-vpn-editor-plugin.h" +#include + #include "nm-core-internal.h" static void nm_vpn_editor_plugin_default_init (NMVpnEditorPluginInterface *iface); @@ -80,11 +82,15 @@ _nm_vpn_editor_plugin_load (const char *plugin_name, gpointer user_data, GError **error) { - GModule *module = NULL; + void *dl_module = NULL; + gboolean loaded_before; NMVpnEditorPluginFactory factory = NULL; - NMVpnEditorPlugin *editor_plugin = NULL; + gs_unref_object NMVpnEditorPlugin *editor_plugin = NULL; gs_free char *plugin_filename_free = NULL; const char *plugin_filename; + gs_free_error GError *factory_error = NULL; + gs_free char *plug_name = NULL; + gs_free char *plug_service = NULL; g_return_val_if_fail (plugin_name && *plugin_name, NULL); @@ -105,8 +111,14 @@ _nm_vpn_editor_plugin_load (const char *plugin_name, plugin_filename_free = g_module_build_path (NMPLUGINDIR, plugin_name); plugin_filename = plugin_filename_free; } + } - /* _nm_utils_check_module_file() fails with ENOENT if the plugin file + dl_module = dlopen (plugin_filename, RTLD_LAZY | RTLD_LOCAL | RTLD_NOLOAD); + if ( !dl_module + && do_file_checks) { + /* If the module is already loaded, we skip the file checks. + * + * _nm_utils_check_module_file() fails with ENOENT if the plugin file * does not exist. That is relevant, because nm-applet checks for that. */ if (!_nm_utils_check_module_file (plugin_filename, check_owner, @@ -116,78 +128,85 @@ _nm_vpn_editor_plugin_load (const char *plugin_name, return NULL; } - module = g_module_open (plugin_filename, G_MODULE_BIND_LAZY | G_MODULE_BIND_LOCAL); - if (!module) { + if (dl_module) { + loaded_before = TRUE; + } else { + loaded_before = FALSE; + dl_module = dlopen (plugin_filename, RTLD_LAZY | RTLD_LOCAL); + } + + if (!dl_module) { g_set_error (error, NM_VPN_PLUGIN_ERROR, NM_VPN_PLUGIN_ERROR_FAILED, - _("cannot load plugin %s"), plugin_name); + _("cannot load plugin \"%s\": %s"), + plugin_name, + dlerror () ?: "unknown reason"); return NULL; } - if (g_module_symbol (module, "nm_vpn_editor_plugin_factory", (gpointer) &factory)) { - gs_free_error GError *factory_error = NULL; - gboolean success = FALSE; - - editor_plugin = factory (&factory_error); - - g_assert (!editor_plugin || G_IS_OBJECT (editor_plugin)); - - if (editor_plugin) { - gs_free char *plug_name = NULL, *plug_service = NULL; - - /* Validate plugin properties */ - - g_object_get (G_OBJECT (editor_plugin), - NM_VPN_EDITOR_PLUGIN_NAME, &plug_name, - NM_VPN_EDITOR_PLUGIN_SERVICE, &plug_service, - NULL); - - if (!plug_name || !*plug_name) { - g_set_error (error, - NM_VPN_PLUGIN_ERROR, - NM_VPN_PLUGIN_ERROR_FAILED, - _("cannot load VPN plugin in '%s': missing plugin name"), - g_module_name (module)); - } else if ( check_service - && g_strcmp0 (plug_service, check_service) != 0) { - g_set_error (error, - NM_VPN_PLUGIN_ERROR, - NM_VPN_PLUGIN_ERROR_FAILED, - _("cannot load VPN plugin in '%s': invalid service name"), - g_module_name (module)); - } else { - /* Success! */ - g_object_set_data_full (G_OBJECT (editor_plugin), "gmodule", module, - (GDestroyNotify) g_module_close); - success = TRUE; - } - } else { - if (factory_error) { - g_propagate_error (error, factory_error); - factory_error = NULL; - } else { - g_set_error (error, - NM_VPN_PLUGIN_ERROR, - NM_VPN_PLUGIN_ERROR_FAILED, - _("unknown error initializing plugin %s"), plugin_name); - } - } - - if (!success) { - g_module_close (module); - g_clear_object (&editor_plugin); - } - } else { + factory = dlsym (dl_module, "nm_vpn_editor_plugin_factory"); + if (!factory) { g_set_error (error, NM_VPN_PLUGIN_ERROR, NM_VPN_PLUGIN_ERROR_FAILED, _("failed to load nm_vpn_editor_plugin_factory() from %s (%s)"), - g_module_name (module), g_module_error ()); - g_module_close (module); + plugin_name, dlerror ()); + dlclose (dl_module); + return NULL; } - return editor_plugin; + editor_plugin = factory (&factory_error); + + if (loaded_before) { + /* we want to leak the library, because the factory will register glib + * types, which cannot be unregistered. + * + * However, if the library was already loaded before, we want to return + * our part of the reference count. */ + dlclose (dl_module); + } + + if (!editor_plugin) { + if (factory_error) { + g_propagate_error (error, factory_error); + factory_error = NULL; + } else { + g_set_error (error, + NM_VPN_PLUGIN_ERROR, + NM_VPN_PLUGIN_ERROR_FAILED, + _("unknown error initializing plugin %s"), plugin_name); + } + return NULL; + } + + g_return_val_if_fail (G_IS_OBJECT (editor_plugin), NULL); + + /* Validate plugin properties */ + g_object_get (G_OBJECT (editor_plugin), + NM_VPN_EDITOR_PLUGIN_NAME, &plug_name, + NM_VPN_EDITOR_PLUGIN_SERVICE, &plug_service, + NULL); + + if (!plug_name || !*plug_name) { + g_set_error (error, + NM_VPN_PLUGIN_ERROR, + NM_VPN_PLUGIN_ERROR_FAILED, + _("cannot load VPN plugin in '%s': missing plugin name"), + plugin_name); + return NULL; + } + if ( check_service + && g_strcmp0 (plug_service, check_service) != 0) { + g_set_error (error, + NM_VPN_PLUGIN_ERROR, + NM_VPN_PLUGIN_ERROR_FAILED, + _("cannot load VPN plugin in '%s': invalid service name"), + plugin_name); + return NULL; + } + + return nm_unauto (&editor_plugin); } /** From cd39cbfc1f56b97e6e3132bccdeb892e12a65350 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 29 Apr 2016 15:00:09 +0200 Subject: [PATCH 2/3] device: don't unload device plugins on failure --- src/devices/nm-device-factory.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/devices/nm-device-factory.c b/src/devices/nm-device-factory.c index 482eb1855b..7873756f9e 100644 --- a/src/devices/nm-device-factory.c +++ b/src/devices/nm-device-factory.c @@ -512,20 +512,20 @@ nm_device_factory_manager_load_factories (NMDeviceFactoryManagerFactoryFunc call continue; } + /* after loading glib types from the plugin, we cannot unload the library anymore. + * Make it resident. */ + g_module_make_resident (plugin); + factory = create_func (&error); if (!factory) { nm_log_warn (LOGD_HW, "(%s): failed to initialize device factory: %s", item, NM_G_ERROR_MSG (error)); g_clear_error (&error); - g_module_close (plugin); continue; } g_clear_error (&error); - if (_add_factory (factory, TRUE, g_module_name (plugin), callback, user_data)) - g_module_make_resident (plugin); - else - g_module_close (plugin); + _add_factory (factory, TRUE, g_module_name (plugin), callback, user_data); g_object_unref (factory); } From c6a92224a4d49136addfd13fb933b6329b47a1f3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 29 Apr 2016 15:00:09 +0200 Subject: [PATCH 3/3] settings: don't unload settings plugins on failure Also, registering a weak-pointer to close the module that was just made as resident is pointless. --- src/settings/nm-settings.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index b91247a9ae..da0a59493e 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -847,18 +847,19 @@ load_plugin: break; } + /* after accessing the plugin we cannot unload it anymore, because the glib + * types cannot be properly unregistered. */ + g_module_make_resident (plugin); + obj = (*factory_func) (); if (!obj || !NM_IS_SETTINGS_PLUGIN (obj)) { g_set_error (error, NM_SETTINGS_ERROR, NM_SETTINGS_ERROR_FAILED, "Plugin '%s' returned invalid system config object.", pname); success = FALSE; - g_module_close (plugin); break; } - g_module_make_resident (plugin); - g_object_weak_ref (obj, (GWeakNotify) g_module_close, plugin); g_object_set_data_full (obj, PLUGIN_MODULE_PATH, path, g_free); path = NULL; if (add_plugin (self, NM_SETTINGS_PLUGIN (obj)))