[PATCH] Stream: don't flush empty buffers created for read errors.
Maxim Dounin
mdounin at mdounin.ru
Thu Jun 2 00:28:06 UTC 2022
Hello!
On Wed, Jun 01, 2022 at 03:04:24PM +0000, Aleksei Bavshin via nginx-devel wrote:
> > 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).
Ah, sorry, missed that the buffer will also had the last_buf set.
This is probably wrong though, as last_buf implies correct end of
data, and we've got an error instead. For example, in HTTP
code last_buf is not set when the upstream response is abnormally
terminated, and, as a result, no final chunk is sent if the
response is using chunked transfer encoding, making it possible
for the client to detect incomplete responses. This is mostly
unrelated to the patch though, and should be addressed separately.
> 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.
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.
> > 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.
That's not exactly the case, see above.
--
Maxim Dounin
http://mdounin.ru/
More information about the nginx-devel
mailing list