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(). > > 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). > > 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; Just a note. If you move ngx_http_upstream_finalize_request() bellow the for loop that copies upstream headers, then this change is also possible: @@ -2855,13 +2851,15 @@ ngx_http_upstream_process_headers(ngx_http_request_t *r, ngx_http_upstream_t *u) if (hh && hh->redirect) { if (hh->copy_handler(r, &h[i], hh->conf) != NGX_OK) { - ngx_http_finalize_request(r, - NGX_HTTP_INTERNAL_SERVER_ERROR); - return NGX_DONE; + return NGX_ERROR; } } > @@ -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; > } > } > > _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel