[PATCH] Stream: don't flush empty buffers created for read errors.

[Roman Arutyunyan]

On Tue, May 24, 2022 at 04:37:11PM +0000, Aleksei Bavshin via nginx-devel wrote:
> In all honesty, the main motivation for the patch is to address a regression in UDP healthchecks caused by the behavior described below. It's already covered by tests, but the corresponding cases were disabled by default.
> 
> I considered
>     cl->buf->flush = !src->read->error;
> or even
>     cl->buf->flush = !src->read->eof; 
> but neither of those are exactly equivalent to the code below (with the latter having significantly different behavior), and IMO it would look less obvious. 
> 
> # HG changeset patch
> # User Aleksei Bavshin <a.bavshin at f5.com>
> # Date 1653330584 25200
> #      Mon May 23 11:29:44 2022 -0700
> # Branch se
> # Node ID 5a98e9cb437f7719afa2bde62de68e174fd8e03e
> # Parent  8902674cc7fe759cada415c10340f31ae4a90fba
> Stream: don't flush empty buffers created for read errors.
> 
> When we generate the last_buf buffer for an UDP upstream recv error, it does
> not contain any data from the wire. ngx_stream_write_filter attempts to forward
> it anyways, which is incorrect (e.g., UDP upstream ECONNREFUSED will be
> translated to an empty packet).
> 
> Reproduction:
> 
> stream {
>     upstream unreachable {
>         server     127.0.0.1:8880;
>     }
>     server {
>         listen     127.0.0.1:8998 udp;
>         proxy_pass unreachable;
>     }
> }
> 
> 1 0.000000000    127.0.0.1 → 127.0.0.1    UDP 47 45588 → 8998 Len=5
> 2 0.000166300    127.0.0.1 → 127.0.0.1    UDP 47 51149 → 8880 Len=5
> 3 0.000172600    127.0.0.1 → 127.0.0.1    ICMP 75 Destination unreachable (Port
> unreachable)
> 4 0.000202400    127.0.0.1 → 127.0.0.1    UDP 42 8998 → 45588 Len=0
> 
> Fixes d127837c714f.
> 
> diff --git a/src/stream/ngx_stream_proxy_module.c b/src/stream/ngx_stream_proxy_module.c
> --- a/src/stream/ngx_stream_proxy_module.c
> +++ b/src/stream/ngx_stream_proxy_module.c
> @@ -1676,7 +1676,7 @@ ngx_stream_proxy_process(ngx_stream_sess
>      ssize_t                       n;
>      ngx_buf_t                    *b;
>      ngx_int_t                     rc;
> -    ngx_uint_t                    flags, *packets;
> +    ngx_uint_t                    flags, flush, *packets;
>      ngx_msec_t                    delay;
>      ngx_chain_t                  *cl, **ll, **out, **busy;
>      ngx_connection_t             *c, *pc, *src, *dst;
> @@ -1803,9 +1803,12 @@ ngx_stream_proxy_process(ngx_stream_sess
>                  break;
>              }
>  
> +            flush = 1;
> +
>              if (n == NGX_ERROR) {
>                  src->read->eof = 1;
>                  n = 0;
> +                flush = 0;
>              }
>  
>              if (n >= 0) {
> @@ -1846,7 +1849,7 @@ ngx_stream_proxy_process(ngx_stream_sess
>  
>                  cl->buf->temporary = (n ? 1 : 0);
>                  cl->buf->last_buf = src->read->eof;
> -                cl->buf->flush = 1;
> +                cl->buf->flush = flush;
>  
>                  (*packets)++;
>                  *received += n;
> _______________________________________________
> nginx-devel mailing list -- nginx-devel at nginx.org
> To unsubscribe send an email to nginx-devel-leave at nginx.org

Looks good