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

[Aleksei Bavshin]

> -----Original Message-----
> From: Maxim Dounin <mdounin at mdounin.ru>
> Sent: Wednesday, June 1, 2022 7:41 AM
> To: Aleksei Bavshin via nginx-devel <nginx-devel at nginx.org>
> Subject: Re: [PATCH] Stream: don't flush empty buffers created for read errors.
> 
> EXTERNAL MAIL: nginx-devel-bounces at nginx.org
> 
> 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.

I think you misunderstood what the patch does. The buffer is guaranteed to be special; it will have last_buf set just because it signals an unrecoverable connection error.
The problem is that if *both* last_buf and flush are set, the buffer becomes special enough to be sent to the wire even if there's no actual buffered data (no packet to terminate).
An alternative approach would be to ignore the flush bit for a buffer with last_buf set:
@@ -234,8 +234,8 @@ ngx_stream_write_filter(ngx_stream_sessi
 
     if (size == 0
         && !(c->buffered & NGX_LOWLEVEL_BUFFERED)
-        && !(last && c->need_last_buf)
-        && !(flush && c->need_flush_buf))
+        && !(last ? c->need_last_buf
+            : (flush && c->need_flush_buf)))
     {
         if (last || flush || sync) {
             for (cl = *out; cl; /* void */) {
but that doesn't look as safe as this patch to me.

> 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.

See above: as we're closing the connection in case of the error, the buffer automatically gets last_buf and becomes 'special'.
If I understand Nginx flow control correctly, special buffer with last_buf set is expected to appear before closing the connection.