From ab58ff2928e9745bfcf63e6cc99f84b3aa5b1d23 Mon Sep 17 00:00:00 2001 From: Yannick Martin Date: Mon, 26 May 2025 15:05:42 +0200 Subject: [PATCH 1/2] fix api deadlock that can appears on two simultaneous actions With this mutex, we can have deadlock in the following case: 1/ Thread A processes a /v1/actions/acknowledge-problem request and locks the checkable 2/ Thread B processes a /v1/actions/add-comment and enters first the ConfigItem::ActivateItems() method and locks the static mutex there and starts the just created comment object, which triggers the OnCommentAdded() event. 3/ Thread A wants to activate the just created ack comment as well but since the mutex is already locked by TB, it blocks. 4/ Thread B's OnCommentAdded() even dispatch causes the IcingaDB::CommentAddedHandler() to be called and implicitly triggers full state update for the checkable. Now, the state serialization of that checkable (remember that's the same checkable currently locked by TA) also includes computing its severity, thus it calls either service->GetSeverity() or host->GetSeverity(). However, since computing the checkable severity (as of now) requires acquiring the object lock, and boom - they deadlock each other. --- lib/config/configitem.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/lib/config/configitem.cpp b/lib/config/configitem.cpp index 9dc0f1aa2..6401b2d70 100644 --- a/lib/config/configitem.cpp +++ b/lib/config/configitem.cpp @@ -679,9 +679,6 @@ bool ConfigItem::CommitItems(const ActivationContext::Ptr& context, WorkQueue& u bool ConfigItem::ActivateItems(const std::vector& newItems, bool runtimeCreated, bool mainConfigActivation, bool withModAttrs, const Value& cookie) { - static std::mutex mtx; - std::unique_lock lock(mtx); - if (withModAttrs) { /* restore modified attributes */ if (Utility::PathExists(Configuration::ModAttrPath)) { From e6fc1b91a7e083c98564dec00bfe217265486073 Mon Sep 17 00:00:00 2001 From: Yannick Martin Date: Tue, 3 Jun 2025 16:33:12 +0200 Subject: [PATCH 2/2] AddComment: return Comment::Ptr instead of String containing the name Mimic 88e5744d54f79ab6a84260bd4a8d2cc0ffd43465 and address nullptr on concurrent add-comment / remove-comment actions. --- lib/icinga/apiactions.cpp | 4 ++-- lib/icinga/comment.cpp | 4 ++-- lib/icinga/comment.hpp | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/icinga/apiactions.cpp b/lib/icinga/apiactions.cpp index 885834edc..781705b64 100644 --- a/lib/icinga/apiactions.cpp +++ b/lib/icinga/apiactions.cpp @@ -316,11 +316,11 @@ Dictionary::Ptr ApiActions::AddComment(const ConfigObject::Ptr& object, return ApiActions::CreateResult(503, "Icinga is reloading."); } - String commentName = Comment::AddComment(checkable, CommentUser, + Comment::Ptr comment = Comment::AddComment(checkable, CommentUser, HttpUtility::GetLastParameter(params, "author"), HttpUtility::GetLastParameter(params, "comment"), false, timestamp); - Comment::Ptr comment = Comment::GetByName(commentName); + String commentName = comment->GetName(); Dictionary::Ptr additional = new Dictionary({ { "name", commentName }, diff --git a/lib/icinga/comment.cpp b/lib/icinga/comment.cpp index 9c0b92359..0d1967440 100644 --- a/lib/icinga/comment.cpp +++ b/lib/icinga/comment.cpp @@ -130,7 +130,7 @@ int Comment::GetNextCommentID() return l_NextCommentID; } -String Comment::AddComment(const Checkable::Ptr& checkable, CommentType entryType, const String& author, +Comment::Ptr Comment::AddComment(const Checkable::Ptr& checkable, CommentType entryType, const String& author, const String& text, bool persistent, double expireTime, bool sticky, const String& id, const MessageOrigin::Ptr& origin) { String fullName; @@ -184,7 +184,7 @@ String Comment::AddComment(const Checkable::Ptr& checkable, CommentType entryTyp Log(LogNotice, "Comment") << "Added comment '" << comment->GetName() << "'."; - return fullName; + return comment; } void Comment::RemoveComment(const String& id, bool removedManually, const String& removedBy, diff --git a/lib/icinga/comment.hpp b/lib/icinga/comment.hpp index 653208478..a230040f8 100644 --- a/lib/icinga/comment.hpp +++ b/lib/icinga/comment.hpp @@ -34,7 +34,7 @@ public: static int GetNextCommentID(); - static String AddComment(const intrusive_ptr& checkable, CommentType entryType, + static Ptr AddComment(const intrusive_ptr& checkable, CommentType entryType, const String& author, const String& text, bool persistent, double expireTime, bool sticky = false, const String& id = String(), const MessageOrigin::Ptr& origin = nullptr);