> On 29 Jan 2024, at 10:43, Maxim Dounin <mdounin@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 <mdounin@mdounin.ru> wrote:
>>>
>>> # HG changeset patch
>>> # User Maxim Dounin <mdounin@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 <mdounin@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.

--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel