Fix recursion in abilities (counting version)

I use the same method what in master for weapon special recursion.

(cherry picked from commit c5351fb998)

data/test/scenarios/wml_tests/UnitsWML/AbilitiesWML/ability_cycle_filter_self_recursion_by_id.cfg

@@ -0,0 +1,57 @@
+#textdomain wesnoth-test
+
+#####
+# API(s) being tested: [filter_self]ability_id_active=
+##
+# Actions:
+# Alice and Bob are both of type Test Melee Quintain.
+# Give Alice a leadership_test_recursion ability, which is active only if it is itself active.
+# Have Alice attack with his weapon.
+##
+# Expected end state:
+# Deterministic end state, without crashing, but BROKE STRICT.
+# All abilities are inactive.
+# Bob takes 10 damage.
+#####
+{COMMON_KEEP_A_B_UNIT_TEST "filter_self_ability_cycle_recursion_by_id" (
+    [event]
+        name=start
+
+        [modify_unit]
+            [filter]
+                id=alice
+            [/filter]
+            [effect]
+                apply_to=new_ability
+                [abilities]
+                    [leadership]
+                        id=leadership_test_recursion
+                        value=50
+                        cumulative=no
+                        affect_self=yes
+                        [filter_self]
+                            ability_id_active=leadership_test_recursion
+                        [/filter_self]
+                    [/leadership]
+                [/abilities]
+            [/effect]
+        [/modify_unit]
+
+        [test_do_attack_by_id]
+            attacker=alice
+            defender=bob
+            weapon=0
+        [/test_do_attack_by_id]
+
+        [store_unit]
+            [filter]
+                id=bob
+            [/filter]
+            variable=bob
+        [/store_unit]
+
+        {ASSERT ({VARIABLE_CONDITIONAL bob.hitpoints equals 90})}
+
+        {SUCCEED}
+    [/event]
+) SIDE1_LEADER="Test Melee Quintain" SIDE2_LEADER="Test Melee Quintain"}

data/test/scenarios/wml_tests/UnitsWML/AbilitiesWML/ability_cycle_recursion_by_id.cfg

@@ -0,0 +1,57 @@
+#textdomain wesnoth-test
+
+#####
+# API(s) being tested: [filter]ability_id_active=
+##
+# Actions:
+# Alice and Bob are both of type Test Melee Quintain.
+# Give Alice and Bob a leadership_test_recursion ability, which is active only if it is itself active.
+# Have Alice attack with his weapon.
+##
+# Expected end state:
+# Deterministic end state, without crashing, but BROKE STRICT.
+# All abilities are inactive.
+# Bob takes 10 damage.
+#####
+{COMMON_KEEP_A_B_UNIT_TEST "ability_cycle_recursion_by_id" (
+    [event]
+        name=start
+
+        [modify_unit]
+            [filter]
+                id=alice
+            [/filter]
+            [effect]
+                apply_to=new_ability
+                [abilities]
+                    [leadership]
+                        id=leadership_test_recursion
+                        value=50
+                        cumulative=no
+                        affect_self=yes
+                        [filter]
+                            ability_id_active=leadership_test_recursion
+                        [/filter]
+                    [/leadership]
+                [/abilities]
+            [/effect]
+        [/modify_unit]
+
+        [test_do_attack_by_id]
+            attacker=alice
+            defender=bob
+            weapon=0
+        [/test_do_attack_by_id]
+
+        [store_unit]
+            [filter]
+                id=bob
+            [/filter]
+            variable=bob
+        [/store_unit]
+
+        {ASSERT ({VARIABLE_CONDITIONAL bob.hitpoints equals 90})}
+
+        {SUCCEED}
+    [/event]
+) SIDE1_LEADER="Test Melee Quintain" SIDE2_LEADER="Test Melee Quintain"}

data/test/scenarios/wml_tests/UnitsWML/AbilitiesWML/adjacent_abilities_recursion_by_id.cfg

@@ -0,0 +1,61 @@
+#textdomain wesnoth-test
+
+#####
+# API(s) being tested: [affect_adjacent][filter]ability_id_active=
+##
+# Actions:
+# Alice and Bob are both of type Test Melee Quintain.
+# Give Alice and Bob a leadership_test_recursion ability, which is active only if an adjacent unit also has it active.
+# Have Alice attack with his weapon.
+##
+# Expected end state:
+# Deterministic end state, without crashing, but BROKE STRICT.
+# All abilities are inactive.
+# Bob takes 10 damage.
+#####
+#####
+{COMMON_KEEP_A_B_UNIT_TEST "adjacent_abilities_recursion_by_id" (
+    [event]
+        name=start
+
+        [modify_unit]
+            [filter]
+            [/filter]
+            [effect]
+                apply_to=new_ability
+                [abilities]
+                    [leadership]
+                        id=leadership_test_recursion
+                        value=50
+                        cumulative=no
+                        affect_self=no
+                        affect_allies=yes
+                        affect_enemies=yes
+                        [affect_adjacent]
+                            [filter]
+                                ability_id_active=leadership_test_recursion
+                            [/filter]
+                        [/affect_adjacent]
+                    [/leadership]
+                [/abilities]
+            [/effect]
+        [/modify_unit]
+
+        [test_do_attack_by_id]
+            attacker=alice
+            defender=bob
+            weapon=0
+        [/test_do_attack_by_id]
+
+        [store_unit]
+            [filter]
+                id=bob
+            [/filter]
+            variable=bob
+        [/store_unit]
+
+        {ASSERT ({VARIABLE_CONDITIONAL bob.hitpoints equals 90})}
+
+        {SUCCEED}
+    [/event]
+) SIDE1_LEADER="Test Melee Quintain" SIDE2_LEADER="Test Melee Quintain"}

src/units/abilities.cpp

@@ -351,8 +351,99 @@ std::vector<std::tuple<std::string, t_string, t_string, t_string>> unit::ability
 	return res;
 }
 
+namespace {
+/**
+ * Value of unit::num_recursion_ at which allocations of further recursion_guards fail. This
+ * value is used per unit.
+ *
+ *
+ * With the limit set to 2, all tests pass, but as the limit only affects cases that would otherwise
+ * lead to a crash, it seems reasonable to leave a little headroom for more complex logic.
+ */
+constexpr unsigned int UNIT_RECURSION_LIMIT = 3;
+};
+
+namespace {
+	/**
+	 * Print "Recursion limit reached" log messages, including deduplication if the same problem has
+	 * already been logged.
+	 */
+	void show_recursion_warning(const unit& unit, const config& filter) {
+		// This function is only called when an ability is checked for the second time
+		// filter has already been parsed multiple times, so I'm not trying to optimize the performance
+		// of this; it's merely to prevent the logs getting spammed. For example, each of
+		// four_cycle_recursion_branching and event_test_filter_attack_student_weapon_condition only log
+		// 3 unique messages, but without deduplication they'd log 1280 and 392 respectively.
+		static std::vector<std::tuple<std::string, std::string>> already_shown;
+
+		auto identifier = std::tuple<std::string, std::string>{unit.id(), filter.debug()};
+		if(utils::contains(already_shown, identifier)) {
+			return;
+		}
+
+		std::string_view filter_text_view = std::get<1>(identifier);
+		utils::trim(filter_text_view);
+		ERR_NG << "Looped recursion error for unit '" << unit.id()
+		<< "' while checking ability '" << filter_text_view << "'";
+
+		// Arbitrary limit, just ensuring that having a huge number of specials causing recursion
+		// warnings can't lead to unbounded memory consumption here.
+		if(already_shown.size() > 100) {
+			already_shown.clear();
+		}
+		already_shown.push_back(std::move(identifier));
+	}
+}//anonymous namespace
+
+unit::recursion_guard unit::update_variables_recursion() const
+{
+	if(num_recursion_ < UNIT_RECURSION_LIMIT) {
+		return recursion_guard(*this);
+	}
+	return recursion_guard();
+}
+
+unit::recursion_guard::recursion_guard() = default;
+
+unit::recursion_guard::recursion_guard(const unit & u)
+	: parent(u.shared_from_this())
+{
+	u.num_recursion_++;
+}
+
+unit::recursion_guard::recursion_guard(unit::recursion_guard&& other)
+{
+	std::swap(parent, other.parent);
+}
+
+unit::recursion_guard::operator bool() const {
+	return bool(parent);
+}
+
+unit::recursion_guard& unit::recursion_guard::operator=(unit::recursion_guard&& other)
+{
+	assert(this != &other);
+	assert(!parent);
+	std::swap(parent, other.parent);
+	return *this;
+}
+
+unit::recursion_guard::~recursion_guard()
+{
+	if(parent) {
+		assert(parent->num_recursion_ > 0);
+		parent->num_recursion_--;
+	}
+}
+
 bool unit::ability_active(const std::string& ability,const config& cfg,const map_location& loc) const
 {
+	unit::recursion_guard filter_lock;
+	filter_lock = update_variables_recursion();
+	if(!filter_lock) {
+		show_recursion_warning(*this, cfg);
+		return false;
+	}
 	bool illuminates = ability == "illuminates";
 
 	if (auto afilter = cfg.optional_child("filter"))
@@ -425,6 +516,14 @@ bool unit::ability_active(const std::string& ability,const config& cfg,const map
 
 bool unit::ability_affects_adjacent(const std::string& ability, const config& cfg,int dir,const map_location& loc,const unit& from) const
 {
+	unit::recursion_guard adj_lock;
+	if(cfg.has_child("affect_adjacent")){
+		adj_lock = update_variables_recursion();
+		if(!adj_lock) {
+			show_recursion_warning(*this, cfg);
+			return false;
+		}
+	}
 	bool illuminates = ability == "illuminates";
 
 	assert(dir >=0 && dir <= 5);
@@ -453,6 +552,14 @@ bool unit::ability_affects_adjacent(const std::string& ability, const config& cf
 bool unit::ability_affects_self(const std::string& ability,const config& cfg,const map_location& loc) const
 {
 	auto filter = cfg.optional_child("filter_self");
+	unit::recursion_guard self_lock;
+	if(filter){
+		self_lock = update_variables_recursion();
+		if(!self_lock) {
+			show_recursion_warning(*this, cfg);
+			return false;
+		}
+	}
 	bool affect_self = cfg["affect_self"].to_bool(true);
 	if (!filter || !affect_self) return affect_self;
 	return unit_filter(vconfig(*filter)).set_use_flat_tod(ability == "illuminates").matches(*this, loc);
@@ -1313,6 +1420,12 @@ namespace { // Helpers for attack_type::special_active()
 		if (!u) {
 			return false;
 		}
+		//update and check variable_recursion for prevent check ability_id/type_active in case of infinite recursion.
+		unit::recursion_guard special_lock = (*u).update_variables_recursion();
+		if(!special_lock) {
+			show_recursion_warning(*u, filter);
+			return false;
+		}
 
 		unit_filter ufilt{vconfig(*filter_child)};
 

src/units/unit.hpp

@@ -1813,6 +1813,44 @@ public:
 	 */
 	bool ability_matches_filter(const config & cfg, const std::string& tag_name, const config & filter) const;
 
+	/**
+	 * Helper similar to std::unique_lock for detecting when calculations such as abilities
+	 * have entered infinite recursion.
+	 *
+	 * This assumes that there's only a single thread accessing the unit, it's a lightweight
+	 * increment/decrement counter rather than a mutex.
+	 */
+	class recursion_guard {
+		friend class unit;
+		/**
+		 * Only expected to be called in update_variables_recursion(), which handles some of the checks.
+		 */
+		explicit recursion_guard(const unit& u);
+	public:
+		/**
+		 * Construct an empty instance, only useful for extending the lifetime of a
+		 * recursion_guard returned from unit.update_variables_recursion() by
+		 * std::moving it to an instance declared in a larger scope.
+		 */
+		explicit recursion_guard();
+
+		/**
+		 * Returns true if a level of recursion was available at the time when update_variables_recursion()
+		 * created this object.
+		 */
+		operator bool() const;
+
+		recursion_guard(recursion_guard&& other);
+		recursion_guard(const recursion_guard& other) = delete;
+		recursion_guard& operator=(recursion_guard&&);
+		recursion_guard& operator=(const recursion_guard&) = delete;
+		~recursion_guard();
+	private:
+		std::shared_ptr<const unit> parent;
+	};
+
+	recursion_guard update_variables_recursion() const;
+
 
 private:
 
@@ -1963,6 +2001,8 @@ private:
 
 	std::string role_;
 	attack_list attacks_;
+	/** Number of instances of recursion_guard that are currently allocated permission to recurse */
+	mutable unsigned int num_recursion_ = 0;
 
 protected:
 	// TODO: I think we actually consider this to be part of the gamestate, so it might be better if it's not mutable,

wml_test_schedule

@@ -383,6 +383,9 @@
 9 four_cycle_recursion_branching
 9 four_cycle_recursion_by_id
 9 four_cycle_recursion_by_tagname
+9 ability_cycle_recursion_by_id
+9 filter_self_ability_cycle_recursion_by_id
+9 adjacent_abilities_recursion_by_id
 0 swarms_filter_student_by_type
 0 swarms_effects_not_checkable
 0 filter_special_id_active