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!)

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)])

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") )
     {

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;
 	}