Hello! On Mon, Jan 29, 2024 at 05:21:43PM +0400, Sergey Kandaurov wrote:
> > > On 29 Jan 2024, at 10:43, Maxim Dounin <mdou...@mdounin.ru> wrote: > > > > Hello! > > > > On Fri, Jan 26, 2024 at 04:26:00PM +0400, Sergey Kandaurov wrote: > > > >>> On 27 Nov 2023, at 06:50, Maxim Dounin <mdou...@mdounin.ru> wrote: > >>> > >>> # HG changeset patch > >>> # User Maxim Dounin <mdou...@mdounin.ru> > >>> # Date 1701049758 -10800 > >>> # Mon Nov 27 04:49:18 2023 +0300 > >>> # Node ID faf0b9defc76b8683af466f8a950c2c241382970 > >>> # Parent a5e39e9d1f4c84dcbe6a2f9e079372a3d63aef0b > >>> Upstream: fixed usage of closed sockets with filter finalization. > >>> > >>> When filter finalization is triggered when working with an upstream > >>> server, > >>> and error_page redirects request processing to some simple handler, > >>> ngx_http_request_finalize() triggers request termination when the response > >>> is sent. In particular, via the upstream cleanup handler, nginx will > >>> close > >>> the upstream connection and the corresponding socket. > >>> > >>> Still, this can happen to be with ngx_event_pipe() on stack. While > >>> the code will set p->downstream_error due to NGX_ERROR returned from the > >>> output filter chain by filter finalization, otherwise the error will be > >>> ignored till control returns to ngx_http_upstream_process_request(). > >>> And event pipe might try reading from the (already closed) socket, > >>> resulting > >>> in "readv() failed (9: Bad file descriptor) while reading upstream" errors > >>> (or even segfaults with SSL). > >>> > >>> Such errors were seen with the following configuration: > >>> > >>> location /t2 { > >>> proxy_pass http://127.0.0.1:8080/big; > >>> > >>> image_filter_buffer 10m; > >>> image_filter resize 150 100; > >>> error_page 415 = /empty; > >>> } > >>> > >>> location /empty { > >>> return 204; > >>> } > >>> > >>> location /big { > >>> # big enough static file > >>> } > >>> > >>> Fix is to set p->upstream_error in ngx_http_upstream_finalize_request(), > >>> so the existing checks in ngx_event_pipe_read_upstream() will prevent > >>> further reading from the closed upstream connection. > >>> > >>> Similarly, p->upstream_error is now checked when handling events at > >>> ngx_event_pipe() exit, as checking p->upstream->fd is not enough if > >>> keepalive upstream connections are being used and the connection was > >>> saved to cache during request termination. > >>> > >> > >> Setting p->upstream_error in ngx_http_upstream_finalize_request() > >> may look suspicious, because it is used to be set on connection errors > >> such as upstream timeout or recv error, or, as a recently introduced > >> exception in the fastcgi module, - also when the FastCGI record ends > >> prematurely, before receiving all the expected content. > >> But technically I think this is quite correct, because we no longer > >> want to receive further data, and also (and you mention this in the > >> commit log) this repeats closing an upstream connection socket in > >> the same place in ngx_http_upstream_finalize_request(). > >> So I think it should be fine. > > > > The biggest concern I personally see here is with the added > > p->upstream_error check at ngx_event_pipe() exit. If there is a > > real upstream error, such as when the connection is reset by the > > upstream server, and if we want the pipe to be active for some > > time (for example, if we want it to continue writing to the > > downstream connection), there will be no ngx_handle_read_event() > > call. For level-triggered event methods this means that the read > > event for the upstream connection will be generated again and > > again. > > > > This shouldn't be the problem for existing ngx_event_pipe() uses > > though, as p->upstream_error is anyway triggers > > ngx_http_upstream_finalize_request(). > > > > Still, we can consider introducing a separate flag, such as > > p->upstream_closed, or clearing p->upstream, and checking these in > > ngx_event_pipe() instead. This probably would be a more clear > > solution. > > > > Updated patch below: > > > > # HG changeset patch > > # User Maxim Dounin <mdou...@mdounin.ru> > > # Date 1706510064 -10800 > > # Mon Jan 29 09:34:24 2024 +0300 > > # Node ID 4a91a03dcd8df0652884ed6ebe9f7437ce82fd26 > > # Parent 7b630f6487068f7cc9dd83762fb4ea39f2f340e9 > > Upstream: fixed usage of closed sockets with filter finalization. > > > > When filter finalization is triggered when working with an upstream server, > > and error_page redirects request processing to some simple handler, > > ngx_http_request_finalize() triggers request termination when the response > > is sent. In particular, via the upstream cleanup handler, nginx will close > > the upstream connection and the corresponding socket. > > > > Still, this can happen to be with ngx_event_pipe() on stack. While > > the code will set p->downstream_error due to NGX_ERROR returned from the > > output filter chain by filter finalization, otherwise the error will be > > ignored till control returns to ngx_http_upstream_process_request(). > > And event pipe might try reading from the (already closed) socket, resulting > > in "readv() failed (9: Bad file descriptor) while reading upstream" errors > > (or even segfaults with SSL). > > > > Such errors were seen with the following configuration: > > > > location /t2 { > > proxy_pass http://127.0.0.1:8080/big; > > > > image_filter_buffer 10m; > > image_filter resize 150 100; > > error_page 415 = /empty; > > } > > > > location /empty { > > return 204; > > } > > > > location /big { > > # big enough static file > > } > > > > Fix is to clear p->upstream in ngx_http_upstream_finalize_request(), > > and ensure that p->upstream is checked in ngx_event_pipe_read_upstream() > > and when handling events at ngx_event_pipe() exit. > > > > diff --git a/src/event/ngx_event_pipe.c b/src/event/ngx_event_pipe.c > > --- a/src/event/ngx_event_pipe.c > > +++ b/src/event/ngx_event_pipe.c > > @@ -57,7 +57,9 @@ ngx_event_pipe(ngx_event_pipe_t *p, ngx_ > > do_write = 1; > > } > > > > - if (p->upstream->fd != (ngx_socket_t) -1) { > > + if (p->upstream > > + && p->upstream->fd != (ngx_socket_t) -1) > > + { > > rev = p->upstream->read; > > > > flags = (rev->eof || rev->error) ? NGX_CLOSE_EVENT : 0; > > @@ -108,7 +110,9 @@ ngx_event_pipe_read_upstream(ngx_event_p > > ngx_msec_t delay; > > ngx_chain_t *chain, *cl, *ln; > > > > - if (p->upstream_eof || p->upstream_error || p->upstream_done) { > > + if (p->upstream_eof || p->upstream_error || p->upstream_done > > + || p->upstream == NULL) > > + { > > return NGX_OK; > > } > > > > 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 > > @@ -4561,6 +4561,10 @@ ngx_http_upstream_finalize_request(ngx_h > > > > u->peer.connection = NULL; > > > > + if (u->pipe) { > > + u->pipe->upstream = NULL; > > + } > > + > > if (u->pipe && u->pipe->temp_file) { > > ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, > > "http upstream temp fd: %d", > > > > Indeed, this fix looks more isolated, I like it. Pushed to http://mdounin.ru/hg/nginx, thanks for the review. -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel