[PATCH] Chunked filter: check if ctx is null

[Maxim Dounin]

Hello!

On Thu, Jan 04, 2018 at 11:08:11PM +0100, Jan Prachar wrote:

> Hello, thank you for response!
> 
> On Thu, 2018-01-04 at 03:42 +0300, Maxim Dounin wrote:
> > Hello!
> >
> > On Wed, Jan 03, 2018 at 07:53:00PM +0100, Jan Pracha=C5=99 wrote:
> >
> > To catch cases when a duplicate response is returned after the
> > header was already sent we have a dedicated check in the
> > ngx_http_send_header() function, see this commit for details:
> >
> > http://hg.nginx.org/nginx/rev/03ff14058272
> >
> > Trying to bypass this check is a bad idea.  The same applies to
> > conditionally sending headers based on the r->headers_sent flag,
> > as it will mean that the check will be bypassed.  This is what the
> > lua module seems to be doing, and it should be fixed to avoid
> > doing this.
> 
> Lua module checks r->header_sent in function
> ngx_http_lua_send_header_if_needed(), which is called with every
> output. See
> https://github.com/openresty/lua-nginx-module/commit/235875b5c6afd49611
> 81fa9ead9c167dc865e737
> 
> So you suggest, that they should have their own flag (like they already
> had - ctx->headers_sent) and always call ngx_http_send_header()
> function, if this flag is not set?

Yes.

> > The other part of the equation is how and why error_page is called
> > after the header as already sent.  If you know a scenario where
> > error_page can be called with the header already sent, you may
> > want focus on reproducing and fixing this.  Normally this is
> > expected to result in the "header already sent" alerts produced by
> > the check discussed above.
> 
> On the nginx side it is cause by this:
> 
> http://hg.nginx.org/nginx/rev/ad3f342f14ba046c
> 
> If writing to client returns an error and thus u->pipe-
> >downstream_error is 1 and then reading from upstream fails and thus u-
> >pipe->upstream_error is 1. ngx_http_upstream_finalize_request() is
> then called with rc=NGX_HTTP_BAD_GATEWAY, where thanks to the above
> commit the ngx_http_finalize_request() function is called also with
> rc=NGX_HTTP_BAD_GATEWAY and thus error_page is called (if it is
> configured for 502 status).
> 
> I think, that the ngx_http_finalize_request() function should be called
> with rc=NGX_ERROR in this case.

I agree, the code in question looks incorrect.  As long as header 
is already sent, it shouldn't call ngx_http_finalize_request() 
with NGX_HTTP_BAD_GATEWAY or any other special response code 
(except may be NGX_HTTP_REQUEST_TIME_OUT and 
NGX_HTTP_CLIENT_CLOSED_REQUEST, which are explicitly handled in 
ngx_http_finalize_request()).

The exact scenario described won't work though.  If writing to the 
client returns an error, c->error will be set, and 
ngx_http_finalize_request() won't call error_page processing.

I was able to trigger the alert using a HEAD request, which 
results in u->pipe->downstream_error being set in 
ngx_http_upstream_send_response() without an actual connection 
error (http://hg.nginx.org/nginx-tests/rev/b17f27fa9081).  The 
same situation might also happen due to various other errors - for 
example, if writing to the client times out.

Please try the following patch:

# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1515130723 -10800
#      Fri Jan 05 08:38:43 2018 +0300
# Node ID 8f0bf141818d82ba9754559c4cb2472554e64e09
# Parent  6d2e92acb013224e6ef2c71c9e61ab07f0b03271
Upstream: fixed "header already sent" alerts on backend errors.

Following ad3f342f14ba046c (1.9.13), it is possible that a request where
header was already sent will be finalized with NGX_HTTP_BAD_GATEWAY or
NGX_HTTP_GATEWAY_TIMEOUT, triggering an attempt to return additional error
response and the "header already sent" alert as a result.

In particular, it is trivial to reproduce the problem with a HEAD request
and caching enabled.  With caching enabled nginx will change HEAD to GET
and will set u->pipe->downstream_error to suppress sending the response
body to the client.  When a backend-related error occurs (for example,
proxy_read_timeout expires), ngx_http_finalize_upstream_request() will
be called with NGX_HTTP_GATEWAY_TIMEOUT.  After ad3f342f14ba046c this will
result in ngx_http_finalize_request(NGX_HTTP_GATEWAY_TIMEOUT).

Fix is to move u->pipe->downstream_error handling to a later point,
where all special response codes are changed to NGX_ERROR.

Reported by Jan Prachar,
http://mailman.nginx.org/pipermail/nginx-devel/2018-January/010737.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
@@ -4374,8 +4374,7 @@ ngx_http_upstream_finalize_request(ngx_h
 
     if (!u->header_sent
         || rc == NGX_HTTP_REQUEST_TIME_OUT
-        || rc == NGX_HTTP_CLIENT_CLOSED_REQUEST
-        || (u->pipe && u->pipe->downstream_error))
+        || rc == NGX_HTTP_CLIENT_CLOSED_REQUEST)
     {
         ngx_http_finalize_request(r, rc);
         return;
@@ -4388,7 +4387,9 @@ ngx_http_upstream_finalize_request(ngx_h
         flush = 1;
     }
 
-    if (r->header_only) {
+    if (r->header_only
+        || (u->pipe && u->pipe->downstream_error))
+    {
         ngx_http_finalize_request(r, rc);
         return;
     }

-- 
Maxim Dounin
http://mdounin.ru/