BUG/MEDIUM: stream: do not abort connection setup too early

Github issue #472 reports a problem with short client connections making
stick-table entries disappear. The problem is in fact totally different
and stems at the connection establishment step.

What happens is that the stick-table there has a single entry. The
"stick-on" directive is forced to purge an existing entry before being
able to create a new one. The new entry will be committed during the
call to process_store_rules() on the response path.

But if the client sends the FIN immediately after the connection is set
up (e.g. using nc -z) then the SHUTR is received and will cancel the
connection setup just after it starts. This cancellation will induce a
call to cs_shutw() which will in turn leave the server-side state in
ST_DIS. This transition from ST_CON to ST_DIS doesn't belong to the
list of handled transition during the connection setup so it will be
handled right after on the regular path, causing the connection to be
closed. Because of this, we never pass through back_establish() and
the backend's analysers are never set on the response channel, which
is why process_store_rules() is not called and the stick-tables entry
never committed.

The comment above the code that causes this transition clearly says
that the function is to be used after the connection is established
with the server, but there's no such protection, and we always have
the AUTO_CLOSE flag there (but there's hardly any available condition
to eliminate it).

This patch adds a test for the connection not being in ST_CON or for
option abortonclose being set. It's sufficient to do the job and it
should not cause issues.

One concern was that the transition could happen during cs_recv()
after the connection switches from CON to RDY then the read0 would
be taken into account and would cause DIS to appear, which is not
handled either. But that cannot happen because cs_recv() doesn't do
anything until it's in ST_EST state, hence the read0() cannot be
called from CON/RDY. Thus the transition from CON to DIS is only
possible in back_handle_st_con() and back_handle_st_rdy() both of
which are called when dealing with the transition already, or when
abortonclose is set and the client aborts before connect() succeeds.

It's possible that some further improvements could be made to detect
this specific transition but it doesn't seem like anything would have
to be added.

This issue was first reported on 2.1. The abortonclose area is very
sensitive so it would be wise to backport slowly, and probably no
further than 2.4.
This commit is contained in:
Willy Tarreau 2022-04-14 17:39:48 +02:00
parent fb1b6f5bc0
commit a544c66716
1 changed files with 4 additions and 2 deletions

View File

@ -2311,10 +2311,12 @@ struct task *process_stream(struct task *t, void *context, unsigned int state)
/* first, let's check if the request buffer needs to shutdown(write), which may /* first, let's check if the request buffer needs to shutdown(write), which may
* happen either because the input is closed or because we want to force a close * happen either because the input is closed or because we want to force a close
* once the server has begun to respond. If a half-closed timeout is set, we adjust * once the server has begun to respond. If a half-closed timeout is set, we adjust
* the other side's timeout as well. * the other side's timeout as well. However this doesn't have effect during the
* connection setup unless the backend has abortonclose set.
*/ */
if (unlikely((req->flags & (CF_SHUTW|CF_SHUTW_NOW|CF_AUTO_CLOSE|CF_SHUTR)) == if (unlikely((req->flags & (CF_SHUTW|CF_SHUTW_NOW|CF_AUTO_CLOSE|CF_SHUTR)) ==
(CF_AUTO_CLOSE|CF_SHUTR))) { (CF_AUTO_CLOSE|CF_SHUTR) &&
(csb->state != CS_ST_CON || (s->be->options & PR_O_ABRT_CLOSE)))) {
channel_shutw_now(req); channel_shutw_now(req);
} }