commit 1df8825: [Project] Css: Fix rules merging

[Vsevolod Stakhov]

Author: Vsevolod Stakhov
Date: 2021-05-04 17:51:30 +0100
URL: https://github.com/rspamd/rspamd/commit/1df88256728426cf64f6a3e5f1a0219d1342e7df (HEAD -> master)

[Project] Css: Fix rules merging

---
 src/libserver/css/css_rule.cxx | 45 +++++++++++++++++++++++++++++++++---------
 src/libserver/css/css_rule.hxx |  2 +-
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/src/libserver/css/css_rule.cxx b/src/libserver/css/css_rule.cxx
index 3afe522e6..02f33aa13 100644
--- a/src/libserver/css/css_rule.cxx
+++ b/src/libserver/css/css_rule.cxx
@@ -26,16 +26,43 @@ namespace rspamd::css {
 /* Class methods */
 void css_rule::override_values(const css_rule &other)
 {
-	values.resize(0);
-	values.reserve(other.values.size());
+	int bits = 0;
+	/* Ensure that our bitset is large enough */
+	static_assert(static_cast<std::size_t>(css_value::css_value_type::CSS_VALUE_NYI) << 1 <
+				  std::numeric_limits<int>::max());
+
+	for (const auto &v : values) {
+		bits |= static_cast<int>(v.type);
+	}
+
+	for (const auto &ov : other.values) {
+		if (isset(&bits, static_cast<int>(ov.type))) {
+			/* We need to override the existing value */
+			/*
+			 * The algorithm is not very efficient,
+			 * so we need to sort the values first and have a O(N) algorithm
+			 * On the other hand, values vectors are usually limited to the
+			 * number of elements about less then 10, so this O(N^2) algorithm
+			 * is probably ok here
+			 */
+			for (auto &v : values) {
+				if (v.type == ov.type) {
+					v = ov;
+				}
+			}
+		}
+	}
 
-	std::copy(other.values.begin(), other.values.end(),
-			std::back_inserter(values));
+	/* Copy only not set values */
+	std::copy_if(other.values.begin(), other.values.end(), std::back_inserter(values),
+			[&bits](const auto &elt) -> bool {
+				return !isset(&bits, static_cast<int>(elt.type));
+			});
 }
 
 void css_rule::merge_values(const css_rule &other)
 {
-	int bits = 0;
+	unsigned int bits = 0;
 	/* Ensure that our bitset is large enough */
 	static_assert(static_cast<std::size_t>(css_value::css_value_type::CSS_VALUE_NYI) << 1 <
 		std::numeric_limits<int>::max());
@@ -51,7 +78,7 @@ void css_rule::merge_values(const css_rule &other)
 	});
 }
 
-auto css_declarations_block::add_rule(rule_shared_ptr &&rule) -> bool
+auto css_declarations_block::add_rule(rule_shared_ptr rule) -> bool
 {
 	auto it = rules.find(rule);
 	auto &&remote_prop = rule->get_prop();
@@ -319,7 +346,7 @@ css_declarations_block::merge_block(const css_declarations_block &other, merge_t
 -> void {
 	const auto &other_rules = other.get_rules();
 
-	for (const auto &rule : other_rules) {
+	for (auto &rule : other_rules) {
 		auto &&found_it = rules.find(rule);
 
 		if (found_it != rules.end()) {
@@ -327,11 +354,11 @@ css_declarations_block::merge_block(const css_declarations_block &other, merge_t
 			switch (how) {
 			case merge_type::merge_override:
 				/* Override */
-				rules.insert(rule);
+				(*found_it)->override_values(*rule);
 				break;
 			case merge_type::merge_duplicate:
 				/* Merge values */
-				(*found_it)->merge_values(*rule);
+				add_rule(rule);
 				break;
 			case merge_type::merge_parent:
 				/* Do not merge parent rule if more specific local one is presented */
diff --git a/src/libserver/css/css_rule.hxx b/src/libserver/css/css_rule.hxx
index adf01437f..113402905 100644
--- a/src/libserver/css/css_rule.hxx
+++ b/src/libserver/css/css_rule.hxx
@@ -89,7 +89,7 @@ public:
 	};
 
 	css_declarations_block() = default;
-	auto add_rule(rule_shared_ptr &&rule) -> bool;
+	auto add_rule(rule_shared_ptr rule) -> bool;
 	auto merge_block(const css_declarations_block &other,
 				  merge_type how = merge_type::merge_duplicate) -> void;
 	auto get_rules(void) const -> const auto & {