From 0ebcd2662d86ad3587d32353e970b64e564df4cf Mon Sep 17 00:00:00 2001
From: Julian Brost <julian.brost@icinga.com>
Date: Tue, 8 Jul 2025 13:52:40 +0200
Subject: [PATCH] No longer allow overriding the frozen attribute of containers

The Array, Dictionary, and Namespace types provide a Freeze() method that makes
them read-only. So far, there was the possibility to call some methods with
`overrideFrozen=true` which would then bypass the corresponding check and allow
modification of the data structures nonetheless.

With 24b57f0d3a222835178e88489eabd595755ed883, this possibility was already
removed from the Namespace type. However, for interface compatibility, it kept
the parameter and just ignores it, throwing an exception on any modification on
a frozen instance.

The only place using `overrideFrozen` was processing of the `-D`/`--define`
command line flag that allows setting additional variables in the DSL. At the
time it is evaluated, there are no user-created data structures yet that could
be frozen, so the only frozen objects that could be encountered are Namespaces
(Icinga doesn't freeze other types by itself) and for these, `overrideFrozen`
already has no effect.

Hence, there is no harm in removing `overrideFrozen` altogether. This
simplifies the code and also means that frozen objects are now indeed read-only
without exceptions, allowing further optimizations regarding locking in the
future.
---
 icinga-app/icinga.cpp                   |  2 -
 lib/base/array.cpp                      | 52 +++++++++++--------------
 lib/base/array.hpp                      | 22 +++++------
 lib/base/dictionary.cpp                 |  9 ++---
 lib/base/dictionary.hpp                 |  4 +-
 lib/base/namespace.cpp                  |  7 +---
 lib/base/namespace.hpp                  |  2 +-
 lib/base/object.cpp                     |  2 +-
 lib/base/object.hpp                     |  2 +-
 lib/base/reference.cpp                  |  2 +-
 lib/config/expression.cpp               | 14 +------
 lib/config/expression.hpp               |  7 ----
 lib/config/vmops.hpp                    |  4 +-
 test/icinga-notification.cpp            |  6 +--
 test/methods-pluginnotificationtask.cpp |  6 +--
 15 files changed, 55 insertions(+), 86 deletions(-)

diff --git a/icinga-app/icinga.cpp b/icinga-app/icinga.cpp
index d4e27c566..63b51b77d 100644
--- a/icinga-app/icinga.cpp
+++ b/icinga-app/icinga.cpp
@@ -420,12 +420,10 @@ static int Main()
 
 			for (size_t i = 1; i < keyTokens.size(); i++) {
 				std::unique_ptr<IndexerExpression> indexerExpr{new IndexerExpression(std::move(expr), MakeLiteral(keyTokens[i]))};
-				indexerExpr->SetOverrideFrozen();
 				expr = std::move(indexerExpr);
 			}
 
 			std::unique_ptr<SetExpression> setExpr{new SetExpression(std::move(expr), OpSetLiteral, MakeLiteral(value))};
-			setExpr->SetOverrideFrozen();
 
 			try {
 				ScriptFrame frame(true);
diff --git a/lib/base/array.cpp b/lib/base/array.cpp
index 08e06fad2..73adcb279 100644
--- a/lib/base/array.cpp
+++ b/lib/base/array.cpp
@@ -45,13 +45,12 @@ Value Array::Get(SizeType index) const
  *
  * @param index The index.
  * @param value The value.
- * @param overrideFrozen Whether to allow modifying frozen arrays.
  */
-void Array::Set(SizeType index, const Value& value, bool overrideFrozen)
+void Array::Set(SizeType index, const Value& value)
 {
 	ObjectLock olock(this);
 
-	if (m_Frozen && !overrideFrozen)
+	if (m_Frozen)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Value in array must not be modified."));
 
 	m_Data.at(index) = value;
@@ -62,13 +61,12 @@ void Array::Set(SizeType index, const Value& value, bool overrideFrozen)
  *
  * @param index The index.
  * @param value The value.
- * @param overrideFrozen Whether to allow modifying frozen arrays.
  */
-void Array::Set(SizeType index, Value&& value, bool overrideFrozen)
+void Array::Set(SizeType index, Value&& value)
 {
 	ObjectLock olock(this);
 
-	if (m_Frozen && !overrideFrozen)
+	if (m_Frozen)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Array must not be modified."));
 
 	m_Data.at(index).Swap(value);
@@ -78,13 +76,12 @@ void Array::Set(SizeType index, Value&& value, bool overrideFrozen)
  * Adds a value to the array.
  *
  * @param value The value.
- * @param overrideFrozen Whether to allow modifying frozen arrays.
  */
-void Array::Add(Value value, bool overrideFrozen)
+void Array::Add(Value value)
 {
 	ObjectLock olock(this);
 
-	if (m_Frozen && !overrideFrozen)
+	if (m_Frozen)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Array must not be modified."));
 
 	m_Data.push_back(std::move(value));
@@ -148,15 +145,14 @@ bool Array::Contains(const Value& value) const
  *
  * @param index The index
  * @param value The value to add
- * @param overrideFrozen Whether to allow modifying frozen arrays.
  */
-void Array::Insert(SizeType index, Value value, bool overrideFrozen)
+void Array::Insert(SizeType index, Value value)
 {
 	ObjectLock olock(this);
 
 	ASSERT(index <= m_Data.size());
 
-	if (m_Frozen && !overrideFrozen)
+	if (m_Frozen)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Array must not be modified."));
 
 	m_Data.insert(m_Data.begin() + index, std::move(value));
@@ -166,13 +162,12 @@ void Array::Insert(SizeType index, Value value, bool overrideFrozen)
  * Removes the specified index from the array.
  *
  * @param index The index.
- * @param overrideFrozen Whether to allow modifying frozen arrays.
  */
-void Array::Remove(SizeType index, bool overrideFrozen)
+void Array::Remove(SizeType index)
 {
 	ObjectLock olock(this);
 
-	if (m_Frozen && !overrideFrozen)
+	if (m_Frozen)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Array must not be modified."));
 
 	if (index >= m_Data.size())
@@ -185,43 +180,42 @@ void Array::Remove(SizeType index, bool overrideFrozen)
  * Removes the item specified by the iterator from the array.
  *
  * @param it The iterator.
- * @param overrideFrozen Whether to allow modifying frozen arrays.
  */
-void Array::Remove(Array::Iterator it, bool overrideFrozen)
+void Array::Remove(Array::Iterator it)
 {
 	ASSERT(OwnsLock());
 
-	if (m_Frozen && !overrideFrozen)
+	if (m_Frozen)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Array must not be modified."));
 
 	m_Data.erase(it);
 }
 
-void Array::Resize(SizeType newSize, bool overrideFrozen)
+void Array::Resize(SizeType newSize)
 {
 	ObjectLock olock(this);
 
-	if (m_Frozen && !overrideFrozen)
+	if (m_Frozen)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Array must not be modified."));
 
 	m_Data.resize(newSize);
 }
 
-void Array::Clear(bool overrideFrozen)
+void Array::Clear()
 {
 	ObjectLock olock(this);
 
-	if (m_Frozen && !overrideFrozen)
+	if (m_Frozen)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Array must not be modified."));
 
 	m_Data.clear();
 }
 
-void Array::Reserve(SizeType newSize, bool overrideFrozen)
+void Array::Reserve(SizeType newSize)
 {
 	ObjectLock olock(this);
 
-	if (m_Frozen && !overrideFrozen)
+	if (m_Frozen)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Array must not be modified."));
 
 	m_Data.reserve(newSize);
@@ -280,11 +274,11 @@ Array::Ptr Array::Reverse() const
 	return result;
 }
 
-void Array::Sort(bool overrideFrozen)
+void Array::Sort()
 {
 	ObjectLock olock(this);
 
-	if (m_Frozen && !overrideFrozen)
+	if (m_Frozen)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Array must not be modified."));
 
 	std::sort(m_Data.begin(), m_Data.end());
@@ -354,7 +348,7 @@ Value Array::GetFieldByName(const String& field, bool sandboxed, const DebugInfo
 	return Get(index);
 }
 
-void Array::SetFieldByName(const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo)
+void Array::SetFieldByName(const String& field, const Value& value, const DebugInfo& debugInfo)
 {
 	ObjectLock olock(this);
 
@@ -364,9 +358,9 @@ void Array::SetFieldByName(const String& field, const Value& value, bool overrid
 		BOOST_THROW_EXCEPTION(ScriptError("Array index '" + Convert::ToString(index) + "' is out of bounds.", debugInfo));
 
 	if (static_cast<size_t>(index) >= GetLength())
-		Resize(index + 1, overrideFrozen);
+		Resize(index + 1);
 
-	Set(index, value, overrideFrozen);
+	Set(index, value);
 }
 
 Array::Iterator icinga::begin(const Array::Ptr& x)
diff --git a/lib/base/array.hpp b/lib/base/array.hpp
index 2c9a9dda7..9589d1901 100644
--- a/lib/base/array.hpp
+++ b/lib/base/array.hpp
@@ -38,9 +38,9 @@ public:
 	Array(std::initializer_list<Value> init);
 
 	Value Get(SizeType index) const;
-	void Set(SizeType index, const Value& value, bool overrideFrozen = false);
-	void Set(SizeType index, Value&& value, bool overrideFrozen = false);
-	void Add(Value value, bool overrideFrozen = false);
+	void Set(SizeType index, const Value& value);
+	void Set(SizeType index, Value&& value);
+	void Add(Value value);
 
 	Iterator Begin();
 	Iterator End();
@@ -48,14 +48,14 @@ public:
 	size_t GetLength() const;
 	bool Contains(const Value& value) const;
 
-	void Insert(SizeType index, Value value, bool overrideFrozen = false);
-	void Remove(SizeType index, bool overrideFrozen = false);
-	void Remove(Iterator it, bool overrideFrozen = false);
+	void Insert(SizeType index, Value value);
+	void Remove(SizeType index);
+	void Remove(Iterator it);
 
-	void Resize(SizeType newSize, bool overrideFrozen = false);
-	void Clear(bool overrideFrozen = false);
+	void Resize(SizeType newSize);
+	void Clear();
 
-	void Reserve(SizeType newSize, bool overrideFrozen = false);
+	void Reserve(SizeType newSize);
 
 	void CopyTo(const Array::Ptr& dest) const;
 	Array::Ptr ShallowClone() const;
@@ -91,7 +91,7 @@ public:
 
 	Array::Ptr Reverse() const;
 
-	void Sort(bool overrideFrozen = false);
+	void Sort();
 
 	String ToString() const override;
 	Value Join(const Value& separator) const;
@@ -100,7 +100,7 @@ public:
 	void Freeze();
 
 	Value GetFieldByName(const String& field, bool sandboxed, const DebugInfo& debugInfo) const override;
-	void SetFieldByName(const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo) override;
+	void SetFieldByName(const String& field, const Value& value, const DebugInfo& debugInfo) override;
 
 private:
 	std::vector<Value> m_Data; /**< The data for the array. */
diff --git a/lib/base/dictionary.cpp b/lib/base/dictionary.cpp
index 43df4af8f..89b6c4cbc 100644
--- a/lib/base/dictionary.cpp
+++ b/lib/base/dictionary.cpp
@@ -86,14 +86,13 @@ const Value * Dictionary::GetRef(const String& key) const
  *
  * @param key The key.
  * @param value The value.
- * @param overrideFrozen Whether to allow modifying frozen dictionaries.
  */
-void Dictionary::Set(const String& key, Value value, bool overrideFrozen)
+void Dictionary::Set(const String& key, Value value)
 {
 	ObjectLock olock(this);
 	std::unique_lock<std::shared_timed_mutex> lock (m_DataMutex);
 
-	if (m_Frozen && !overrideFrozen)
+	if (m_Frozen)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Value in dictionary must not be modified."));
 
 	m_Data[key] = std::move(value);
@@ -290,9 +289,9 @@ Value Dictionary::GetFieldByName(const String& field, bool, const DebugInfo& deb
 		return GetPrototypeField(const_cast<Dictionary *>(this), field, false, debugInfo);
 }
 
-void Dictionary::SetFieldByName(const String& field, const Value& value, bool overrideFrozen, const DebugInfo&)
+void Dictionary::SetFieldByName(const String& field, const Value& value, const DebugInfo&)
 {
-	Set(field, value, overrideFrozen);
+	Set(field, value);
 }
 
 bool Dictionary::HasOwnField(const String& field) const
diff --git a/lib/base/dictionary.hpp b/lib/base/dictionary.hpp
index ffccd630f..ba2fbe82c 100644
--- a/lib/base/dictionary.hpp
+++ b/lib/base/dictionary.hpp
@@ -43,7 +43,7 @@ public:
 	Value Get(const String& key) const;
 	bool Get(const String& key, Value *result) const;
 	const Value * GetRef(const String& key) const;
-	void Set(const String& key, Value value, bool overrideFrozen = false);
+	void Set(const String& key, Value value);
 	bool Contains(const String& key) const;
 
 	Iterator Begin();
@@ -71,7 +71,7 @@ public:
 	void Freeze();
 
 	Value GetFieldByName(const String& field, bool sandboxed, const DebugInfo& debugInfo) const override;
-	void SetFieldByName(const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo) override;
+	void SetFieldByName(const String& field, const Value& value, const DebugInfo& debugInfo) override;
 	bool HasOwnField(const String& field) const override;
 	bool GetOwnField(const String& field, Value *result) const override;
 
diff --git a/lib/base/namespace.cpp b/lib/base/namespace.cpp
index c3b859cff..1f53efc92 100644
--- a/lib/base/namespace.cpp
+++ b/lib/base/namespace.cpp
@@ -143,13 +143,8 @@ Value Namespace::GetFieldByName(const String& field, bool, const DebugInfo& debu
 		return GetPrototypeField(const_cast<Namespace *>(this), field, false, debugInfo); /* Ignore indexer not found errors similar to the Dictionary class. */
 }
 
-void Namespace::SetFieldByName(const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo)
+void Namespace::SetFieldByName(const String& field, const Value& value, const DebugInfo& debugInfo)
 {
-	// The override frozen parameter is mandated by the interface but ignored here. If the namespace is frozen, this
-	// disables locking for read operations, so it must not be modified again to ensure the consistency of the internal
-	// data structures.
-	(void) overrideFrozen;
-
 	Set(field, value, false, debugInfo);
 }
 
diff --git a/lib/base/namespace.hpp b/lib/base/namespace.hpp
index 94f2055d3..1a028e2c5 100644
--- a/lib/base/namespace.hpp
+++ b/lib/base/namespace.hpp
@@ -80,7 +80,7 @@ public:
 	size_t GetLength() const;
 
 	Value GetFieldByName(const String& field, bool sandboxed, const DebugInfo& debugInfo) const override;
-	void SetFieldByName(const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo) override;
+	void SetFieldByName(const String& field, const Value& value, const DebugInfo& debugInfo) override;
 	bool HasOwnField(const String& field) const override;
 	bool GetOwnField(const String& field, Value *result) const override;
 
diff --git a/lib/base/object.cpp b/lib/base/object.cpp
index 92a43b912..5c7c67a8e 100644
--- a/lib/base/object.cpp
+++ b/lib/base/object.cpp
@@ -125,7 +125,7 @@ Value Object::GetFieldByName(const String& field, bool sandboxed, const DebugInf
 	return GetField(fid);
 }
 
-void Object::SetFieldByName(const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo)
+void Object::SetFieldByName(const String& field, const Value& value, const DebugInfo& debugInfo)
 {
 	Type::Ptr type = GetReflectionType();
 
diff --git a/lib/base/object.hpp b/lib/base/object.hpp
index 008426b8d..7f520672e 100644
--- a/lib/base/object.hpp
+++ b/lib/base/object.hpp
@@ -171,7 +171,7 @@ public:
 	virtual void SetField(int id, const Value& value, bool suppress_events = false, const Value& cookie = Empty);
 	virtual Value GetField(int id) const;
 	virtual Value GetFieldByName(const String& field, bool sandboxed, const DebugInfo& debugInfo) const;
-	virtual void SetFieldByName(const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo);
+	virtual void SetFieldByName(const String& field, const Value& value, const DebugInfo& debugInfo);
 	virtual bool HasOwnField(const String& field) const;
 	virtual bool GetOwnField(const String& field, Value *result) const;
 	virtual void ValidateField(int id, const Lazy<Value>& lvalue, const ValidationUtils& utils);
diff --git a/lib/base/reference.cpp b/lib/base/reference.cpp
index b0104af6c..9382bde30 100644
--- a/lib/base/reference.cpp
+++ b/lib/base/reference.cpp
@@ -24,7 +24,7 @@ Value Reference::Get() const
 
 void Reference::Set(const Value& value)
 {
-	m_Parent->SetFieldByName(m_Index, value, false, DebugInfo());
+	m_Parent->SetFieldByName(m_Index, value, DebugInfo());
 }
 
 Object::Ptr Reference::GetParent() const
diff --git a/lib/config/expression.cpp b/lib/config/expression.cpp
index 09b860cde..bec121ec3 100644
--- a/lib/config/expression.cpp
+++ b/lib/config/expression.cpp
@@ -652,7 +652,7 @@ ExpressionResult SetExpression::DoEvaluate(ScriptFrame& frame, DebugHint *dhint)
 		}
 	}
 
-	VMOps::SetField(parent, index, operand2.GetValue(), m_OverrideFrozen, m_DebugInfo);
+	VMOps::SetField(parent, index, operand2.GetValue(), m_DebugInfo);
 
 	if (psdhint) {
 		psdhint->AddMessage("=", m_DebugInfo);
@@ -666,11 +666,6 @@ ExpressionResult SetExpression::DoEvaluate(ScriptFrame& frame, DebugHint *dhint)
 	return Empty;
 }
 
-void SetExpression::SetOverrideFrozen()
-{
-	m_OverrideFrozen = true;
-}
-
 ExpressionResult SetConstExpression::DoEvaluate(ScriptFrame& frame, DebugHint *dhint) const
 {
 	auto globals = ScriptGlobal::GetGlobals();
@@ -772,7 +767,7 @@ bool IndexerExpression::GetReference(ScriptFrame& frame, bool init_dict, Value *
 				old_value = VMOps::GetField(vparent, vindex, frame.Sandboxed, m_Operand1->GetDebugInfo());
 
 			if (old_value.IsEmpty() && !old_value.IsString())
-				VMOps::SetField(vparent, vindex, new Dictionary(), m_OverrideFrozen, m_Operand1->GetDebugInfo());
+				VMOps::SetField(vparent, vindex, new Dictionary(), m_Operand1->GetDebugInfo());
 		}
 
 		*parent = VMOps::GetField(vparent, vindex, frame.Sandboxed, m_DebugInfo);
@@ -798,11 +793,6 @@ bool IndexerExpression::GetReference(ScriptFrame& frame, bool init_dict, Value *
 	return true;
 }
 
-void IndexerExpression::SetOverrideFrozen()
-{
-	m_OverrideFrozen = true;
-}
-
 void icinga::BindToScope(std::unique_ptr<Expression>& expr, ScopeSpecifier scopeSpec)
 {
 	auto *dexpr = dynamic_cast<DictExpression *>(expr.get());
diff --git a/lib/config/expression.hpp b/lib/config/expression.hpp
index 644548d28..915da40c5 100644
--- a/lib/config/expression.hpp
+++ b/lib/config/expression.hpp
@@ -657,14 +657,11 @@ public:
 		: BinaryExpression(std::move(operand1), std::move(operand2), debugInfo), m_Op(op)
 	{ }
 
-	void SetOverrideFrozen();
-
 protected:
 	ExpressionResult DoEvaluate(ScriptFrame& frame, DebugHint *dhint) const override;
 
 private:
 	CombinedSetOp m_Op;
-	bool m_OverrideFrozen{false};
 
 	friend void BindToScope(std::unique_ptr<Expression>& expr, ScopeSpecifier scopeSpec);
 };
@@ -755,11 +752,7 @@ public:
 		: BinaryExpression(std::move(operand1), std::move(operand2), debugInfo)
 	{ }
 
-	void SetOverrideFrozen();
-
 protected:
-	bool m_OverrideFrozen{false};
-
 	ExpressionResult DoEvaluate(ScriptFrame& frame, DebugHint *dhint) const override;
 	bool GetReference(ScriptFrame& frame, bool init_dict, Value *parent, String *index, DebugHint **dhint) const override;
 
diff --git a/lib/config/vmops.hpp b/lib/config/vmops.hpp
index ea3098359..c6ba03bcc 100644
--- a/lib/config/vmops.hpp
+++ b/lib/config/vmops.hpp
@@ -246,12 +246,12 @@ public:
 		return object->GetFieldByName(field, sandboxed, debugInfo);
 	}
 
-	static inline void SetField(const Object::Ptr& context, const String& field, const Value& value, bool overrideFrozen, const DebugInfo& debugInfo = DebugInfo())
+	static inline void SetField(const Object::Ptr& context, const String& field, const Value& value, const DebugInfo& debugInfo = DebugInfo())
 	{
 		if (!context)
 			BOOST_THROW_EXCEPTION(ScriptError("Cannot set field '" + field + "' on a value that is not an object.", debugInfo));
 
-		return context->SetFieldByName(field, value, overrideFrozen, debugInfo);
+		return context->SetFieldByName(field, value, debugInfo);
 	}
 
 private:
diff --git a/test/icinga-notification.cpp b/test/icinga-notification.cpp
index a0aeb7df8..5b32178ba 100644
--- a/test/icinga-notification.cpp
+++ b/test/icinga-notification.cpp
@@ -36,9 +36,9 @@ struct DuplicateDueToFilterHelper
 		nc->SetExecute(new Function("", [this]() { ++called; }), true);
 		nc->Register();
 
-		n->SetFieldByName("host_name", "example.com", false, DebugInfo());
-		n->SetFieldByName("service_name", "disk", false, DebugInfo());
-		n->SetFieldByName("command", "mail", false, DebugInfo());
+		n->SetFieldByName("host_name", "example.com", DebugInfo());
+		n->SetFieldByName("service_name", "disk", DebugInfo());
+		n->SetFieldByName("command", "mail", DebugInfo());
 		n->SetUsersRaw(new Array({"jdoe"}), true);
 		n->SetTypeFilter(typeFilter);
 		n->SetStateFilter(stateFilter);
diff --git a/test/methods-pluginnotificationtask.cpp b/test/methods-pluginnotificationtask.cpp
index ec582dc8a..d1db38722 100644
--- a/test/methods-pluginnotificationtask.cpp
+++ b/test/methods-pluginnotificationtask.cpp
@@ -56,9 +56,9 @@ BOOST_AUTO_TEST_CASE(truncate_long_output)
 	nc->SetName("mail", true);
 	nc->Register();
 
-	n->SetFieldByName("host_name", "example.com", false, DebugInfo());
-	n->SetFieldByName("service_name", "disk", false, DebugInfo());
-	n->SetFieldByName("command", "mail", false, DebugInfo());
+	n->SetFieldByName("host_name", "example.com", DebugInfo());
+	n->SetFieldByName("service_name", "disk", DebugInfo());
+	n->SetFieldByName("command", "mail", DebugInfo());
 	n->OnAllConfigLoaded(); // link Service
 
 	Checkable::ExecuteCommandProcessFinishedHandler = [&promise](const Value&, const ProcessResult& pr) {