Merge pull request #7168 from Icinga/bugfix/config-packages-regression

Fix deadlock in API config packages

lib/remote/configobjectutility.cpp

@@ -16,6 +16,7 @@ using namespace icinga;
 
 String ConfigObjectUtility::GetConfigDir()
 {
+	/* This may throw an exception the caller above must handle. */
 	return ConfigPackageUtility::GetPackageDir() + "/_api/" +
 		ConfigPackageUtility::GetActiveStage("_api");
 }
@@ -25,7 +26,10 @@ String ConfigObjectUtility::GetObjectConfigPath(const Type::Ptr& type, const Str
 	String typeDir = type->GetPluralName();
 	boost::algorithm::to_lower(typeDir);
 
-	return GetConfigDir() + "/conf.d/" + typeDir +
+	/* This may throw an exception the caller above must handle. */
+	String prefix = GetConfigDir();
+
+	return prefix + "/conf.d/" + typeDir +
 		"/" + EscapeName(fullName) + ".conf";
 }
 
@@ -85,7 +89,8 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full
 	const String& config, const Array::Ptr& errors, const Array::Ptr& diagnosticInformation)
 {
 	{
-		boost::mutex::scoped_lock lock(ConfigPackageUtility::GetStaticMutex());
+		boost::mutex::scoped_lock lock(ConfigPackageUtility::GetStaticPackageMutex());
+
 		if (!ConfigPackageUtility::PackageExists("_api")) {
 			ConfigPackageUtility::CreatePackage("_api");
 
@@ -101,7 +106,15 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full
 		return false;
 	}
 
-	String path = GetObjectConfigPath(type, fullName);
+	String path;
+
+	try {
+		path = GetObjectConfigPath(type, fullName);
+	} catch (const std::exception& ex) {
+		errors->Add("Config package broken: " + DiagnosticInformation(ex, false));
+		return false;
+	}
+
 	Utility::MkDirP(Utility::DirName(path), 0700);
 
 	std::ofstream fp(path.CStr(), std::ofstream::out | std::ostream::trunc);
@@ -215,7 +228,14 @@ bool ConfigObjectUtility::DeleteObjectHelper(const ConfigObject::Ptr& object, bo
 		return false;
 	}
 
-	String path = GetObjectConfigPath(object->GetReflectionType(), name);
+	String path;
+
+	try {
+		path = GetObjectConfigPath(object->GetReflectionType(), name);
+	} catch (const std::exception& ex) {
+		errors->Add("Config package broken: " + DiagnosticInformation(ex, false));
+		return false;
+	}
 
 	Utility::Remove(path);
 

lib/remote/configpackageshandler.cpp

@@ -63,12 +63,19 @@ void ConfigPackagesHandler::HandleGet(
 	ArrayData results;
 
 	{
-		boost::mutex::scoped_lock lock(ConfigPackageUtility::GetStaticMutex());
+		boost::mutex::scoped_lock lock(ConfigPackageUtility::GetStaticPackageMutex());
+
 		for (const String& package : packages) {
+			String activeStage;
+
+			try {
+				activeStage = ConfigPackageUtility::GetActiveStage(package);
+			} catch (const std::exception&) { } /* Should never happen. */
+
 			results.emplace_back(new Dictionary({
 				{ "name", package },
 				{ "stages", Array::FromVector(ConfigPackageUtility::GetStages(package)) },
-				{ "active-stage", ConfigPackageUtility::GetActiveStage(package) }
+				{ "active-stage", activeStage }
 			}));
 		}
 	}
@@ -104,7 +111,8 @@ void ConfigPackagesHandler::HandlePost(
 	}
 
 	try {
-		boost::mutex::scoped_lock lock(ConfigPackageUtility::GetStaticMutex());
+		boost::mutex::scoped_lock lock(ConfigPackageUtility::GetStaticPackageMutex());
+
 		ConfigPackageUtility::CreatePackage(packageName);
 	} catch (const std::exception& ex) {
 		HttpUtility::SendJsonError(response, params, 500, "Could not create package '" + packageName + "'.",

lib/remote/configpackageutility.cpp

@@ -186,7 +186,8 @@ void ConfigPackageUtility::TryActivateStageCallback(const ProcessResult& pr, con
 	/* validation went fine, activate stage and reload */
 	if (pr.ExitStatus == 0) {
 		{
-			boost::mutex::scoped_lock lock(GetStaticMutex());
+			boost::mutex::scoped_lock lock(GetStaticPackageMutex());
+
 			ActivateStage(packageName, stageName);
 		}
 
@@ -251,7 +252,7 @@ std::vector<String> ConfigPackageUtility::GetStages(const String& packageName)
 String ConfigPackageUtility::GetActiveStageFromFile(const String& packageName)
 {
 	/* Lock the transaction, reading this only happens on startup or when something really is broken. */
-	boost::mutex::scoped_lock lock(GetStaticMutex());
+	boost::mutex::scoped_lock lock(GetStaticActiveStageMutex());
 
 	String path = GetPackageDir() + "/" + packageName + "/active-stage";
 
@@ -271,7 +272,7 @@ String ConfigPackageUtility::GetActiveStageFromFile(const String& packageName)
 
 void ConfigPackageUtility::SetActiveStageToFile(const String& packageName, const String& stageName)
 {
-	boost::mutex::scoped_lock lock(GetStaticMutex());
+	boost::mutex::scoped_lock lock(GetStaticActiveStageMutex());
 
 	String activeStagePath = GetPackageDir() + "/" + packageName + "/active-stage";
 
@@ -380,7 +381,13 @@ bool ConfigPackageUtility::ValidateName(const String& name)
 	return (!boost::regex_search(name.GetData(), what, expr));
 }
 
-boost::mutex& ConfigPackageUtility::GetStaticMutex()
+boost::mutex& ConfigPackageUtility::GetStaticPackageMutex()
+{
+	static boost::mutex mutex;
+	return mutex;
+}
+
+boost::mutex& ConfigPackageUtility::GetStaticActiveStageMutex()
 {
 	static boost::mutex mutex;
 	return mutex;

lib/remote/configpackageutility.hpp

@@ -44,7 +44,8 @@ public:
 	static bool ContainsDotDot(const String& path);
 	static bool ValidateName(const String& name);
 
-	static boost::mutex& GetStaticMutex();
+	static boost::mutex& GetStaticPackageMutex();
+	static boost::mutex& GetStaticActiveStageMutex();
 
 private:
 	static void CollectDirNames(const String& path, std::vector<String>& dirs);

lib/remote/configstageshandler.cpp

@@ -120,7 +120,8 @@ void ConfigStagesHandler::HandlePost(
 		if (!files)
 			BOOST_THROW_EXCEPTION(std::invalid_argument("Parameter 'files' must be specified."));
 
-		boost::mutex::scoped_lock lock(ConfigPackageUtility::GetStaticMutex());
+		boost::mutex::scoped_lock lock(ConfigPackageUtility::GetStaticPackageMutex());
+
 		stageName = ConfigPackageUtility::CreateStage(packageName, files);
 
 		/* validate the config. on success, activate stage and reload */