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ř _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
