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@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/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org