Segfault when interpreting cached X-Accel-Redirect response

Maxim Dounin mdounin at mdounin.ru
Sat Feb 3 01:25:07 UTC 2024


Hello!

On Fri, Feb 02, 2024 at 01:47:51PM +0100, Jan Prachař wrote:

> On Fri, 2024-02-02 at 12:48 +0100, Jiří Setnička via nginx-devel wrote:
> > Hello,
> > 
> > Also, I believe that the core of the problem is because of the 
> > ngx_http_finalize_request(r, NGX_DONE); call in the 
> > ngx_http_upstream_process_headers function. This call is needed when 
> > doing an internal redirect after the real upstream request (to close the 
> > upstream request), but when serving from the cache, there is no upstream 
> > request to close and this call causes ngx_http_set_lingering_close to be 
> > called from the ngx_http_finalize_connection with no active request on 
> > the connection yielding to the segfault.
> 
> Hello,
> 
> I am Jiří's colleague, and so I have taken a closer look at the problem. Another
> indication of the issue is the alert in the error log for non-keepalive connections,
> stating "http request count is zero while closing request."
> 
> Upon reviewing the nginx source code, I discovered that the function
> ngx_http_upstream_finalize_request(), when called with rc = NGX_DECLINED, does not invoke
> ngx_http_finalize_request(). However, when there is nothing to clean up (u->cleanup ==
> NULL), it does. Therefore, I believe the appropriate fix is to follow the patch below.
> 
> Best, Jan Prachař
> 
> # User Jan Prachař <jan.prachar at gmail.com>
> # Date 1706877176 -3600
> #      Fri Feb 02 13:32:56 2024 +0100
> # Node ID 851c994b48c48c9cd3d32b9aa402f4821aeb8bb2
> # Parent  cf3d537ec6706f8713a757df256f2cfccb8f9b01
> Upstream: Fix "request count is zero" when procesing X-Accel-Redirect
> 
> ngx_http_upstream_finalize_request(r, u, NGX_DECLINED) should not call
> ngx_http_finalize_request().
> 
> diff -r cf3d537ec670 -r 851c994b48c4 src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c      Thu Nov 26 21:00:25 2020 +0100
> +++ b/src/http/ngx_http_upstream.c      Fri Feb 02 13:32:56 2024 +0100
> @@ -4340,6 +4340,11 @@
>  
>      if (u->cleanup == NULL) {
>          /* the request was already finalized */
> +
> +        if (rc == NGX_DECLINED) {
> +            return;
> +        }
> +
>          ngx_http_finalize_request(r, NGX_DONE);
>          return;
>      }

I somewhat agree: the approach suggested by Jiří certainly looks 
incorrect. The ngx_http_upstream_cache_send() function, which 
calls ngx_http_upstream_process_headers() with r->cached set, can 
be used in two contexts: before the cleanup handler is installed 
(i.e., when sending a cached response during upstream request 
initialization) and after it is installed (i.e., when sending a 
stale cached response on upstream errors).  In the latter case 
skipping finalization would mean a socket leak.

Still, checking for NGX_DECLINED explicitly also looks wrong, for 
a number of reasons.

First, the specific code path isn't just for "nothing to clean 
up", it's for the very specific case when the request was already 
finalized due to filter finalization, see 5994:5abf5af257a7.  This 
code path is not expected to be triggered when the cleanup handler 
isn't installed yet - before the cleanup handler is installed, 
upstream code is expected to call ngx_http_finalize_request() 
directly instead.  And it would be semantically wrong to check for 
NGX_DECLINED: if it's here, it means something already gone wrong.

I think the generic issue here is that 
ngx_http_upstream_process_headers(), which is normally used for 
upstream responses and calls ngx_http_upstream_finalize_request(), 
is also used for cached responses.  Still, it assumes it is used 
for an upstream response, and calls 
ngx_http_upstream_finalize_request().

As can be seen from the rest of the 
ngx_http_upstream_process_headers() code, apart from the issue 
with X-Accel-Redirect, it can also call 
ngx_http_upstream_finalize_request(NGX_HTTP_INTERNAL_SERVER_ERROR) 
when hh->copy_handler() or ngx_http_upstream_copy_header_line() 
fails.  This will similarly end up in 
ngx_http_finalize_request(NGX_DONE) since there is no u->cleanup, 
leading to a request hang.  And it would be certainly wrong to 
check for NGX_HTTP_INTERNAL_SERVER_ERROR similarly to NGX_DECLINED 
in your patch, because it can theoretically happen after filter 
finalization.

Proper solution would probably require re-thinking 
ngx_http_upstream_process_headers() interface.

Some preliminary code below: it disables X-Accel-Redirect 
processing altogether if ngx_http_upstream_process_headers() is 
called when returning a cached response (this essentially means 
that "proxy_ignore_headers X-Accel-Expires" is preserved in the 
cache file, which seems to be the right thing to do as we don't 
save responses with X-Accel-Redirect to cache unless it is 
ignored), and returns NGX_ERROR in other places to trigger 
appropriate error handling instead of calling 
ngx_http_upstream_finalize_request() directly (this no longer 
tries to return 500 Internal Server Error response though, as 
doing so might be unsafe after copying some of the cached headers 
to the response).

Please take a look if it works for you.

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
@@ -588,10 +588,6 @@ ngx_http_upstream_init_request(ngx_http_
         if (rc == NGX_OK) {
             rc = ngx_http_upstream_cache_send(r, u);
 
-            if (rc == NGX_DONE) {
-                return;
-            }
-
             if (rc == NGX_HTTP_UPSTREAM_INVALID_HEADER) {
                 rc = NGX_DECLINED;
                 r->cached = 0;
@@ -1088,7 +1084,7 @@ 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;
+            return NGX_ERROR;
         }
 
         return ngx_http_cache_send(r);
@@ -2516,7 +2512,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;
     }
 
@@ -2576,10 +2579,6 @@ ngx_http_upstream_test_next(ngx_http_req
             u->cache_status = NGX_HTTP_CACHE_STALE;
             rc = ngx_http_upstream_cache_send(r, u);
 
-            if (rc == NGX_DONE) {
-                return NGX_OK;
-            }
-
             if (rc == NGX_HTTP_UPSTREAM_INVALID_HEADER) {
                 rc = NGX_HTTP_INTERNAL_SERVER_ERROR;
             }
@@ -2621,10 +2620,6 @@ ngx_http_upstream_test_next(ngx_http_req
         u->cache_status = NGX_HTTP_CACHE_REVALIDATED;
         rc = ngx_http_upstream_cache_send(r, u);
 
-        if (rc == NGX_DONE) {
-            return NGX_OK;
-        }
-
         if (rc == NGX_HTTP_UPSTREAM_INVALID_HEADER) {
             rc = NGX_HTTP_INTERNAL_SERVER_ERROR;
         }
@@ -2827,7 +2822,8 @@ ngx_http_upstream_process_headers(ngx_ht
     }
 
     if (u->headers_in.x_accel_redirect
-        && !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_REDIRECT))
+        && !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_REDIRECT)
+        && !r->cached)
     {
         ngx_http_upstream_finalize_request(r, u, NGX_DECLINED);
 
@@ -2918,18 +2914,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;
         }
     }
 
@@ -4442,10 +4434,6 @@ ngx_http_upstream_next(ngx_http_request_
             u->cache_status = NGX_HTTP_CACHE_STALE;
             rc = ngx_http_upstream_cache_send(r, u);
 
-            if (rc == NGX_DONE) {
-                return;
-            }
-
             if (rc == NGX_HTTP_UPSTREAM_INVALID_HEADER) {
                 rc = NGX_HTTP_INTERNAL_SERVER_ERROR;
             }

-- 
Maxim Dounin
http://mdounin.ru/


More information about the nginx-devel mailing list