commit daeba37: [Rework] Html/CSS: Remove css C bindings as they are useless now

Vsevolod Stakhov vsevolod at highsecure.ru
Tue Jun 15 14:28:04 UTC 2021


Author: Vsevolod Stakhov
Date: 2021-06-15 12:54:47 +0100
URL: https://github.com/rspamd/rspamd/commit/daeba37c5557e5c33d6e752a204b723e657532a4

[Rework] Html/CSS: Remove css C bindings as they are useless now

---
 src/libserver/css/css.cxx         | 56 +++++++++++----------------------------
 src/libserver/css/css.h           | 45 -------------------------------
 src/libserver/css/css.hxx         |  7 ++++-
 src/libserver/css/css_parser.cxx  | 36 +++++++++----------------
 src/libserver/css/css_parser.hxx  |  4 +--
 src/libserver/css/parse_error.hxx |  4 +--
 src/libserver/html/html.cxx       | 22 ++++++++-------
 src/libserver/html/html.hxx       |  7 ++++-
 8 files changed, 58 insertions(+), 123 deletions(-)

diff --git a/src/libserver/css/css.cxx b/src/libserver/css/css.cxx
index 9b0e02230..12f7753c7 100644
--- a/src/libserver/css/css.cxx
+++ b/src/libserver/css/css.cxx
@@ -14,7 +14,6 @@
  * limitations under the License.
  */
 
-#include "css.h"
 #include "css.hxx"
 #include "contrib/robin-hood/robin_hood.h"
 #include "css_parser.hxx"
@@ -23,45 +22,6 @@
 #define DOCTEST_CONFIG_IMPLEMENT
 #include "doctest/doctest.h"
 
-static void
-rspamd_css_dtor(void *p)
-{
-	rspamd::css::css_style_sheet *style =
-			reinterpret_cast<rspamd::css::css_style_sheet *>(p);
-
-	delete style;
-}
-
-rspamd_css_ptr
-rspamd_css_parse_style(rspamd_mempool_t *pool, const gchar *begin, gsize len,
-					   rspamd_css_ptr existing_style,
-					   GError **err)
-{
-	auto parse_res = rspamd::css::parse_css(pool, {(const char* )begin, len},
-			reinterpret_cast<rspamd::css::css_style_sheet*>(existing_style));
-
-	if (parse_res.has_value()) {
-		/*
-		 * Detach style pointer from the unique_ptr as it will be managed by
-		 * C memory pool
-		 */
-		auto *detached_style = reinterpret_cast<rspamd_css_ptr>(parse_res.value().release());
-
-		/* We attach dtor merely if the existing style is not null */
-		if (!existing_style) {
-			rspamd_mempool_add_destructor(pool, rspamd_css_dtor, (void *) detached_style);
-		}
-
-		return detached_style;
-	}
-	else {
-		g_set_error(err, g_quark_from_static_string("css"),
-				static_cast<int>(parse_res.error().type),
-				"parse error");
-		return nullptr;
-	}
-}
-
 namespace rspamd::css {
 
 INIT_LOG_MODULE_PUBLIC(css);
@@ -136,4 +96,20 @@ css_style_sheet::add_selector_rule(std::unique_ptr<css_selector> &&selector,
 	}
 }
 
+auto
+css_parse_style(rspamd_mempool_t *pool,
+					 std::string_view input,
+					 std::shared_ptr<css_style_sheet> &&existing)
+					 -> css_return_pair
+{
+	auto parse_res = rspamd::css::parse_css(pool, input,
+			std::forward<std::shared_ptr<css_style_sheet>>(existing));
+
+	if (parse_res.has_value()) {
+		return std::make_pair(parse_res.value(), css_parse_error());
+	}
+
+	return std::make_pair(nullptr, parse_res.error());
+}
+
 }
\ No newline at end of file
diff --git a/src/libserver/css/css.h b/src/libserver/css/css.h
deleted file mode 100644
index 607f1fa2c..000000000
--- a/src/libserver/css/css.h
+++ /dev/null
@@ -1,45 +0,0 @@
-/*-
- * Copyright 2021 Vsevolod Stakhov
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *   http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-#ifndef RSPAMD_CSS_H
-#define RSPAMD_CSS_H
-
-#include "config.h"
-#include "mem_pool.h"
-
-#ifdef  __cplusplus
-extern "C" {
-#endif
-typedef void * rspamd_css_ptr;
-
-rspamd_css_ptr rspamd_css_parse_style (rspamd_mempool_t *pool,
-									   const gchar *begin,
-									   gsize len,
-									   rspamd_css_ptr existing_style,
-									   GError **err);
-
-/*
- * Unescape css
- */
-const gchar *rspamd_css_unescape (rspamd_mempool_t *pool,
-							const guchar *begin,
-							gsize len,
-							gsize *outlen);
-#ifdef  __cplusplus
-}
-#endif
-
-#endif //RSPAMD_CSS_H
diff --git a/src/libserver/css/css.hxx b/src/libserver/css/css.hxx
index 739ad3251..a169a1052 100644
--- a/src/libserver/css/css.hxx
+++ b/src/libserver/css/css.hxx
@@ -21,7 +21,6 @@
 #include <string>
 #include <memory>
 #include "logger.h"
-#include "css.h"
 #include "css_rule.hxx"
 #include "css_selector.hxx"
 
@@ -50,6 +49,12 @@ private:
 	std::unique_ptr<impl> pimpl;
 };
 
+using css_return_pair = std::pair<std::shared_ptr<css_style_sheet>, css_parse_error>;
+auto css_parse_style(rspamd_mempool_t *pool,
+					 std::string_view input,
+					 std::shared_ptr<css_style_sheet> &&existing) ->
+					 css_return_pair;
+
 }
 
 #endif //RSPAMD_CSS_H
\ No newline at end of file
diff --git a/src/libserver/css/css_parser.cxx b/src/libserver/css/css_parser.cxx
index d40520381..6526ebc57 100644
--- a/src/libserver/css/css_parser.cxx
+++ b/src/libserver/css/css_parser.cxx
@@ -148,15 +148,14 @@ class css_parser {
 public:
 	css_parser(void) = delete; /* Require mempool to be set for logging */
 	explicit css_parser(rspamd_mempool_t *pool) : pool (pool) {
-		/* Int this case we need to remove style in case of parser error */
-		owned_style = true;
+		style_object.reset();
 	}
 
 	/*
 	 * This constructor captures existing via unique_ptr, but it does not
 	 * destruct it on errors (we assume that it is owned somewhere else)
 	 */
-	explicit css_parser(css_style_sheet *existing, rspamd_mempool_t *pool) :
+	explicit css_parser(std::shared_ptr<css_style_sheet> &&existing, rspamd_mempool_t *pool) :
 			style_object(existing), pool(pool) {}
 
 	/*
@@ -169,9 +168,9 @@ public:
 	std::unique_ptr<css_consumed_block> consume_css_rule(const std::string_view &sv);
 	bool consume_input(const std::string_view &sv);
 
-	auto get_object_maybe(void) -> tl::expected<std::unique_ptr<css_style_sheet>, css_parse_error> {
+	auto get_object_maybe(void) -> tl::expected<std::shared_ptr<css_style_sheet>, css_parse_error> {
 		if (style_object) {
-			return std::move(style_object);
+			return style_object;
 		}
 
 		return tl::make_unexpected(error);
@@ -180,15 +179,8 @@ public:
 	/* Helper parser methods */
 	static bool need_unescape(const std::string_view &sv);
 
-	~css_parser() {
-		if (!owned_style && style_object) {
-			/* Avoid double free */
-			(void)style_object.release();
-		}
-	}
-
 private:
-	std::unique_ptr<css_style_sheet> style_object;
+	std::shared_ptr<css_style_sheet> style_object;
 	std::unique_ptr<css_tokeniser> tokeniser;
 
 	css_parse_error error;
@@ -197,7 +189,6 @@ private:
 	int rec_level = 0;
 	const int max_rec = 20;
 	bool eof = false;
-	bool owned_style = false;
 
 	/* Consumers */
 	auto component_value_consumer(std::unique_ptr<css_consumed_block> &top) -> bool;
@@ -618,7 +609,7 @@ bool css_parser::consume_input(const std::string_view &sv)
 	}
 
 	if (!style_object) {
-		style_object = std::make_unique<css_style_sheet>(pool);
+		style_object = std::make_shared<css_style_sheet>(pool);
 	}
 
 	for (auto &&rule : rules) {
@@ -772,10 +763,10 @@ get_rules_parser_functor(rspamd_mempool_t *pool,
  * Wrapper for the parser
  */
 auto parse_css(rspamd_mempool_t *pool, const std::string_view &st,
-						  css_style_sheet *other)
-	-> tl::expected<std::unique_ptr<css_style_sheet>, css_parse_error>
+			   std::shared_ptr<css_style_sheet> &&other)
+	-> tl::expected<std::shared_ptr<css_style_sheet>, css_parse_error>
 {
-	css_parser parser(other, pool);
+	css_parser parser(std::forward<std::shared_ptr<css_style_sheet>>(other), pool);
 	std::string_view processed_input;
 
 	if (css_parser::need_unescape(st)) {
@@ -878,14 +869,13 @@ TEST_SUITE("css parser") {
 		}
 
 		/* We now merge all styles together */
-		css_style_sheet *merged = nullptr;
+		std::shared_ptr<css_style_sheet> merged;
 		for (const auto &c : cases) {
-			auto ret = parse_css(pool, c, merged);
-			/* Avoid destruction as we are using this to interoperate with C */
-			merged = ret->release();
+			auto ret = parse_css(pool, c, std::move(merged));
+			merged.swap(ret.value());
 		}
 
-		CHECK(merged != nullptr);
+		CHECK(merged.get() != nullptr);
 
 		rspamd_mempool_delete(pool);
 	}
diff --git a/src/libserver/css/css_parser.hxx b/src/libserver/css/css_parser.hxx
index 1e0762d78..35b51ed23 100644
--- a/src/libserver/css/css_parser.hxx
+++ b/src/libserver/css/css_parser.hxx
@@ -193,8 +193,8 @@ class css_style_sheet;
  * Update the existing stylesheet with another stylesheet
  */
 auto parse_css(rspamd_mempool_t *pool, const std::string_view &st,
-					  css_style_sheet *other)
-	-> tl::expected<std::unique_ptr<css_style_sheet>, css_parse_error>;
+			   std::shared_ptr<css_style_sheet> &&other)
+	-> tl::expected<std::shared_ptr<css_style_sheet>, css_parse_error>;
 
 /*
  * Creates a functor to consume css selectors sequence
diff --git a/src/libserver/css/parse_error.hxx b/src/libserver/css/parse_error.hxx
index 458469afc..450c49d68 100644
--- a/src/libserver/css/parse_error.hxx
+++ b/src/libserver/css/parse_error.hxx
@@ -33,6 +33,7 @@ enum class css_parse_error_type {
 	PARSE_ERROR_BAD_NESTING,
 	PARSE_ERROR_NYI,
 	PARSE_ERROR_UNKNOWN_ERROR,
+	PARSE_ERROR_NO_ERROR,
 };
 
 struct css_parse_error {
@@ -41,9 +42,8 @@ struct css_parse_error {
 
 	explicit css_parse_error (css_parse_error_type type, const std::string &description) :
 		type(type), description(description) {}
-	explicit css_parse_error (css_parse_error_type type) :
+	explicit css_parse_error (css_parse_error_type type = css_parse_error_type::PARSE_ERROR_NO_ERROR) :
 			type(type) {}
-	css_parse_error() = default;
 };
 
 }
diff --git a/src/libserver/html/html.cxx b/src/libserver/html/html.cxx
index f30b9d1b8..5c0753d4a 100644
--- a/src/libserver/html/html.cxx
+++ b/src/libserver/html/html.cxx
@@ -21,11 +21,11 @@
 #include "html_block.hxx"
 #include "html.hxx"
 #include "libserver/css/css_value.hxx"
+#include "libserver/css/css.hxx"
 
 #include "url.h"
 #include "contrib/libucl/khash.h"
 #include "libmime/images.h"
-#include "css/css.h"
 #include "libutil/cxx/utf8_util.h"
 
 #include "html_tag_defs.hxx"
@@ -1381,7 +1381,7 @@ html_process_input(rspamd_mempool_t *pool,
 			 * We just search for the first </s substring and then pass
 			 * the content to the parser (if needed)
 			 */
-			goffset end_style = rspamd_substring_search (p, end - p,
+			auto end_style = rspamd_substring_search (p, end - p,
 					"</", 2);
 			if (end_style == -1 || g_ascii_tolower (p[end_style + 2]) != 's') {
 				/* Invalid style */
@@ -1390,14 +1390,18 @@ html_process_input(rspamd_mempool_t *pool,
 			else {
 
 				if (allow_css) {
-					GError *err = nullptr;
-					hc->css_style = rspamd_css_parse_style(pool, p, end_style, hc->css_style,
-							&err);
-
-					if (err) {
-						msg_info_pool ("cannot parse css: %e", err);
-						g_error_free (err);
+					auto ret_maybe =  rspamd::css::parse_css(pool, {p, std::size_t(end_style)},
+							std::move(hc->css_style));
+
+					if (!ret_maybe.has_value()) {
+						auto err_str = fmt::format("cannot parse css (error code: {}): {}",
+								static_cast<int>(ret_maybe.error().type),
+								ret_maybe.error().description.value_or("unknown error"));
+						msg_info_pool ("cannot parse css: %*s",
+								(int)err_str.size(), err_str.data());
 					}
+
+					hc->css_style = ret_maybe.value();
 				}
 
 				p += end_style;
diff --git a/src/libserver/html/html.hxx b/src/libserver/html/html.hxx
index 3fd778ade..d4a2fb58b 100644
--- a/src/libserver/html/html.hxx
+++ b/src/libserver/html/html.hxx
@@ -29,6 +29,11 @@
 #include <memory>
 #include "function2/function2.hpp"
 
+namespace rspamd::css {
+/* Forward declaration */
+class css_style_sheet;
+}
+
 namespace rspamd::html {
 
 struct html_block;
@@ -42,7 +47,7 @@ struct html_content {
 	std::vector<html_image *> images;
 	std::vector<std::unique_ptr<struct html_tag>> all_tags;
 	std::string parsed;
-	void *css_style;
+	std::shared_ptr<css::css_style_sheet> css_style;
 
 	/* Preallocate and reserve all internal structures */
 	html_content() {


More information about the Commits mailing list