[PATCH] Chunked filter: check if ctx is null

[Jan Prachař]

On Fri, 2018-01-05 at 08:41 +0300, Maxim Dounin wrote:
> 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/235875b5c6afd4
> > 9611
> > 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.

Thanks. I will report it to lua module developers.

> > > 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.

You are right, I checked it and in my scenario actually the client
timeout happened.

> Please try the following patch:

The patch works for me (request is terminated without an internal
redirect).

--
Jan Prachař