From 2c316e86a137b0622863c18b2b6e9f09c693fbc3 Mon Sep 17 00:00:00 2001
From: Vsevolod Stakhov <vsevolod@highsecure.ru>
Date: Tue, 26 Feb 2019 16:51:07 +0000
Subject: [PATCH] [Fix] Fix maps object update race condition

Issue: #2467
---
 src/libserver/cfg_utils.c   | 22 ++++++++++--------
 src/libserver/dynamic_cfg.c | 27 +++++++++++++---------
 src/libutil/map.c           | 12 ++--------
 src/libutil/map.h           |  2 +-
 src/libutil/map_helpers.c   | 45 ++++++++++++++++++++++++-------------
 src/libutil/map_helpers.h   |  6 ++---
 src/lua/lua_map.c           | 14 +++++++-----
 src/plugins/surbl.c         | 16 +++++++++----
 8 files changed, 85 insertions(+), 59 deletions(-)

diff --git a/src/libserver/cfg_utils.c b/src/libserver/cfg_utils.c
index 5c6fc6672..af31db849 100644
--- a/src/libserver/cfg_utils.c
+++ b/src/libserver/cfg_utils.c
@@ -58,7 +58,7 @@ static gchar * rspamd_ucl_read_cb (gchar * chunk,
 	gint len,
 	struct map_cb_data *data,
 	gboolean final);
-static void rspamd_ucl_fin_cb (struct map_cb_data *data);
+static void rspamd_ucl_fin_cb (struct map_cb_data *data, void **target);
 static void rspamd_ucl_dtor_cb (struct map_cb_data *data);
 
 guint rspamd_config_log_id = (guint)-1;
@@ -1358,7 +1358,7 @@ rspamd_ucl_read_cb (gchar * chunk,
 }
 
 static void
-rspamd_ucl_fin_cb (struct map_cb_data *data)
+rspamd_ucl_fin_cb (struct map_cb_data *data, void **target)
 {
 	struct rspamd_ucl_map_cbdata *cbdata = data->cur_data, *prev =
 		data->prev_data;
@@ -1368,13 +1368,6 @@ rspamd_ucl_fin_cb (struct map_cb_data *data)
 	const ucl_object_t *cur;
 	struct rspamd_config *cfg = data->map->cfg;
 
-	if (prev != NULL) {
-		if (prev->buf != NULL) {
-			g_string_free (prev->buf, TRUE);
-		}
-		g_free (prev);
-	}
-
 	if (cbdata == NULL) {
 		msg_err_config ("map fin error: new data is NULL");
 		return;
@@ -1400,6 +1393,17 @@ rspamd_ucl_fin_cb (struct map_cb_data *data)
 		}
 		ucl_object_unref (obj);
 	}
+
+	if (target) {
+		*target = data->cur_data;
+	}
+
+	if (prev != NULL) {
+		if (prev->buf != NULL) {
+			g_string_free (prev->buf, TRUE);
+		}
+		g_free (prev);
+	}
 }
 
 static void
diff --git a/src/libserver/dynamic_cfg.c b/src/libserver/dynamic_cfg.c
index 1e970a17b..984a26697 100644
--- a/src/libserver/dynamic_cfg.c
+++ b/src/libserver/dynamic_cfg.c
@@ -175,22 +175,12 @@ json_config_read_cb (gchar * chunk,
 }
 
 static void
-json_config_fin_cb (struct map_cb_data *data)
+json_config_fin_cb (struct map_cb_data *data, void **target)
 {
 	struct config_json_buf *jb;
 	ucl_object_t *top;
 	struct ucl_parser *parser;
 
-	if (data->cur_data && data->prev_data) {
-		jb = data->prev_data;
-		/* Clean prev data */
-		if (jb->buf) {
-			g_string_free (jb->buf, TRUE);
-		}
-
-		g_free (jb);
-	}
-
 	/* Now parse json */
 	if (data->cur_data) {
 		jb = data->cur_data;
@@ -201,6 +191,7 @@ json_config_fin_cb (struct map_cb_data *data)
 
 	if (jb->buf == NULL) {
 		msg_err ("no data read");
+
 		return;
 	}
 
@@ -225,6 +216,20 @@ json_config_fin_cb (struct map_cb_data *data)
 	ucl_object_unref (jb->cfg->current_dynamic_conf);
 	apply_dynamic_conf (top, jb->cfg);
 	jb->cfg->current_dynamic_conf = top;
+
+	if (target) {
+		*target = data->cur_data;
+	}
+
+	if (data->prev_data) {
+		jb = data->prev_data;
+		/* Clean prev data */
+		if (jb->buf) {
+			g_string_free (jb->buf, TRUE);
+		}
+
+		g_free (jb);
+	}
 }
 
 static void
diff --git a/src/libutil/map.c b/src/libutil/map.c
index bab145e5e..d4687e433 100644
--- a/src/libutil/map.c
+++ b/src/libutil/map.c
@@ -1153,11 +1153,7 @@ rspamd_map_periodic_dtor (struct map_periodic_cbdata *periodic)
 
 	if (periodic->need_modify) {
 		/* We are done */
-		periodic->map->fin_callback (&periodic->cbdata);
-
-		if (periodic->cbdata.cur_data) {
-			*periodic->map->user_data = periodic->cbdata.cur_data;
-		}
+		periodic->map->fin_callback (&periodic->cbdata, periodic->map->user_data);
 	}
 	else {
 		/* Not modified */
@@ -2038,11 +2034,7 @@ rspamd_map_preload (struct rspamd_config *cfg)
 			}
 
 			if (succeed) {
-				map->fin_callback (&fake_cbd.cbdata);
-
-				if (fake_cbd.cbdata.cur_data) {
-					*map->user_data = fake_cbd.cbdata.cur_data;
-				}
+				map->fin_callback (&fake_cbd.cbdata, map->user_data);
 			}
 			else {
 				msg_info_map ("preload of %s failed", map->name);
diff --git a/src/libutil/map.h b/src/libutil/map.h
index 80aa03825..acf6eea4e 100644
--- a/src/libutil/map.h
+++ b/src/libutil/map.h
@@ -21,7 +21,7 @@ struct map_cb_data;
  */
 typedef gchar * (*map_cb_t)(gchar *chunk, gint len,
 	struct map_cb_data *data, gboolean final);
-typedef void (*map_fin_cb_t)(struct map_cb_data *data);
+typedef void (*map_fin_cb_t)(struct map_cb_data *data, void **target);
 typedef void (*map_dtor_t)(struct map_cb_data *data);
 
 typedef gboolean (*rspamd_map_traverse_cb)(gconstpointer key,
diff --git a/src/libutil/map_helpers.c b/src/libutil/map_helpers.c
index ece3017ab..ec4d95183 100644
--- a/src/libutil/map_helpers.c
+++ b/src/libutil/map_helpers.c
@@ -815,16 +815,11 @@ rspamd_kv_list_read (
 }
 
 void
-rspamd_kv_list_fin (struct map_cb_data *data)
+rspamd_kv_list_fin (struct map_cb_data *data, void **target)
 {
 	struct rspamd_map *map = data->map;
 	struct rspamd_hash_map_helper *htb;
 
-	if (data->prev_data) {
-		htb = (struct rspamd_hash_map_helper *)data->prev_data;
-		rspamd_map_helper_destroy_hash (htb);
-	}
-
 	if (data->cur_data) {
 		htb = (struct rspamd_hash_map_helper *)data->cur_data;
 		msg_info_map ("read hash of %d elements", kh_size (htb->htb));
@@ -832,6 +827,15 @@ rspamd_kv_list_fin (struct map_cb_data *data)
 		data->map->nelts = kh_size (htb->htb);
 		data->map->digest = rspamd_cryptobox_fast_hash_final (&htb->hst);
 	}
+
+	if (target) {
+		*target = data->cur_data;
+	}
+
+	if (data->prev_data) {
+		htb = (struct rspamd_hash_map_helper *)data->prev_data;
+		rspamd_map_helper_destroy_hash (htb);
+	}
 }
 
 void
@@ -870,16 +874,11 @@ rspamd_radix_read (
 }
 
 void
-rspamd_radix_fin (struct map_cb_data *data)
+rspamd_radix_fin (struct map_cb_data *data, void **target)
 {
 	struct rspamd_map *map = data->map;
 	struct rspamd_radix_map_helper *r;
 
-	if (data->prev_data) {
-		r = (struct rspamd_radix_map_helper *)data->prev_data;
-		rspamd_map_helper_destroy_radix (r);
-	}
-
 	if (data->cur_data) {
 		r = (struct rspamd_radix_map_helper *)data->cur_data;
 		msg_info_map ("read radix trie of %z elements: %s",
@@ -888,6 +887,15 @@ rspamd_radix_fin (struct map_cb_data *data)
 		data->map->nelts = kh_size (r->htb);
 		data->map->digest = rspamd_cryptobox_fast_hash_final (&r->hst);
 	}
+
+	if (target) {
+		*target = data->cur_data;
+	}
+
+	if (data->prev_data) {
+		r = (struct rspamd_radix_map_helper *)data->prev_data;
+		rspamd_map_helper_destroy_radix (r);
+	}
 }
 
 void
@@ -1088,14 +1096,11 @@ rspamd_glob_list_read_multiple (
 
 
 void
-rspamd_regexp_list_fin (struct map_cb_data *data)
+rspamd_regexp_list_fin (struct map_cb_data *data, void **target)
 {
 	struct rspamd_regexp_map_helper *re_map;
 	struct rspamd_map *map = data->map;
 
-	if (data->prev_data) {
-		rspamd_map_helper_destroy_regexp (data->prev_data);
-	}
 	if (data->cur_data) {
 		re_map = data->cur_data;
 		rspamd_re_map_finalize (re_map);
@@ -1105,6 +1110,14 @@ rspamd_regexp_list_fin (struct map_cb_data *data)
 		data->map->nelts = kh_size (re_map->htb);
 		data->map->digest = rspamd_cryptobox_fast_hash_final (&re_map->hst);
 	}
+
+	if (target) {
+		*target = data->cur_data;
+	}
+
+	if (data->prev_data) {
+		rspamd_map_helper_destroy_regexp (data->prev_data);
+	}
 }
 void
 rspamd_regexp_list_dtor (struct map_cb_data *data)
diff --git a/src/libutil/map_helpers.h b/src/libutil/map_helpers.h
index f7c86436f..4645024e3 100644
--- a/src/libutil/map_helpers.h
+++ b/src/libutil/map_helpers.h
@@ -52,7 +52,7 @@ gchar * rspamd_radix_read (
 		gint len,
 		struct map_cb_data *data,
 		gboolean final);
-void rspamd_radix_fin (struct map_cb_data *data);
+void rspamd_radix_fin (struct map_cb_data *data, void **target);
 void rspamd_radix_dtor (struct map_cb_data *data);
 
 /**
@@ -63,7 +63,7 @@ gchar * rspamd_kv_list_read (
 		gint len,
 		struct map_cb_data *data,
 		gboolean final);
-void rspamd_kv_list_fin (struct map_cb_data *data);
+void rspamd_kv_list_fin (struct map_cb_data *data, void **target);
 void rspamd_kv_list_dtor (struct map_cb_data *data);
 
 /**
@@ -91,7 +91,7 @@ gchar * rspamd_glob_list_read_multiple (
 		gint len,
 		struct map_cb_data *data,
 		gboolean final);
-void rspamd_regexp_list_fin (struct map_cb_data *data);
+void rspamd_regexp_list_fin (struct map_cb_data *data, void **target);
 void rspamd_regexp_list_dtor (struct map_cb_data *data);
 
 /**
diff --git a/src/lua/lua_map.c b/src/lua/lua_map.c
index 98b025ea2..3ba7d0ed1 100644
--- a/src/lua/lua_map.c
+++ b/src/lua/lua_map.c
@@ -416,7 +416,7 @@ lua_map_read (gchar *chunk, gint len,
 }
 
 static void
-lua_map_fin (struct map_cb_data *data)
+lua_map_fin (struct map_cb_data *data, void **target)
 {
 	struct lua_map_callback_data *cbdata;
 	struct rspamd_lua_map **pmap;
@@ -432,10 +432,6 @@ lua_map_fin (struct map_cb_data *data)
 		return;
 	}
 
-	if (data->prev_data) {
-		data->prev_data = NULL;
-	}
-
 	if (cbdata->ref == -1) {
 		msg_err_map ("map has no callback set");
 	}
@@ -454,6 +450,14 @@ lua_map_fin (struct map_cb_data *data)
 	}
 
 	cbdata->data = rspamd_fstring_assign (cbdata->data, "", 0);
+
+	if (target) {
+		*target = data->cur_data;
+	}
+
+	if (data->prev_data) {
+		data->prev_data = NULL;
+	}
 }
 
 static void
diff --git a/src/plugins/surbl.c b/src/plugins/surbl.c
index 842071afb..26af1210c 100644
--- a/src/plugins/surbl.c
+++ b/src/plugins/surbl.c
@@ -270,11 +270,15 @@ read_exceptions_list (gchar * chunk,
 }
 
 static void
-fin_exceptions_list (struct map_cb_data *data)
+fin_exceptions_list (struct map_cb_data *data, void **target)
 {
 	GHashTable **t;
 	gint i;
 
+	if (target) {
+		*target = data->cur_data;
+	}
+
 	if (data->prev_data) {
 		t = data->prev_data;
 		for (i = 0; i < MAX_LEVELS; i++) {
@@ -385,11 +389,15 @@ read_redirectors_list (gchar * chunk,
 			   final);
 }
 
-void
-fin_redirectors_list (struct map_cb_data *data)
+static void
+fin_redirectors_list (struct map_cb_data *data, void **target)
 {
 	GHashTable *tld_hash;
 
+	if (target) {
+		*target = data->cur_data;
+	}
+
 	if (data->prev_data) {
 		tld_hash = data->prev_data;
 
@@ -397,7 +405,7 @@ fin_redirectors_list (struct map_cb_data *data)
 	}
 }
 
-void
+static void
 dtor_redirectors_list (struct map_cb_data *data)
 {
 	GHashTable *tld_hash;
-- 
2.39.5