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.
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.
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).
Additionally, I believe the special handling of NGX_DECLINED in
ngx_http_upstream_finalize_request() can be removed. The updated patch is
provided below.
diff --git a/nginx/src/http/ngx_http_upstream.c
b/nginx/src/http/ngx_http_upstream.c
index f5db65338..13c25721d 100644
--- a/nginx/src/http/ngx_http_upstream.c
+++ b/nginx/src/http/ngx_http_upstream.c
@@ -53,6 +53,8 @@ static ngx_int_t
ngx_http_upstream_test_next(ngx_http_request_t *r,
static ngx_int_t ngx_http_upstream_intercept_errors(ngx_http_request_t *r,
ngx_http_upstream_t *u);
static ngx_int_t ngx_http_upstream_test_connect(ngx_connection_t *c);
+static ngx_int_t ngx_http_upstream_process_redirect(ngx_http_request_t *r,
+ ngx_http_upstream_t *u);
static ngx_int_t ngx_http_upstream_process_headers(ngx_http_request_t *r,
ngx_http_upstream_t *u);
static ngx_int_t ngx_http_upstream_process_trailers(ngx_http_request_t *r,
@@ -588,10 +590,6 @@ ngx_http_upstream_init_request(ngx_http_request_t *r)
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 +1086,7 @@ ngx_http_upstream_cache_send(ngx_http_request_t *r,
ngx_http_upstream_t *u)
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 +2514,19 @@ ngx_http_upstream_process_header(ngx_http_request_t *r,
ngx_http_upstream_t *u)
}
}
+ rc = ngx_http_upstream_process_redirect(r, u);
+
+ if (rc == NGX_DONE) {
+ return;
+ }
+
+ if (rc == NGX_ERROR) {
+ ngx_http_upstream_finalize_request(r, u, NGX_ERROR);
+ return;
+ }
+
if (ngx_http_upstream_process_headers(r, u) != NGX_OK) {
+ ngx_http_upstream_finalize_request(r, u, NGX_ERROR);
return;
}
@@ -2576,10 +2586,6 @@ ngx_http_upstream_test_next(ngx_http_request_t *r,
ngx_http_upstream_t *u)
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 +2627,6 @@ ngx_http_upstream_test_next(ngx_http_request_t *r,
ngx_http_upstream_t *u)
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;
}
@@ -2811,7 +2813,7 @@ ngx_http_upstream_test_connect(ngx_connection_t *c)
static ngx_int_t
-ngx_http_upstream_process_headers(ngx_http_request_t *r, ngx_http_upstream_t
*u)
+ngx_http_upstream_process_redirect(ngx_http_request_t *r, ngx_http_upstream_t
*u)
{
ngx_str_t uri, args;
ngx_uint_t i, flags;
@@ -2822,15 +2824,9 @@ ngx_http_upstream_process_headers(ngx_http_request_t *r,
ngx_http_upstream_t *u)
umcf = ngx_http_get_module_main_conf(r, ngx_http_upstream_module);
- if (u->headers_in.no_cache || u->headers_in.expired) {
- u->cacheable = 0;
- }
-
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);
-
part = &u->headers_in.headers.part;
h = part->elts;
@@ -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;
}
}
}
+ r->count++;
+
+ ngx_http_upstream_finalize_request(r, u, NGX_DONE);
+
uri = u->headers_in.x_accel_redirect->value;
if (uri.data[0] == '@') {
@@ -2888,6 +2886,25 @@ ngx_http_upstream_process_headers(ngx_http_request_t *r,
ngx_http_upstream_t *u)
return NGX_DONE;
}
+ return NGX_OK;
+}
+
+
+static ngx_int_t
+ngx_http_upstream_process_headers(ngx_http_request_t *r, ngx_http_upstream_t
*u)
+{
+ ngx_uint_t i;
+ ngx_list_part_t *part;
+ ngx_table_elt_t *h;
+ ngx_http_upstream_header_t *hh;
+ ngx_http_upstream_main_conf_t *umcf;
+
+ umcf = ngx_http_get_module_main_conf(r, ngx_http_upstream_module);
+
+ if (u->headers_in.no_cache || u->headers_in.expired) {
+ u->cacheable = 0;
+ }
+
part = &u->headers_in.headers.part;
h = part->elts;
@@ -2918,18 +2935,14 @@ ngx_http_upstream_process_headers(ngx_http_request_t *r,
ngx_http_upstream_t *u)
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;
}
}
@@ -4429,10 +4442,6 @@ ngx_http_upstream_next(ngx_http_request_t *r,
ngx_http_upstream_t
*u,
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;
}
@@ -4604,10 +4613,6 @@ ngx_http_upstream_finalize_request(ngx_http_request_t *r,
r->read_event_handler = ngx_http_block_reading;
- if (rc == NGX_DECLINED) {
- return;
- }
-
r->connection->log->action = "sending to client";
if (!u->header_sent
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
