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