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