From adf76b2fea33b896105b7487f032d163b1683e97 Mon Sep 17 00:00:00 2001 From: Maxim Dounin Date: Tue, 20 Feb 2024 01:23:43 +0300 Subject: [PATCH] Upstream: fixed X-Accel-Redirect handling from cache files. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The X-Accel-Redirect header might appear in cache files if its handling is ignored with the "proxy_ignore_headers" directive. If the cache file is later served with different settings, ngx_http_upstream_process_headers() used to call ngx_http_upstream_finalize_request(NGX_DECLINED), which is not expected to happen before the cleanup handler is installed and resulted in ngx_http_finalize_request(NGX_DONE) (after 2b032e8df, nginx 1.7.11), leading to unexpected request counter decrement, "request count is zero" alerts, and segmentation faults. Similarly, errors in ngx_http_upstream_process_headers() resulted in ngx_http_upstream_finalize_request(NGX_HTTP_INTERNAL_SERVER_ERROR) being called. This is also not expected to happen before the cleanup handler is installed, and resulted in ngx_http_finalize_request(NGX_DONE) without proper request finalization. Fix is to avoid calling ngx_http_upstream_finalize_request() from ngx_http_upstream_process_headers(), notably when the cleanup handler is not yet installed. Errors are now simply return NGX_ERROR, so the caller is responsible for proper finalization by calling either ngx_http_finalize_request() or ngx_http_upstream_finalize_request(). And X-Accel-Redirect handling now does not call ngx_http_upstream_finalize_request(NGX_DECLINED) if no cleanup handler is installed. Reported by Jiří Setnička (https://mailman.nginx.org/pipermail/nginx-devel/2024-February/HWLYHOO3DDB3XTFT6X3GRMXIEJ3SJRUA.html). --- src/http/ngx_http_upstream.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c index 6f328e6a4..0d7680071 100644 --- a/src/http/ngx_http_upstream.c +++ b/src/http/ngx_http_upstream.c @@ -1136,8 +1136,10 @@ ngx_http_upstream_cache_send(ngx_http_request_t *r, ngx_http_upstream_t *u) if (rc == NGX_OK) { - if (ngx_http_upstream_process_headers(r, u) != NGX_OK) { - return NGX_DONE; + rc = ngx_http_upstream_process_headers(r, u); + + if (rc != NGX_OK) { + return rc; } return ngx_http_cache_send(r); @@ -2642,7 +2644,14 @@ done: u->peer.notify(&u->peer, u->peer.data, NGX_HTTP_UPSTREAM_NOTIFY_HEADER); } - if (ngx_http_upstream_process_headers(r, u) != NGX_OK) { + rc = ngx_http_upstream_process_headers(r, u); + + if (rc == NGX_DONE) { + return; + } + + if (rc == NGX_ERROR) { + ngx_http_upstream_finalize_request(r, u, NGX_ERROR); return; } @@ -3097,7 +3106,9 @@ ngx_http_upstream_process_headers(ngx_http_request_t *r, ngx_http_upstream_t *u) if (u->headers_in.x_accel_redirect && !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_REDIRECT)) { - ngx_http_upstream_finalize_request(r, u, NGX_DECLINED); + if (u->cleanup) { + ngx_http_upstream_finalize_request(r, u, NGX_DECLINED); + } part = &u->headers_in.headers.part; h = part->elts; @@ -3186,18 +3197,14 @@ ngx_http_upstream_process_headers(ngx_http_request_t *r, ngx_http_upstream_t *u) if (hh) { if (hh->copy_handler(r, &h[i], hh->conf) != NGX_OK) { - ngx_http_upstream_finalize_request(r, u, - NGX_HTTP_INTERNAL_SERVER_ERROR); - return NGX_DONE; + return NGX_ERROR; } continue; } if (ngx_http_upstream_copy_header_line(r, &h[i], 0) != NGX_OK) { - ngx_http_upstream_finalize_request(r, u, - NGX_HTTP_INTERNAL_SERVER_ERROR); - return NGX_DONE; + return NGX_ERROR; } }