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.
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/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
