Segfault when interpreting cached X-Accel-Redirect response

[Roman Arutyunyan]

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.

Thanks for reporting this.

You're right, ngx_http_upstream_finalize_request(NGX_DECLINED) should not
decrement the request count, but in your case it does.

> Best, Jan Prachař
> 
> # User Jan Prachař <jan.prachar at 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

Nitpicking: the message after prefix should start with lower case and end
with a period.

In case of a keepalive connection the problem is use-after-free.
For non-keepalive the message you mentioned above may be the only manifestation
of the problem, however the message itself may be missing depending on random
factors and the behavior of the system allocator.

I suggest the following commit log:

Upstream: fixed r->count underflow while using X-Accel-Redirect.

A response with X-Accel-Redirect could be cached by using proxy_ignore_headers
directive.  If this directive is then removed, this response could be served
from the cache and interpreted as a redirection.  While doing that,
ngx_http_upstream_finalize_request(r, u, NGX_DECLINED) is called with
u->cleanup == NULL, which leads to an extra ngx_http_finalize_request() call
which decreases r->main->count.  Note that the code path for existing
u->cleanup does not call ngx_http_finalize_request() for NGX_DECLINED.

For non-keepalive connections the problem manifested itself with the
"http request count is zero" alert.  For keepalive connections it resulted in
use-after-free memory access.

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

The patch seems ok, but needs to be tested.

--
Roman Arutyunyan