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.
(cherry picked from commit a544c667161baec1c370ca88f345d3bd76f4a13a)
[cf: Use the backend stream-int (si_b) instead of the conn-stream (csb)]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit e8b2d1aa203a16a0caaaa3d73e31e24fbd44bfc2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
diff --git a/src/stream.c b/src/stream.c
index f97a75d..c241642 100644
--- a/src/stream.c
+++ b/src/stream.c
@@ -2255,10 +2255,12 @@
/* 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
* 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)) ==
- (CF_AUTO_CLOSE|CF_SHUTR))) {
+ (CF_AUTO_CLOSE|CF_SHUTR) &&
+ (si_b->state != SI_ST_CON || (s->be->options & PR_O_ABRT_CLOSE)))) {
channel_shutw_now(req);
}