[389-commits] dirsrvtests/suites ldap/servers
Mark Reynolds
mreynolds at fedoraproject.org
Mon Feb 9 21:53:25 UTC 2015
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(-)
New commits:
commit d7be25e108a9ef57a4ccae38c88550d2fbe85578
Author: Mark Reynolds <mreynolds at redhat.com>
Date: Mon Feb 9 16:23:04 2015 -0500
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: nhosoi(Thanks!)
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;
}
More information about the 389-commits
mailing list