]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
3 years agoMEDIUM: mqtt: support mqtt_is_valid and mqtt_field_value converters for MQTTv3.1
Dhruv Jain [Mon, 21 Mar 2022 14:34:00 +0000 (20:04 +0530)] 
MEDIUM: mqtt: support mqtt_is_valid and mqtt_field_value converters for MQTTv3.1

In MQTTv3.1, protocol name is "MQIsdp" and protocol level is 3. The mqtt
converters(mqtt_is_valid and mqtt_field_value) did not work for clients on
mqttv3.1 because the mqtt_parse_connect() marked the CONNECT message invalid
if either the protocol name is not "MQTT" or the protocol version is other than
v3.1.1 or v5.0. To fix it, we have added the mqttv3.1 protocol name and version
as part of the checks.

This patch fixes the mqtt converters to support mqttv3.1 clients as well (issue #1600).
It must be backported to 2.4.

3 years agoBUG/MINOR: quic: Wrong TX packet related counters handling
Frédéric Lécaille [Mon, 21 Mar 2022 15:12:19 +0000 (16:12 +0100)] 
BUG/MINOR: quic: Wrong TX packet related counters handling

During the packet number space discarding, do no reset tx.in_flight counter
before decrement it from other variables.
Furthermore path prep_in_flight counter was not decremented.

3 years agoBUG/MINOR: quic: Non initialized variable in quic_build_post_handshake_frames()
Frédéric Lécaille [Mon, 21 Mar 2022 11:01:22 +0000 (12:01 +0100)] 
BUG/MINOR: quic: Non initialized variable in quic_build_post_handshake_frames()

<cid> could be accessed before being initialized.

3 years agoBUG/MINOR: quic: Incorrect peer address validation
Frédéric Lécaille [Mon, 21 Mar 2022 11:18:00 +0000 (12:18 +0100)] 
BUG/MINOR: quic: Incorrect peer address validation

We must consider the peer address as validated as soon as we received an
handshake packet. An ACK frame in handshake packet was too restrictive.
Rename the concerned flag to reflect this situation.

3 years agoBUG/MINOR: quic: 1RTT packets ignored after mux was released
Frédéric Lécaille [Mon, 21 Mar 2022 10:37:13 +0000 (11:37 +0100)] 
BUG/MINOR: quic: 1RTT packets ignored after mux was released

We must be able to handle 1RTT packets after the mux has terminated its job
(qc->mux_state == QC_MUX_RELEASED). So the condition (qc->mux_state != QC_MUX_READY)
in qc_qel_may_rm_hp() is not correct when we want to wait for the mux to be started.
Add a check in qc_parse_pkt_frms() to ensure is started before calling it. All
the STREAM frames will be ignored when the mux will be released.

3 years agoBUG/MINOR: quic: Missing TX packet initializations
Frédéric Lécaille [Mon, 21 Mar 2022 09:43:53 +0000 (10:43 +0100)] 
BUG/MINOR: quic: Missing TX packet initializations

The most important one is the ->flags member which leads to an erratic xprt behavior.
For instance a non ack-eliciting packet could be seen as ack-eliciting leading the
xprt to try to retransmit a packet which are not ack-eliciting. In this case, the
xprt does nothing and remains indefinitively in a blocking state.

3 years agoBUG/MINOR: mux-quic: Missing I/O handler events initialization
Frédéric Lécaille [Fri, 18 Mar 2022 21:49:22 +0000 (22:49 +0100)] 
BUG/MINOR: mux-quic: Missing I/O handler events initialization

This could lead to a mux erratic behavior. Sometimes the application layer could
not wakeup the mux I/O handler because it estimated it had already subscribed
to write events (see h3_snd_buf() end of implementation).

3 years agoBUG/MINOR: mux-quic: Access to empty frame list from qc_send_frames()
Frédéric Lécaille [Fri, 18 Mar 2022 17:38:19 +0000 (18:38 +0100)] 
BUG/MINOR: mux-quic: Access to empty frame list from qc_send_frames()

This was revealed by libasan when each time qc_send_frames() is run at the first
time:

=================================================================
==84177==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fbaaca2b3c8 at pc 0x560a4fdb7c2e bp 0x7fbaaca2b300 sp 0x7fbaaca2b2f8
READ of size 1 at 0x7fbaaca2b3c8 thread T6
    #0 0x560a4fdb7c2d in qc_send_frames src/mux_quic.c:473
    #1 0x560a4fdb83be in qc_send src/mux_quic.c:563
    #2 0x560a4fdb8a6e in qc_io_cb src/mux_quic.c:638
    #3 0x560a502ab574 in run_tasks_from_lists src/task.c:580
    #4 0x560a502ad589 in process_runnable_tasks src/task.c:883
    #5 0x560a501e3c88 in run_poll_loop src/haproxy.c:2675
    #6 0x560a501e4519 in run_thread_poll_loop src/haproxy.c:2846
    #7 0x7fbabd120ea6 in start_thread nptl/pthread_create.c:477
    #8 0x7fbabcb19dee in __clone (/lib/x86_64-linux-gnu/libc.so.6+0xfddee)

Address 0x7fbaaca2b3c8 is located in stack of thread T6 at offset 56 in frame
    #0 0x560a4fdb7f00 in qc_send src/mux_quic.c:514

  This frame has 1 object(s):
    [32, 48) 'frms' (line 515) <== Memory access at offset 56 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
Thread T6 created by T0 here:
    #0 0x7fbabd1bd2a2 in __interceptor_pthread_create ../../../../src/libsanitizer/asan/asan_interceptors.cpp:214
    #1 0x560a5036f9b8 in setup_extra_threads src/thread.c:221
    #2 0x560a501e70fd in main src/haproxy.c:3457
    #3 0x7fbabca42d09 in __libc_start_main ../csu/libc-start.c:308

SUMMARY: AddressSanitizer: stack-buffer-overflow src/mux_quic.c:473 in qc_send_frames

3 years agoBUG/MINOR: quic: Unsent frame because of qc_build_frms()
Frédéric Lécaille [Fri, 18 Mar 2022 16:49:29 +0000 (17:49 +0100)] 
BUG/MINOR: quic: Unsent frame because of qc_build_frms()

There are non already identified rare cases where qc_build_frms() does not manage
to size frames to be encoded in a packet leading qc_build_frm() to fail to add
such frame to the packet to be built. In such cases we must move back such
frames to their origin frame list passed as parameter to qc_build_frms(): <frms>.
because they were added to the packet frame list (but not built). If this
this packet is not retransmitted, the frame is lost for ever! Furthermore we must
not modify the buffer.

3 years agoBUG/MINOR: quic: Possible leak in quic_build_post_handshake_frames()
Frédéric Lécaille [Fri, 18 Mar 2022 16:45:28 +0000 (17:45 +0100)] 
BUG/MINOR: quic: Possible leak in quic_build_post_handshake_frames()

Rework this function to leave the connection passed as parameter in the same state
it was before entering this function.

3 years agoBUG/MINOR: quic: Possible crash in parse_retry_token()
Frédéric Lécaille [Thu, 17 Mar 2022 15:22:02 +0000 (16:22 +0100)] 
BUG/MINOR: quic: Possible crash in parse_retry_token()

We must check the decoded length of this incoming data before copying into our
internal structure. This could lead to crashes.
Reproduced with such a packet captured from QUIC interop.
    {
    0xc5, 0x00, 0x00, 0x00, 0x01, 0x12, 0xf2, 0x65,
0x4d, 0x9d, 0x58, 0x90, 0x23, 0x7e, 0x67, 0xef,
0xf8, 0xef, 0x5b, 0x87, 0x48, 0xbe, 0xde, 0x7a, /* corrupted byte: 0x11, */
0x01, 0xdc, 0x41, 0xbf, 0xfb, 0x07, 0x39, 0x9f,
0xfd, 0x96, 0x67, 0x5f, 0x58, 0x03, 0x57, 0x74,
0xc7, 0x26, 0x00, 0x45, 0x25, 0xdc, 0x7f, 0xf1,
0x22, 0x1d,
}

3 years agoMEDIUM: quic: Rework of the TX packets memory handling
Frédéric Lécaille [Thu, 17 Mar 2022 10:28:10 +0000 (11:28 +0100)] 
MEDIUM: quic: Rework of the TX packets memory handling

The TX packet refcounting had come with the multithreading support but not only.
It is very useful to ease the management of the memory allocated for TX packets
with TX frames attached to. At some locations of the code we have to move TX
frames from a packet to a new one during retranmission when the packet has been
deemed as lost or not. When deemed lost the memory allocated for the paquet must
be released contrary to when its frames are retransmitted when probing (PTO).

For now on, thanks to this patch we handle the TX packets memory this way. We
increment the packet refcount when:
  - we insert it in its packet number space tree,
  - we attache an ack-eliciting frame to it.
And reciprocally we decrement this refcount when:
  - we remove an ack-eliciting frame from the packet,
  - we delete the packet from its packet number space tree.

Note that an optimization WOULD NOT be to fully reuse (without releasing its
memorya TX packet to retransmit its contents (its ack-eliciting frames). Its
information (timestamp, in flight length) to be processed by packet loss detection
and the congestion control.

3 years agoMEDIUM: quic: Limit the number of ACK ranges
Frédéric Lécaille [Tue, 15 Mar 2022 17:44:20 +0000 (18:44 +0100)] 
MEDIUM: quic: Limit the number of ACK ranges

When building a packet with an ACK frame, we store the largest acknowledged
packet number sent in this frame in the packet (quic_tx_packet struc).
When receiving an ack for such a packet we can purge the tree of acknowledged
packet number ranges from the range sent before this largest acknowledged
packet number.

3 years agoCLEANUP: quic: "largest_acked_pn" pktns struc member moving
Frédéric Lécaille [Tue, 15 Mar 2022 11:07:41 +0000 (12:07 +0100)] 
CLEANUP: quic: "largest_acked_pn" pktns struc member moving

This struct member stores the largest acked packet number which was received. It
is used to build (TX) packet. But this is confusing to store it in the tx packet
of the packet number space structure even if it is used to build and transmit
packets.

3 years agoMINOR: quic: Code factorization (TX buffer reuse)
Frédéric Lécaille [Mon, 14 Mar 2022 11:21:03 +0000 (12:21 +0100)] 
MINOR: quic: Code factorization (TX buffer reuse)

Add qc_may_reuse_cbuf() function used by qc_prep_pkts() and qc_prep_app_pkts().
Simplification of the factorized section code: there is no need to check there
is enough room to mark the end of the data in the TX buf. This is done by
the callers (qc_prep_pkts() and qc_prep_app_pkts()). Add a diagram to explain
the conditions which must be verified to be able to reuse a cbuf struct.

This should improve the QUIC stack implementation maintenability.

3 years agoREGTESTS: Do not use REQUIRE_VERSION for HAProxy 2.5+
Tim Duesterhus [Fri, 11 Mar 2022 21:46:16 +0000 (22:46 +0100)] 
REGTESTS: Do not use REQUIRE_VERSION for HAProxy 2.5+

Introduced in:

0657b9338 MINOR: stream: add "last_rule_file" and "last_rule_line" samples

3 years agoCLEANUP: Reapply ist.cocci
Tim Duesterhus [Tue, 15 Mar 2022 12:11:08 +0000 (13:11 +0100)] 
CLEANUP: Reapply ist.cocci

This makes use of the newly added:

    - i.ptr = p;
    - i.len = strlen(i.ptr);
    + i = ist(p);

patch.

3 years agoDEV: coccinelle: Add a new pattern to ist.cocci
Tim Duesterhus [Tue, 15 Mar 2022 12:11:07 +0000 (13:11 +0100)] 
DEV: coccinelle: Add a new pattern to ist.cocci

This was previously ignored in "DEV: coccinelle: Fix incorrect replacement in ist.cocci",
but is now properly replaced by a simple `ist()` call.

3 years agoCLEANUP: Reapply ist.cocci with `--include-headers-for-types --recursive-includes`
Tim Duesterhus [Tue, 15 Mar 2022 12:11:06 +0000 (13:11 +0100)] 
CLEANUP: Reapply ist.cocci with `--include-headers-for-types --recursive-includes`

Previous uses of `ist.cocci` did not add `--include-headers-for-types` and
`--recursive-includes` preventing Coccinelle seeing `struct ist` members of
other structs.

Reapply the patch with proper flags to further clean up the use of the ist API.

The command used was:

    spatch -sp_file dev/coccinelle/ist.cocci -in_place --include-headers --include-headers-for-types --recursive-includes --dir src/

3 years agoDEV: coccinelle: Fix incorrect replacement in ist.cocci
Tim Duesterhus [Tue, 15 Mar 2022 12:11:05 +0000 (13:11 +0100)] 
DEV: coccinelle: Fix incorrect replacement in ist.cocci

We must not use `ist2()` if the value of `i.len` is derived from the value of
`i.ptr`:

    i.ptr = "foo";
    i.len = strlen(i.ptr);

3 years agoBUG/MINOR: http-rules: Don't free new rule on allocation failure
Christopher Faulet [Mon, 21 Mar 2022 07:21:19 +0000 (08:21 +0100)] 
BUG/MINOR: http-rules: Don't free new rule on allocation failure

If allocation of a new HTTP rule fails, we must not release it calling
free_act_rule(). The regression was introduced by the commit dd7e6c6dc
("BUG/MINOR: http-rules: completely free incorrect TCP rules on error").

This patch must only be backported if the commit above is backported. It should
fix the issues #1627, #1628 and #1629.

3 years agoBUG/MINOR: rules: Initialize the list element when allocating a new rule
Christopher Faulet [Mon, 21 Mar 2022 06:55:34 +0000 (07:55 +0100)] 
BUG/MINOR: rules: Initialize the list element when allocating a new rule

dd7e6c6dc ("BUG/MINOR: http-rules: completely free incorrect TCP rules on
error") and 388c0f2a6 ("BUG/MINOR: tcp-rules: completely free incorrect TCP
rules on error") introduced a regression because the list element of a new
rule is not intialized. Thus HAProxy crashes when an incorrect rule is
released.

This patch must be backported if above commits are backported. Note that
new_act_rule() only exists since the 2.5. It relies on the commit d535f807b
("MINOR: rules: add a new function new_act_rule() to allocate act_rules").

3 years agoBUG/MEDIUM: mux-h2: make use of http-request and keep-alive timeouts
Willy Tarreau [Fri, 18 Mar 2022 14:57:34 +0000 (15:57 +0100)] 
BUG/MEDIUM: mux-h2: make use of http-request and keep-alive timeouts

Christian Ruppert reported an issue explaining that it's not possible to
forcefully close H2 connections which do not receive requests anymore if
they continue to send control traffic (window updates, ping etc). This
will indeed refresh the timeout. In H1 we don't have this problem because
any single byte is part of the stream, so the control frames in H2 would
be equivalent to TCP acks in H1, that would not contribute to the timeout
being refreshed.

What misses from H2 is the use of http-request and keep-alive timeouts.
These were not implemented because initially it was hard to see how they
could map to H2. But if we consider the real use of the keep-alive timeout,
that is, how long do we keep a connection alive with no request, then it's
pretty obvious that it does apply to H2 as well. Similarly, http-request
may definitely be honored as soon as a HEADERS frame starts to appear
while there is no stream. This will also allow to deal with too long
CONTINUATION frames.

This patch moves the timeout update to a new function, h2c_update_timeout(),
which is in charge of this. It also adds an "idle_start" timestamp in the
connection, which is set when nb_cs reaches zero or when a headers frame
start to arrive, so that it cannot be delayed too long.

This patch should be backported to recent stable releases after some
observation time. It depends on previous patch "MEDIUM: mux-h2: slightly
relax timeout management rules".

3 years agoMEDIUM: mux-h2: slightly relax timeout management rules
Willy Tarreau [Fri, 18 Mar 2022 13:59:54 +0000 (14:59 +0100)] 
MEDIUM: mux-h2: slightly relax timeout management rules

The H2 timeout rules were arranged to cover complex situations In 2.1
with commit c2ea47fb1 ("BUG/MEDIUM: mux-h2: do not enforce timeout on
long connections").

It turns out that such rules while complex, do not perfectly cover all
use cases. The real intent is to say that as long as there are attached
streams, the connection must not timeout. Then once all these streams
have quit (possibly for timeout reasons) then the mux should take over
the management of timeouts.

We do have this nb_cs field which indicates the number of attached
streams, and it's updated even when leaving orphaned streams. So
checking it alone is sufficient to know whether it's the mux or the
streams that are in charge of the timeouts.

In its current state, this doesn't cause visible effects except that
it makes it impossible to implement more subtle parsing timeouts.

This would need to be backported as far as 2.0 along with the next
commit that will depend on it.

3 years agoBUG/MEDIUM: trace: avoid race condition when retrieving session from conn->owner
Willy Tarreau [Fri, 18 Mar 2022 16:37:20 +0000 (17:37 +0100)] 
BUG/MEDIUM: trace: avoid race condition when retrieving session from conn->owner

There's a rare race condition possible when trying to retrieve session from
a back connection's owner, that was fixed in 2.4 and described in commit
3aab17bd5 ("BUG/MAJOR: connection: reset conn->owner when detaching from
session list").

It also affects the trace code which does the same, so the same fix is
needed, i.e. check from conn->session_list that the connection is still
enlisted. It's visible when sending a few tens to hundreds of parallel
requests to an h2 backend and enabling traces in parallel.

This should be backported as far as 2.2 which is the oldest version
supporting traces.

3 years agoBUG/MEDIUM: stream-int: do not rely on the connection error once established
Willy Tarreau [Thu, 17 Mar 2022 15:19:09 +0000 (16:19 +0100)] 
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.

3 years agoBUG/MEDIUM: mux-h1: only turn CO_FL_ERROR to CS_FL_ERROR with empty ibuf
Willy Tarreau [Thu, 17 Mar 2022 16:10:36 +0000 (17:10 +0100)] 
BUG/MEDIUM: mux-h1: only turn CO_FL_ERROR to CS_FL_ERROR with empty ibuf

A connection-level error must not be turned to a stream-level error if there
are still pending data for that stream, otherwise it can cause the truncation
of the last pending data.

This must be backported to affected releases, at least as far as 2.4,
maybe further.

3 years agoCI: github actions: switch to LibreSSL-3.5.1
Ilya Shipitsin [Wed, 16 Mar 2022 07:10:47 +0000 (12:10 +0500)] 
CI: github actions: switch to LibreSSL-3.5.1

3 years agoBUG/MINOR: httpclient: CF_SHUTW_NOW should be tested with channel_is_empty()
William Lallemand [Thu, 17 Mar 2022 14:14:15 +0000 (15:14 +0100)] 
BUG/MINOR: httpclient: CF_SHUTW_NOW should be tested with channel_is_empty()

CF_SHUTW_NOW shouldn't be a condition alone to exit the io handler, it
must be tested with the emptiness of the response channel.

Must be backported to 2.5.

3 years agoBUG/MINOR: httpclient: process the response when received before the end of the request
William Lallemand [Thu, 17 Mar 2022 13:57:23 +0000 (14:57 +0100)] 
BUG/MINOR: httpclient: process the response when received before the end of the request

A server could reply a response with a shut before the end of the htx
transfer, in this case the httpclient would leave before computing the
received response.

This patch fixes the issue by calling the "process_data" label instead of
the "more" label which don't do the si_shut.

Must be bacported in 2.5.

3 years agoBUG/MINOR: httpclient: only check co_data() instead of HTTP_MSG_DATA
William Lallemand [Thu, 17 Mar 2022 13:45:46 +0000 (14:45 +0100)] 
BUG/MINOR: httpclient: only check co_data() instead of HTTP_MSG_DATA

Checking msg >= HTTP_MSG_DATA was useful to check if we received all the
data. However it does not work correctly in case of errors because we
don't reach this state, preventing to catch the error in the httpclient.

The consequence of this problem is that we don't get the status code of
the error response upon an error.

Fix the issue by only checking co_data().

Must be backported to 2.5.

3 years agoBUG/MINOR: http-rules: completely free incorrect TCP rules on error
Willy Tarreau [Thu, 17 Mar 2022 19:29:06 +0000 (20:29 +0100)] 
BUG/MINOR: http-rules: completely free incorrect TCP rules on error

When a http-request or http-response rule fails to parse, we currently
free only the rule without its contents, which makes ASAN complain.
Now that we have a new function for this, let's completely free the
rule. This relies on this commit:

  MINOR: actions: add new function free_act_rule() to free a single rule

It's probably not needed to backport this since we're on the exit path
anyway.

3 years agoBUG/MINOR: tcp-rules: completely free incorrect TCP rules on error
Willy Tarreau [Thu, 17 Mar 2022 19:26:54 +0000 (20:26 +0100)] 
BUG/MINOR: tcp-rules: completely free incorrect TCP rules on error

When a tcp-request or tcp-response rule fails to parse, we currently
free only the rule without its contents, which makes ASAN complain.
Now that we have a new function for this, let's completely free the
rule. Reg-tests are now completely OK with ASAN. This relies on this
commit:

  MINOR: actions: add new function free_act_rule() to free a single rule

It's probably not needed to backport this since we're on the exit path
anyway.

3 years agoMINOR: actions: add new function free_act_rule() to free a single rule
Willy Tarreau [Thu, 17 Mar 2022 19:23:43 +0000 (20:23 +0100)] 
MINOR: actions: add new function free_act_rule() to free a single rule

There was free_act_rules() that frees all rules from a head but nothing
to free a single rule. Currently some rulesets partially free their own
rules on parsing error, and we're seeing some regtests emit errors under
ASAN because of this.

Let's first extract the code to free a rule into its own function so
that it becomes possible to use it on a single rule.

3 years agoBUG/MINOR: logs: fix logsrv leaks on clean exit
Willy Tarreau [Thu, 17 Mar 2022 18:47:33 +0000 (19:47 +0100)] 
BUG/MINOR: logs: fix logsrv leaks on clean exit

Log servers are a real mess because:
  - entries are duplicated using memcpy() without their strings being
    reallocated, which results in these ones not being freeable every
    time.

  - a new field, ring_name, was added in 2.2 by commit 99c453df9
    ("MEDIUM: ring: new section ring to declare custom ring buffers.")
    but it's never initialized during copies, causing the same issue

  - no attempt is made at freeing all that.

Of course, running "haproxy -c" under ASAN quickly notices that and
dumps a core.

This patch adds the missing strdup() and initialization where required,
adds a new free_logsrv() function to cleanly free() such a structure,
calls it from the proxy when iterating over logsrvs instead of silently
leaking their file names and ring names, and adds the same logsrv loop
to the proxy_free_defaults() function so that we don't leak defaults
sections on exit.

It looks a bit entangled, but it comes as a whole because all this stuff
is inter-dependent and was missing.

It's probably preferable not to backport this in the foreseable future
as it may reveal other jokes if some obscure parts continue to memcpy()
the logsrv struct.

3 years agoBUG/MINOR: server/ssl: free the SNI sample expression
William Lallemand [Wed, 16 Mar 2022 16:48:19 +0000 (17:48 +0100)] 
BUG/MINOR: server/ssl: free the SNI sample expression

ASAN complains about the SNI expression not being free upon an haproxy
-c. Indeed the httpclient is now initialized with a sni expression and
this one is never free in the server release code.

Must be backported in 2.5 and could be backported in every stable
versions.

3 years agoBUILD: httpclient: fix build without SSL
William Lallemand [Wed, 16 Mar 2022 15:39:23 +0000 (16:39 +0100)] 
BUILD: httpclient: fix build without SSL

src/http_client.c: In function ‘httpclient_cfg_postparser’:
src/http_client.c:1065:8: error: unused variable ‘errmsg’ [-Werror=unused-variable]
 1065 |  char *errmsg = NULL;
      |        ^~~~~~
src/http_client.c:1064:6: error: unused variable ‘err_code’ [-Werror=unused-variable]
 1064 |  int err_code = 0;
      |      ^~~~~~~~

Fix the build of the httpclient without SSL, the problem was introduced
with previous patch 71e3158 ("BUG/MINOR: httpclient: send the SNI using
the host header")

Must be backported in 2.5 as well.

3 years agoBUG/MINOR: httpclient: send the SNI using the host header
William Lallemand [Wed, 16 Mar 2022 14:47:47 +0000 (15:47 +0100)] 
BUG/MINOR: httpclient: send the SNI using the host header

Generate an SNI expression which uses the Host header of the request.
This is mandatory for most of the SSL servers nowadays.

Must be backported in 2.5 with the previous patch which export
server_parse_sni_expr().

3 years agoMINOR: server: export server_parse_sni_expr() function
William Lallemand [Wed, 16 Mar 2022 14:44:42 +0000 (15:44 +0100)] 
MINOR: server: export server_parse_sni_expr() function

Export the server_parse_sni_expr() function in order to create a SNI
expression in a server which was not parsed from the configuration.

3 years agoDEV: udp: add support for random packet corruption
Willy Tarreau [Wed, 16 Mar 2022 14:07:51 +0000 (15:07 +0100)] 
DEV: udp: add support for random packet corruption

-c sets a corruption rate in % of the packets.
-o sets the start offset of the area to be corrupted.
-w sets the length of the area to be corrupted.

A single byte within that area will then be randomly XORed with a
random value.

3 years agoDEV: udp: switch parser to getopt() instead of positional arguments
Willy Tarreau [Wed, 16 Mar 2022 13:49:33 +0000 (14:49 +0100)] 
DEV: udp: switch parser to getopt() instead of positional arguments

In order to ease addition of new types of perturbations to udp-perturb,
let's first switch to getopt() and get rid of the positional arguments.
The random seed was already a conditional option of the rate which was
a conditional option as well. We add -r and -s for the rate and the
seed, and new options will follow.

3 years agoBUG/MEDIUM: sink: Properly get the stream-int in appctx callback functions
Christopher Faulet [Wed, 16 Mar 2022 09:01:26 +0000 (10:01 +0100)] 
BUG/MEDIUM: sink: Properly get the stream-int in appctx callback functions

The appctx owner is not a stream-interface anymore. It is now a
conn-stream. However, sink code was not updated accordingly. It is now
fixed.

It is 2.6-specific, no backport is needed.

3 years agoBUG/MEDIUM: cli/debug: Properly get the stream-int in all debug I/O handlers
Christopher Faulet [Wed, 16 Mar 2022 08:52:10 +0000 (09:52 +0100)] 
BUG/MEDIUM: cli/debug: Properly get the stream-int in all debug I/O handlers

The appctx owner is not a stream-interface anymore. It is now a conn-stream.
In the cli I/O handler for the command "debug dev fd", we still handle it as
a stream-interface. It is now fixed.

It is 2.6-specific, no backport is needed.

3 years agoBUG/MEDIUM: applet: Don't call .release callback function twice
Christopher Faulet [Tue, 15 Mar 2022 10:29:59 +0000 (11:29 +0100)] 
BUG/MEDIUM: applet: Don't call .release callback function twice

Since the CS/SI refactoring, the .release callback function may be called
twice. The first call when a shutdown for read or for write is performed.
The second one when the applet is detached from its conn-stream. The second
call must be guarded, just like the first one, to only be performed is the
stream-interface is not the in disconnected (SI_ST_DIS) or closed
(SI_ST_CLO) state.

To simplify the fix, we now always rely on si_applet_release() function.

It is 2.6-specific, no backport is needed.

3 years agoBUG/MINOR: httpclient/lua: stuck when closing without data
William Lallemand [Tue, 15 Mar 2022 09:52:07 +0000 (10:52 +0100)] 
BUG/MINOR: httpclient/lua: stuck when closing without data

The httpclient lua code is lacking the end callback, which means it
won't be able to wake up the lua code after a longjmp if the connection
was closed without any data.

Must be backported to 2.5.

3 years agoBUG/MAJOR: quic: Possible crash with full congestion control window
Frédéric Lécaille [Sun, 13 Mar 2022 18:19:12 +0000 (19:19 +0100)] 
BUG/MAJOR: quic: Possible crash with full congestion control window

This commit reverts this one:
  "d5066dd9d BUG/MEDIUM: quic: qc_prep_app_pkts() retries on qc_build_pkt() failures"

After having filled the congestion control window, qc_build_pkt() always fails.
Then depending on the relative position of the writer and  reader indexes for the
TX buffer, this could lead this function to try to reuse the buffer even if not full.
In such case, we do not always mark the end of the data in this TX buffer. This
is something the reader cannot understand: it reads a false datagram length,
then a wrong packet address from the TX buffer, leading to an invalid pointer
dereferencing.

3 years agoBUG/MEDIUM: quic: Blocked STREAM when retransmitted
Frédéric Lécaille [Sun, 13 Mar 2022 11:31:36 +0000 (12:31 +0100)] 
BUG/MEDIUM: quic: Blocked STREAM when retransmitted

STREAM frames which are not acknowledged in order are inserted in ->tx.acked_frms
tree ordered by the STREAM frame offset values. Then, they are consumed in order
by qcs_try_to_consume(). But, when we retransmit frames, we possibly have to
insert the same STREAM frame node (with the same offset) in this tree.
The problem is when they have different lengths. Unfortunately the restransmitted
frames are not inserted because of the tree nature (EB_ROOT_UNIQUE). If the STREAM
frame which has been successfully inserted has a smaller length than the
retransmitted ones, when it is consumed they are tailing bytes in the STREAM
(retransmitted ones) which indefinitively remains in the STREAM TX buffer
which will never properly be consumed, leading to a blocking state.

At this time this may happen because we sometimes build STREAM frames
with null lengths. But this is another issue.

The solution is to use an EB_ROOT tree to support the insertion of STREAM frames
with the same offset but with different lengths. As qcs_try_to_consume() support
the STREAM frames retransmission this modification should not have any impact.

3 years agoREGTESTS: fix the race conditions in be2hex.vtc
Christopher Faulet [Mon, 14 Mar 2022 14:36:27 +0000 (15:36 +0100)] 
REGTESTS: fix the race conditions in be2hex.vtc

In the same way than for be2hex.vtc, a "Connection: close" header is added
to all responses to avoid any connection reuse. This should avoid any "HTTP
header incomplete" errors.

3 years agoBUG/MEDIUM: httpclient: must manipulate head, not first
William Lallemand [Thu, 10 Mar 2022 16:23:40 +0000 (17:23 +0100)] 
BUG/MEDIUM: httpclient: must manipulate head, not first

The httpclient mistakenly use the htx_get_first{_blk}() functions instead
of the htx_get_head{_blk}() functions. Which could stop the httpclient
because it will be without the start line, waiting for data that won't never
come.

Must be backported in 2.5.

3 years agoBUG/MINOR: httpclient: remove the UNUSED block when parsing headers
William Lallemand [Wed, 9 Mar 2022 17:56:02 +0000 (18:56 +0100)] 
BUG/MINOR: httpclient: remove the UNUSED block when parsing headers

Remove the UNUSED blocks when iterating on headers, we should not stop
when encountering one. We should only stop iterating once we found the
EOH block. It doesn't provoke a problem, since we don't manipulates
the headers before treating them, but it could evolve in the future.

Must be backported to 2.5.

3 years agoBUG/MINOR: httpclient: consume partly the blocks when necessary
William Lallemand [Wed, 9 Mar 2022 10:58:51 +0000 (11:58 +0100)] 
BUG/MINOR: httpclient: consume partly the blocks when necessary

Consume partly the blocks in the httpclient I/O handler when there is
not enough room in the destination buffer for the whole block or when
the block is not contained entirely in the channel's output.

It prevents the I/O handler to be stuck in cases when we need to modify
the buffer with a filter for exemple.

Must be backported in 2.5.

3 years agoCLEANUP: htx: remove unused co_htx_remove_blk()
William Lallemand [Thu, 24 Feb 2022 16:15:06 +0000 (17:15 +0100)] 
CLEANUP: htx: remove unused co_htx_remove_blk()

Remove the unused co_htx_remove_blk(), this function was confusing
because you need to check the output size from the caller anyway.

3 years agoBUG/MEDIUM: httpclient: don't consume data before it was analyzed
William Lallemand [Thu, 24 Feb 2022 15:55:41 +0000 (16:55 +0100)] 
BUG/MEDIUM: httpclient: don't consume data before it was analyzed

In httpclient_applet_io_handler(), on the response path, we don't check
if the data are in the output part of the channel, and could consume
them before they were analyzed.

To fix this issue, this patch checks for the stline and the headers if
the msg_state is >= HTTP_MSG_DATA which means the stline and headers
were analyzed. For the data part, it checks if each htx blocks is in the
output before copying it.

Must be backported in 2.5.

3 years ago[RELEASE] Released version 2.6-dev3 v2.6-dev3
Willy Tarreau [Fri, 11 Mar 2022 17:09:24 +0000 (18:09 +0100)] 
[RELEASE] Released version 2.6-dev3

Released version 2.6-dev3 with the following main changes :
    - DEBUG: rename WARN_ON_ONCE() to CHECK_IF()
    - DEBUG: improve BUG_ON output message accuracy
    - DEBUG: implement 4 levels of choices between warn and crash.
    - DEBUG: add two new macros to enable debugging in hot paths
    - DEBUG: buf: replace some sensitive BUG_ON() with BUG_ON_HOT()
    - DEBUG: buf: add BUG_ON_HOT() to most buffer management functions
    - MINOR: channel: don't use co_set_data() to decrement output
    - DEBUG: channel: add consistency checks using BUG_ON_HOT() in some key functions
    - MINOR: conn-stream: Improve API to have safe/unsafe accessors
    - MEDIUM: tree-wide: Use unsafe conn-stream API when it is relevant
    - CLEANUP: stream-int: Make si_cs_send() function static
    - REORG: stream-int: Uninline si_sync_recv() and make si_cs_recv() private
    - BUG/MEDIUM: mux-fcgi: Don't rely on SI src/dst addresses for FCGI health-checks
    - BUG/MEDIUM: htx: Fix a possible null derefs in htx_xfer_blks()
    - REGTESTS: fix the race conditions in normalize_uri.vtc
    - DEBUG: stream-int: Fix BUG_ON used to test appctx in si_applet_ops callbacks
    - BUILD: debug: fix build warning on older compilers around DEBUG_STRICT_ACTION
    - CLEANUP: connection: Indicate unreachability to the compiler in conn_recv_proxy
    - MINOR: connection: Transform safety check in PROXYv2 parsing into BUG_ON()
    - DOC: install: it's DEBUG_CFLAGS, not DEBUG, which is set to -g
    - DOC: install: describe the DEP variable
    - DOC: install: describe how to choose options used in the DEBUG variable
    - MINOR: queue: Replace if() + abort() with BUG_ON()
    - CLEANUP: adjust indentation in bidir STREAM handling function
    - MINOR: quic: simplify copy of STREAM frames to RX buffer
    - MINOR: quic: handle partially received buffered stream frame
    - MINOR: mux-quic: define flag for last received frame
    - BUG/MINOR: quic: support FIN on Rx-buffered STREAM frames
    - MEDIUM: quic: rearchitecture Rx path for bidirectional STREAM frames
    - REGTESTS: fix the race conditions in secure_memcmp.vtc
    - CLEANUP: stream: Remove useless tests on conn-stream in stream_dump()
    - BUILD: ssl: another build warning on LIBRESSL_VERSION_NUMBER
    - MINOR: quic: Ensure PTO timer is not set in the past
    - MINOR: quic: Post handshake I/O callback switching
    - MINOR: quic: Drop the packets of discarded packet number spaces
    - CLEANUP: quic: Useless tests in qc_try_rm_hp()
    - CLEANUP: quic: Indentation fix in qc_prep_pkts()
    - MINOR: quic: Assemble QUIC TLS flags at the same level
    - BUILD: conn_stream: avoid null-deref warnings on gcc 6
    - BUILD: connection: do not declare register_mux_proto() inline
    - BUILD: http_rules: do not declare http_*_keywords_registre() inline
    - BUILD: trace: do not declare trace_registre_source() inline
    - BUILD: tcpcheck: do not declare tcp_check_keywords_register() inline
    - DEBUG: reduce the footprint of BUG_ON() calls
    - BUG/MEDIUM: httpclient/lua: infinite appctx loop with POST
    - BUG/MINOR: pool: always align pool_heads to 64 bytes
    - DEV: udp: add a tiny UDP proxy for testing
    - DEV: udp: implement pseudo-random reordering/loss
    - DEV: udp: add an optional argument to set the prng seed
    - BUG/MINOR: quic: fix segfault on CC if mux uninitialized
    - BUG/MEDIUM: pools: fix ha_free() on area in the process of being freed
    - CLEANUP: tree-wide: remove a few rare non-ASCII chars
    - CI: coverity: simplify debugging options
    - CLEANUP: quic: complete ABORT_NOW with a TODO comment
    - MINOR: quic: qc_prep_app_pkts() implementation
    - MINOR: quic: Send short packet from a frame list
    - MINOR: quic: Make qc_build_frms() build ack-eliciting frames from a list
    - MINOR: quic: Export qc_send_app_pkts()
    - MINOR: mux-quic: refactor transport parameters init
    - MINOR: mux-quic: complete functions to detect stream type
    - MINOR: mux-quic: define new unions for flow-control fields
    - MEDIUM: mux-quic: use direct send transport API for STREAMs
    - MINOR: mux-quic: retry send opportunistically for remaining frames
    - MEDIUM: mux-quic: implement MAX_STREAMS emission for bidir streams
    - BUILD: fix kFreeBSD build.
    - MINOR: quic: Retry on qc_build_pkt() failures
    - BUG/MINOR: quic: Missing recovery start timer reset
    - CLEANUP: quic: Remove QUIC path manipulations out of the congestion controller
    - MINOR: quic: Add a "slow start" callback to congestion controller
    - MINOR: quic: Persistent congestion detection outside of controllers
    - CLEANUP: quic: Remove useless definitions from quic_cc_event struct
    - BUG/MINOR: quic: Confusion betwen "in_flight" and "prep_in_flight" in quic_path_prep_data()
    - MINOR: quic: More precise window update calculation
    - CLEANUP: quic: Remove window redundant variable from NewReno algorithm state struct
    - MINOR: quic: Add quic_max_int_by_size() function
    - BUG/MAJOR: quic: Wrong quic_max_available_room() returned value
    - MINOR: pools: add a new global option "no-memory-trimming"
    - BUG/MINOR: add missing modes in proxy_mode_str()
    - BUG/MINOR: cli: shows correct mode in "show sess"
    - BUG/MEDIUM: quic: do not drop packet on duplicate stream/decoding error
    - MINOR: stats: Add dark mode support for socket rows
    - BUILD: fix recent build breakage of freebsd caused by kFreeBSD build fix
    - BUG/MINOR: httpclient: Set conn-stream/channel EOI flags at the end of request
    - BUG/MINOR: hlua: Set conn-stream/channel EOI flags at the end of request
    - BUG/MINOR: stats: Set conn-stream/channel EOI flags at the end of request
    - BUG/MINOR: cache: Set conn-stream/channel EOI flags at the end of request
    - BUG/MINOR: promex: Set conn-stream/channel EOI flags at the end of request
    - BUG/MEDIUM: stream: Use the front analyzers for new listener-less streams
    - DEBUG: cache: Update underlying buffer when loading HTX message in cache applet
    - BUG/MEDIUM: mcli: Properly handle errors and timeouts during reponse processing
    - DEBUG: stream: Add the missing descriptions for stream trace events
    - DEBUG: stream: Fix stream trace message to print response buffer state
    - MINOR: proxy: Store monitor_uri as a `struct ist`
    - MINOR: proxy: Store fwdfor_hdr_name as a `struct ist`
    - MINOR: proxy: Store orgto_hdr_name as a `struct ist`
    - MEDIUM: proxy: Store server_id_hdr_name as a `struct ist`
    - CLEANUP: fcgi: Replace memcpy() on ist by istcat()
    - CLEANUP: fcgi: Use `istadv()` in `fcgi_strm_send_params`
    - BUG/MAJOR: mux-pt: Always destroy the backend connection on detach
    - DOC: sample fetch methods: move distcc_* to the right locations
    - MINOR: rules: record the last http/tcp rule that gave a final verdict
    - MINOR: stream: add "last_rule_file" and "last_rule_line" samples
    - BUG/MINOR: session: fix theoretical risk of memleak in session_accept_fd()
    - MINOR: quic: Add max_idle_timeout advertisement handling
    - MEDIUM: quic: Remove the QUIC connection reference counter
    - BUG/MINOR: quic: ACK_REQUIRED and ACK_RECEIVED flag collision
    - BUG/MINOR: quic: Missing check when setting the anti-amplification limit as reached
    - MINOR: quic: Add a function to compute the current PTO
    - MEDIUM: quic: Implement the idle timeout feature
    - BUG/MEDIUM: quic: qc_prep_app_pkts() retries on qc_build_pkt() failures
    - CLEANUP: quic: Comments fix for qc_prep_(app)pkts() functions
    - MINOR: mux-quic: prevent push frame for unidir streams
    - MINOR: mux-quic: improve opportunistic retry sending for STREAM frames
    - MINOR: quic: implement sending confirmation
    - MEDIUM: mux-quic: improve bidir STREAM frames sending
    - MEDIUM: check: do not auto configure SSL/PROXY for dynamic servers
    - REGTESTS: server: test SSL/PROXY with checks for dynamic servers
    - MEDIUM: server: remove experimental-mode for dynamic servers
    - BUG/MINOR: buffer: fix debugging condition in b_peek_varint()

3 years agoBUG/MINOR: buffer: fix debugging condition in b_peek_varint()
Willy Tarreau [Fri, 11 Mar 2022 15:55:49 +0000 (16:55 +0100)] 
BUG/MINOR: buffer: fix debugging condition in b_peek_varint()

The BUG_ON_HOT() test condition added to b_peek_varint() by commit
8873b85bd ("DEBUG: buf: add BUG_ON_HOT() to most buffer management
functions") was wrong as <data> in this function is not b->data,
so that was triggering during live dumps of H2 traces on the CLI
when built with -DDEBUG_STRICT=2. No backport is needed.

3 years agoMEDIUM: server: remove experimental-mode for dynamic servers
Amaury Denoyelle [Wed, 9 Mar 2022 14:07:31 +0000 (15:07 +0100)] 
MEDIUM: server: remove experimental-mode for dynamic servers

Dynamic servers feature is now judged to be stable enough. Remove the
experimental-mode requirement for "add/del server" commands. This should
facilitate dynamic servers adoption.

3 years agoREGTESTS: server: test SSL/PROXY with checks for dynamic servers
Amaury Denoyelle [Wed, 9 Mar 2022 15:43:34 +0000 (16:43 +0100)] 
REGTESTS: server: test SSL/PROXY with checks for dynamic servers

Complete the dynamic servers regtest to ensure there is no implicit
interaction for checks and SSL/PROXY server settings.

3 years agoMEDIUM: check: do not auto configure SSL/PROXY for dynamic servers
Amaury Denoyelle [Wed, 9 Mar 2022 13:20:10 +0000 (14:20 +0100)] 
MEDIUM: check: do not auto configure SSL/PROXY for dynamic servers

For server checks, SSL and PROXY is automatically inherited from the
server settings if no specific check port is specified. Change this
behavior for dynamic servers : explicit "check-ssl"/"check-send-proxy"
are required for them.

Without this change, it is impossible to add a dynamic server with
SSL/PROXY settings and checks without, if the check port is not
explicit. This is because "no-check-ssl"/"no-check-send-proxy" keywords
are not available for dynamic servers.

This change respects the principle that dynamic servers on the CLI
should not reuse the same shortcuts used during the config file parsing.
Mostly because we expect this feature to be manipulated by automated
tools, contrary to the config file which should aim to be the shortest
possible for human readability.

Update the documentation of the "check" keyword to reflect this change.

3 years agoMEDIUM: mux-quic: improve bidir STREAM frames sending
Amaury Denoyelle [Thu, 10 Mar 2022 15:45:53 +0000 (16:45 +0100)] 
MEDIUM: mux-quic: improve bidir STREAM frames sending

The current implementation of STREAM frames emission has some
limitation. Most notably when we cannot sent all frames in a single
qc_send run.

In this case, frames are left in front of the MUX list. It will be
re-send individually before other frames, possibly another frame from
the same STREAM with new data. An opportunity to merge the frames is
lost here.

This method is now improved. If a frame cannot be send entirely, it is
discarded. On the next qc_send run, we retry to send to this position. A
new field qcs.sent_offset is used to remember this. A new frame list is
used for each qc_send.

The impact of this change is not precisely known. The most notable point
is that it is a more logical method of emission. It might also improve
performance as we do not keep old STREAM frames which might delay other
streams.

3 years agoMINOR: quic: implement sending confirmation
Amaury Denoyelle [Thu, 10 Mar 2022 15:44:14 +0000 (16:44 +0100)] 
MINOR: quic: implement sending confirmation

Implement a new MUX function qcc_notify_send. This function must be
called by the transport layer to confirm the sending of STREAM data to
the MUX.

For the moment, the function has no real purpose. However, it will be
useful to solve limitations on push frame and implement the flow
control.

3 years agoMINOR: mux-quic: improve opportunistic retry sending for STREAM frames
Amaury Denoyelle [Thu, 10 Mar 2022 15:42:23 +0000 (16:42 +0100)] 
MINOR: mux-quic: improve opportunistic retry sending for STREAM frames

For the moment, the transport layer function qc_send_app_pkts lacks
features. Most notably, it only send up to a single Tx buffer and won't
retry even if there is frames left and its Tx buffer is now empty.

To overcome this limitation, the MUX implements an opportunistic retry
sending mechanism. qc_send_app_pkts is repeatedly called until the
transport layer is blocked on an external condition (such as congestion
control or a sendto syscall error).

The blocking was detected by inspecting the frame list before and after
qc_send_app_pkts. If no frame has been poped by the function, we
considered the transport layer to be blocked and we stop to send. The
MUX is subscribed on the lower layer to send the frames left.

However, in case of STREAM frames, qc_send_app_pkts might use only a
portion of the data and update the frame offset. So, for STREAM frames,
a new mechanism is implemented : if the offset field of the first frame
has not been incremented, it means the transport layer is blocked.

This should improve transfers execution. Before this change, there is a
possibility of interrupted transfer if the mux has not sent everything
possible and is waiting on a transport signaling which will never
happen.

In the future, qc_send_app_pkts should be extended to retry sending by
itself. All this code burden will be removed from the MUX.

3 years agoMINOR: mux-quic: prevent push frame for unidir streams
Amaury Denoyelle [Thu, 10 Mar 2022 15:46:18 +0000 (16:46 +0100)] 
MINOR: mux-quic: prevent push frame for unidir streams

For the moment, unidirectional streams handling is not identical to
bidirectional ones in MUX/H3 layer, both in Rx and Tx path. As a safety,
skip over uni streams in qc_send.

In fact, this change has no impact because qcs.tx.buf is emptied before
we start using qcs_push_frame, which prevents the call to
qcs_push_frame. However, this condition will soon change to improve
bidir streams emission, so an explicit check on stream type must be
done.

It is planified to unify uni and bidir streams handling in a future
stage. When implemented, the check will be removed.

3 years agoCLEANUP: quic: Comments fix for qc_prep_(app)pkts() functions
Frédéric Lécaille [Thu, 10 Mar 2022 16:42:58 +0000 (17:42 +0100)] 
CLEANUP: quic: Comments fix for qc_prep_(app)pkts() functions

Fix the comments for these two functions about their returned values.

3 years agoBUG/MEDIUM: quic: qc_prep_app_pkts() retries on qc_build_pkt() failures
Frédéric Lécaille [Thu, 10 Mar 2022 16:06:59 +0000 (17:06 +0100)] 
BUG/MEDIUM: quic: qc_prep_app_pkts() retries on qc_build_pkt() failures

The "stop_build" label aim is to try to reuse the TX buffer when there is not
enough contiguous room to build a packet. It was defined but not used!

3 years agoMEDIUM: quic: Implement the idle timeout feature
Frédéric Lécaille [Thu, 10 Mar 2022 14:11:57 +0000 (15:11 +0100)] 
MEDIUM: quic: Implement the idle timeout feature

The aim of the idle timeout is to silently closed the connection after a period
of inactivity depending on the "max_idle_timeout" transport parameters advertised
by the endpoints. We add a new task to implement this timer. Its expiry is
updated each time we received an ack-eliciting packet, and each time we send
an ack-eliciting packet if no other such packet was sent since we received
the last ack-eliciting packet. Such conditions may be implemented thanks
to QUIC_FL_CONN_IDLE_TIMER_RESTARTED_AFTER_READ new flag.

3 years agoMINOR: quic: Add a function to compute the current PTO
Frédéric Lécaille [Thu, 10 Mar 2022 14:05:32 +0000 (15:05 +0100)] 
MINOR: quic: Add a function to compute the current PTO

There was not such a function at this time. This is needed to implement the idle
timeout feature.

3 years agoBUG/MINOR: quic: Missing check when setting the anti-amplification limit as reached
Frédéric Lécaille [Thu, 10 Mar 2022 09:38:20 +0000 (10:38 +0100)] 
BUG/MINOR: quic: Missing check when setting the anti-amplification limit as reached

Ensure the peer address is not validated before setting the anti-amplication
limit as reached.

3 years agoBUG/MINOR: quic: ACK_REQUIRED and ACK_RECEIVED flag collision
Frédéric Lécaille [Thu, 10 Mar 2022 09:00:36 +0000 (10:00 +0100)] 
BUG/MINOR: quic: ACK_REQUIRED and ACK_RECEIVED flag collision

This packet number space flags were defined with the same value because
defined at different places in the file. Assemble them at the same location
with different values.

This bug could unvalidate the peer address after it was validated
during the handshake leading to the anti-amplication limit to be
enabled again after having been disabled. The situation could not
be unblocked (deadlock).

3 years agoMEDIUM: quic: Remove the QUIC connection reference counter
Frédéric Lécaille [Tue, 8 Mar 2022 15:59:54 +0000 (16:59 +0100)] 
MEDIUM: quic: Remove the QUIC connection reference counter

There is no need to use such a reference counter anymore since the QUIC
connections are always handled by the same thread.
quic_conn_drop() is removed. Its code is merged into quic_conn_release().

3 years agoMINOR: quic: Add max_idle_timeout advertisement handling
Frédéric Lécaille [Tue, 8 Mar 2022 13:08:16 +0000 (14:08 +0100)] 
MINOR: quic: Add max_idle_timeout advertisement handling

When we store the remote transport parameters, we compute the maximum idle
timeout for the connection which is the minimum of the two advertised
max_idle_timeout transport parameter values if both have non-null values, or the
maximum if one of the value is set and non-null.

3 years agoBUG/MINOR: session: fix theoretical risk of memleak in session_accept_fd()
Willy Tarreau [Fri, 11 Mar 2022 06:25:11 +0000 (07:25 +0100)] 
BUG/MINOR: session: fix theoretical risk of memleak in session_accept_fd()

Andrew Suffield reported in issue #1596 that we've had a bug in
session_accept_fd() since 2.4 with commit 1b3c931bf ("MEDIUM:
connections: Introduce a new XPRT method, start().") where an error
label is wrong and may cause the leak of the freshly allocated session
in case conn_xprt_start() returns < 0.

The code was checked there and the only two transport layers available
at this point are raw_sock and ssl_sock. The former doesn't provide a
->start() method hence conn_xprt_start() will always return zero. The
second does provide such a function, but it may only return <0 if the
underlying transport (raw_sock) has such a method and fails, which is
thus not the case.

So fortunately it is not possible to trigger this leak.

The patch above also touched the accept code in quic_sock() which was
mostly a plain copy of the session code, but there the move didn't
have this impact, and since then it was simplified and the next change
moved it to its final destination with the proper error label.

This should be backported as far as 2.4 as a long-term safety measure
(e.g. if in the future we have a reason for making conn_xprt_start()
to start failing), but will not have any positive nor negative effect
in the short term.

3 years agoMINOR: stream: add "last_rule_file" and "last_rule_line" samples
Willy Tarreau [Wed, 9 Mar 2022 16:33:05 +0000 (17:33 +0100)] 
MINOR: stream: add "last_rule_file" and "last_rule_line" samples

These two sample fetch methods report respectively the file name and the
line number where was located the last rule that was final. This is aimed
at being used on log-format lines to help admins figure what rule in the
configuration gave a final verdict, and help understand the condition
that led to the action.

For example, it's now possible to log the last matched rule by adding
this to the log-format:

  ... lr=%[last_rule_file]:%[last_rule_line]

A regtest is provided to test various combinations of final rules, some
even on top of each other from different rulesets.

3 years agoMINOR: rules: record the last http/tcp rule that gave a final verdict
Willy Tarreau [Wed, 9 Mar 2022 16:23:10 +0000 (17:23 +0100)] 
MINOR: rules: record the last http/tcp rule that gave a final verdict

When a tcp-{request,response} content or http-request/http-response
rule delivers a final verdict (deny, accept, redirect etc), the last
evaluated one will now be recorded in the stream. The purpose is to
permit to log the last one that performed a final action. For now
the log is not produced.

3 years agoDOC: sample fetch methods: move distcc_* to the right locations
Willy Tarreau [Thu, 10 Mar 2022 09:39:58 +0000 (10:39 +0100)] 
DOC: sample fetch methods: move distcc_* to the right locations

The distcc* sample fetch methods were surprisingly located within the
"internal state" section, while they in fact depend on L6 contents.
This can be backported to all versions where they appear.

3 years agoBUG/MAJOR: mux-pt: Always destroy the backend connection on detach
Christopher Faulet [Wed, 9 Mar 2022 14:55:58 +0000 (15:55 +0100)] 
BUG/MAJOR: mux-pt: Always destroy the backend connection on detach

In TCP, when a conn-stream is detached from a backend connection, the
connection must be always closed. It was only performed if an error or a
shutdown occurred or if there was no connection owner. But it is a problem,
because, since the 2.3, backend connections are always owned by a
session. This way it is possible to have idle connections attached to a
session instead of a server. But there is no idle connections in TCP. In
addition, when a session owns a connection it is responsible to close it
when it is released. But it only works for idle connections. And it only
works if the session is released.

Thus there is the place for bugs here. And indeed, a connection leak may
occur if a connection retry is performed because of a timeout. In this case,
the underlying connection is still alive and is waiting to be fully
established. Thus, when the conn-stream is detached from the connection, the
connection is not closed. Because the PT multiplexer is quite simple, there
is no timeout at this stage. We depend on the kenerl to be notified and
finally close the connection. With an unreachable server, orphan backend
connections may be accumulated for a while. It may be perceived as a leak.

Because there is no reason to keep such backend connections, we just close
it now. Frontend connections are still closed by the session or when an
error or a shutdown occurs.

This patch should fix the issue #1522. It must be backported as far as
2.0. Note that the 2.2 and 2.0 are not affected by this bug because there is
no owner for backend TCP connections. But it is probably a good idea to
backport the patch on these versions to avoid any future bugs.

3 years agoCLEANUP: fcgi: Use `istadv()` in `fcgi_strm_send_params`
Tim Duesterhus [Fri, 4 Mar 2022 23:52:45 +0000 (00:52 +0100)] 
CLEANUP: fcgi: Use `istadv()` in `fcgi_strm_send_params`

Found manually, while creating the previous commits to turn `struct proxy`
members into ists.

There is an existing Coccinelle rule to replace this pattern by `istadv()` in
`ist.cocci`:

    @@
    struct ist i;
    expression e;
    @@

    - i.ptr += e;
    - i.len -= e;
    + i = istadv(i, e);

But apparently it is not smart enough to match ists that are stored in another
struct. It would be useful to make the existing rule more generic, so that it
might catch similar cases in the future.

3 years agoCLEANUP: fcgi: Replace memcpy() on ist by istcat()
Tim Duesterhus [Fri, 4 Mar 2022 23:52:44 +0000 (00:52 +0100)] 
CLEANUP: fcgi: Replace memcpy() on ist by istcat()

This is a little cleaner, because the length of the resulting string does not
need to be calculated manually.

3 years agoMEDIUM: proxy: Store server_id_hdr_name as a `struct ist`
Tim Duesterhus [Fri, 4 Mar 2022 23:52:43 +0000 (00:52 +0100)] 
MEDIUM: proxy: Store server_id_hdr_name as a `struct ist`

The server_id_hdr_name is already processed as an ist in various locations lets
also just store it as such.

see 0643b0e7e ("MINOR: proxy: Make `header_unique_id` a `struct ist`") for a
very similar past commit.

3 years agoMINOR: proxy: Store orgto_hdr_name as a `struct ist`
Tim Duesterhus [Fri, 4 Mar 2022 23:52:42 +0000 (00:52 +0100)] 
MINOR: proxy: Store orgto_hdr_name as a `struct ist`

The orgto_hdr_name is already processed as an ist in `http_process_request`,
lets also just store it as such.

see 0643b0e7e ("MINOR: proxy: Make `header_unique_id` a `struct ist`") for a
very similar past commit.

3 years agoMINOR: proxy: Store fwdfor_hdr_name as a `struct ist`
Tim Duesterhus [Fri, 4 Mar 2022 23:52:41 +0000 (00:52 +0100)] 
MINOR: proxy: Store fwdfor_hdr_name as a `struct ist`

The fwdfor_hdr_name is already processed as an ist in `http_process_request`,
lets also just store it as such.

see 0643b0e7e ("MINOR: proxy: Make `header_unique_id` a `struct ist`") for a
very similar past commit.

3 years agoMINOR: proxy: Store monitor_uri as a `struct ist`
Tim Duesterhus [Fri, 4 Mar 2022 23:52:40 +0000 (00:52 +0100)] 
MINOR: proxy: Store monitor_uri as a `struct ist`

The monitor_uri is already processed as an ist in `http_wait_for_request`, lets
also just store it as such.

see 0643b0e7e ("MINOR: proxy: Make `header_unique_id` a `struct ist`") for a
very similar past commit.

3 years agoDEBUG: stream: Fix stream trace message to print response buffer state
Christopher Faulet [Tue, 8 Mar 2022 14:48:55 +0000 (15:48 +0100)] 
DEBUG: stream: Fix stream trace message to print response buffer state

Channels buffer state is displayed in the strem trace messages. However,
because of a typo, the request buffer was used instead of the response one.

This patch should be backported as far as 2.2.

3 years agoDEBUG: stream: Add the missing descriptions for stream trace events
Christopher Faulet [Tue, 8 Mar 2022 14:47:02 +0000 (15:47 +0100)] 
DEBUG: stream: Add the missing descriptions for stream trace events

The description for STRM_EV_FLT_ANA and STRM_EV_FLT_ERR was missing.

This patch should be backported as far as 2.2.

3 years agoBUG/MEDIUM: mcli: Properly handle errors and timeouts during reponse processing
Christopher Faulet [Mon, 7 Mar 2022 17:20:21 +0000 (18:20 +0100)] 
BUG/MEDIUM: mcli: Properly handle errors and timeouts during reponse processing

The response analyzer of the master CLI only handles read errors. So if
there is a write error, the session remains stuck because some outgoing data
are blocked in the channel and the response analyzer waits everything to be
sent. Because the maxconn is set to 10 for the master CLI, it may be
unresponsive if this happens to many times.

Now read and write errors, timeouts and client aborts are handled.

This patch should solve the issue #1512. It must be backported as far as
2.0.

3 years agoDEBUG: cache: Update underlying buffer when loading HTX message in cache applet
Christopher Faulet [Mon, 7 Mar 2022 15:44:30 +0000 (16:44 +0100)] 
DEBUG: cache: Update underlying buffer when loading HTX message in cache applet

In the I/O handler of the cache applet, we must update the underlying buffer
when the HTX message is loaded, using htx_from_buf() function instead of
htxbuf(). It is important because the applet will update the message by
adding new HTX blocks. This way, the state of the underlying buffer remains
consistant with the state of the HTX message.

It is especially important if HAProxy is compiled with "DEBUG_STRICT=2"
mode. Without this patch, channel_add_input() call crashed if the channel
was empty at the begining of the I/O handler.

Note that it is more a build/debug issue than a bug. But this patch may
prevent future bugs. For now it is safe because htx_to_buf() function is
systematically called, updating accordingly the underlying buffer.

This patch may be backported as far as 2.0.

3 years agoBUG/MEDIUM: stream: Use the front analyzers for new listener-less streams
Christopher Faulet [Mon, 7 Mar 2022 14:31:46 +0000 (15:31 +0100)] 
BUG/MEDIUM: stream: Use the front analyzers for new listener-less streams

For now, for a stream, request analyzers are set at 2 stages. The first one
is when the stream is created. The session's listener analyzers, if any, are
set on the request channel. In addition, some HTTP analyzers are set for HTX
streams (AN_REQ_WAIT_HTTP and AN_REQ_HTTP_PROCESS_FE). The second one is
when the backend is set on the stream. At the stage, request analyzers are
updated using the backend settings.

It is an issue for client applets because there is no listener attached to
the stream. In addtion, it may have no specific/dedicated backend. Thus,
several request analyzers are missing. Among others, the HTTP analyzers for
HTTP applets. The HTTP client is the only one affected for now.

To fix the bug, when a stream is created without a listener, we use the
frontend to set the request analyzers. Note that there is no issue with the
response channel because its analyzers are set when the server connection is
established.

This patch may be backported to all stable versions. Because only the HTTP
client is affected, it must at least be backported to 2.5. It is related to
the issue #1593.

3 years agoBUG/MINOR: promex: Set conn-stream/channel EOI flags at the end of request
Christopher Faulet [Mon, 7 Mar 2022 14:56:20 +0000 (15:56 +0100)] 
BUG/MINOR: promex: Set conn-stream/channel EOI flags at the end of request

This bug is the same than for the HTTP client. See "BUG/MINOR: httpclient:
Set conn-stream/channel EOI flags at the end of request" for details.

This patch must be backported as far as 2.0. But only CF_EOI must be set
because applets are not attached to a conn-stream on older versions.

3 years agoBUG/MINOR: cache: Set conn-stream/channel EOI flags at the end of request
Christopher Faulet [Mon, 7 Mar 2022 14:53:57 +0000 (15:53 +0100)] 
BUG/MINOR: cache: Set conn-stream/channel EOI flags at the end of request

This bug is the same than for the HTTP client. See "BUG/MINOR: httpclient:
Set conn-stream/channel EOI flags at the end of request" for details.

Note that because a filter is always attached to the stream when the cache
is used, there is no issue because there is no direct forwarding in this
case. Thus the stream analyzers are able to see the HTX_FL_EOM flag on the
HTX messge.

This patch must be backported as far as 2.0. But only CF_EOI must be set
because applets are not attached to a conn-stream on older versions.

3 years agoBUG/MINOR: stats: Set conn-stream/channel EOI flags at the end of request
Christopher Faulet [Mon, 7 Mar 2022 14:52:42 +0000 (15:52 +0100)] 
BUG/MINOR: stats: Set conn-stream/channel EOI flags at the end of request

This bug is the same than for the HTTP client. See "BUG/MINOR: httpclient:
Set conn-stream/channel EOI flags at the end of request" for details.

This patch must be backported as far as 2.0. But only CF_EOI must be set
because applets are not attached to a conn-stream on older versions.

3 years agoBUG/MINOR: hlua: Set conn-stream/channel EOI flags at the end of request
Christopher Faulet [Mon, 7 Mar 2022 14:50:54 +0000 (15:50 +0100)] 
BUG/MINOR: hlua: Set conn-stream/channel EOI flags at the end of request

This bug is the same than for the HTTP client. See "BUG/MINOR: httpclient:
Set conn-stream/channel EOI flags at the end of request" for details.

This patch must be backported as far as 2.0. But only CF_EOI must be set
because applets are not attached to a conn-stream on older versions.

3 years agoBUG/MINOR: httpclient: Set conn-stream/channel EOI flags at the end of request
Christopher Faulet [Thu, 3 Mar 2022 14:38:39 +0000 (15:38 +0100)] 
BUG/MINOR: httpclient: Set conn-stream/channel EOI flags at the end of request

In HTX, HTX_FL_EOM flag is added on the message to notifiy the end of the
message was received. In addition, the producer must set CS_FL_EOI flag on
the conn-stream. If it is a mux, the stream-interface is responsible to set
CF_EOI flag on the input channel. But, for now, if the producer is an
applet, in addition to the conn-stream flag, it must also set the channel
one.

These flags are used to notify the stream that the message is finished and
no more data are expected. It is especially important when the message
itself it directly forwarded from one side to the other. Because in this
case, the stream has no way to see the HTX_FL_EOM flag on the
message. Otherwise, the stream will detect a client or a server abort,
depending on the side.

For the HTTP client, it is not really easy to diagnose this error because
there is also another bug hiding this one. All HTTP request analyzers are
not set on the input channel. This will be fixed by another patch.

This patch must be backported to 2.5. It is related to the issue #1593.

3 years agoBUILD: fix recent build breakage of freebsd caused by kFreeBSD build fix
David Carlier [Tue, 8 Mar 2022 14:49:29 +0000 (14:49 +0000)] 
BUILD: fix recent build breakage of freebsd caused by kFreeBSD build fix

Supporting kFreebsd previously led to FreeBSD (< 14) build breakage:

 In file included from src/cpuset.c:5:
 In file included from include/haproxy/cpuset.h:4:
 include/haproxy/cpuset-t.h:46:2: error: unknown type name 'cpu_set_t'; did you mean 'cpuset_t'?
         CPUSET_REPR cpuset;
         ^~~~~~~~~~~
         cpuset_t
 include/haproxy/cpuset-t.h:21:22: note: expanded from macro 'CPUSET_REPR'
 # define CPUSET_REPR cpu_set_t
                      ^

3 years agoMINOR: stats: Add dark mode support for socket rows
Marno Krahmer [Tue, 8 Mar 2022 12:45:09 +0000 (13:45 +0100)] 
MINOR: stats: Add dark mode support for socket rows

In commit e9ed63e548 dark mode support was added to the stats page. The
initial commit does not include  dark mode color overwrites for the
.socket CSS class. This commit colors socket rows the same way as
backends that acre active but do not have a health check defined.

This fixes an issue where reading information from socket lines became
really hard in dark mode due to suboptimal coloring of the cell
background and the font in it.

3 years agoBUG/MEDIUM: quic: do not drop packet on duplicate stream/decoding error
Amaury Denoyelle [Tue, 8 Mar 2022 09:48:35 +0000 (10:48 +0100)] 
BUG/MEDIUM: quic: do not drop packet on duplicate stream/decoding error

Change the return value to success in qc_handle_bidi_strm_frm for two
specific cases :
* if STREAM frame is an already received offset
* if application decoding failed

This ensures that the packet is not dropped and properly acknowledged.
Previous to this fix, the return code was set to error which prevented
the ACK to be generated.

The impact of the bug might be noticeable in environment with packet
loss and retransmission. Due to haproxy not generating ACK for packets
containing STREAM frames with already received offset, the client will
probably retransmit them again, which will worsen the network
transmission.

3 years agoBUG/MINOR: cli: shows correct mode in "show sess"
William Lallemand [Tue, 8 Mar 2022 11:05:31 +0000 (12:05 +0100)] 
BUG/MINOR: cli: shows correct mode in "show sess"

The "show sess" cli command only handles "http" or "tcp" as a fallback
mode, replace this by a call to proxy_mode_str() to show all the modes.

Could be backported in every maintained versions.

3 years agoBUG/MINOR: add missing modes in proxy_mode_str()
William Lallemand [Tue, 8 Mar 2022 10:50:59 +0000 (11:50 +0100)] 
BUG/MINOR: add missing modes in proxy_mode_str()

Add the missing PR_MODE_SYSLOG and PR_MODE_PEERS in proxy_mode_str().

Could be backported in every maintained versions.

3 years agoMINOR: pools: add a new global option "no-memory-trimming"
Willy Tarreau [Tue, 8 Mar 2022 09:41:40 +0000 (10:41 +0100)] 
MINOR: pools: add a new global option "no-memory-trimming"

Some users with very large numbers of connections have been facing
extremely long malloc_trim() calls on reload that managed to trigger
the watchdog! That's a bit counter-productive. It's even possible
that some implementations are not perfectly reliable or that their
trimming time grows quadratically with the memory used. Instead of
constantly trying to work around these issues, let's offer an option
to disable this mechanism, since nobody had been complaining in the
past, and this was only meant to be an improvement.

This should be backported to 2.4 where trimming on reload started to
appear.

3 years agoBUG/MAJOR: quic: Wrong quic_max_available_room() returned value
Frédéric Lécaille [Fri, 4 Mar 2022 14:44:21 +0000 (15:44 +0100)] 
BUG/MAJOR: quic: Wrong quic_max_available_room() returned value

Around limits for QUIC integer encoding, this functions could return
wrong values which lead to qc_build_frms() to prepare wrong CRYPTO (less chances)
or STREAM frames (more chances). qc_do_build_pkt() could build wrong packets
with bad CRYPTO/STREAM frames which could not be decoded by the peer.
In such a case ngtcp2 closes the connection with an ENCRYPTION_ERROR error
in a transport CONNECTION_CLOSE frame.

3 years agoMINOR: quic: Add quic_max_int_by_size() function
Frédéric Lécaille [Fri, 4 Mar 2022 14:38:51 +0000 (15:38 +0100)] 
MINOR: quic: Add quic_max_int_by_size() function

This function returns the maximum integer which may be encoded with a number of
bytes passed as parameter. Useful to precisely compute the number of bytes which
may used to fulfill a buffer with lengths as QUIC enteger encoded prefixes for the
number of following bytes.

3 years agoCLEANUP: quic: Remove window redundant variable from NewReno algorithm state struct
Frédéric Lécaille [Thu, 3 Mar 2022 07:24:53 +0000 (08:24 +0100)] 
CLEANUP: quic: Remove window redundant variable from NewReno algorithm state struct

We use the window variable which is stored in the path struct.