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

[Maxim Dounin]

details:   http://freenginx.org/hg/nginx/rev/6765e5f6d991
branches:  
changeset: 9217:6765e5f6d991
user:      Maxim Dounin <mdounin at mdounin.ru>
date:      Tue Feb 20 01:23:43 2024 +0300
description:
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 5994:5abf5af257a7,
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).

diffstat:

 src/http/ngx_http_upstream.c |  27 +++++++++++++++++----------
 1 files changed, 17 insertions(+), 10 deletions(-)

diffs (64 lines):

diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -1087,8 +1087,10 @@ ngx_http_upstream_cache_send(ngx_http_re
 
     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);
@@ -2516,7 +2518,14 @@ ngx_http_upstream_process_header(ngx_htt
         }
     }
 
-    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;
     }
 
@@ -2829,7 +2838,9 @@ ngx_http_upstream_process_headers(ngx_ht
     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;
@@ -2918,18 +2929,14 @@ ngx_http_upstream_process_headers(ngx_ht
 
         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;
         }
     }