From: Vsevolod Stakhov Date: Sat, 25 Apr 2026 19:22:22 +0000 (+0100) Subject: [Fix] upstream consumers: make NULL/nil branches sound X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=254ac8ee0eff8fe79803564fe95e1f191ed2ad8a;p=thirdparty%2Frspamd.git [Fix] upstream consumers: make NULL/nil branches sound 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. --- diff --git a/lualib/lua_redis.lua b/lualib/lua_redis.lua index e6465fa17d..a9e3ec2ef7 100644 --- a/lualib/lua_redis.lua +++ b/lualib/lua_redis.lua @@ -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 diff --git a/src/libserver/fuzzy_backend/fuzzy_backend_redis.c b/src/libserver/fuzzy_backend/fuzzy_backend_redis.c index c182896be6..4f7a8540ea 100644 --- a/src/libserver/fuzzy_backend/fuzzy_backend_redis.c +++ b/src/libserver/fuzzy_backend/fuzzy_backend_redis.c @@ -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); diff --git a/src/libserver/http/http_connection.c b/src/libserver/http/http_connection.c index 117d2e4334..3cdcb88448 100644 --- a/src/libserver/http/http_connection.c +++ b/src/libserver/http/http_connection.c @@ -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 */