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

[Roman Arutyunyan]

On Sat, Jun 04, 2022 at 12:13:05AM +0300, Maxim Dounin wrote:
> Hello!
> 
> On Thu, Jun 02, 2022 at 03:20:54AM +0000, Aleksei Bavshin via nginx-devel wrote:
> 
> > > I don't think it's the way to go.  Semantically, flush and
> > > last_buf are mutually exclusive: last_buf implies flushing of all
> > > remaining data, and there should be no flush set if last_buf is
> > > set.
> > > 
> > > The "cl->buf->flush = !src->read->eof" approach might be actually
> > > better and more obvious: it simply ensures that last_buf and flush
> > > are not set at the same time.  I don't immediately see any cases
> > > where it can break things, and this should fix the original
> > > problem with empty UDP packets being sent.
> > 
> > Thanks for confirming that, I wasn't sure it is the right approach exactly because I had doubts regarding the mutual exclusivity of the flags.
> > Updated patch below; tests are passing on both branches and all systems we have in CI.
> > 
> > # HG changeset patch
> > # User Aleksei Bavshin <a.bavshin at f5.com>
> > # Date 1653330584 25200
> > #      Mon May 23 11:29:44 2022 -0700
> > # Node ID 40926829be83986a3a5a5f941d2014000a0acd2e
> > # Parent  e0cfab501dd11fdd23ad492419692269d3a01fc7
> > 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).
> > 
> > This happens because we mark the buffer as both 'flush' and 'last_buf', and
> > ngx_stream_write_filter has special handling for flush with certain types of
> > connections (see d127837c714f, 32b0ba4855a6). The flags are meant to be
> > mutually exclusive, so the fix is to ensure that flush and last_buf are not set
> > at the same time.
> > 
> > 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
> > @@ -1735,7 +1735,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 = !src->read->eof;
> >  
> >                  (*packets)++;
> >                  *received += n;
> 
> Looks good to me.
> 
> Roman, please take another look.

Good to me too.

This version is equivalent to the original patch for UDP.  The only difference
is a side-effect for TCP.  Now flush is not set in the last buffer.

> 
> -- 
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx-devel mailing list -- nginx-devel at nginx.org
> To unsubscribe send an email to nginx-devel-leave at nginx.org