]> git.ipfire.org Git - thirdparty/rspamd.git/commitdiff
[Fix] upstream consumers: make NULL/nil branches sound 6008/head
authorVsevolod Stakhov <vsevolod@rspamd.com>
Sat, 25 Apr 2026 19:22:22 +0000 (20:22 +0100)
committerVsevolod Stakhov <vsevolod@rspamd.com>
Sat, 25 Apr 2026 19:22:22 +0000 (20:22 +0100)
A NULL guard is only useful if the branch behind it logs the failure,
propagates it correctly to the caller, and leaves internal state
consistent. Re-audited every NULL/nil-upstream branch (pre-existing
and newly added by this branch) and tightened the silent or
state-corrupting ones:

* fuzzy_backend_redis: the three rspamd_upstream_get NULL branches in
  read / count / version paths invoked the caller's callback with an
  empty result and returned silently. Admins had no signal that fuzzy
  was being skipped because every backend was dead or pending DNS.
  Each branch now also msg_err_redis_session's the reason.

* libserver/http_connection.c: when ctx->http_proxies is configured
  but every proxy upstream is unavailable, the code silently fell
  back to a direct connection - a security/privacy footgun for
  configs that meant to force traffic through a proxy. Added an
  msg_info to surface the fallback so the admin notices.

* lua_redis prepare_redis_call: the previous patch in this branch
  marked skipped servers as "tempfail" but did not insert a
  placeholder into `options`, so the load_script_task /
  load_script_taskless consumer loop's iteration index no longer
  matched the original servers_ready index. A successful upload to
  one server would then write "done" into the wrong slot of
  servers_ready (the slot for a different, possibly skipped server),
  corrupting the script-load state machine. Insert a `{ skip = true,
  upstream = s }` placeholder so the indexes stay aligned, and skip
  the placeholder in both consumer loops.

lualib/lua_redis.lua
src/libserver/fuzzy_backend/fuzzy_backend_redis.c
src/libserver/http/http_connection.c

index e6465fa17d852acbcc48912042d6d54d1ec53380..a9e3ec2ef7f1d4fbb5c5f0cd9fbded67de8a1ec4 100644 (file)
@@ -1423,38 +1423,43 @@ local function prepare_redis_call(script)
   for idx, s in ipairs(servers) do
     local server_addr = s:get_addr()
     if not server_addr then
-      -- Pending DNS resolution; mark as tempfail so the next attempt retries
+      -- Pending DNS resolution; mark as tempfail so the next attempt retries.
+      -- Insert a placeholder opt (server_idx-aligned) so options/servers_ready
+      -- indexing stays consistent for the consumer loop in load_script_task /
+      -- load_script_taskless. The placeholder is detected via .skip = true.
       script.servers_ready[idx] = "tempfail"
       logger.infox(rspamd_config,
           'skipping SCRIPT LOAD for upstream %s: address not resolved yet',
           s:get_name())
+      table.insert(options, { skip = true, upstream = s, server_idx = idx })
     else
-    local cur_opts = {
-      host = server_addr,
-      timeout = script.redis_params['timeout'],
-      cmd = 'SCRIPT',
-      args = { 'LOAD', script.script },
-      upstream = s
-    }
-
-    -- By default we start from unsent status
-    if not script.servers_ready[idx] then
-      script.servers_ready[idx] = "unsent"
-    end
+      local cur_opts = {
+        host = server_addr,
+        timeout = script.redis_params['timeout'],
+        cmd = 'SCRIPT',
+        args = { 'LOAD', script.script },
+        upstream = s,
+        server_idx = idx,
+      }
+
+      -- By default we start from unsent status
+      if not script.servers_ready[idx] then
+        script.servers_ready[idx] = "unsent"
+      end
 
-    if script.redis_params['username'] then
-      cur_opts['username'] = script.redis_params['username']
-    end
+      if script.redis_params['username'] then
+        cur_opts['username'] = script.redis_params['username']
+      end
 
-    if script.redis_params['password'] then
-      cur_opts['password'] = script.redis_params['password']
-    end
+      if script.redis_params['password'] then
+        cur_opts['password'] = script.redis_params['password']
+      end
 
-    if script.redis_params['db'] then
-      cur_opts['dbname'] = script.redis_params['db']
-    end
+      if script.redis_params['db'] then
+        cur_opts['dbname'] = script.redis_params['db']
+      end
 
-    table.insert(options, cur_opts)
+      table.insert(options, cur_opts)
     end
   end
 
@@ -1490,7 +1495,9 @@ local function load_script_task(script, task, is_write)
   local opts = prepare_redis_call(script)
 
   for idx, opt in ipairs(opts) do
-    if script.servers_ready[idx] ~= 'done' then
+    -- opt.skip means the upstream was not resolved when prepare_redis_call ran;
+    -- servers_ready is already tempfailed and will be retried on the next pass.
+    if not opt.skip and script.servers_ready[idx] ~= 'done' then
       opt.task = task
       opt.is_write = is_write
       opt.callback = function(err, data)
@@ -1553,7 +1560,7 @@ local function load_script_taskless(script, cfg, ev_base, is_write)
   local opts = prepare_redis_call(script)
 
   for idx, opt in ipairs(opts) do
-    if script.servers_ready[idx] ~= 'done' then
+    if not opt.skip and script.servers_ready[idx] ~= 'done' then
       opt.config = cfg
       opt.ev_base = ev_base
       opt.is_write = is_write
index c182896be6dad643b9db3827bea5a6e27477d5a5..4f7a8540eab5b5f5d0e2242b2bddc6cb573c92e6 100644 (file)
@@ -753,6 +753,8 @@ void rspamd_fuzzy_backend_check_redis(struct rspamd_fuzzy_backend *bk,
                                                         0);
 
        if (up == NULL) {
+               msg_err_redis_session("cannot select fuzzy redis upstream: "
+                                                         "all backends are dead or pending DNS resolution");
                rspamd_fuzzy_redis_session_dtor(session, TRUE);
                if (cb) {
                        memset(&mf_result, 0, sizeof(mf_result));
@@ -901,6 +903,8 @@ void rspamd_fuzzy_backend_count_redis(struct rspamd_fuzzy_backend *bk,
                                                         0);
 
        if (up == NULL) {
+               msg_err_redis_session("cannot select fuzzy redis upstream for count: "
+                                                         "all backends are dead or pending DNS resolution");
                rspamd_fuzzy_redis_session_dtor(session, TRUE);
                if (cb) {
                        cb(0, ud);
@@ -1047,6 +1051,8 @@ void rspamd_fuzzy_backend_version_redis(struct rspamd_fuzzy_backend *bk,
                                                         0);
 
        if (up == NULL) {
+               msg_err_redis_session("cannot select fuzzy redis upstream for version: "
+                                                         "all backends are dead or pending DNS resolution");
                rspamd_fuzzy_redis_session_dtor(session, TRUE);
                if (cb) {
                        cb(0, ud);
index 117d2e4334a05be90a81cd5b716e55d3b8605163..3cdcb884480093189e18faf246f54f32202bfd2e 100644 (file)
@@ -1335,6 +1335,16 @@ rspamd_http_connection_new_client(struct rspamd_http_context *ctx,
                                                                                                         RSPAMD_HTTP_CONN_OWN_SOCKET | RSPAMD_HTTP_CONN_FLAG_PROXY,
                                                                                                         up);
                }
+               else {
+                       /*
+                        * Proxies are configured but none usable right now (all dead or
+                        * pending DNS resolution). Surface this so the admin knows
+                        * traffic is going direct instead of silently bypassing the
+                        * configured proxy chain.
+                        */
+                       msg_info("no http proxy upstream available "
+                                        "(all dead or pending DNS); falling back to direct connect");
+               }
        }
 
        /* Unproxied version */