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

[Maxim Dounin]

Hello!

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;

I tend to think this patch is wrong, for at least two reasons:

1. It introduces a zero-sized non-special buffer in the chain.  
This is generally wrong, explicitly rejected in many code paths 
(see "zero size buf in ..." alerts), and might easily result in 
infinite loops in common processing patterns unless explicitly 
handled.

2. Semantically, in UDP proxying a sequence of one or more buffer 
ending with a flush buffer corresponds to an UDP packet.  That is, 
the empty buffer added by the code is a start of the packet - and 
it can be handled as such by intermediate filters.  Consider, for 
example, a filter which prepends each packet with "foo:" - with 
this change, a packet with "foo:" will still be sent in case of an 
error.

I think a better solution would be to avoid adding buffers in case 
of errors at all.

-- 
Maxim Dounin
http://mdounin.ru/