Upstream: fixed X-Accel-Redirect handling from cache files.

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).

diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
index 6f328e6a4238929e687a3182458aa134de8c0aa1..0d76800715ea0b6c07fcc08333cdfb473927f1c0 100644 (file)
--- 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;
         }
     }