Hello!

On Tue, Feb 06, 2024 at 01:36:20PM +0100, Jan Prachař wrote:

> Hello,
> 
> I have one last note bellow.
> 
> On Tue, 2024-02-06 at 14:08 +0300, Maxim Dounin wrote:
> > Hello!
> > 
> > On Tue, Feb 06, 2024 at 11:42:40AM +0100, Jan Prachař wrote:
> > 
> > > Hello Maxim,
> > > 
> > > On Tue, 2024-02-06 at 00:46 +0300, Maxim Dounin wrote:
> > > > Hello!
> > > > 
> > > > On Mon, Feb 05, 2024 at 06:01:54PM +0100, Jan Prachař wrote:
> > > > 
> > > > > Hello,
> > > > > 
> > > > > thank you for your responses.
> > > > > 
> > > > > On Sat, 2024-02-03 at 04:25 +0300, Maxim Dounin wrote:
> > > > > > 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.prac...@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.
> > > > > 
> > > > > The provided patch works as expected, with no observed issues.
> > > > > 
> > > > > Considering that proxy_ignore_headers for caching headers is 
> > > > > preserved with the
> > > > > cached file, it seems reasonable to extend the same behavior to
> > > > > X-Accel-Redirect.
> > > > 
> > > > Yes, such handling is (mostly) in line with some 
> > > > proxy_ignore_headers handling, that is, X-Accel-Expires, Expires, 
> > > > Cache-Control, Set-Cookie, Vary, and X-Accel-Buffering, as these 
> > > > affect creation of a cache file, but not sending an already cached 
> > > > response to clients.
> > > > 
> > > > Still, X-Accel-Limit-Rate from a cache file will be applied to the 
> > > > response if not ignored by the current configuration.  Similarly, 
> > > > X-Accel-Charset is also applied as long as no longer ignored.
> > > > 
> > > > As such, I mostly consider this to be a neutral argument.
> > > > 
> > > > Further, we might reconsider X-Accel-Redirect handling if caching 
> > > > of X-Accel-Redirect responses will be introduced (see 
> > > > https://trac.nginx.org/nginx/ticket/407 for a feature request).
> > > > 
> > > > > From my perspective, the updated code in 
> > > > > ngx_http_upstream_process_headers() is
> > > > > a bit confusing. The function can return NGX_DONE, but this return 
> > > > > code is only
> > > > > handled in one place where ngx_http_upstream_process_headers() is 
> > > > > called.
> > > > 
> > > > I've removed NGX_DONE handling from the other call since NGX_DONE 
> > > > return code isn't possible there due to r->cached being set just 
> > > > before the call.
> > > > 
> > > > We can instead assume it can be returned and handle appropriately: 
> > > > this will also make handling X-Accel-Redirect from cached files 
> > > > easier if we'll decide to (instead of checking r->cached, we'll 
> > > > have to call ngx_http_upstream_finalize_request(NGX_DECLINED) 
> > > > conditionally, only if u->cleanup is set).
> > > > 
> > > > > If I may suggest, splitting the function might be helpful – redirect 
> > > > > processing
> > > > > would only occur for direct upstream responses, while the rest of the 
> > > > > header
> > > > > processing would be called always (i.e., also for cached responses).
> > > > 
> > > > I can't say I like this idea.  Processing of X-Accel-Redirect is a 
> > > > part of headers processing, and quite naturally handled in 
> > > > ngx_http_upstream_process_headers().  Moving it to a separate function 
> > > > will needlessly complicate things.
> > > > 
> > > > > Additionally, I believe the special handling of NGX_DECLINED in
> > > > > ngx_http_upstream_finalize_request() can be removed. The updated 
> > > > > patch is
> > > > > provided below.
> > > > 
> > > > Not really.  The ngx_http_upstream_finalize_request(NGX_DECLINED) 
> > > > call ensures that the upstream handling is properly finalized, 
> > > > notably the upstream connection is closed.  For short responses 
> > > > after X-Accel-Redirect, this might not be important, because the 
> > > > upstream connection will be closed anyway during request 
> > > > finalization.  But if the redirected request processing takes a 
> > > > while, the upstream connection will still be open, and might 
> > > > receive further events - leading to unexpected behaviour (not to 
> > > > mention that various upstream timing variables, such as 
> > > > $upstream_response_time, will be wrong).
> > > 
> > > In my previous patch I replaced
> > > 
> > > ngx_http_upstream_finalize_request(NGX_DECLINED);
> > > 
> > > by
> > > 
> > > r->count++;
> > > ngx_http_upstream_finalize_request(NGX_DONE);
> > > 
> > > The upstream connection is still finalized and closed, allowing
> > > for the removal of the special handling of NGX_DECLINED from
> > > ngx_http_upstream_finalize_request().
> > 
> > Ah, sorry, missed this.
> > 
> > Yes, r->count++ followed by a real request finalization is a 
> > possible alternative to special handling of NGX_DECLINED without 
> > calling ngx_http_finalize_request().  Still, without special 
> > handling in ngx_http_upstream_finalize_request() this won't be 
> > entirely correct: as can be seen from the code, c->log->action 
> > will be incorrectly set to "sending to client".
> > 
> > > > 
> > > > Below is a patch which preserves proper NGX_DONE processing, and 
> > > > handles X-Accel-Redirect from cached files by checking r->cleanup 
> > > > when calling ngx_http_upstream_finalize_request(NGX_DECLINED).  I 
> > > > tend to think this might be the best solution after all, providing 
> > > > better compatibility for further improvements.
> > > > 
> > > > # HG changeset patch
> > > > # User Maxim Dounin <mdou...@mdounin.ru>
> > > > # Date 1707167064 -10800
> > > > #      Tue Feb 06 00:04:24 2024 +0300
> > > > # Node ID 6e7f0d6d857473517048b8838923253d5230ace0
> > > > # Parent  631ee3c6d38cfdf97dec67c3d2c457af5d91db01
> > > > 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), 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).
> 
> It might be worth mentioning that it has been broken
> since commit 5994:5abf5af257a7.

I don't think it really matters, given that 5994:5abf5af257a7 
happened almost 10 years ago, in nginx 1.7.11.  (And even before 
5994:5abf5af257a7, the behaviour wasn't semantically correct, 
though was safe in practice.)

OTOH, I have no objections to mention this, adjusted the first 
paragraph of the commit log as follows:

...
resulted in ngx_http_finalize_request(NGX_DONE) (after 5994:5abf5af257a7,
nginx 1.7.11), leading to ...

Just in case, full patch below.

# HG changeset patch
# User Maxim Dounin <mdou...@mdounin.ru>
# Date 1707231687 -10800
#      Tue Feb 06 18:01:27 2024 +0300
# Node ID 0eb9c806b2827cd5cc409db31e87dd4a9f1d15b0
# Parent  631ee3c6d38cfdf97dec67c3d2c457af5d91db01
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).

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;
         }
     }
 


-- 
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel

Reply via email to