commit 0f32df6: [Fix] Core: Fix address rotation bug

Vsevolod Stakhov vsevolod at highsecure.ru
Mon Feb 18 13:14:04 UTC 2019


Author: Vsevolod Stakhov
Date: 2019-02-18 12:58:57 +0000
URL: https://github.com/rspamd/rspamd/commit/0f32df6f44c75cb9be69618e699fb2972cc7d421 (HEAD -> master)

[Fix] Core: Fix address rotation bug
Previously, upstream.get_addr function returned the new address of the
upstream. Unfortunately, it was used for printing addresses. It caused
the following situation: let's imagine we have A1 and A2 where A1 was
initially selected. So the connection was performed to A1:

                           Current addr   Selected addr

   Connect+---------+      A2+------>A1   A1
                    |
+-+Print failure<---+      A1+------>A2   A2
|                        +----+
+->Mark failure+-------->+ A2 |
                         +----+

But the failure OP as well as log message told about `A2` where the real
problem happened with `A1`.

This commit adds distinguishing between getting the next and the current
address of the upstream resolving this issue.

---
 src/fuzzy_storage.c                   |  4 ++--
 src/libserver/dns.c                   |  2 +-
 src/libserver/fuzzy_backend_redis.c   |  8 ++++----
 src/libstat/backends/redis_backend.c  |  6 +++---
 src/libstat/learn_cache/redis_cache.c |  2 +-
 src/libutil/upstream.c                |  8 +++++++-
 src/libutil/upstream.h                |  9 ++++++++-
 src/lua/lua_upstream.c                |  2 +-
 src/plugins/fuzzy_check.c             | 14 +++++++-------
 src/plugins/surbl.c                   |  5 +++--
 src/rspamd_proxy.c                    | 25 +++++++++++++++----------
 test/rspamd_upstream_test.c           | 20 ++++++++++----------
 12 files changed, 62 insertions(+), 43 deletions(-)

diff --git a/src/fuzzy_storage.c b/src/fuzzy_storage.c
index bbd6b36ca..d0a4f3602 100644
--- a/src/fuzzy_storage.c
+++ b/src/fuzzy_storage.c
@@ -568,7 +568,7 @@ fuzzy_mirror_error_handler (struct rspamd_http_connection *conn, GError *err)
 	msg_info ("abnormally closing connection from backend: %s:%s, "
 			"error: %e",
 			bk_conn->mirror->name,
-			rspamd_inet_address_to_string (rspamd_upstream_addr (bk_conn->up)),
+			rspamd_inet_address_to_string (rspamd_upstream_addr_cur (bk_conn->up)),
 			err);
 
 	fuzzy_mirror_close_connection (bk_conn);
@@ -604,7 +604,7 @@ rspamd_fuzzy_send_update_mirror (struct rspamd_fuzzy_storage_ctx *ctx,
 	}
 
 	conn->sock = rspamd_inet_address_connect (
-			rspamd_upstream_addr (conn->up),
+			rspamd_upstream_addr_next (conn->up),
 			SOCK_STREAM, TRUE);
 
 	if (conn->sock == -1) {
diff --git a/src/libserver/dns.c b/src/libserver/dns.c
index 7ad266c2f..4fbf40728 100644
--- a/src/libserver/dns.c
+++ b/src/libserver/dns.c
@@ -256,7 +256,7 @@ rspamd_dns_server_init (struct upstream *up, guint idx, gpointer ud)
 	void *serv;
 	struct rdns_upstream_elt *elt;
 
-	addr = rspamd_upstream_addr (up);
+	addr = rspamd_upstream_addr_next (up);
 
 	if (r->cfg) {
 		serv = rdns_resolver_add_server (r->r, rspamd_inet_address_to_string (addr),
diff --git a/src/libserver/fuzzy_backend_redis.c b/src/libserver/fuzzy_backend_redis.c
index fbccac5ab..956979d42 100644
--- a/src/libserver/fuzzy_backend_redis.c
+++ b/src/libserver/fuzzy_backend_redis.c
@@ -648,7 +648,7 @@ rspamd_fuzzy_backend_check_redis (struct rspamd_fuzzy_backend *bk,
 			0);
 
 	session->up = up;
-	addr = rspamd_upstream_addr (up);
+	addr = rspamd_upstream_addr_next (up);
 	g_assert (addr != NULL);
 	session->ctx = rspamd_redis_pool_connect (backend->pool,
 			backend->dbname, backend->password,
@@ -774,7 +774,7 @@ rspamd_fuzzy_backend_count_redis (struct rspamd_fuzzy_backend *bk,
 			0);
 
 	session->up = up;
-	addr = rspamd_upstream_addr (up);
+	addr = rspamd_upstream_addr_next (up);
 	g_assert (addr != NULL);
 	session->ctx = rspamd_redis_pool_connect (backend->pool,
 			backend->dbname, backend->password,
@@ -899,7 +899,7 @@ rspamd_fuzzy_backend_version_redis (struct rspamd_fuzzy_backend *bk,
 			0);
 
 	session->up = up;
-	addr = rspamd_upstream_addr (up);
+	addr = rspamd_upstream_addr_next (up);
 	g_assert (addr != NULL);
 	session->ctx = rspamd_redis_pool_connect (backend->pool,
 			backend->dbname, backend->password,
@@ -1459,7 +1459,7 @@ rspamd_fuzzy_backend_update_redis (struct rspamd_fuzzy_backend *bk,
 			0);
 
 	session->up = up;
-	addr = rspamd_upstream_addr (up);
+	addr = rspamd_upstream_addr_next (up);
 	g_assert (addr != NULL);
 	session->ctx = rspamd_redis_pool_connect (backend->pool,
 			backend->dbname, backend->password,
diff --git a/src/libstat/backends/redis_backend.c b/src/libstat/backends/redis_backend.c
index d7402db98..d54767c12 100644
--- a/src/libstat/backends/redis_backend.c
+++ b/src/libstat/backends/redis_backend.c
@@ -975,7 +975,7 @@ rspamd_redis_async_stat_cb (struct rspamd_stat_async_elt *elt, gpointer d)
 					0);
 
 	g_assert (cbdata->selected != NULL);
-	addr = rspamd_upstream_addr (cbdata->selected);
+	addr = rspamd_upstream_addr_next (cbdata->selected);
 	g_assert (addr != NULL);
 
 	if (rspamd_inet_address_get_af (addr) == AF_UNIX) {
@@ -1522,7 +1522,7 @@ rspamd_redis_runtime (struct rspamd_task *task,
 	rt->stcf = stcf;
 	rt->redis_object_expanded = object_expanded;
 
-	addr = rspamd_upstream_addr (up);
+	addr = rspamd_upstream_addr_next (up);
 	g_assert (addr != NULL);
 
 	if (rspamd_inet_address_get_af (addr) == AF_UNIX) {
@@ -1693,7 +1693,7 @@ rspamd_redis_learn_tokens (struct rspamd_task *task, GPtrArray *tokens,
 		}
 	}
 
-	addr = rspamd_upstream_addr (up);
+	addr = rspamd_upstream_addr_next (up);
 	g_assert (addr != NULL);
 
 	if (rspamd_inet_address_get_af (addr) == AF_UNIX) {
diff --git a/src/libstat/learn_cache/redis_cache.c b/src/libstat/learn_cache/redis_cache.c
index 6a0aa1da7..aea024e06 100644
--- a/src/libstat/learn_cache/redis_cache.c
+++ b/src/libstat/learn_cache/redis_cache.c
@@ -385,7 +385,7 @@ rspamd_stat_cache_redis_runtime (struct rspamd_task *task,
 	rt->task = task;
 	rt->ctx = ctx;
 
-	addr = rspamd_upstream_addr (up);
+	addr = rspamd_upstream_addr_next (up);
 	g_assert (addr != NULL);
 
 	if (rspamd_inet_address_get_af (addr) == AF_UNIX) {
diff --git a/src/libutil/upstream.c b/src/libutil/upstream.c
index eb88e501a..938c49011 100644
--- a/src/libutil/upstream.c
+++ b/src/libutil/upstream.c
@@ -630,7 +630,7 @@ rspamd_upstream_dtor (struct upstream *up)
 }
 
 rspamd_inet_addr_t*
-rspamd_upstream_addr (struct upstream *up)
+rspamd_upstream_addr_next (struct upstream *up)
 {
 	guint idx, next_idx;
 	struct upstream_addr_elt *e1, *e2;
@@ -646,6 +646,12 @@ rspamd_upstream_addr (struct upstream *up)
 	return e2->addr;
 }
 
+rspamd_inet_addr_t*
+rspamd_upstream_addr_cur (const struct upstream *up)
+{
+	return g_ptr_array_index (up->addrs.addr, up->addrs.cur);
+}
+
 const gchar*
 rspamd_upstream_name (struct upstream *up)
 {
diff --git a/src/libutil/upstream.h b/src/libutil/upstream.h
index 5c0c92afc..4db962765 100644
--- a/src/libutil/upstream.h
+++ b/src/libutil/upstream.h
@@ -207,12 +207,19 @@ void rspamd_upstreams_add_watch_callback (struct upstream_list *ups,
 										  GFreeFunc free_func,
 										  gpointer ud);
 
+/**
+ * Returns the next IP address of the upstream (internal rotation)
+ * @param up
+ * @return
+ */
+rspamd_inet_addr_t* rspamd_upstream_addr_next (struct upstream *up);
+
 /**
  * Returns the current IP address of the upstream
  * @param up
  * @return
  */
-rspamd_inet_addr_t* rspamd_upstream_addr (struct upstream *up);
+rspamd_inet_addr_t* rspamd_upstream_addr_cur (const struct upstream *up);
 
 /**
  * Add custom address for an upstream (ownership of addr is transferred to upstream)
diff --git a/src/lua/lua_upstream.c b/src/lua/lua_upstream.c
index 1a4d6b128..37c541d9d 100644
--- a/src/lua/lua_upstream.c
+++ b/src/lua/lua_upstream.c
@@ -110,7 +110,7 @@ lua_upstream_get_addr (lua_State *L)
 	struct upstream *up = lua_check_upstream (L);
 
 	if (up) {
-		rspamd_lua_ip_push (L, rspamd_upstream_addr (up));
+		rspamd_lua_ip_push (L, rspamd_upstream_addr_next (up));
 	}
 	else {
 		lua_pushnil (L);
diff --git a/src/plugins/fuzzy_check.c b/src/plugins/fuzzy_check.c
index 760429ba2..58cdd3376 100644
--- a/src/plugins/fuzzy_check.c
+++ b/src/plugins/fuzzy_check.c
@@ -2209,7 +2209,7 @@ fuzzy_check_io_callback (gint fd, short what, void *arg)
 		msg_err_task ("got error on IO with server %s(%s), on %s, %d, %s",
 			rspamd_upstream_name (session->server),
 				rspamd_inet_address_to_string_pretty (
-						rspamd_upstream_addr (session->server)),
+						rspamd_upstream_addr_cur (session->server)),
 			session->state == 1 ? "read" : "write",
 			errno,
 			strerror (errno));
@@ -2255,7 +2255,7 @@ fuzzy_check_timer_callback (gint fd, short what, void *arg)
 		msg_err_task ("got IO timeout with server %s(%s), after %d retransmits",
 				rspamd_upstream_name (session->server),
 				rspamd_inet_address_to_string_pretty (
-						rspamd_upstream_addr (session->server)),
+						rspamd_upstream_addr_cur (session->server)),
 				session->retransmits);
 		rspamd_upstream_fail (session->server, FALSE);
 		if (session->item) {
@@ -2464,7 +2464,7 @@ fuzzy_controller_io_callback (gint fd, short what, void *arg)
 		msg_err_task ("got error in IO with server %s(%s), %d, %s",
 				rspamd_upstream_name (session->server),
 				rspamd_inet_address_to_string_pretty (
-						rspamd_upstream_addr (session->server)),
+						rspamd_upstream_addr_cur (session->server)),
 				errno, strerror (errno));
 		rspamd_upstream_fail (session->server, FALSE);
 	}
@@ -2568,7 +2568,7 @@ fuzzy_controller_timer_callback (gint fd, short what, void *arg)
 				"after %d retransmits",
 				rspamd_upstream_name (session->server),
 				rspamd_inet_address_to_string_pretty (
-						rspamd_upstream_addr (session->server)),
+						rspamd_upstream_addr_cur (session->server)),
 				session->retransmits);
 
 		if (session->session) {
@@ -2725,7 +2725,7 @@ register_fuzzy_client_call (struct rspamd_task *task,
 		selected = rspamd_upstream_get (rule->servers, RSPAMD_UPSTREAM_ROUND_ROBIN,
 				NULL, 0);
 		if (selected) {
-			addr = rspamd_upstream_addr (selected);
+			addr = rspamd_upstream_addr_next (selected);
 			if ((sock = rspamd_inet_address_connect (addr, SOCK_DGRAM, TRUE)) == -1) {
 				msg_warn_task ("cannot connect to %s(%s), %d, %s",
 						rspamd_upstream_name (selected),
@@ -2853,7 +2853,7 @@ register_fuzzy_controller_call (struct rspamd_http_connection_entry *entry,
 	while ((selected = rspamd_upstream_get (rule->servers,
 			RSPAMD_UPSTREAM_SEQUENTIAL, NULL, 0))) {
 		/* Create UDP socket */
-		addr = rspamd_upstream_addr (selected);
+		addr = rspamd_upstream_addr_next (selected);
 
 		if ((sock = rspamd_inet_address_connect (addr,
 				SOCK_DGRAM, TRUE)) == -1) {
@@ -3216,7 +3216,7 @@ fuzzy_check_send_lua_learn (struct fuzzy_rule *rule,
 		while ((selected = rspamd_upstream_get (rule->servers,
 				RSPAMD_UPSTREAM_SEQUENTIAL, NULL, 0))) {
 			/* Create UDP socket */
-			addr = rspamd_upstream_addr (selected);
+			addr = rspamd_upstream_addr_next (selected);
 
 			if ((sock = rspamd_inet_address_connect (addr,
 					SOCK_DGRAM, TRUE)) == -1) {
diff --git a/src/plugins/surbl.c b/src/plugins/surbl.c
index 94d88334e..5949f5bb6 100644
--- a/src/plugins/surbl.c
+++ b/src/plugins/surbl.c
@@ -1624,7 +1624,8 @@ surbl_redirector_error (struct rspamd_http_connection *conn,
 
 	task = param->task;
 	msg_err_surbl ("connection with http server %s terminated incorrectly: %e",
-		rspamd_inet_address_to_string (rspamd_upstream_addr (param->redirector)),
+		rspamd_inet_address_to_string (
+				rspamd_upstream_addr_cur (param->redirector)),
 		err);
 	rspamd_upstream_fail (param->redirector, FALSE);
 	rspamd_session_remove_event (param->task->s, free_redirector_session,
@@ -1715,7 +1716,7 @@ register_redirector_call (struct rspamd_url *url, struct rspamd_task *task,
 				RSPAMD_UPSTREAM_ROUND_ROBIN, url->host, url->hostlen);
 
 		if (selected) {
-			s = rspamd_inet_address_connect (rspamd_upstream_addr (selected),
+			s = rspamd_inet_address_connect (rspamd_upstream_addr_next (selected),
 					SOCK_STREAM, TRUE);
 		}
 
diff --git a/src/rspamd_proxy.c b/src/rspamd_proxy.c
index bb0028f0b..31aeceb12 100644
--- a/src/rspamd_proxy.c
+++ b/src/rspamd_proxy.c
@@ -1309,7 +1309,8 @@ proxy_backend_mirror_error_handler (struct rspamd_http_connection *conn, GError
 	msg_info_session ("abnormally closing connection from backend: %s:%s, "
 			"error: %e",
 			bk_conn->name,
-			rspamd_inet_address_to_string (rspamd_upstream_addr (bk_conn->up)),
+			rspamd_inet_address_to_string (
+					rspamd_upstream_addr_cur (bk_conn->up)),
 			err);
 
 	if (err) {
@@ -1337,7 +1338,8 @@ proxy_backend_mirror_finish_handler (struct rspamd_http_connection *conn,
 			bk_conn->parser_from_ref, msg->body_buf.begin, msg->body_buf.len)) {
 		msg_warn_session ("cannot parse results from the mirror backend %s:%s",
 				bk_conn->name,
-				rspamd_inet_address_to_string (rspamd_upstream_addr (bk_conn->up)));
+				rspamd_inet_address_to_string (
+						rspamd_upstream_addr_cur (bk_conn->up)));
 		bk_conn->err = "cannot parse ucl";
 	}
 
@@ -1387,7 +1389,7 @@ proxy_open_mirror_connections (struct rspamd_proxy_session *session)
 		}
 
 		bk_conn->backend_sock = rspamd_inet_address_connect (
-				rspamd_upstream_addr (bk_conn->up),
+				rspamd_upstream_addr_next (bk_conn->up),
 				SOCK_STREAM, TRUE);
 
 		if (bk_conn->backend_sock == -1) {
@@ -1432,7 +1434,7 @@ proxy_open_mirror_connections (struct rspamd_proxy_session *session)
 
 		if (m->local ||
 				rspamd_inet_address_is_local (
-						rspamd_upstream_addr (bk_conn->up), FALSE)) {
+						rspamd_upstream_addr_cur (bk_conn->up), FALSE)) {
 
 			if (session->fname) {
 				rspamd_http_message_add_header (msg, "File", session->fname);
@@ -1509,7 +1511,8 @@ proxy_backend_master_error_handler (struct rspamd_http_connection *conn, GError
 	session = bk_conn->s;
 	msg_info_session ("abnormally closing connection from backend: %s, error: %e,"
 			" retries left: %d",
-		rspamd_inet_address_to_string (rspamd_upstream_addr (session->master_conn->up)),
+		rspamd_inet_address_to_string (
+				rspamd_upstream_addr_cur (session->master_conn->up)),
 		err,
 		session->ctx->max_retries - session->retries);
 	session->retries ++;
@@ -1531,7 +1534,7 @@ proxy_backend_master_error_handler (struct rspamd_http_connection *conn, GError
 			msg_info_session ("retry connection to: %s"
 					" retries left: %d",
 					rspamd_inet_address_to_string (
-							rspamd_upstream_addr (session->master_conn->up)),
+							rspamd_upstream_addr_cur (session->master_conn->up)),
 					session->ctx->max_retries - session->retries);
 		}
 	}
@@ -1821,14 +1824,15 @@ retry:
 		}
 
 		session->master_conn->backend_sock = rspamd_inet_address_connect (
-				rspamd_upstream_addr (session->master_conn->up),
+				rspamd_upstream_addr_next (session->master_conn->up),
 				SOCK_STREAM, TRUE);
 
 		if (session->master_conn->backend_sock == -1) {
 			msg_err_session ("cannot connect upstream: %s(%s)",
 					host ? hostbuf : "default",
-							rspamd_inet_address_to_string (rspamd_upstream_addr (
-									session->master_conn->up)));
+							rspamd_inet_address_to_string (
+									rspamd_upstream_addr_cur (
+											session->master_conn->up)));
 			rspamd_upstream_fail (session->master_conn->up, TRUE);
 			session->retries ++;
 			goto retry;
@@ -1872,7 +1876,8 @@ retry:
 
 		if (backend->local ||
 				rspamd_inet_address_is_local (
-						rspamd_upstream_addr (session->master_conn->up), FALSE)) {
+						rspamd_upstream_addr_cur (
+								session->master_conn->up), FALSE)) {
 
 			if (session->fname) {
 				rspamd_http_message_add_header (msg, "File", session->fname);
diff --git a/test/rspamd_upstream_test.c b/test/rspamd_upstream_test.c
index 4e4f1ae87..668651c8f 100644
--- a/test/rspamd_upstream_test.c
+++ b/test/rspamd_upstream_test.c
@@ -87,27 +87,27 @@ rspamd_upstream_test_func (void)
 	rspamd_parse_inet_address (&paddr, "::1", 0);
 	g_assert (rspamd_upstream_add_addr (up, paddr));
 	/* Rewind to start */
-	addr = rspamd_upstream_addr (up);
-	addr = rspamd_upstream_addr (up);
+	addr = rspamd_upstream_addr_next (up);
+	addr = rspamd_upstream_addr_next (up);
 	/* cur should be zero here */
-	addr = rspamd_upstream_addr (up);
-	next_addr = rspamd_upstream_addr (up);
+	addr = rspamd_upstream_addr_next (up);
+	next_addr = rspamd_upstream_addr_next (up);
 	g_assert (rspamd_inet_address_get_af (addr) == AF_INET);
 	g_assert (rspamd_inet_address_get_af (next_addr) == AF_INET);
-	next_addr = rspamd_upstream_addr (up);
+	next_addr = rspamd_upstream_addr_next (up);
 	g_assert (rspamd_inet_address_get_af (next_addr) == AF_INET6);
-	next_addr = rspamd_upstream_addr (up);
+	next_addr = rspamd_upstream_addr_next (up);
 	g_assert (rspamd_inet_address_get_af (next_addr) == AF_INET);
-	next_addr = rspamd_upstream_addr (up);
+	next_addr = rspamd_upstream_addr_next (up);
 	g_assert (rspamd_inet_address_get_af (next_addr) == AF_INET);
-	next_addr = rspamd_upstream_addr (up);
+	next_addr = rspamd_upstream_addr_next (up);
 	g_assert (rspamd_inet_address_get_af (next_addr) == AF_INET6);
 	/* Test errors with IPv6 */
 	rspamd_upstream_fail (up, TRUE);
 	/* Now we should have merely IPv4 addresses in rotation */
-	addr = rspamd_upstream_addr (up);
+	addr = rspamd_upstream_addr_next (up);
 	for (i = 0; i < 256; i++) {
-		next_addr = rspamd_upstream_addr (up);
+		next_addr = rspamd_upstream_addr_next (up);
 		g_assert (rspamd_inet_address_get_af (addr) == AF_INET);
 		g_assert (rspamd_inet_address_get_af (next_addr) == AF_INET);
 		g_assert (rspamd_inet_address_compare (addr, next_addr) != 0);


More information about the Commits mailing list