From d60fce3ee6512fe9e8f1199996cc67c6b233da8b Mon Sep 17 00:00:00 2001 From: Mark Reynolds Date: Mon, 9 Feb 2015 16:23:04 -0500 Subject: [PATCH] Ticket 47451 - dynamic plugins - fix crash caused by invalid plugin config Bug Description: After making an invalid plugin change, that should cause the old plugin entry to be reloaded, the server will crash. Fix Description: If a plugin fails to start after applying invalid config changes, we need to make sure it is removed from the global plugin lists, or else a freed plugin can be deferenced later. Also moved the plugin validation check in dse_modify to happen in the preop stage, so invalid configurations do not persist in the dse.ldif https://fedorahosted.org/389/ticket/47451 Valgrind: passed Reviewed by: ? --- dirsrvtests/suites/dynamic-plugins/plugin_tests.py | 18 +++++ ldap/servers/slapd/dse.c | 85 +++++++++++----------- ldap/servers/slapd/plugin.c | 56 +++++++------- 3 files changed, 91 insertions(+), 68 deletions(-) diff --git a/dirsrvtests/suites/dynamic-plugins/plugin_tests.py b/dirsrvtests/suites/dynamic-plugins/plugin_tests.py index 315547b..a552142 100644 --- a/dirsrvtests/suites/dynamic-plugins/plugin_tests.py +++ b/dirsrvtests/suites/dynamic-plugins/plugin_tests.py @@ -2174,6 +2174,8 @@ def test_rootdn(inst, args=None): PLUGIN_DN = 'cn=' + PLUGIN_ROOTDN_ACCESS + ',cn=plugins,cn=config' + log.info('Testing ' + PLUGIN_ROOTDN_ACCESS + '...') + ############################################################################ # Configure plugin ############################################################################ @@ -2223,12 +2225,28 @@ def test_rootdn(inst, args=None): # Change the config ############################################################################ + # Bind as the user who can make updates to the config try: inst.simple_bind_s(USER1_DN, 'password') except ldap.LDAPError, e: log.error('test_rootdn: failed to bind as user1') assert False + # First, test that invalid plugin changes are rejected + try: + inst.modify_s(PLUGIN_DN, [(ldap.MOD_REPLACE, 'rootdn-deny-ip', '12.12.ZZZ.12')]) + log.fatal('test_rootdn: Incorrectly allowed to add invalid "rootdn-deny-ip: 12.12.ZZZ.12"') + assert False + except ldap.LDAPError: + pass + + try: + inst.modify_s(PLUGIN_DN, [(ldap.MOD_REPLACE, 'rootdn-allow-host', 'host._.com')]) + log.fatal('test_rootdn: Incorrectly allowed to add invalid "rootdn-allow-host: host._.com"') + assert False + except ldap.LDAPError: + pass + # Remove the restriction try: inst.modify_s(PLUGIN_DN, [(ldap.MOD_DELETE, 'rootdn-allow-ip', None)]) diff --git a/ldap/servers/slapd/dse.c b/ldap/servers/slapd/dse.c index f80178e..9bb5914 100644 --- a/ldap/servers/slapd/dse.c +++ b/ldap/servers/slapd/dse.c @@ -157,7 +157,7 @@ static struct dse_node *dse_find_node( struct dse* pdse, const Slapi_DN *dn ); static int dse_modify_plugin(Slapi_Entry *pre_entry, Slapi_Entry *post_entry, char *returntext); static int dse_add_plugin(Slapi_Entry *entry, char *returntext); static int dse_delete_plugin(Slapi_Entry *entry, char *returntext); -static void dse_post_modify_plugin(Slapi_Entry *entryBefore, Slapi_Entry *entryAfter, LDAPMod **mods); +static int dse_pre_modify_plugin(Slapi_Entry *entryBefore, Slapi_Entry *entryAfter, LDAPMod **mods); /* richm: In almost all modes e.g. db2ldif, ldif2db, etc. we do not need/want @@ -1916,13 +1916,41 @@ dse_modify(Slapi_PBlock *pb) /* JCM There should only be one exit point from thi } } else { /* - * Check if we are enabling/disabling a plugin + * If we are using dynamic plugins, and we are modifying a plugin + * we need to do some additional checks. First, check if we are + * enabling/disabling a plugin. Then make sure the plugin still + * starts after applying the plugin changes. */ - if((plugin_started = dse_modify_plugin(ec, ecc, returntext)) == -1){ - returncode = LDAP_UNWILLING_TO_PERFORM; - rc = SLAPI_DSE_CALLBACK_ERROR; - } else { - rc = SLAPI_DSE_CALLBACK_OK; + rc = SLAPI_DSE_CALLBACK_OK; + if(config_get_dynamic_plugins() && + slapi_entry_attr_hasvalue(ec, SLAPI_ATTR_OBJECTCLASS, "nsSlapdPlugin") ) + { + if((plugin_started = dse_modify_plugin(ec, ecc, returntext)) == -1){ + returncode = LDAP_UNWILLING_TO_PERFORM; + rc = SLAPI_DSE_CALLBACK_ERROR; + retval = -1; + goto done; + } + /* + * If this is not a internal operation, make sure the plugin + * can be restarted. + */ + if(!internal_op){ + if(dse_pre_modify_plugin(ec, ecc, mods)){ + char *errtext; + slapi_pblock_get(pb, SLAPI_PB_RESULT_TEXT, &errtext); + if (errtext) { + PL_strncpyz(returntext, + "Failed to apply plugin config change, " + "check the errors log for more info.", + sizeof(returntext)); + } + returncode = LDAP_UNWILLING_TO_PERFORM; + rc = SLAPI_DSE_CALLBACK_ERROR; + retval = -1; + goto done; + } + } } } } @@ -2041,44 +2069,20 @@ dse_modify(Slapi_PBlock *pb) /* JCM There should only be one exit point from thi } } } - /* - * Perform postop plugin configuration changes unless this is an internal operation - */ - if(!internal_op){ - if(returncode == LDAP_SUCCESS){ - dse_post_modify_plugin(ec, ecc, mods); - } else if(plugin_started){ - if(plugin_started == 1){ - /* the op failed, turn the plugin off */ - plugin_delete(ecc, returntext, 0 /* not locked */); - } else if (plugin_started == 2){ - /* - * This probably can't happen, but... - * the op failed, turn the plugin back on. - */ - plugin_add(ecc, returntext, 0 /* not locked */); - } - } - } slapi_send_ldap_result( pb, returncode, NULL, returntext[0] ? returntext : NULL, 0, NULL ); return dse_modify_return(retval, ec, ecc); } -static void -dse_post_modify_plugin(Slapi_Entry *entryBefore, Slapi_Entry *entryAfter, LDAPMod **mods) +static int +dse_pre_modify_plugin(Slapi_Entry *entryBefore, Slapi_Entry *entryAfter, LDAPMod **mods) { char *enabled = NULL; int restart_plugin = 1; + int rc = 0; int i; - if (!slapi_entry_attr_hasvalue(entryBefore, SLAPI_ATTR_OBJECTCLASS, "nsSlapdPlugin") || - !config_get_dynamic_plugins() ){ - /* not a plugin, or we aren't applying updates dynamically - just move on */ - return; - } - /* * Only check the mods if the plugin is enabled - no need to restart a plugin if it's not running. */ @@ -2094,14 +2098,15 @@ dse_post_modify_plugin(Slapi_Entry *entryBefore, Slapi_Entry *entryAfter, LDAPMo } if(restart_plugin){ /* for all other plugin config changes, restart the plugin */ if(plugin_restart(entryBefore, entryAfter) != LDAP_SUCCESS){ - slapi_log_error(SLAPI_LOG_FATAL,"dse_post_modify_plugin", "The configuration change " - "for plugin (%s) could not be applied dynamically, and will be ignored until " - "the server is restarted.\n", + slapi_log_error(SLAPI_LOG_FATAL,"dse_pre_modify_plugin", + "The configuration change for plugin (%s) could not be applied.\n", slapi_entry_get_dn(entryBefore)); + rc = -1; } } } slapi_ch_free_string(&enabled); + return rc; } /* @@ -2118,12 +2123,6 @@ dse_modify_plugin(Slapi_Entry *pre_entry, Slapi_Entry *post_entry, char *returnt { int rc = LDAP_SUCCESS; - if (!slapi_entry_attr_hasvalue(post_entry, SLAPI_ATTR_OBJECTCLASS, "nsSlapdPlugin") || - !config_get_dynamic_plugins() ) - { - return rc; - } - if( slapi_entry_attr_hasvalue(pre_entry, "nsslapd-pluginEnabled", "on") && slapi_entry_attr_hasvalue(post_entry, "nsslapd-pluginEnabled", "off") ) { diff --git a/ldap/servers/slapd/plugin.c b/ldap/servers/slapd/plugin.c index b0b18e7..94aba7f 100644 --- a/ldap/servers/slapd/plugin.c +++ b/ldap/servers/slapd/plugin.c @@ -99,6 +99,7 @@ static int plugin_delete_check_dependency(struct slapdplugin *plugin_entry, int static char *plugin_get_type_str( int type ); static void plugin_cleanup_list(); static int plugin_remove_plugins(struct slapdplugin *plugin_entry, char *plugin_type); +static void plugin_remove_from_shutdown(struct slapdplugin *plugin_entry); static PLHashTable *global_plugin_dns = NULL; @@ -2310,39 +2311,39 @@ plugin_restart(Slapi_Entry *pentryBefore, Slapi_Entry *pentryAfter) /* We can not restart the critical plugins */ if(plugin_is_critical(pentryBefore)){ LDAPDebug(LDAP_DEBUG_PLUGIN, "plugin_restart: Plugin (%s) is critical to server operation. " - "Any changes will not take effect until the server is restarted.\n", - slapi_entry_get_dn(pentryBefore),0,0); + "Any changes will not take effect until the server is restarted.\n", + slapi_entry_get_dn(pentryBefore),0,0); return 1; /* failure - dse code will log a fatal message */ } slapi_rwlock_wrlock(global_rwlock); slapi_td_set_plugin_locked(); - if(plugin_delete(pentryBefore, returntext, 1) == LDAP_SUCCESS){ - if(plugin_add(pentryAfter, returntext, 1) == LDAP_SUCCESS){ - LDAPDebug(LDAP_DEBUG_PLUGIN, "plugin_restart: Plugin (%s) has been successfully " - "restarted after configuration change.\n", - slapi_entry_get_dn(pentryAfter),0,0); - } else { - LDAPDebug(LDAP_DEBUG_ANY, "plugin_restart: Plugin (%s) failed to restart after " - "configuration change (%s). Reverting to original plugin entry.\n", - slapi_entry_get_dn(pentryAfter), returntext, 0); - if(plugin_add(pentryBefore, returntext, 1) == LDAP_SUCCESS){ - LDAPDebug(LDAP_DEBUG_ANY, "plugin_restart: Plugin (%s) failed to reload " - "original plugin (%s)\n",slapi_entry_get_dn(pentryBefore), returntext, 0); - } - rc = 1; - } - } else { - LDAPDebug(LDAP_DEBUG_ANY,"plugin_restart: failed to disable/stop the plugin (%s): %s\n", - slapi_entry_get_dn(pentryBefore), returntext, 0); - rc = 1; - } + if(plugin_delete(pentryBefore, returntext, 1) == LDAP_SUCCESS){ + if(plugin_add(pentryAfter, returntext, 1) == LDAP_SUCCESS){ + LDAPDebug(LDAP_DEBUG_PLUGIN, "plugin_restart: Plugin (%s) has been successfully " + "restarted after configuration change.\n", + slapi_entry_get_dn(pentryAfter),0,0); + } else { + LDAPDebug(LDAP_DEBUG_ANY, "plugin_restart: Plugin (%s) failed to restart after " + "configuration change (%s). Reverting to original plugin entry.\n", + slapi_entry_get_dn(pentryAfter), returntext, 0); + if(plugin_add(pentryBefore, returntext, 1) != LDAP_SUCCESS){ + LDAPDebug(LDAP_DEBUG_ANY, "plugin_restart: Plugin (%s) failed to reload " + "original plugin (%s)\n",slapi_entry_get_dn(pentryBefore), returntext, 0); + } + rc = 1; + } + } else { + LDAPDebug(LDAP_DEBUG_ANY,"plugin_restart: failed to disable/stop the plugin (%s): %s\n", + slapi_entry_get_dn(pentryBefore), returntext, 0); + rc = 1; + } - slapi_rwlock_unlock(global_rwlock); - slapi_td_set_plugin_unlocked(); + slapi_rwlock_unlock(global_rwlock); + slapi_td_set_plugin_unlocked(); - return rc; + return rc; } static int @@ -2946,6 +2947,11 @@ plugin_setup(Slapi_Entry *plugin_entry, struct slapi_componentid *group, PR_snprintf (returntext, SLAPI_DSE_RETURNTEXT_SIZE,"Init function \"%s\" for \"%s\" plugin in " "library \"%s\" failed.",plugin->plg_initfunc, plugin->plg_name, plugin->plg_libpath); status = -1; + /* + * The init function might have added the plugin to the global list before + * it failed - attempt to remove it just in case it was added. + */ + plugin_remove_plugins(plugin, value); slapi_ch_free((void**)&value); goto PLUGIN_CLEANUP; } -- 1.9.3