tree 5e438d8ae67b597e97799c488a5a6290cea5e91f
parent a1d43d1fad324b53b75887216e6c44be357c8bf3
author Willy Tarreau <w@1wt.eu> 1647530349 +0100
committer Christopher Faulet <cfaulet@haproxy.com> 1648498183 +0200

BUG/MEDIUM: stream-int: do not rely on the connection error once established

Historically the stream-interface code used to check for connection
errors by itself. Later this was partially deferred to muxes, but
only once the mux is installed or the connection is at least in the
established state. But probably as a safety practice the connection
error tests remained.

The problem is that they are causing trouble on when a response received
from a mux is mixed with an error report. The typical case is an upload
that is interrupted by the server sending an error or redirect without
draining all data, causing an RST to be queued just after the data. In
this case the mux has the data, the CO_FL_ERROR flag is present on the
connection, and unfortunately the stream-interface refuses to retrieve
the data due to this flag, and return an error to the client.

It's about time to only rely on CS_FL_ERROR which is set by the mux, but
the stream-interface is still responsible for the connection during its
setup. However everywhere the CO_FL_ERROR is checked, CS_FL_ERROR is
also checked.

This commit addresses this by:
  - adding a new function si_is_conn_error() that checks the SI state
    and only reports the status of CO_FL_ERROR for states before
    SI_ST_EST.

  - eliminating all checks for CO_FL_ERORR in places where CS_FL_ERROR
    is already checked and either the presence of a mux was already
    validated or the stream-int's state was already checked as being
    SI_ST_EST or higher.

CO_FL_ERROR tests on the send() direction are also inappropriate as they
may cause the loss of pending data. Now this doesn't happen anymore and
such events are only converted to CS_FL_ERROR by the mux once notified of
the problem. As such, this must not cause the loss of any error event.

Now an early error reported on a backend mux doesn't prevent the queued
response from being read and forwarded to the client (the list of syscalls
below was trimmed and epoll_ctl is not represented):

  recvfrom(10, "POST / HTTP/1.1\r\nConnection: clo"..., 16320, 0, NULL, NULL) = 66
  sendto(11, "POST / HTTP/1.1\r\ntransfer-encodi"..., 47, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 47
  epoll_wait(3, [{events=EPOLLIN|EPOLLERR|EPOLLHUP|EPOLLRDHUP, data={u32=11, u64=11}}], 200, 15001) = 1
  recvfrom(11, "HTTP/1.1 200 OK\r\ncontent-length:"..., 16320, 0, NULL, NULL) = 57
  sendto(10, "HTTP/1.1 200 OK\r\ncontent-length:"..., 57, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 57
  epoll_wait(3, [{events=EPOLLIN|EPOLLERR|EPOLLHUP|EPOLLRDHUP, data={u32=11, u64=11}}], 200, 13001) = 1
  epoll_wait(3, [{events=EPOLLIN, data={u32=10, u64=10}}], 200, 13001) = 1
  recvfrom(10, "A\n0123456789\r\n0\r\n\r\n", 16320, 0, NULL, NULL) = 19
  shutdown(10, SHUT_WR)                   = 0
  close(11)                               = 0
  close(10)                               = 0

Above the server is an haproxy configured with the following:

   listen blah
        bind :8002
        mode http
        timeout connect 5s
        timeout client  5s
        timeout server  5s
        option httpclose
        option nolinger
        http-request return status 200 hdr connection close

And the client takes care of sending requests and data in two distinct
parts:

   while :; do
     ./dev/tcploop/tcploop 8001 C T S:"POST / HTTP/1.1\r\nConnection: close\r\nTransfer-encoding: chunked\r\n\r\n" P1 S:"A\n0123456789\r\n0\r\n\r\n" P R F;
   done

With this, a small percentage of the requests will reproduce the behavior
above. Note that this fix requires the following patch to be applied for
the test above to work:

  BUG/MEDIUM: mux-h1: only turn CO_FL_ERROR to CS_FL_ERROR with empty ibuf

This should be backported with after a few weeks of observation, and
likely one version at a time. During the backports, the patch might
need to be adjusted at each check of CO_FL_ERORR to follow the
principles explained above.

(cherry picked from commit d1480cc8a44395c27c715c0dd431d3e37a83a4c8)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit ba25116c1e0f5c37d5141cf38cc1ede26de21d98)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>