From: Vsevolod Stakhov <vsevolod@rspamd.com>
Date: Sat, 28 May 2022 11:34:33 +0000 (+0100)
Subject: [Fix] Another try to fix race condition in the runtime destruction
X-Git-Tag: 3.3~220
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=563faab280101acdcbfe8a8f681dec74b50db85c;p=rspamd.git

[Fix] Another try to fix race condition in the runtime destruction
---

diff --git a/src/libserver/rspamd_symcache.h b/src/libserver/rspamd_symcache.h
index 95ca614af..8707c631f 100644
--- a/src/libserver/rspamd_symcache.h
+++ b/src/libserver/rspamd_symcache.h
@@ -520,6 +520,12 @@ const struct rspamd_symcache_item_stat *
  * @param task
  */
 void rspamd_symcache_enable_profile (struct rspamd_task *task);
+
+/**
+ * Destroy internal state of the symcache runtime
+ * @param task
+ */
+void rspamd_symcache_runtime_destroy (struct rspamd_task *task);
 #ifdef  __cplusplus
 }
 #endif
diff --git a/src/libserver/symcache/symcache_c.cxx b/src/libserver/symcache/symcache_c.cxx
index 4d1f28800..580e0d137 100644
--- a/src/libserver/symcache/symcache_c.cxx
+++ b/src/libserver/symcache/symcache_c.cxx
@@ -668,4 +668,11 @@ rspamd_symcache_finalize_item(struct rspamd_task *task,
 	auto *real_dyn_item = C_API_SYMCACHE_DYN_ITEM(item);
 
 	cache_runtime->finalize_item(task, real_dyn_item);
+}
+
+void
+rspamd_symcache_runtime_destroy (struct rspamd_task *task)
+{
+	auto *cache_runtime = C_API_SYMCACHE_RUNTIME(task->symcache_runtime);
+	cache_runtime->savepoint_dtor();
 }
\ No newline at end of file
diff --git a/src/libserver/symcache/symcache_runtime.cxx b/src/libserver/symcache/symcache_runtime.cxx
index 4bfa99529..f406a1d58 100644
--- a/src/libserver/symcache/symcache_runtime.cxx
+++ b/src/libserver/symcache/symcache_runtime.cxx
@@ -45,8 +45,6 @@ symcache_runtime::create(struct rspamd_task *task, symcache &cache) -> symcache_
 			sizeof(struct cache_dynamic_item) * cur_order->size());
 
 	checkpoint->order = cache.get_cache_order();
-	rspamd_mempool_add_destructor(task->task_pool,
-			symcache_runtime::savepoint_dtor, checkpoint);
 
 	/* Calculate profile probability */
 	ev_now_update_if_cheap(task->event_loop);
@@ -647,6 +645,7 @@ rspamd_symcache_delayed_item_fin(gpointer ud)
 {
 	auto *cbd = (struct rspamd_symcache_delayed_cbdata *) ud;
 
+	cbd->event = nullptr;
 	cbd->runtime->unset_slow();
 	ev_timer_stop(cbd->task->event_loop, &cbd->tm);
 }
@@ -656,12 +655,15 @@ rspamd_symcache_delayed_item_cb(EV_P_ ev_timer *w, int what)
 {
 	auto *cbd = (struct rspamd_symcache_delayed_cbdata *) w->data;
 
-	cbd->event = NULL;
+	if (cbd->event) {
+		cbd->event = nullptr;
 
-	/* Timer will be stopped here */
-	rspamd_session_remove_event (cbd->task->s,
-			rspamd_symcache_delayed_item_fin, cbd);
-	cbd->runtime->process_item_rdeps(cbd->task, cbd->item);
+		/* Timer will be stopped here */
+		rspamd_session_remove_event (cbd->task->s,
+				rspamd_symcache_delayed_item_fin, cbd);
+
+		cbd->runtime->process_item_rdeps(cbd->task, cbd->item);
+	}
 
 }
 
@@ -671,7 +673,7 @@ rspamd_delayed_timer_dtor(gpointer d)
 	auto *cbd = (struct rspamd_symcache_delayed_cbdata *) d;
 
 	if (cbd->event) {
-		/* Event has not been executed */
+		/* Event has not been executed, this will also stop a timer */
 		rspamd_session_remove_event (cbd->task->s,
 				rspamd_symcache_delayed_item_fin, cbd);
 		cbd->event = nullptr;
@@ -785,6 +787,11 @@ auto symcache_runtime::process_item_rdeps(struct rspamd_task *task, cache_item *
 {
 	auto *cache_ptr = reinterpret_cast<symcache *>(task->cfg->cache);
 
+	// Avoid race condition with the runtime destruction and the delay timer
+	if (!order) {
+		return;
+	}
+
 	for (const auto &rdep: item->rdeps) {
 		if (rdep.item) {
 			auto *dyn_item = get_dynamic_item(rdep.item->id);
diff --git a/src/libserver/symcache/symcache_runtime.hxx b/src/libserver/symcache/symcache_runtime.hxx
index 237cad2d2..358cba4fb 100644
--- a/src/libserver/symcache/symcache_runtime.hxx
+++ b/src/libserver/symcache/symcache_runtime.hxx
@@ -60,13 +60,6 @@ class symcache_runtime {
 	mutable struct cache_dynamic_item dynamic_items[];
 	/* We allocate this structure merely in memory pool, so destructor is absent */
 	~symcache_runtime() = delete;
-	/* Dropper for a shared ownership */
-	static auto savepoint_dtor(void *ptr) -> void {
-		auto *real_savepoint = (symcache_runtime *)ptr;
-
-		/* Drop shared ownership */
-		real_savepoint->order.reset();
-	}
 
 	auto process_symbol(struct rspamd_task *task, symcache &cache, cache_item *item,
 			cache_dynamic_item *dyn_item) -> bool;
@@ -78,6 +71,12 @@ class symcache_runtime {
 						 cache_dynamic_item *dyn_item, bool check_only) -> bool;
 
 public:
+	/* Dropper for a shared ownership */
+	auto savepoint_dtor() -> void {
+
+		/* Drop shared ownership */
+		order.reset();
+	}
 	/**
 	 * Creates a cache runtime using task mempool
 	 * @param task
diff --git a/src/libserver/task.c b/src/libserver/task.c
index 2b2dc727d..46aabcb62 100644
--- a/src/libserver/task.c
+++ b/src/libserver/task.c
@@ -309,8 +309,17 @@ rspamd_task_free (struct rspamd_task *task)
 		rspamd_message_unref (task->message);
 
 		if (task->flags & RSPAMD_TASK_FLAG_OWN_POOL) {
+			rspamd_mempool_destructors_enforce (task->task_pool);
+
+			if (task->symcache_runtime) {
+				rspamd_symcache_runtime_destroy (task);
+			}
+
 			rspamd_mempool_delete (task->task_pool);
 		}
+		else if (task->symcache_runtime) {
+			rspamd_symcache_runtime_destroy (task);
+		}
 	}
 }