Fix race condition in the ConfigItem class

fixes #10643

lib/cli/daemoncommand.cpp

@@ -230,7 +230,9 @@ int DaemonCommand::Run(const po::variables_map& vm, const std::vector<std::strin
 	else if (!vm.count("no-config"))
 		configs.push_back(Application::GetSysconfDir() + "/icinga2/icinga2.conf");
 
-	if (!DaemonUtility::LoadConfigFiles(configs, Application::GetObjectsPath(), Application::GetVarsPath()))
+	std::vector<ConfigItem::Ptr> newItems;
+
+	if (!DaemonUtility::LoadConfigFiles(configs, newItems, Application::GetObjectsPath(), Application::GetVarsPath()))
 		return EXIT_FAILURE;
 
 	if (vm.count("validate")) {
@@ -258,11 +260,20 @@ int DaemonCommand::Run(const po::variables_map& vm, const std::vector<std::strin
 		}
 	}
 
+	/* restore the previous program state */
+	try {
+		ConfigObject::RestoreObjects(Application::GetStatePath());
+	} catch (const std::exception& ex) {
+		Log(LogCritical, "cli")
+		    << "Failed to restore state file: " << DiagnosticInformation(ex);
+		return EXIT_FAILURE;
+	}
+
 	{
 		WorkQueue upq(25000, Application::GetConcurrency());
 
 		// activate config only after daemonization: it starts threads and that is not compatible with fork()
-		if (!ConfigItem::ActivateItems(upq, true)) {
+		if (!ConfigItem::ActivateItems(upq, newItems)) {
 			Log(LogCritical, "cli", "Error activating configuration.");
 			return EXIT_FAILURE;
 		}

lib/cli/daemonutility.cpp

@@ -138,13 +138,16 @@ bool DaemonUtility::ValidateConfigFiles(const std::vector<std::string>& configs,
 }
 
 bool DaemonUtility::LoadConfigFiles(const std::vector<std::string>& configs,
+    std::vector<ConfigItem::Ptr>& newItems,
     const String& objectsFile, const String& varsfile)
 {
+	ActivationScope ascope;
+
 	if (!DaemonUtility::ValidateConfigFiles(configs, objectsFile))
 		return false;
 
 	WorkQueue upq(25000, Application::GetConcurrency());
-	bool result = ConfigItem::CommitItems(upq);
+	bool result = ConfigItem::CommitItems(ascope.GetContext(), upq, newItems);
 
 	if (!result)
 		return false;

lib/cli/daemonutility.hpp

@@ -21,6 +21,7 @@
 #define DAEMONUTILITY_H
 
 #include "cli/i2-cli.hpp"
+#include "config/configitem.hpp"
 #include "base/string.hpp"
 #include <boost/program_options.hpp>
 
@@ -34,7 +35,8 @@ class I2_CLI_API DaemonUtility
 {
 public:
 	static bool ValidateConfigFiles(const std::vector<std::string>& configs, const String& objectsFile = String());
-	static bool LoadConfigFiles(const std::vector<std::string>& configs, const String& objectsFile = String(), const String& varsfile = String());
+	static bool LoadConfigFiles(const std::vector<std::string>& configs, std::vector<ConfigItem::Ptr>& newItems,
+	    const String& objectsFile = String(), const String& varsfile = String());
 };
 
 }

lib/config/CMakeLists.txt

@@ -34,7 +34,7 @@ add_flex_bison_dependency(config_lexer config_parser)
 include_directories(${CMAKE_CURRENT_BINARY_DIR} ${CMAKE_CURRENT_SOURCE_DIR})
 
 set(config_SOURCES
-  applyrule.cpp
+  activationcontext.cpp applyrule.cpp
   configcompilercontext.cpp configcompiler.cpp configitembuilder.cpp
   configitem.cpp ${FLEX_config_lexer_OUTPUTS} ${BISON_config_parser_OUTPUTS}
   expression.cpp objectrule.cpp

lib/config/activationcontext.cpp

@@ -0,0 +1,78 @@
+/******************************************************************************
+ * Icinga 2                                                                   *
+ * Copyright (C) 2012-2015 Icinga Development Team (http://www.icinga.org)    *
+ *                                                                            *
+ * This program is free software; you can redistribute it and/or              *
+ * modify it under the terms of the GNU General Public License                *
+ * as published by the Free Software Foundation; either version 2             *
+ * of the License, or (at your option) any later version.                     *
+ *                                                                            *
+ * This program is distributed in the hope that it will be useful,            *
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of             *
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the              *
+ * GNU General Public License for more details.                               *
+ *                                                                            *
+ * You should have received a copy of the GNU General Public License          *
+ * along with this program; if not, write to the Free Software Foundation     *
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA.             *
+ ******************************************************************************/
+
+#include "config/activationcontext.hpp"
+#include "base/exception.hpp"
+
+using namespace icinga;
+
+boost::thread_specific_ptr<std::stack<ActivationContext::Ptr> > ActivationContext::m_ActivationStack;
+
+std::stack<ActivationContext::Ptr>& ActivationContext::GetActivationStack(void)
+{
+	std::stack<ActivationContext::Ptr> *actx = m_ActivationStack.get();
+
+	if (!actx) {
+		actx = new std::stack<ActivationContext::Ptr>();
+		m_ActivationStack.reset(actx);
+	}
+
+	return *actx;
+}
+
+void ActivationContext::PushContext(const ActivationContext::Ptr& context)
+{
+	GetActivationStack().push(context);
+}
+
+void ActivationContext::PopContext(void)
+{
+	ASSERT(!GetActivationStack().empty());
+	GetActivationStack().pop();
+}
+
+ActivationContext::Ptr ActivationContext::GetCurrentContext(void)
+{
+	std::stack<ActivationContext::Ptr>& astack = GetActivationStack();
+
+	if (astack.empty())
+		BOOST_THROW_EXCEPTION(std::runtime_error("Objects may not be created outside of an activation context."));
+
+	return astack.top();
+}
+
+ActivationScope::ActivationScope(const ActivationContext::Ptr& context)
+    : m_Context(context)
+{
+	if (!m_Context)
+		m_Context = new ActivationContext();
+
+	ActivationContext::PushContext(m_Context);
+}
+
+ActivationScope::~ActivationScope(void)
+{
+	ActivationContext::PopContext();
+}
+
+ActivationContext::Ptr ActivationScope::GetContext(void) const
+{
+	return m_Context;
+}
+

lib/config/activationcontext.hpp

@@ -0,0 +1,63 @@
+/******************************************************************************
+ * Icinga 2                                                                   *
+ * Copyright (C) 2012-2015 Icinga Development Team (http://www.icinga.org)    *
+ *                                                                            *
+ * This program is free software; you can redistribute it and/or              *
+ * modify it under the terms of the GNU General Public License                *
+ * as published by the Free Software Foundation; either version 2             *
+ * of the License, or (at your option) any later version.                     *
+ *                                                                            *
+ * This program is distributed in the hope that it will be useful,            *
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of             *
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the              *
+ * GNU General Public License for more details.                               *
+ *                                                                            *
+ * You should have received a copy of the GNU General Public License          *
+ * along with this program; if not, write to the Free Software Foundation     *
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA.             *
+ ******************************************************************************/
+
+#ifndef ACTIVATIONCONTEXT_H
+#define ACTIVATIONCONTEXT_H
+
+#include "config/i2-config.hpp"
+#include "base/object.hpp"
+#include <boost/thread/tss.hpp>
+#include <stack>
+
+namespace icinga
+{
+
+class I2_CONFIG_API ActivationContext : public Object
+{
+public:
+	DECLARE_PTR_TYPEDEFS(ActivationContext);
+
+	static ActivationContext::Ptr GetCurrentContext(void);
+
+private:
+	static void PushContext(const ActivationContext::Ptr& context);
+	static void PopContext(void);
+
+	static std::stack<ActivationContext::Ptr>& GetActivationStack(void);
+
+	static boost::thread_specific_ptr<std::stack<ActivationContext::Ptr> > m_ActivationStack;
+
+	friend class ActivationScope;
+};
+
+class I2_CONFIG_API ActivationScope
+{
+public:
+	ActivationScope(const ActivationContext::Ptr& context = ActivationContext::Ptr());
+	~ActivationScope(void);
+
+	ActivationContext::Ptr GetContext(void) const;
+
+private:
+	ActivationContext::Ptr m_Context;
+};
+
+}
+
+#endif /* ACTIVATIONCONTEXT_H */

lib/config/configitem.cpp

@@ -44,9 +44,8 @@ using namespace icinga;
 boost::mutex ConfigItem::m_Mutex;
 ConfigItem::TypeMap ConfigItem::m_Items;
 ConfigItem::ItemList ConfigItem::m_UnnamedItems;
-ConfigItem::ItemList ConfigItem::m_CommittedItems;
 
-REGISTER_SCRIPTFUNCTION(commit_objects, &ConfigItem::CommitAndActivate);
+REGISTER_SCRIPTFUNCTION(__run_with_activation_context, &ConfigItem::RunWithActivationContext);
 
 /**
  * Constructor for the ConfigItem class.
@@ -235,11 +234,6 @@ ConfigObject::Ptr ConfigItem::Commit(bool discard)
 		throw;
 	}
 
-	{
-		boost::mutex::scoped_lock lock(m_Mutex);
-		m_CommittedItems.push_back(this);
-	}
-
 	Dictionary::Ptr persistentItem = new Dictionary();
 
 	persistentItem->Set("type", GetType());
@@ -291,14 +285,15 @@ void ConfigItem::Register(void)
 {
 	Type::Ptr type = Type::GetByName(m_Type);
 
+	m_ActivationContext = ActivationContext::GetCurrentContext();
+
+	boost::mutex::scoped_lock lock(m_Mutex);
+
 	/* If this is a non-abstract object with a composite name
 	 * we register it in m_UnnamedItems instead of m_Items. */
-	if (!m_Abstract && dynamic_cast<NameComposer *>(type.get())) {
+	if (!m_Abstract && dynamic_cast<NameComposer *>(type.get()))
-		boost::mutex::scoped_lock lock(m_Mutex);
 		m_UnnamedItems.push_back(this);
-	} else {
+	else {
-		boost::mutex::scoped_lock lock(m_Mutex);
-
 		ItemMap::const_iterator it = m_Items[m_Type].find(m_Name);
 
 		if (it != m_Items[m_Type].end()) {
@@ -324,7 +319,6 @@ void ConfigItem::Unregister(void)
 	boost::mutex::scoped_lock lock(m_Mutex);
 	m_UnnamedItems.erase(std::remove(m_UnnamedItems.begin(), m_UnnamedItems.end(), this), m_UnnamedItems.end());
 	m_Items[m_Type].erase(m_Name);
-	m_CommittedItems.erase(std::remove(m_CommittedItems.begin(), m_CommittedItems.end(), this), m_CommittedItems.end());
 }
 
 /**
@@ -351,7 +345,7 @@ ConfigItem::Ptr ConfigItem::GetByTypeAndName(const String& type, const String& n
 	return it2->second;
 }
 
-void ConfigItem::OnAllConfigLoadedWrapper(void)
+void ConfigItem::OnAllConfigLoadedHelper(void)
 {
 	try {
 		m_Object->OnAllConfigLoaded();
@@ -369,7 +363,13 @@ void ConfigItem::OnAllConfigLoadedWrapper(void)
 	}
 }
 
-bool ConfigItem::CommitNewItems(WorkQueue& upq, std::vector<ConfigItem::Ptr>& newItems)
+void ConfigItem::CreateChildObjectsHelper(const Type::Ptr& type)
+{
+	ActivationScope ascope(m_ActivationContext);
+	m_Object->CreateChildObjects(type);
+}
+
+bool ConfigItem::CommitNewItems(const ActivationContext::Ptr& context, WorkQueue& upq, std::vector<ConfigItem::Ptr>& newItems)
 {
 	typedef std::pair<ConfigItem::Ptr, bool> ItemPair;
 	std::vector<ItemPair> items;
@@ -379,17 +379,31 @@ bool ConfigItem::CommitNewItems(WorkQueue& upq, std::vector<ConfigItem::Ptr>& ne
 
 		BOOST_FOREACH(const TypeMap::value_type& kv, m_Items) {
 			BOOST_FOREACH(const ItemMap::value_type& kv2, kv.second) {
-				if (!kv2.second->m_Abstract && !kv2.second->m_Object)
+				if (kv2.second->m_Abstract || kv2.second->m_Object)
-					items.push_back(std::make_pair(kv2.second, false));
+					continue;
+
+				if (kv2.second->m_ActivationContext != context)
+					continue;
+
+				items.push_back(std::make_pair(kv2.second, false));
 			}
 		}
 
+		ItemList newUnnamedItems;
+
 		BOOST_FOREACH(const ConfigItem::Ptr& item, m_UnnamedItems) {
-			if (!item->m_Abstract && !item->m_Object)
+			if (item->m_ActivationContext != context) {
-				items.push_back(std::make_pair(item, true));
+				newUnnamedItems.push_back(item);
+				continue;
+			}
+
+			if (item->m_Abstract || item->m_Object)
+				continue;
+
+			items.push_back(std::make_pair(item, true));
 		}
 
-		m_UnnamedItems.clear();
+		m_UnnamedItems.swap(newUnnamedItems);
 	}
 
 	if (items.empty())
@@ -405,13 +419,6 @@ bool ConfigItem::CommitNewItems(WorkQueue& upq, std::vector<ConfigItem::Ptr>& ne
 	if (upq.HasExceptions())
 		return false;
 
-	std::vector<ConfigItem::Ptr> new_items;
-
-	{
-		boost::mutex::scoped_lock lock(m_Mutex);
-		new_items.swap(m_CommittedItems);
-	}
-
 	std::set<String> types;
 
 	std::vector<Type::Ptr> all_types;
@@ -451,12 +458,14 @@ bool ConfigItem::CommitNewItems(WorkQueue& upq, std::vector<ConfigItem::Ptr>& ne
 			if (unresolved_dep)
 				continue;
 
-			BOOST_FOREACH(const ConfigItem::Ptr& item, new_items) {
+			BOOST_FOREACH(const ItemPair& ip, items) {
+				const ConfigItem::Ptr& item = ip.first;
+
 				if (!item->m_Object)
 					continue;
 
 				if (item->m_Type == type)
-					upq.Enqueue(boost::bind(&ConfigItem::OnAllConfigLoadedWrapper, item));
+					upq.Enqueue(boost::bind(&ConfigItem::OnAllConfigLoadedHelper, item));
 			}
 
 			completed_types.insert(type);
@@ -467,12 +476,14 @@ bool ConfigItem::CommitNewItems(WorkQueue& upq, std::vector<ConfigItem::Ptr>& ne
 				return false;
 
 			BOOST_FOREACH(const String& loadDep, ptype->GetLoadDependencies()) {
-				BOOST_FOREACH(const ConfigItem::Ptr& item, new_items) {
+				BOOST_FOREACH(const ItemPair& ip, items) {
+					const ConfigItem::Ptr& item = ip.first;
+
 					if (!item->m_Object)
 						continue;
 
 					if (item->m_Type == loadDep)
-						upq.Enqueue(boost::bind(&ConfigObject::CreateChildObjects, item->m_Object, ptype));
+						upq.Enqueue(boost::bind(&ConfigItem::CreateChildObjectsHelper, item, ptype));
 				}
 			}
 
@@ -481,7 +492,7 @@ bool ConfigItem::CommitNewItems(WorkQueue& upq, std::vector<ConfigItem::Ptr>& ne
 			if (upq.HasExceptions())
 				return false;
 
-			if (!CommitNewItems(upq, newItems))
+			if (!CommitNewItems(context, upq, newItems))
 				return false;
 		}
 	}
@@ -489,13 +500,11 @@ bool ConfigItem::CommitNewItems(WorkQueue& upq, std::vector<ConfigItem::Ptr>& ne
 	return true;
 }
 
-bool ConfigItem::CommitItems(WorkQueue& upq)
+bool ConfigItem::CommitItems(const ActivationContext::Ptr& context, WorkQueue& upq, std::vector<ConfigItem::Ptr>& newItems)
 {
 	Log(LogInformation, "ConfigItem", "Committing config items");
 
-	std::vector<ConfigItem::Ptr> newItems;
+	if (!CommitNewItems(context, upq, newItems)) {
-
-	if (!CommitNewItems(upq, newItems)) {
 		upq.ReportExceptions("config");
 
 		BOOST_FOREACH(const ConfigItem::Ptr& item, newItems) {
@@ -505,6 +514,8 @@ bool ConfigItem::CommitItems(WorkQueue& upq)
 		return false;
 	}
 
+	ASSERT(newItems.size() > 0);
+
 	ApplyRule::CheckMatches();
 
 	/* log stats for external parsers */
@@ -525,34 +536,27 @@ bool ConfigItem::CommitItems(WorkQueue& upq)
 	return true;
 }
 
-bool ConfigItem::ActivateItems(WorkQueue& upq, bool restoreState, bool runtimeCreated)
+bool ConfigItem::ActivateItems(WorkQueue& upq, const std::vector<ConfigItem::Ptr>& newItems, bool runtimeCreated)
 {
 	static boost::mutex mtx;
 	boost::mutex::scoped_lock lock(mtx);
 
-	if (restoreState) {
-		/* restore the previous program state */
-		try {
-			ConfigObject::RestoreObjects(Application::GetStatePath());
-		} catch (const std::exception& ex) {
-			Log(LogCritical, "ConfigItem")
-			    << "Failed to restore state file: " << DiagnosticInformation(ex);
-		}
-	}
-
 	Log(LogInformation, "ConfigItem", "Triggering Start signal for config items");
 
-	BOOST_FOREACH(const ConfigType::Ptr& type, ConfigType::GetTypes()) {
+	BOOST_FOREACH(const ConfigItem::Ptr& item, newItems) {
-		BOOST_FOREACH(const ConfigObject::Ptr& object, type->GetObjects()) {
+		if (!item->m_Object)
-			if (object->IsActive())
+			continue;
-				continue;
+
+		ConfigObject::Ptr object = item->m_Object;
+
+		if (object->IsActive())
+			continue;
 
 #ifdef I2_DEBUG
-			Log(LogDebug, "ConfigItem")
+		Log(LogDebug, "ConfigItem")
-			    << "Activating object '" << object->GetName() << "' of type '" << object->GetType()->GetName() << "'";
+		    << "Activating object '" << object->GetName() << "' of type '" << object->GetType()->GetName() << "'";
 #endif /* I2_DEBUG */
-			upq.Enqueue(boost::bind(&ConfigObject::Activate, object, runtimeCreated));
+		upq.Enqueue(boost::bind(&ConfigObject::Activate, object, runtimeCreated));
-		}
 	}
 
 	upq.Join();
@@ -563,10 +567,13 @@ bool ConfigItem::ActivateItems(WorkQueue& upq, bool restoreState, bool runtimeCr
 	}
 
 #ifdef I2_DEBUG
-	BOOST_FOREACH(const ConfigType::Ptr& type, ConfigType::GetTypes()) {
+	BOOST_FOREACH(const ConfigItem::Ptr& item, newItems) {
-		BOOST_FOREACH(const ConfigObject::Ptr& object, type->GetObjects()) {
+		ConfigObject::Ptr object = item->m_Object;
-			ASSERT(object->IsActive());
+
-		}
+		if (item->m_Abstract)
+			continue;
+
+		ASSERT(object && object->IsActive());
 	}
 #endif /* I2_DEBUG */
 
@@ -575,14 +582,22 @@ bool ConfigItem::ActivateItems(WorkQueue& upq, bool restoreState, bool runtimeCr
 	return true;
 }
 
-bool ConfigItem::CommitAndActivate(void)
+bool ConfigItem::RunWithActivationContext(const Function::Ptr& function)
 {
-	WorkQueue upq(25000, Application::GetConcurrency());
+	ActivationScope scope;
 
-	if (!CommitItems(upq))
+	{
+		ScriptFrame frame;
+		function->Invoke();
+	}
+
+	WorkQueue upq(25000, Application::GetConcurrency());
+	std::vector<ConfigItem::Ptr> newItems;
+
+	if (!CommitItems(scope.GetContext(), upq, newItems))
 		return false;
 
-	if (!ActivateItems(upq, false))
+	if (!ActivateItems(upq, newItems))
 		return false;
 
 	return true;

lib/config/configitem.hpp

@@ -22,12 +22,14 @@
 
 #include "config/i2-config.hpp"
 #include "config/expression.hpp"
+#include "config/activationcontext.hpp"
 #include "base/configobject.hpp"
 #include "base/workqueue.hpp"
 
 namespace icinga
 {
 
+
 /**
  * A configuration item. Non-abstract configuration items can be used to
  * create configuration objects at runtime.
@@ -55,7 +57,6 @@ public:
 	boost::shared_ptr<Expression> GetExpression(void) const;
 	boost::shared_ptr<Expression> GetFilter(void) const;
 
-	ConfigObject::Ptr Commit(bool discard = true);
 	void Register(void);
 	void Unregister(void);
 
@@ -67,10 +68,10 @@ public:
 	static ConfigItem::Ptr GetByTypeAndName(const String& type,
 	    const String& name);
 
-	static bool CommitItems(WorkQueue& upq);
+	static bool CommitItems(const ActivationContext::Ptr& context, WorkQueue& upq, std::vector<ConfigItem::Ptr>& newItems);
-	static bool ActivateItems(WorkQueue& upq, bool restoreState, bool runtimeCreated = false);
+	static bool ActivateItems(WorkQueue& upq, const std::vector<ConfigItem::Ptr>& newItems, bool runtimeCreated = false);
 
-	static bool CommitAndActivate(void);
+	static bool RunWithActivationContext(const Function::Ptr& function);
 
 	static std::vector<ConfigItem::Ptr> GetItems(const String& type);
 
@@ -86,6 +87,7 @@ private:
 	Dictionary::Ptr m_Scope; /**< variable scope. */
 	String m_Zone; /**< The zone. */
 	String m_Package;
+	ActivationContext::Ptr m_ActivationContext;
 
 	ConfigObject::Ptr m_Object;
 
@@ -102,9 +104,12 @@ private:
 	static ConfigItem::Ptr GetObjectUnlocked(const String& type,
 	    const String& name);
 
-	static bool CommitNewItems(WorkQueue& upq, std::vector<ConfigItem::Ptr>& newItems);
+	ConfigObject::Ptr Commit(bool discard = true);
 
-	void OnAllConfigLoadedWrapper(void);
+	static bool CommitNewItems(const ActivationContext::Ptr& context, WorkQueue& upq, std::vector<ConfigItem::Ptr>& newItems);
+
+	void OnAllConfigLoadedHelper(void);
+	void CreateChildObjectsHelper(const Type::Ptr& type);
 };
 
 }

lib/remote/configobjectutility.cpp

@@ -121,14 +121,17 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full
 	Expression *expr = ConfigCompiler::CompileFile(path, String(), "_api");
 
 	try {
+		ActivationScope ascope;
+
 		ScriptFrame frame;
 		expr->Evaluate(frame);
 		delete expr;
 		expr = NULL;
 
 		WorkQueue upq;
+		std::vector<ConfigItem::Ptr> newItems;
 
-		if (!ConfigItem::CommitItems(upq) || !ConfigItem::ActivateItems(upq, false, true)) {
+		if (!ConfigItem::CommitItems(ascope.GetContext(), upq, newItems) || !ConfigItem::ActivateItems(upq, newItems, true)) {
 			if (errors) {
 				if (unlink(path.CStr()) < 0) {
 					BOOST_THROW_EXCEPTION(posix_error()

test/livestatus.cpp

@@ -84,11 +84,14 @@ struct GlobalConfigFixture {
 		std::vector<std::string> configs;
 		configs.push_back(TestConfig);
 
-		DaemonUtility::LoadConfigFiles(configs, "icinga2.debug", "icinga2.vars");
+		ActivationScope ascope;
+		std::vector<ConfigItem::Ptr> newItems;
+
+		DaemonUtility::LoadConfigFiles(configs, newItems, "icinga2.debug", "icinga2.vars");
 
 		/* ignore config errors */
 		WorkQueue upq;
-		ConfigItem::ActivateItems(upq, false);
+		ConfigItem::ActivateItems(upq, newItems);
 	}
 
 	~GlobalConfigFixture()