]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
2 years agoMINOR: tasks: do not keep cpu and latency times in struct task
Willy Tarreau [Wed, 7 Sep 2022 07:17:45 +0000 (09:17 +0200)] 
MINOR: tasks: do not keep cpu and latency times in struct task

It was a mistake to put these two fields in the struct task. This
was added in 1.9 via commit 9efd7456e ("MEDIUM: tasks: collect per-task
CPU time and latency"). These fields are used solely by streams in
order to report the measurements via the lat_ns* and cpu_ns* sample
fetch functions when task profiling is enabled. For the rest of the
tasks, this is pure CPU waste when profiling is enabled, and memory
waste 100% of the time, as the point where these latencies and usages
are measured is in the profiling array.

Let's move the fields to the stream instead, and have process_stream()
retrieve the relevant info from the thread's context.

The struct task is now back to 120 bytes, i.e. almost two cache lines,
with 32 bit still available.

2 years agoBUG/MINOR: stream/sched: take into account CPU profiling for the last call
Willy Tarreau [Wed, 7 Sep 2022 14:17:49 +0000 (16:17 +0200)] 
BUG/MINOR: stream/sched: take into account CPU profiling for the last call

When task profiling is enabled, the reported CPU time for short requests
and responses (e.g. redirect) is always zero in the logs, because
process_stream() is only called once and the CPU time is measured after
it returns. This is particuarly annoying when dealing with denies and in
general anything that deals with parasitic traffic because it can be
difficult to figure where the CPU is spent.

The solution taken in this patch consists in having process_stream()
update the cpu time itself before logging and quitting. It's very simple.
It will not take into account the time taken to produce the log nor
freeing the stream, but that's marginal compared to always logging zero.
The task's wake_date is also reset so that the scheduler doesn't have to
perform these operations again. This is dependent on the following patch:

   MINOR: sched: store the current profile entry in the thread context

It should be backported to 2.6 as it does help for troubleshooting.

2 years agoMINOR: sched: store the current profile entry in the thread context
Willy Tarreau [Wed, 7 Sep 2022 14:16:39 +0000 (16:16 +0200)] 
MINOR: sched: store the current profile entry in the thread context

The profile entry that corresponds to the current task/tasklet being
profiled is now stored into the thread's context. This will allow it
to be accessed from the tasks themselves. This is needed for an upcoming
fix.

2 years agoBUG/MINOR: sched: properly account for the CPU time of dying tasks
Willy Tarreau [Wed, 7 Sep 2022 13:11:25 +0000 (15:11 +0200)] 
BUG/MINOR: sched: properly account for the CPU time of dying tasks

When task profiling is enabled, the scheduler can measure and report
the cumulated time spent in each task and their respective latencies. But
this was wrong for tasks with few wakeups as well as for self-waking ones,
because the call date needed to measure how long it takes to process the
task is retrieved in the task itself (->wake_date was turned to the call
date), and we could face two conditions:
  - a new wakeup while the task is executing would reset the ->wake_date
    field before returning and make abnormally low values being reported;
    that was likely the case for taskèrun_applet for self-waking applets;

  - when the task dies, NULL is returned and the call date couldn't be
    retrieved, so that CPU time was not being accounted for. This was
    particularly visible with process_stream() which is usually called
    only twice per request, and whose time was systematically halved.

The cleanest solution here is to keep in mind that the scheduler already
uses quite a bit of local context in th_ctx, and place the intermediary
values there so that they cannot vanish. The wake_date has to be reset
immediately once read, and only its copy is used along the function. Note
that this must be done both for tasks and tasklet, and that until recently
tasklets were also able to report wrong values due to their sole dependency
on TH_FL_TASK_PROFILING between tests.

One nice benefit for future improvements is that such information will now
be available from the task without having to be stored into the task itself
anymore.

Since the tasklet part was computed on wrapping 32-bit arithmetics and
the task one was on 64-bit, the values were now consistently moved to
32-bit as it's already largely sufficient (4s spent in a task is more
than twice what the watchdog would tolerate). Some further cleanups might
be necessary, but the patch aimed at staying minimal.

Task profiling output after 1 million HTTP request previously looked like
this:

  Tasks activity:
    function                      calls   cpu_tot   cpu_avg   lat_tot   lat_avg
    h1_io_cb                    2012338   4.850s    2.410us   12.91s    6.417us
    process_stream              2000136   9.594s    4.796us   34.26s    17.13us
    sc_conn_io_cb               2000135   1.973s    986.0ns   30.24s    15.12us
    h1_timeout_task                 137      -         -      2.649ms   19.34us
    accept_queue_process             49   152.3us   3.107us   321.7yr   6.564yr
    main+0x146430                     7   5.250us   750.0ns   25.92us   3.702us
    srv_cleanup_idle_conns            1   559.0ns   559.0ns   918.0ns   918.0ns
    task_run_applet                   1      -         -      2.162us   2.162us

  Now it looks like this:
  Tasks activity:
    function                      calls   cpu_tot   cpu_avg   lat_tot   lat_avg
    h1_io_cb                    2014194   4.794s    2.380us   13.75s    6.826us
    process_stream              2000151   20.01s    10.00us   36.04s    18.02us
    sc_conn_io_cb               2000148   2.167s    1.083us   32.27s    16.13us
    h1_timeout_task                 198   54.24us   273.0ns   3.487ms   17.61us
    accept_queue_process             52   158.3us   3.044us   409.9us   7.882us
    main+0x1466e0                    18   16.77us   931.0ns   63.98us   3.554us
    srv_cleanup_toremove_conns        8   282.1us   35.26us   546.8us   68.35us
    srv_cleanup_idle_conns            3   149.2us   49.73us   8.131us   2.710us
    task_run_applet                   3   268.1us   89.38us   11.61us   3.871us

Note the two-fold difference on process_stream().

This feature is essentially used for debugging so it has extremely limited
impact. However it's used quite a bit more in bug reports and it would be
desirable that at least 2.6 gets this fix backported. It depends on at least
these two previous patches which will then also have to be backported:

     MINOR: task: permanently enable latency measurement on tasklets
     CLEANUP: task: rename ->call_date to ->wake_date

2 years agoCLEANUP: task: rename ->call_date to ->wake_date
Willy Tarreau [Wed, 7 Sep 2022 12:49:50 +0000 (14:49 +0200)] 
CLEANUP: task: rename ->call_date to ->wake_date

This field is misnamed because its real and important content is the
date the task was woken up, not the date it was called. It temporarily
holds the call date during execution but this remains confusing. In
fact before the latency measurements were possible it was indeed a call
date. Thus is will now be called wake_date.

This change is necessary because a subsequent fix will require the
introduction of the real call date in the thread ctx.

2 years agoMINOR: task: permanently enable latency measurement on tasklets
Willy Tarreau [Tue, 6 Sep 2022 17:17:45 +0000 (19:17 +0200)] 
MINOR: task: permanently enable latency measurement on tasklets

When tasklet latency measurement was enabled in 2.4 with commit b2285de04
("MINOR: tasks: also compute the tasklet latency when DEBUG_TASK is set"),
the feature was conditionned on DEBUG_TASK because the field would add 8
bytes to the struct tasklet.

This approach was not a very good idea because the struct ends on an int
anyway thus it does finish with a 32-bit hole regardless of the presence
of this field. What is true however is that adding it turned a 64-byte
struct to 72-byte when caller debugging is enabled.

This patch revisits this with a minor change. Now only the lowest 32
bits of the call date are stored, so they always fit in the remaining
hole, and this allows to remove the dependency on DEBUG_TASK. With
debugging off, we're now seeing a 48-byte struct, and with debugging
on it's exactly 64 bytes, thus still exactly one cache line. 32 bits
allow a latency of 4 seconds on a tasklet, which already indicates a
completely dead process, so there's no point storing the upper bits at
all. And even in the event it would happen once in a while, the lost
upper bits do not really add any value to the debug reports. Also, now
one tasklet wakeup every 4 billion will not be sampled due to the test
on the value itself. Similarly we just don't care, it's statistics and
the measurements are not 9-digit accurate anyway.

2 years agoBUG/MINOR: task: make task_instant_wakeup() work on a task not a tasklet
Willy Tarreau [Tue, 6 Sep 2022 14:31:30 +0000 (16:31 +0200)] 
BUG/MINOR: task: make task_instant_wakeup() work on a task not a tasklet

There's a subtle (harmless) bug in task_instant_wakeup(). As it uses
some tasklet code instead of some task code, the debug part also acts
on the tasklet equivalent, and the call_date is only set when DEBUG_TASK
is set instead of inconditionally like with tasks. As such, without this
debugging macro, call dates are not updated for tasks woken this way.

There isn't any impact yet because this function was introduced in 2.6 to
solve certain classes of issues and is not used yet, and in the worst case
it would only affect the reported latency time.

This may be backported to 2.6 in case a future fix would depend on it but
currently will not fix existing code.

2 years agoBUG/MINOR: task: always reset a new tasklet's call date
Willy Tarreau [Tue, 6 Sep 2022 17:06:52 +0000 (19:06 +0200)] 
BUG/MINOR: task: always reset a new tasklet's call date

The tasklet's call date was not reset, so if profiling was enabled while
some tasklets were in the run queue, their initial random value could be
used to preload a bogus initial latency value into the task profiling bin.
Let's just zero the initial value.

This should be backported to 2.4 as it was brought with initial commit
b2285de04 ("MINOR: tasks: also compute the tasklet latency when DEBUG_TASK
is set"). The impact is very low though.

2 years agoBUG/MINOR: quic: Wrong connection ID to thread ID association
Frédéric Lécaille [Wed, 7 Sep 2022 13:06:52 +0000 (15:06 +0200)] 
BUG/MINOR: quic: Wrong connection ID to thread ID association

To work, quic_pin_cid_to_tid() must set cid[0] to a value with <target_id>
as <global.nbthread> modulo. For each integer n, (n - (n % m)) + d has always
d as modulo m (with d < m).

So, this statement seemed correct:

     cid[0] = cid[0] - (cid[0] % global.nbthread) + target_tid;

except when n wraps or when another modulo is applied to the addition result.
Here, for 8bit modulo arithmetic, if m does not divides 256, this cannot
works for values which wraps when we increment them by d.
For instance n=255 m=3 and d=1 the formula result is 0 (should be d).

To fix this, we first limit c[0] to 255 - <target_id> to prevent c[0] from wrapping.

Thank you to @esb for having reported this issue in GH #1855.

Must be backported to 2.6

2 years agoMINOR: quic: No TRACE_LEAVE() in retrieve_qc_conn_from_cid()
Frédéric Lécaille [Wed, 7 Sep 2022 10:15:43 +0000 (12:15 +0200)] 
MINOR: quic: No TRACE_LEAVE() in retrieve_qc_conn_from_cid()

This macro was confused with TRACE_ENTER().

Should be backported to 2.6.

2 years agoMINOR: quic: Add traces about sent or resent TX frames
Frédéric Lécaille [Fri, 2 Sep 2022 20:16:10 +0000 (22:16 +0200)] 
MINOR: quic: Add traces about sent or resent TX frames

Very useful to help in debugging issues, especially during retransmissions.

Should be backported to 2.6

2 years agoMINOR: quic: add QUIC support when no client_hello_cb
William Lallemand [Wed, 7 Sep 2022 09:21:34 +0000 (11:21 +0200)] 
MINOR: quic: add QUIC support when no client_hello_cb

Add QUIC support to the ssl_sock_switchctx_cbk() variant used only when
no client_hello_cb is available.

This could be used with libreSSL implementation of QUIC for example.
It also works with quictls when HAVE_SSL_CLIENT_HELLO_CB is removed from
openss-compat.h

2 years agoBUILD: quic: fix the #ifdef in ssl_quic_initial_ctx()
William Lallemand [Wed, 7 Sep 2022 09:11:59 +0000 (11:11 +0200)] 
BUILD: quic: fix the #ifdef in ssl_quic_initial_ctx()

As done on with ssl_sock_initial_ctx(), cleanup the ifdef for the
client_hello_cb and the no anti replay.

2 years agoBUILD: ssl: fix the ifdef mess in ssl_sock_initial_ctx
William Lallemand [Wed, 7 Sep 2022 08:54:17 +0000 (10:54 +0200)] 
BUILD: ssl: fix the ifdef mess in ssl_sock_initial_ctx

ssl_sock_initial_ctx uses the wrong #ifdef to check the availability of
the client_hello_cb.

Cleanup the #ifdef, add comments and indentation.

2 years agoBUILD: quic: enable early data only with >= openssl 1.1.1
William Lallemand [Fri, 2 Sep 2022 14:24:39 +0000 (16:24 +0200)] 
BUILD: quic: enable early data only with >= openssl 1.1.1

Disable the early data in the QUIC code when not built with openssl >=
1.1.1.

LibreSSL 3.6.0 is impacted.

2 years agoBUILD: quic: temporarly ignore chacha20_poly1305 for libressl
William Lallemand [Fri, 2 Sep 2022 13:35:09 +0000 (15:35 +0200)] 
BUILD: quic: temporarly ignore chacha20_poly1305 for libressl

LibreSSL does not implement EVP_chacha20_poly1305() with EVP_CIPHER but
uses the EVP_AEAD API instead:

https://man.openbsd.org/EVP_AEAD_CTX_init

This patch disables this cipher for libreSSL for now.

2 years agoBUILD: ssl: fix ssl_sock_switchtx_cbk when no client_hello_cb
William Lallemand [Fri, 2 Sep 2022 13:27:32 +0000 (15:27 +0200)] 
BUILD: ssl: fix ssl_sock_switchtx_cbk when no client_hello_cb

When building HAProxy with USE_QUIC and libressl 3.6.0, the
ssl_sock_switchtx_cbk symbol is not found because libressl does not
implement the client_hello_cb.

A ssl_sock_switchtx_cbk version for the servername callback is available
but wasn't exported correctly.

2 years agoBUILD: quic: add some ifdef around the SSL_ERROR_* for libressl
William Lallemand [Thu, 1 Sep 2022 14:15:10 +0000 (16:15 +0200)] 
BUILD: quic: add some ifdef around the SSL_ERROR_* for libressl

SSL_ERROR_WANT_ASYNC, SSL_ERROR_WANT_ASYNC_JOB and
SSL_ERROR_WANT_CLIENT_HELLO_CB does not seems supported by libressl.

2 years agoBUG/MINOR: quic: Possible crash when verifying certificates
Frédéric Lécaille [Tue, 6 Sep 2022 17:37:08 +0000 (19:37 +0200)] 
BUG/MINOR: quic: Possible crash when verifying certificates

This verification is done by ssl_sock_bind_verifycbk() which is set at different
locations in the ssl_sock.c code . About QUIC connections, there are a lot of chances
the connection object is not initialized when entering this function. What must
be accessed is the SSL object to retrieve the connection or quic_conn objects,
then the bind_conf object of the listener. If the connection object is not found,
we try to find the quic_conn object.

Modify ssl_sock_dump_errors() interface which takes a connection object as parameter
to also passed a quic_conn object as parameter. Again this function try first
to access the connection object if not NULL or the quic_conn object if not.

There is a remaining thing to do for QUIC: store the certificate verification error
code as it is currently stored in the connection object. This error code is at least
used by the "bc_err" and "fc_err" sample fetches.

There are chances this bug is in relation with GH #1851. Thank you to @tasavis
for the report.

Must be merged into 2.6.

2 years agoBUG/MINOR: h1: Support headers case adjustment for TCP proxies
Christopher Faulet [Tue, 6 Sep 2022 08:09:40 +0000 (10:09 +0200)] 
BUG/MINOR: h1: Support headers case adjustment for TCP proxies

On frontend side, "h1-case-adjust-bogus-client" option is now supported in
TCP mode. It is important to be able to adjust the case of response headers
when a connection is routed to an HTTP backend. In this case, the client
connection is upgraded to H1.

On backend side, "h1-case-adjust-bogus-server" option is now also supported
in TCP mode to be able to perform HTTP health-checks with a case adjustment
of the request headers.

This patch should be backported as far as 2.0.

2 years agoMINOR: http-check: Remove support for headers/body in "option httpchk" version
Christopher Faulet [Mon, 5 Sep 2022 07:05:17 +0000 (09:05 +0200)] 
MINOR: http-check: Remove support for headers/body in "option httpchk" version

This trick is deprecated since the health-check refactoring, It is now
invalid. It means the following line will trigger an error during the
configuration parsing:

  option httpchk OPTIONS * HTTP/1.1\r\nHost:\ www

It must be replaced by:

  option httpchk OPTIONS * HTTP/1.1
  http-check send hdr Host www

2 years agoBUG/MINOR: quic: Possible crash with "tls-ticket-keys" on QUIC bind lines
Frédéric Lécaille [Tue, 6 Sep 2022 15:04:55 +0000 (17:04 +0200)] 
BUG/MINOR: quic: Possible crash with "tls-ticket-keys" on QUIC bind lines

ssl_tlsext_ticket_key_cb() is called when "tls-ticket-keys" option is used on a
"bind" line. It needs to have an access to the TLS ticket keys which have been
stored into the listener bind_conf struct. The fix consists in nitializing the
<ref> variable (references to TLS secret keys) the correct way when this callback
is called for a QUIC connection. The bind_conf struct is store into the quic_conn
object (QUIC connection).

This issue may be in relation with GH #1851. Thank you for @tasavis for the report.

Must be backported to 2.6.

2 years agoBUG/MINOR: quic: Retransmitted frames marked as acknowledged
Frédéric Lécaille [Tue, 6 Sep 2022 07:55:21 +0000 (09:55 +0200)] 
BUG/MINOR: quic: Retransmitted frames marked as acknowledged

Obviously, frames which are duplicated from others must not be retransmitted if
the original frame they were duplicated from was already acknowledged.
This should have been detected by qc_build_frms() which skips such frames,
except if the QUIC xprt does really bad things which are not supported by
the upper layer. This will have to be checked with Amaury.

To prevent the retransmision of these frames which leads to crashes as reported by
hpn0t0ad this gdb backtrace in GH #1809 where the frame builder tries to copy a huge
number of bytes to the packet buffer:

Thread 7 (Thread 0x7fddf373a700 (LWP 13)):
 #0  __memmove_sse2_unaligned_erms () at ../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:520
No locals.
 #1  0x000055b17435705e in quic_build_stream_frame (buf=0x7fddf372ef78, end=<optimized out>, frm=0x7fdde08d3470, conn=<optimized out>) at src/quic_frame.c:515
        to_copy = 18446697703428890384
        stream = 0x7fdde08d3490
        wrap = <optimized out>

which matches this part of quic_frame.c code:

    wrap = (const unsigned char *)b_wrap(stream->buf);
    if (stream->data + stream->len > wrap) {
        size_t to_copy = wrap - stream->data;
        memcpy(*buf, stream->data, to_copy);
        *buf += to_copy;

we release as soon as possible the impacted frames as there is really no need
to retransmit such frames.

Thank you to @hpn0t0ad for having provided us with useful traces in github
issue #1809.

Must be backported in 2.6.

2 years agoBUILD: makefile: enable crypt(3) for NetBSD
Brad Smith [Sat, 13 Aug 2022 04:57:31 +0000 (00:57 -0400)] 
BUILD: makefile: enable crypt(3) for NetBSD

Allow NetBSD to support encrypted passwords in Userlists.

2 years agoMINOR: Revert part of clarifying samples support per os commit
Brad Smith [Fri, 26 Aug 2022 03:13:38 +0000 (23:13 -0400)] 
MINOR: Revert part of clarifying samples support per os commit

Commit 5c83e3a1563cd7face299bf08037e51f976eb5e3 made some adjustments
to clarify which TCP_INFO information is supported by each respective
OS.

There was a comment like so..

Note that fc_rtt and fc_rttvar are supported on any OS that has TCP_INFO,
not just linux/freebsd/netbsd, so we continue to expose them unconditionally.

But the diff didn't do so in a consistent manner.

2 years ago[RELEASE] Released version 2.7-dev5 v2.7-dev5
Willy Tarreau [Fri, 2 Sep 2022 17:36:50 +0000 (19:36 +0200)] 
[RELEASE] Released version 2.7-dev5

Released version 2.7-dev5 with the following main changes :
    - BUG/MINOR: mux-quic: Fix memleak on QUIC stream buffer for unacknowledged data
    - BUG/MEDIUM: cpu-map: fix thread 1's affinity affecting all threads
    - MINOR: cpu-map: remove obsolete diag warning about combined ranges
    - BUG/MAJOR: mworker: fix infinite loop on master with no proxies.
    - REGTESTS: launch http_reuse_always in mworker mode
    - BUG/MINOR: quix: Memleak for non in flight TX packets
    - BUG/MINOR: quic: Wrong list_for_each_entry() use when building packets from qc_do_build_pkt()
    - BUG/MINOR: quic: Safer QUIC frame builders
    - MINOR: quic: Replace MT_LISTs by LISTs for RX packets.
    - BUG/MEDIUM: applet: fix incorrect check for abnormal return condition from handler
    - BUG/MINOR: applet: make the call_rate only count the no-progress calls
    - MEDIUM: peers: limit the number of updates sent at once
    - BUILD: tcp_sample: fix build of get_tcp_info() on OpenBSD
    - BUG/MINOR: resolvers: return the correct value in resolvers_finalize_config()
    - BUG/MINOR: mworker: does not create the "default" resolvers in wait mode
    - BUG/MINOR: tcpcheck: Disable QUICKACK only if data should be sent after connect
    - REGTESTS: Fix prometheus script to perform HTTP health-checks
    - MINOR: resolvers: shut the warning when "default" resolvers is implicit
    - Revert "BUG/MINOR: quix: Memleak for non in flight TX packets"
    - BUG/MINOR: quic: Leak in qc_release_lost_pkts() for non in flight TX packets
    - BUG/MINOR: quic: Stalled connections (missing I/O handler wakeup)
    - CLEANUP: quic: No more use ->rx_list MT_LIST entry point (quic_rx_packet)
    - CLEANUP: quic: Remove a useless check in qc_lstnr_pkt_rcv()
    - MINOR: quic: Remove useless traces about references to TX packets
    - Revert "MINOR: quic: Remove useless traces about references to TX packets"
    - DOC: configuration: do-resolve doesn't work with a port in the string
    - MINOR: sample: add the host_only and port_only converters
    - BUG/MINOR: httpclient: fix resolution with port
    - DOC: configuration.txt: do-resolve must use host_only to remove its port.
    - BUG/MINOR: quic: Null packet dereferencing from qc_dup_pkt_frms() trace
    - BUG/MINOR: quic: Frames added to packets even if not built.
    - BUG/MEDIUM: spoe: Properly update streams waiting for a ACK in async mode
    - BUG/MEDIUM: peers: Add connect and server timeut to peers proxy
    - BUG/MEDIUM: peers: Don't use resync timer when local resync is in progress
    - BUG/MEDIUM: peers: Don't start resync on reload if local peer is not up-to-date
    - BUG/MINOR: hlua: Rely on CF_EOI to detect end of message in HTTP applets
    - BUG/MEDIUM: mux-h1: do not refrain from signaling errors after end of input
    - BUG/MINOR: epoll: do not actively poll for Rx after an error
    - MINOR: raw-sock: don't try to send if an error was already reported
    - BUG/MINOR: quic: Missing header protection AES cipher context initialisations (draft-v2)
    - MINOR: quic: Add a trace to distinguish the datagram from the packets inside
    - BUG/MINOR: ssl: fix deinit of the ca-file tree
    - BUG/MINOR: ssl: leak of ckch_inst_link in ckch_inst_free()
    - BUG/MINOR: tcpcheck: Disable QUICKACK for default tcp-check (with no rule)
    - BUG/MEDIUM: ssl: Fix a UAF when old ckch instances are released
    - BUG/MINOR: ssl: revert two wrong fixes with ckhi_link
    - BUG/MINOR: dev/udp: properly preset the rx address size
    - BUILD: debug: make sure debug macros are never empty
    - MINOR: quic: Move traces about RX/TX bytes from QUIC_EV_CONN_PRSAFRM event
    - BUG/MINOR: quic: TX frames memleak
    - BUG/MINOR: ssl: leak of ckch_inst_link in ckch_inst_free() v2
    - MINOR: sink/ring: rotate non-empty file-backed contents only
    - BUG/MINOR: regex: Properly handle PCRE2 lib compiled without JIT support
    - REGTESTS: http_request_buffer: Add a barrier to not mix up log messages
    - BUG/MEDIUM: mux-h1: always use RST to kill idle connections in pools
    - MINOR: backend: always satisfy the first req reuse rule with l7 retries
    - BUG/MINOR: quic: Do not ack when probing
    - MINOR: quic: Add TX frames addresses to traces to several trace events
    - MINOR: quic: Trace typo fix in qc_release_frm()
    - BUG/MINOR: quic: Frames leak during retransmissions
    - BUG/MINOR: h2: properly set the direction flag on HTX response
    - BUG/MEDIUM: httpclient: always detach the caller before self-killing
    - BUG/MINOR: httpclient: only ask for more room on failed writes
    - BUG/MINOR: httpclient: keep-alive was accidentely disabled
    - MEDIUM: httpclient: enable ALPN support on outgoing https connections
    - BUG/MINOR: mux-h2: fix the "show fd" dest buffer for the subscriber
    - BUG/MINOR: mux-h1: fix the "show fd" dest buffer for the subscriber
    - BUG/MINOR: mux-fcgi: fix the "show fd" dest buffer for the subscriber
    - DEBUG: stream: minor rearrangement of a few fields in struct stream.
    - MINOR: debug: report applet pointer and handler in crashes when known
    - MINOR: mux-h2: extract the stream dump function out of h2_show_fd()
    - MINOR: mux-h2: extract the connection dump function out of h2_show_fd()
    - MINOR: muxes: add a "show_sd" helper to complete "show sess" dumps
    - MINOR: mux-h2: provide a "show_sd" helper to output stream debugging info
    - MINOR: mux-h2: insert line breaks in "show sess all" output for legibility
    - MINOR: mux-quic: provide a "show_sd" helper to output stream debugging info
    - MINOR: mux-h1: split "show_fd" into connection and stream
    - MINOR: mux-h1: provide a "show_sd" helper to output stream debugging info
    - BUG/MINOR: http-act: initialize http fmt head earlier

2 years agoBUG/MINOR: http-act: initialize http fmt head earlier
Willy Tarreau [Fri, 2 Sep 2022 17:19:01 +0000 (19:19 +0200)] 
BUG/MINOR: http-act: initialize http fmt head earlier

In github issue #1850, Christian Ruppert reported a case of crash in
2.6 when failing to parse some http rules. This started to happen
with 2.6 commit dd7e6c6 ("BUG/MINOR: http-rules: completely free
incorrect TCP rules on error") but has some of its roots in 2.2
commit 2eb539687 ("MINOR: http-rules: Add release functions for
existing HTTP actions").

The cause is that when the release function is set for HTTP actions,
the rule->arg.http.fmt list head is not yet initialized, hence is
NULL, thus the release function crashes when it tries to iterate over
it. In fact this code was initially not written with the perspective
of releasing such elements upon error, so the arg list initialization
happened after error checking.

This patch just moves the list initialization just after setting the
release pointer and that's OK.

This patch must be backported to 2.6 since the problem is visible
there. It could be backported to 2.5 but the issue is not triggered
there without the first mentioned patch above that landed in 2.6, so
it will not bring any obvious benefit.

2 years agoMINOR: mux-h1: provide a "show_sd" helper to output stream debugging info
Willy Tarreau [Fri, 2 Sep 2022 14:32:31 +0000 (16:32 +0200)] 
MINOR: mux-h1: provide a "show_sd" helper to output stream debugging info

With this, it now becomes possible to see the state of each H1 stream from
"show sess all". Example (added lines highlighted with '>'):

  0x7fc9b40460a0: [02/Sep/2022:16:29:31.267228] id=49 proto=tcpv4 source=127.0.0.1:53548
    flags=0xc4a, conn_retries=0, conn_exp=<NEVER> conn_et=0x000 srv_conn=0x2dc4b20, pend_pos=(nil) waiting=0 epoch=0
    frontend=decrypt (id=2 mode=http), listener=? (id=3) addr=127.0.0.1:8001
    backend=decrypt (id=2 mode=http) addr=127.0.0.1:25168
    server=httpterm (id=1) addr=127.0.0.1:8000
    task=0x7fc9b4046490 (state=0x00 nice=0 calls=4 rate=0 exp=3s tid=7(1/7) age=2s)
    txn=0x7fc9b4046650 flags=0x3000 meth=1 status=200 req.st=MSG_DONE rsp.st=MSG_DATA req.f=0x4c rsp.f=0x0d
    scf=0x7fc9b4046030 flags=0x00000080 state=EST endp=CONN,0x7fc9b4041f00,0x02804001 sub=1
>       h1s=0x7fc9b4041f00 h1s.flg=0x104010 .sd.flg=0x2804001 .req.state=MSG_DONE .res.state=MSG_DATA
>       .meth=GET status=200 .sd.flg=0x02804001 .sc.flg=0x00000080 .sc.app=0x7fc9b40460a0
>       .subs=0x7fc9b4046040(ev=1 tl=0x7fc9b4046540 tl.calls=9 tl.ctx=0x7fc9b4046030 tl.fct=sc_conn_io_cb)
>       h1c=0x7fc9b402b3f0 h1c.flg=0x302200 .sub=1 .ibuf=0@(nil)+0/0 .obuf=0@(nil)+0/0
        co0=0x7fc9bc02e740 ctrl=tcpv4 xprt=RAW mux=H1 data=STRM target=LISTENER:0x2dc3c40
        flags=0x00000300 fd=79 fd.state=421 updt=0 fd.tmask=0x80
    scb=0x7fc9b4046590 flags=0x00000011 state=EST endp=CONN,0x7fc9b4048660,0x02840001 sub=0
>       h1s=0x7fc9b4048660 h1s.flg=0x4010 .sd.flg=0x2840001 .req.state=MSG_DONE .res.state=MSG_DATA
>       .meth=GET status=200 .sd.flg=0x02840001 .sc.flg=0x00000011 .sc.app=0x7fc9b40460a0 .subs=(nil)
>       h1c=0x7fc9b4048490 h1c.flg=0x80002200 .sub=0 .ibuf=0@(nil)+0/0 .obuf=0@(nil)+0/0
        co1=0x7fc9b4048270 ctrl=tcpv4 xprt=RAW mux=H1 data=STRM target=SERVER:0x2dc4b20
        flags=0x00000300 fd=131 fd.state=10122 updt=0 fd.tmask=0x80
    req=0x7fc9b40460c0 (f=0x49c40080 an=0x8000 pipe=0 tofwd=0 total=56)
        an_exp=<NEVER> rex=<NEVER> wex=<NEVER>
        buf=0x7fc9b40460c8 data=(nil) o=0 p=0 i=0 size=0
        htx=0xdd90a0 flags=0x0 size=0 data=0 used=0 wrap=NO extra=0
    res=0x7fc9b4046120 (f=0x80070202 an=0x4000000 pipe=0 tofwd=-1 total=603840788)
        an_exp=<NEVER> rex=<NEVER> wex=<NEVER>
        buf=0x7fc9b4046128 data=(nil) o=0 p=0 i=0 size=0
        htx=0xdd90a0 flags=0x0 size=0 data=0 used=0 wrap=NO extra=0

2 years agoMINOR: mux-h1: split "show_fd" into connection and stream
Willy Tarreau [Fri, 2 Sep 2022 14:11:28 +0000 (16:11 +0200)] 
MINOR: mux-h1: split "show_fd" into connection and stream

We now have two functions, one for dumping connections and the other
one for dumping the streams. This will permit to use it from show_sd.
A few optional line breaks were inserted where relevant to keep lines
homogenous when a prefix is passed.

2 years agoMINOR: mux-quic: provide a "show_sd" helper to output stream debugging info
Willy Tarreau [Fri, 2 Sep 2022 14:00:40 +0000 (16:00 +0200)] 
MINOR: mux-quic: provide a "show_sd" helper to output stream debugging info

It's very limited but at least provides the very basic info about QCS and
QCC when issuing "show sess all":

  scf=0x7fa9642394a0 flags=0x00000080 state=EST endp=CONN,0x7fa9642351f0,0x02001001 sub=3
>     qcs=0x7fa9642351f0 .flg=0x5 .id=396 .st=HCR .ctx=0x7fa9642353f0, .err=0
>     qcc=0x7fa96405ce20 .flg=0 .nbsc=100 .nbhreq=100, .task=0x7fa964054260
      co0=0x7fa96405cd50 ctrl=quic4 xprt=QUIC mux=QUIC data=STRM target=LISTENER:0x328c530
      flags=0x00200300 fd=-1 fd.state=00 updt=0 fd.tmask=0x0

It will need to be improved but it's better than nothing already. This
should be backported to 2.6 if the other dumps are backported.

2 years agoMINOR: mux-h2: insert line breaks in "show sess all" output for legibility
Willy Tarreau [Fri, 2 Sep 2022 13:22:12 +0000 (15:22 +0200)] 
MINOR: mux-h2: insert line breaks in "show sess all" output for legibility

h2s and h2c were extremely long in the "show sess all" output, around 300
chars each. This adds a few line breaks to improve legibility, there are
now 3 lines for each, which are around the same length as the other ones
while keeping a natural arrangement. E.g (lines highlighted with '>'):

  0x7faad8144f80: [02/Sep/2022:15:49:40.171620] id=105283 proto=tcpv4 source=127.0.0.1:42942
    flags=0x100c4a, conn_retries=0, conn_exp=<NEVER> conn_et=0x000 srv_conn=0x1f44b20, pend_pos=(nil) waiting=0 epoch=0
    frontend=decrypt (id=2 mode=http), listener=? (id=3) addr=127.0.0.1:8001
    backend=decrypt (id=2 mode=http) addr=127.0.0.1:18144
    server=httpterm (id=1) addr=127.0.0.1:8000
    task=0x7faad812b7c0 (state=0x00 nice=0 calls=2 rate=0 exp=4s tid=7(1/7) age=0s)
    txn=0x7faad81453e0 flags=0x43000 meth=1 status=200 req.st=MSG_DONE rsp.st=MSG_DATA req.f=0x4c rsp.f=0x0d
    scf=0x7faad81625d0 flags=0x00000080 state=EST endp=CONN,0x7faad811d380,0x02805001 sub=1
>       h2s=0x7faad811d380 h2s.id=2113 .st=HCR .flg=0x207001 .rxbuf=0@(nil)+0/0
>       .sc=0x7faad81625d0(.flg=0x00000080 .app=0x7faad8144f80) .sd=0x7faad8119dc0(.flg=0x02805001)
>       .subs=0x7faad81625e0(ev=1 tl=0x7faad86d6500 tl.calls=4 tl.ctx=0x7faad81625d0 tl.fct=sc_conn_io_cb)
>       h2c=0x7faad802c640 h2c.st0=FRH .err=0 .maxid=2157 .lastid=-1 .flg=0x0600 .nbst=1 .nbsc=1
>       .fctl_cnt=0 .send_cnt=0 .tree_cnt=1 .orph_cnt=0 .sub=1 .dsi=2157 .dbuf=0@(nil)+0/0
>       .msi=-1 .mbuf=[6..6|32],h=[0@(nil)+0/0],t=[0@(nil)+0/0]
        co0=0x7faae402efc0 ctrl=tcpv4 xprt=RAW mux=H2 data=STRM target=LISTENER:0x1f43c40
        flags=0x00000300 fd=95 fd.state=121 updt=0 fd.tmask=0x80
    scb=0x7faad8145370 flags=0x00000011 state=EST endp=CONN,0x7faad8115630,0x02840001 sub=1
        co1=0x7faad86c0730 ctrl=tcpv4 xprt=RAW mux=H1 data=STRM target=SERVER:0x1f44b20
        flags=0x00000300 fd=1656 fd.state=10121 updt=0 fd.tmask=0x80
    req=0x7faad8144fa0 (f=0x49c40000 an=0x8000 pipe=0 tofwd=0 total=110)
        an_exp=<NEVER> rex=<NEVER> wex=<NEVER>
        buf=0x7faad8144fa8 data=(nil) o=0 p=0 i=0 size=0
        htx=0xdd90a0 flags=0x0 size=0 data=0 used=0 wrap=NO extra=0
    res=0x7faad8145000 (f=0x80040202 an=0x4000000 pipe=0 tofwd=-1 total=60365)
        an_exp=<NEVER> rex=<NEVER> wex=<NEVER>
        buf=0x7faad8145008 data=(nil) o=0 p=0 i=0 size=0
        htx=0xdd90a0 flags=0x0 size=0 data=0 used=0 wrap=NO extra=0

2 years agoMINOR: mux-h2: provide a "show_sd" helper to output stream debugging info
Willy Tarreau [Fri, 2 Sep 2022 13:11:40 +0000 (15:11 +0200)] 
MINOR: mux-h2: provide a "show_sd" helper to output stream debugging info

With this, it now becomes possible to see the state of each H2 stream from
"show sess all". Lines are still too long and need to be split, but that's
for another patch.

2 years agoMINOR: muxes: add a "show_sd" helper to complete "show sess" dumps
Willy Tarreau [Fri, 2 Sep 2022 13:00:48 +0000 (15:00 +0200)] 
MINOR: muxes: add a "show_sd" helper to complete "show sess" dumps

This helper will be called for muxes that provide it and will be used
to let the mux provide extra information about the stream attached to
a stream descriptor. A line prefix is passed in argument so that the
mux is free to break long lines without breaking indent. No prefix
means no line breaks should be produced (e.g. for short dumps).

2 years agoMINOR: mux-h2: extract the connection dump function out of h2_show_fd()
Willy Tarreau [Thu, 1 Sep 2022 17:25:57 +0000 (19:25 +0200)] 
MINOR: mux-h2: extract the connection dump function out of h2_show_fd()

The function will be reusable to dump connections, so let's extract it.

2 years agoMINOR: mux-h2: extract the stream dump function out of h2_show_fd()
Willy Tarreau [Thu, 1 Sep 2022 17:06:44 +0000 (19:06 +0200)] 
MINOR: mux-h2: extract the stream dump function out of h2_show_fd()

The function will be reusable to dump streams, so let's extract it.
Note that due to "last_h2s" being originally printed as a prefix for
the stream dump, now the pointer is displayed by the caller instead.

2 years agoMINOR: debug: report applet pointer and handler in crashes when known
Willy Tarreau [Fri, 2 Sep 2022 07:13:12 +0000 (09:13 +0200)] 
MINOR: debug: report applet pointer and handler in crashes when known

When an appctx is found looping over itself, we report a number of info
but not the pointers to the definition nor the handler, which can be quite
handy in some cases. Let's add them and try to decode the symbol.

2 years agoDEBUG: stream: minor rearrangement of a few fields in struct stream.
Willy Tarreau [Fri, 2 Sep 2022 13:42:33 +0000 (15:42 +0200)] 
DEBUG: stream: minor rearrangement of a few fields in struct stream.

Some recent traces started to show confusing stream pointers ending with
0xe. The reason was that the stream's obj_type was almost unused in the
past and was stuffed in a hole in the structure. But now it's present in
all "show sess all" outputs and having to mentally match this value against
another one that's 0x17e lower is painful. The solution here is to move the
obj_type at the top, like in almost every other structure, but without
breaking the efficient layout.

This patch moves a few fields around and manages to both plug some holes
(16 bytes saved, 976 to 960) and avoid channels needlessly crossing cache
boundaries (res was spread over 3 lines vs 2 now).

Nothing else was changed. It would be desirable to backport this to 2.6
since it's where dumps are currently being processed the most.

2 years agoBUG/MINOR: mux-fcgi: fix the "show fd" dest buffer for the subscriber
Willy Tarreau [Fri, 2 Sep 2022 12:22:38 +0000 (14:22 +0200)] 
BUG/MINOR: mux-fcgi: fix the "show fd" dest buffer for the subscriber

Commit 1776ffb97 ("MINOR: mux-fcgi: make the "show fd" helper also decode
the fstrm subscriber when known") improved the output of "show fd" for the
FCGI mux, but the output is sent to the trash buffer instead of the msg
argument. It turns out that this has no effect right now as the caller
passes the trash but this is risky.

This should be backported to 2.4.

2 years agoBUG/MINOR: mux-h1: fix the "show fd" dest buffer for the subscriber
Willy Tarreau [Fri, 2 Sep 2022 12:21:05 +0000 (14:21 +0200)] 
BUG/MINOR: mux-h1: fix the "show fd" dest buffer for the subscriber

Commit 150c4f8b7 ("MINOR: mux-h1: make the "show fd" helper also decode
the h1s subscriber when known") improved the output of "show fd" for the
H1 mux, but the output is sent to the trash buffer instead of the msg
argument. It turns out that this has no effect right now as the caller
passes the trash but this is risky.

This should be backported to 2.4.

2 years agoBUG/MINOR: mux-h2: fix the "show fd" dest buffer for the subscriber
Willy Tarreau [Thu, 1 Sep 2022 16:02:15 +0000 (18:02 +0200)] 
BUG/MINOR: mux-h2: fix the "show fd" dest buffer for the subscriber

Commit 98e40b981 ("MINOR: mux-h2: make the "show fd" helper also decode
the h2s subscriber when known") improved the output of "show fd" for the
H2 mux, but the output is sent to the trash buffer instead of the msg
argument. It turns out that this has no effect right now as the caller
passes the trash but this is risky.

This should be backported to 2.4.

2 years agoMEDIUM: httpclient: enable ALPN support on outgoing https connections
Willy Tarreau [Fri, 2 Sep 2022 07:02:21 +0000 (09:02 +0200)] 
MEDIUM: httpclient: enable ALPN support on outgoing https connections

Since everything is available for this, let's enable ALPN with the
usual "h2,http/1.1" on the https server. This will allow HTTPS requests
to use HTTP/2 when available.

It may be needed to permit to disable this (or to set the string) in
case some client code explicitly checks for the "HTTP/1.1" string, but
since httpclient is quite young it's unlikely that such code already
exists.

2 years agoBUG/MINOR: httpclient: keep-alive was accidentely disabled
Willy Tarreau [Fri, 19 Aug 2022 15:20:43 +0000 (17:20 +0200)] 
BUG/MINOR: httpclient: keep-alive was accidentely disabled

The servers were not set with default settings, meaning that a few
settings including the pool_max_delay were not set, thus disabling
connection pools, which is the cause of the fact that keep-alive was
disabled as reported in issue #1831. There might possibly be other
issues pending since all these fields were left to zero.

Note that this patch alone will not fix keep-alive because the applet
does not enforce SE_FL_NOT_FIRST and relies on the default http-reuse
safe, thus if servers are not shared, all requests are considered
first ones and do not reuse existing connections.

In 2.7, commit ecb40b2c3 ("MINOR: backend: always satisfy the first
req reuse rule with l7 retries") addressed this in a more elegant way
by fixing http-reuse to take into account the fact that properly
configured l7 retries provide exactly the capability that reuse safe
was trying to cover, and this patch is suitable for backporting.

This patch should be backported to 2.6 only.

2 years agoBUG/MINOR: httpclient: only ask for more room on failed writes
Willy Tarreau [Fri, 2 Sep 2022 09:42:50 +0000 (11:42 +0200)] 
BUG/MINOR: httpclient: only ask for more room on failed writes

There's a tiny issue in the I/O handler by which both a failed request
emission and missing response data will want to subscribe for more room
on output. That's not correct in that only the case where the request
buffer is full should cause this, the other one should just wait for
incoming data. This could theoretically cause spurious wakeups at
certain key points (e.g. connect() time maybe) though this could not
be reproduced but better fix this while it's easy enough.

It doesn't seem necessary to backport it right now, though this may
have to in case a concrete reproducible case is discovered.

2 years agoBUG/MEDIUM: httpclient: always detach the caller before self-killing
Willy Tarreau [Thu, 1 Sep 2022 18:40:26 +0000 (20:40 +0200)] 
BUG/MEDIUM: httpclient: always detach the caller before self-killing

If the caller dies before the server responds, the httpclient can crash
in hc_cli_res_end_cb() when unregistering because it dereferences
hc->caller which was already freed during the caller's unregistration.
The easiest way to reproduce it is by sending twice the following
request on the same CLI connection in expert mode, with httpterm
running on local port 8000:

   httpclient GET http://127.0.0.1:8000/?t=600

Note the 600ms delay that's larger than socat's default 500.

The code checks for a NULL everywhere hc->caller is used, but the NULL
was forgotten in this specific case. It must be placed in the second
half of httpclient_stop_and_destroy() which is responsible for signaling
the client that the caller leaves.

This must be backported to 2.6.

2 years agoBUG/MINOR: h2: properly set the direction flag on HTX response
Willy Tarreau [Fri, 2 Sep 2022 09:15:37 +0000 (11:15 +0200)] 
BUG/MINOR: h2: properly set the direction flag on HTX response

In 1.9-dev, a new flag was introduced on the start line with commit
f1ba18d7b ("MEDIUM: htx: Don't rely on h1_sl anymore except during H1
header parsing") to designate a response message: HTX_SL_F_IS_RESP.

Unfortunately as it was done in parallel to the mux_h2 support for
the backend, it was never integrated there. It was not used by then
so this remained unnoticed for a while.

However the http_client now uses it, and missing that flag prevents
it from using the H2 mux, so let's properly add it.

There's no point in backporting this far away, but since the http_client
is fully operational in 2.6 it would make sense to backport this fix at
least there to secure the code.

2 years agoBUG/MINOR: quic: Frames leak during retransmissions
Frédéric Lécaille [Thu, 1 Sep 2022 08:51:19 +0000 (10:51 +0200)] 
BUG/MINOR: quic: Frames leak during retransmissions

The frame which are retransmitted by qc_dgrams_retransmit() are duplicated
from sent but not acknowledged packets and added to local frames lists.
Some may not have been sent. If not replaced somewhere (linked to the
connection) they are lost for ever (leak). We splice the list remaining
contents to the packets number space frame list to avoid such a situation.

Must be backported to 2.6.

2 years agoMINOR: quic: Trace typo fix in qc_release_frm()
Frédéric Lécaille [Wed, 31 Aug 2022 15:54:30 +0000 (17:54 +0200)] 
MINOR: quic: Trace typo fix in qc_release_frm()

Grammar fix without any impact.

2 years agoMINOR: quic: Add TX frames addresses to traces to several trace events
Frédéric Lécaille [Wed, 31 Aug 2022 15:48:53 +0000 (17:48 +0200)] 
MINOR: quic: Add TX frames addresses to traces to several trace events

This should be useful to diagnose TX frames related issues.

2 years agoBUG/MINOR: quic: Do not ack when probing
Frédéric Lécaille [Tue, 30 Aug 2022 14:24:54 +0000 (16:24 +0200)] 
BUG/MINOR: quic: Do not ack when probing

<force_ack> boolean variable passed to qc_do_build_pkt() which builds a clear
packet is there to force this function to build an ACK frame regardless of
others conditions. This is used during handshake, when we acknowledge every
handshake packets received.

This variable was already taken into an account by the local variable <must_ack>
which is there at least to ignore any other conditions than this one: "are
we building a probing packet?". Indeed we do not want to add ACK frames when
we probe the peers. This is to have more chances to embed the new duplicated frames
into another packets without splitting them. So, the test on <force_ack> boolean
value is useless, silly and brakes the rule which consists in not acknowledging
when probing.

Must be backported to 2.6.

2 years agoMINOR: backend: always satisfy the first req reuse rule with l7 retries
Willy Tarreau [Thu, 1 Sep 2022 17:58:58 +0000 (19:58 +0200)] 
MINOR: backend: always satisfy the first req reuse rule with l7 retries

The "first req" rule consists in not delivering a connection's first
request to a connection that's not known for being safe so that we
don't deliver a broken page to a client if the server didn't intend to
keep it alive. That's what's used by "http-reuse safe" particularly.

But the reason this rule was created was precisely because haproxy was
not able to re-emit the request to the server in case of connection
breakage, which is precisely what l7 retries later brought. As such,
there's no reason for enforcing this rule when l7 retries are properly
enabled because such a blank page will trigger a retry and will not be
delivered to the client.

This patch simply checks that the l7 retries are enabled for the 3 cases
that can be triggered on a dead or dying connection (failure, empty, and
timeout), and if all 3 are enabled, then regular idle connections can be
reused.

This could almost be marked as a bug fix because a lot of users relying
on l7 retries do not necessarily think about using http-reuse always due
to the recommendation against it in the doc, while the protection that
the safe mode offers is never used in that mode, and it forces the http
client not to reuse existing persistent connections since it never sets
the "not first" flag.

It could also be decided that the protection is not used either when
the origin is an applet, as in this case this is internal code that
we can decide to let handle the retry by itself (all info are still
present). But at least the httpclient will be happy with this alone.

It would make sense to backport this at least to 2.6 in order to let
the httpclient reuse connections, maybe to older releases if some
users report low reuse counts.

2 years agoBUG/MEDIUM: mux-h1: always use RST to kill idle connections in pools
Willy Tarreau [Thu, 1 Sep 2022 13:49:23 +0000 (15:49 +0200)] 
BUG/MEDIUM: mux-h1: always use RST to kill idle connections in pools

When idle H1 connections cannot be stored into a server pool or are later
evicted, they're often seen closed with a FIN then an RST. The problem is
that this is sufficient to leave them in TIME_WAIT in the local sockets
table and port exhaustion may happen.

The reason is that in h1_release() we rely on h1_shutw_conn() which itself
decides whether to close in silent or normal mode only based on the
H1C_F_ST_SILENT_SHUT flag. This flag is only set by h1_shutw() based on
the requested mode. But when the connection is in the idle list, the mode
ought to always be silent.

What this patch does is to set the flag before trying to add to the idle
list, and remove it after removing from the idle list. This way if the
connection fails to be added or has to be killed, it's closed with an
RST.

This must be backported as far as 2.4. It's not sure whether older
versions need an equivalent.

2 years agoREGTESTS: http_request_buffer: Add a barrier to not mix up log messages
Christopher Faulet [Thu, 1 Sep 2022 17:46:28 +0000 (19:46 +0200)] 
REGTESTS: http_request_buffer: Add a barrier to not mix up log messages

Depending on the timing, time to time, the log messages can be mixed. A
client can start and be fully handled by HAProxy (including its log message)
before the log message of the previous client was emitted or received.  To
fix the issue, a barrier was added to be sure to eval the "expect" rule on
logs before starting the next client.

This patch should fix the issue #1847. It may be backported to all branches
containing this reg-tests.

2 years agoBUG/MINOR: regex: Properly handle PCRE2 lib compiled without JIT support
Christopher Faulet [Thu, 1 Sep 2022 17:34:00 +0000 (19:34 +0200)] 
BUG/MINOR: regex: Properly handle PCRE2 lib compiled without JIT support

The PCRE2 JIT support is buggy. If HAProxy is compiled with USE_PCRE2_JIT
option while the PCRE2 library is compiled without the JIT support, any
matching will fail because pcre2_jit_compile() return value is not properly
handled. We must fall back on pcre2_match() if PCRE2_ERROR_JIT_BADOPTION
error is returned.

This patch should fix the issue #1848. It must be backported as far as 2.4.

2 years agoMINOR: sink/ring: rotate non-empty file-backed contents only
Willy Tarreau [Wed, 31 Aug 2022 16:52:17 +0000 (18:52 +0200)] 
MINOR: sink/ring: rotate non-empty file-backed contents only

If the service is rechecked before a reload, that may cause the config
to be parsed twice and file-backed rings to be lost.

Here we make sure that such a ring does contain information before
deciding to rotate it. This way the first process starting after some
writes will cause a rotate but not subsequent ones until new writes
are applied.

An attempt was also made to disable rotations on checks but this was a
bad idea, as the ring is still initialized and this causes the contents
to be lost. The choice of initializing the ring during parsing is
questionable but the config check ought to be as close as possible to a
real start, and we could imagine that the ring is used by some code
during startup (e.g. lua). So this approach was abandonned and config
checks also cause a rotation, as the purpose of this rotation is to
preserve latest information against accidental removal.

2 years agoBUG/MINOR: ssl: leak of ckch_inst_link in ckch_inst_free() v2
William Lallemand [Wed, 31 Aug 2022 12:26:49 +0000 (14:26 +0200)] 
BUG/MINOR: ssl: leak of ckch_inst_link in ckch_inst_free() v2

ckch_inst_free() unlink the ckch_inst_link structure but never free it.

It can't be fixed simply because cli_io_handler_commit_cafile_crlfile()
is using a cafile_entry list to iterate a list of ckch_inst entries
to free. So both cli_io_handler_commit_cafile_crlfile() and
ckch_inst_free() would modify the list at the same time.

In order to let the caller manipulate the ckch_inst_link,
ckch_inst_free() now checks if the element is still attached before
trying to detach and free it.

For this trick to work, the caller need to do a LIST_DEL_INIT() during
the iteration over the ckch_inst_link.

list_for_each_entry was also replace by a while (!LIST_ISEMPTY()) on the
head list in cli_io_handler_commit_cafile_crlfile() so the iteration
works correctly, because it could have been stuck on the first detached
element. list_for_each_entry_safe() is not enough to fix the issue since
multiple element could have been removed.

Must be backported as far as 2.5.

2 years agoBUG/MINOR: quic: TX frames memleak
Frédéric Lécaille [Wed, 31 Aug 2022 13:02:53 +0000 (15:02 +0200)] 
BUG/MINOR: quic: TX frames memleak

Missing call to pool_free() for quic_frame objects

Must be backported to 2.6.

2 years agoMINOR: quic: Move traces about RX/TX bytes from QUIC_EV_CONN_PRSAFRM event
Frédéric Lécaille [Tue, 30 Aug 2022 13:25:47 +0000 (15:25 +0200)] 
MINOR: quic: Move traces about RX/TX bytes from QUIC_EV_CONN_PRSAFRM event

Move these traces to QUIC_EV_CONN_SPPKTS trace event. They were displayed
at a useless location. Make them displayed just after having sent a packet
and when checking the anti-amplication limit.
Useful to diagnose issues in relation with the recovery.

2 years agoBUILD: debug: make sure debug macros are never empty
Willy Tarreau [Wed, 31 Aug 2022 08:52:25 +0000 (10:52 +0200)] 
BUILD: debug: make sure debug macros are never empty

As outlined in commit f7ebe584d7 ("BUILD: debug: Add braces to if
statement calling only CHECK_IF()"), the BUG_ON() family of macros
is incorrectly defined to be empty when debugging is disabled, and
that can lead to trouble. Make sure they always fall back to the
usual "do { } while (0)". This may be backported to 2.6 if needed,
though no such issue was met there to date.

2 years agoBUG/MINOR: dev/udp: properly preset the rx address size
Willy Tarreau [Wed, 31 Aug 2022 06:55:12 +0000 (08:55 +0200)] 
BUG/MINOR: dev/udp: properly preset the rx address size

addrlen was not preset to sizeof(addr) on rx, resulting in the address
often not being filled and response packets not always flowing back.

Let's also consistently use "addr" in the bind call (it points to
frt_addr there but it's a bit confusing).

2 years agoBUG/MINOR: ssl: revert two wrong fixes with ckhi_link
William Lallemand [Tue, 30 Aug 2022 15:32:38 +0000 (17:32 +0200)] 
BUG/MINOR: ssl: revert two wrong fixes with ckhi_link

This reverts commit 056ad01d55675ab2d65c7b41a2e1096db27b3d14.
This reverts commit ddd480cbdc0d54b3426ce9b6dd68cd849747cb07.

The architecture is ambiguous here: ckch_inst_free() is detaching and
freeing the "ckch_inst_link" linked list which must be free'd only from
the cafile_entry side.

The problem was also hidden by the fix ddd480c ("BUG/MEDIUM: ssl: Fix a
UAF when old ckch instances are released") which change the ckchi_link
inner loop by a safe one. However this can't fix entirely the problem
since both __ckch_inst_free_locked() could remove several nodes in the
ckchi_link linked list.

This revert is voluntary reintroducing a memory leak before really fixing
the problem.

Must be backported in 2.5 + 2.6.

2 years agoBUG/MEDIUM: ssl: Fix a UAF when old ckch instances are released
Christopher Faulet [Tue, 30 Aug 2022 14:27:49 +0000 (16:27 +0200)] 
BUG/MEDIUM: ssl: Fix a UAF when old ckch instances are released

When old chck instances is released at the end of "commit ssl ca-file" or
"commit ssl crl-file" commands, the link is released. But we walk through
the list using the unsafe macro. list_for_each_entry_safe() must be used.

This bug was introduced by commit 056ad01d5 ("BUG/MINOR: ssl: leak of
ckch_inst_link in ckch_inst_free()"). Thus this patch must be backported as
far as 2.5.

2 years agoBUG/MINOR: tcpcheck: Disable QUICKACK for default tcp-check (with no rule)
Christopher Faulet [Tue, 30 Aug 2022 08:31:15 +0000 (10:31 +0200)] 
BUG/MINOR: tcpcheck: Disable QUICKACK for default tcp-check (with no rule)

The commit 871dd8211 ("BUG/MINOR: tcpcheck: Disable QUICKACK only if data
should be sent after connect") introduced a regression. It removes the test
on the next rule to be able to disable TCP_QUICKACK when only a connect is
performed (so no next rule).

This patch must be backported as far as 2.2.

2 years agoBUG/MINOR: ssl: leak of ckch_inst_link in ckch_inst_free()
William Lallemand [Mon, 29 Aug 2022 16:53:34 +0000 (18:53 +0200)] 
BUG/MINOR: ssl: leak of ckch_inst_link in ckch_inst_free()

ckch_inst_free() unlink the ckch_inst_link structure but never free it.
It can cause a memory leak upon a ckch_inst_free() done with CLI
operation.

Bug introduced by commit 4458b97 ("MEDIUM: ssl: Chain ckch instances in
ca-file entries").

Must be backported as far as 2.5.

2 years agoBUG/MINOR: ssl: fix deinit of the ca-file tree
William Lallemand [Mon, 29 Aug 2022 16:36:18 +0000 (18:36 +0200)] 
BUG/MINOR: ssl: fix deinit of the ca-file tree

Commit b0c4827 ("BUG/MINOR: ssl: free the cafile entries on deinit")
introduced a double free.

The node was never removed from the tree before its free.

Fix issue #1836.

Must be backported where b0c4827 was backported. (2.6 for now).

2 years agoMINOR: quic: Add a trace to distinguish the datagram from the packets inside
Frédéric Lécaille [Mon, 29 Aug 2022 16:05:44 +0000 (18:05 +0200)] 
MINOR: quic: Add a trace to distinguish the datagram from the packets inside

Without such a trace, we do not know when a datagram is sent. Only trace for
the packets inside the datagrams were displayed.

Must be backported to 2.6.

2 years agoBUG/MINOR: quic: Missing header protection AES cipher context initialisations (draft-v2)
Frédéric Lécaille [Mon, 29 Aug 2022 14:42:06 +0000 (16:42 +0200)] 
BUG/MINOR: quic: Missing header protection AES cipher context initialisations (draft-v2)

This bug arrived with this commit:
   "MINOR: quic: Add reusable cipher contexts for header protection"

haproxy could crash because of missing cipher contexts initializations for
the header protection and draft-v2 Initial secrets. This was due to the fact
that these initialization both for RX and TX secrets were done outside of
qc_new_isecs(). The role of this function is definitively to initialize these
cipher contexts in addition to the derived secrets. Indeed this function is called
by qc_new_conn() which initializes the connection but also by qc_conn_finalize()
which also calls qc_new_isecs() in case of a different QUIC version was negotiated
by the peers from the one used by the client for its first Initial packet.

This was reported by "v2" QUIC interop test with at least picoquic as client.

Must be backported to 2.6.

2 years agoMINOR: raw-sock: don't try to send if an error was already reported
Willy Tarreau [Mon, 29 Aug 2022 14:48:14 +0000 (16:48 +0200)] 
MINOR: raw-sock: don't try to send if an error was already reported

There's no point trying to send() on a socket on which an error was already
reported. This wastes syscalls. Till now it was possible to occasionally
see an attempt to sendto() after epoll_wait() had reported EPOLLERR.

2 years agoBUG/MINOR: epoll: do not actively poll for Rx after an error
Willy Tarreau [Mon, 29 Aug 2022 16:15:11 +0000 (18:15 +0200)] 
BUG/MINOR: epoll: do not actively poll for Rx after an error

In 2.2, commit 5d7dcc2a8 ("OPTIM: epoll: always poll for recv if neither
active nor ready") was added to compensate for the fact that our iocbs
are almost always asynchronous now and do not have the opportunity to
update the FD correctly. As such, they just perform a wakeup, the FD is
turned to inactive, the tasklet wakes up, performs the I/O, updates the
FD, most of the time this is done withing the same polling loop, and the
update cancels itself in the poller without having to switch the FD off
then on.

The issue was that when deciding to claim an FD was active for reads
if it was active for writes, we forgot one situation that unfortunately
causes excessive wakeups: dealing with errors. Indeed, errors are
reported and keep ringing as long as the FD is active for sending even
if the consumer disabled the FD for receiving. Usually this only causes
one extra wakeup for the time it takes to consider a potential write
subscriber and to call it, though with many tasks in a run queue, it
can last a bit longer and be reported more often.

The fix consists in checking that we really want to get more receive
events on this FD, that is:
  - that no prevous EPOLLERR was reported
  - that the FD doesn't carry a sticky error
  - that the FD is not shut for reads

With this, after the last epoll_wait() reports EPOLLERR, one last recv()
is performed to flush pending data and the FD is immediately unregistered.

It's probably not needed to backport this as its effects are not much
visible, though it should not harm.

Before, EPOLLERR was seen twice:

  accept4(4, {sa_family=AF_INET, sin_port=htons(22314), sin_addr=inet_addr("127.0.0.1")}, [128 => 16], SOCK_NONBLOCK) = 8
  accept4(4, 0x261b160, [128], SOCK_NONBLOCK) = -1 EAGAIN (Resource temporarily unavailable)
  recvfrom(8, "POST / HTTP/1.1\r\nConnection: close\r\nTransfer-encoding: chunk"..., 16320, 0, NULL, NULL) = 66
  socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 9
  connect(9, {sa_family=AF_INET, sin_port=htons(8002), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRESS (Operation now in progress)
  epoll_ctl(3, EPOLL_CTL_ADD, 8, {events=EPOLLIN|EPOLLRDHUP, data={u32=8, u64=8}}) = 0
  epoll_ctl(3, EPOLL_CTL_ADD, 9, {events=EPOLLIN|EPOLLOUT|EPOLLRDHUP, data={u32=9, u64=9}}) = 0
  epoll_wait(3, [{events=EPOLLOUT, data={u32=9, u64=9}}], 200, 355) = 1
  recvfrom(9, 0x25cfb30, 16320, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
  sendto(9, "POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n", 47, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 47
  epoll_ctl(3, EPOLL_CTL_MOD, 9, {events=EPOLLIN|EPOLLRDHUP, data={u32=9, u64=9}}) = 0
  epoll_wait(3, [{events=EPOLLIN|EPOLLERR|EPOLLHUP|EPOLLRDHUP, data={u32=9, u64=9}}], 200, 354) = 1
  recvfrom(9, "HTTP/1.1 200 OK\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", 16320, 0, NULL, NULL) = 57
  sendto(8, "HTTP/1.1 200 OK\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", 57, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 57
->epoll_wait(3, [{events=EPOLLIN|EPOLLERR|EPOLLHUP|EPOLLRDHUP, data={u32=9, u64=9}}], 200, 354) = 1
  epoll_ctl(3, EPOLL_CTL_DEL, 9, 0x7ffe0b65fb24) = 0
  epoll_wait(3, [{events=EPOLLIN, data={u32=8, u64=8}}], 200, 354) = 1
  recvfrom(8, "A\n0123456789\r\n0\r\n\r\n", 16320, 0, NULL, NULL) = 19
  close(9)                          = 0
  close(8)                          = 0

After, EPOLLERR is only seen only once, with one less call to epoll_wait():

  accept4(4, {sa_family=AF_INET, sin_port=htons(22362), sin_addr=inet_addr("127.0.0.1")}, [128 => 16], SOCK_NONBLOCK) = 8
  accept4(4, 0x20d0160, [128], SOCK_NONBLOCK) = -1 EAGAIN (Resource temporarily unavailable)
  recvfrom(8, "POST / HTTP/1.1\r\nConnection: close\r\nTransfer-encoding: chunk"..., 16320, 0, NULL, NULL) = 66
  socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 9
  connect(9, {sa_family=AF_INET, sin_port=htons(8002), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRESS (Operation now in progress)
  epoll_ctl(3, EPOLL_CTL_ADD, 8, {events=EPOLLIN|EPOLLRDHUP, data={u32=8, u64=8}}) = 0
  epoll_ctl(3, EPOLL_CTL_ADD, 9, {events=EPOLLIN|EPOLLOUT|EPOLLRDHUP, data={u32=9, u64=9}}) = 0
  epoll_wait(3, [{events=EPOLLOUT, data={u32=9, u64=9}}], 200, 411) = 1
  recvfrom(9, 0x2084b30, 16320, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
  sendto(9, "POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n", 47, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 47
  epoll_ctl(3, EPOLL_CTL_MOD, 9, {events=EPOLLIN|EPOLLRDHUP, data={u32=9, u64=9}}) = 0
  epoll_wait(3, [{events=EPOLLIN|EPOLLERR|EPOLLHUP|EPOLLRDHUP, data={u32=9, u64=9}}], 200, 411) = 1
  recvfrom(9, "HTTP/1.1 200 OK\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", 16320, 0, NULL, NULL) = 57
  sendto(8, "HTTP/1.1 200 OK\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", 57, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 57
  epoll_ctl(3, EPOLL_CTL_DEL, 9, 0x7ffc95d46f04) = 0
  epoll_wait(3, [{events=EPOLLIN, data={u32=8, u64=8}}], 200, 411) = 1
  recvfrom(8, "A\n0123456789\r\n0\r\n\r\n", 16320, 0, NULL, NULL) = 19
  close(9)                          = 0
  close(8)                          = 0

2 years agoBUG/MEDIUM: mux-h1: do not refrain from signaling errors after end of input
Willy Tarreau [Mon, 29 Aug 2022 08:22:56 +0000 (10:22 +0200)] 
BUG/MEDIUM: mux-h1: do not refrain from signaling errors after end of input

In 2.6-dev4, a fix for truncated response was brought with commit 99bbdbcc2
("BUG/MEDIUM: mux-h1: only turn CO_FL_ERROR to CS_FL_ERROR with empty ibuf"),
trying to address the situation where an error is present at the connection
level but some data are still pending to be read by the stream. However,
this patch did not consider the case where the stream was no longer willing
to read the pending data, resulting in a situation where some aborted
transfers could lead to excessive CPU usage by causing constant stream
wakeups for which no error was reported. This perfectly matches what was
observed and reported in github issue #1842. It's not trivial to reproduce,
but aborting HTTP/1 pipelining in the middle of transfers seems to give
good results (using h2load and Ctrl-C in the middle).

The fix was incorrct as the error should be held only if there were data
that the stream was able to read. This is the approach taken by this patch,
which also checks via SE_FL_EOI | SE_FL_EOS that the stream will be able
to consume the pending data.

Note that the loop was provoked by the attempt by sc_conn_io_cb() itself
to call sc_conn_send() which resulted in a write subscription in
h1_subscribe() which immediately calls a tasklet_wakeup() since the
event is ready, and that it is now stopped by the presence of SE_FL_ERROR
that is checked in sc_conn_io_cb(). It seems that an extra check down the
send() path to refrain from subscribing when the connection is in error
could speed up error detection or at least avoid a risk of loops in this
case, but this is tricky. In addition, there's already SE_FL_ERR_PENDING
that seems more suitable for reporting when there are pending data, but
similarly, it probably isn't checked well enough to be suitable for
backports.

FWIW the issue may (unreliably) be reproduced by chaining haproxy to
httpterm and issuing:

  (printf "GET /?s=10g HTTP/1.1\r\n\r\n"; sleep 0.1; printf "\r\n") | \
    nc6 --half-close 0 8001 | head -c1000000000 >/dev/null

It's necessary to play with the size of the head command that's supposed
to trigger the error at some point. A variant involving h2load in h1 mode
and multiple pipelined streams, that is stopped with Ctrl-C also tends to
work.

As the fix above was backported as far as 2.0, it would be tempting to
backport this one as far. However tests have shown that the oldest
version that can trigger this issue is 2.5, maybe due to subtle
differences in older ones, so it's probably not worth going further
until an issue is reported. Note that in 2.5 and older, the SE_FL_*
flags are applied on the conn_stream instead, as CS_FL_*.

Special thanks go to Felipe W Damasio for providing lots of detailed data
allowing to quickly spot the root cause of the problem.

2 years agoBUG/MINOR: hlua: Rely on CF_EOI to detect end of message in HTTP applets
Christopher Faulet [Mon, 29 Aug 2022 13:37:16 +0000 (15:37 +0200)] 
BUG/MINOR: hlua: Rely on CF_EOI to detect end of message in HTTP applets

applet:getline() and applet:receive() functions for HTTP applets must rely
on the channel flags to detect the end of the message and not on HTX
flags. It means CF_EOI must be used instead of HTX_FL_EOM.

It is important because the HTX flag is transient. Because there is no flag
on HTTP applets to save the info, it is not reliable. However CF_EOI once
set is never removed. So it is safer to rely on it. Otherwise, the call to
these functions hang.

This patch must be backported as far as 2.4.

2 years agoBUG/MEDIUM: peers: Don't start resync on reload if local peer is not up-to-date
Christopher Faulet [Fri, 26 Aug 2022 16:46:16 +0000 (18:46 +0200)] 
BUG/MEDIUM: peers: Don't start resync on reload if local peer is not up-to-date

On a reload, if the previous resync was not finished, the freshly old worker
must not try to start a new resync. Otherwise, it will compete with the
older wokers, slowing down or blocking the resync. Only an up-to-date woker
must try to perform a local resync.

This patch must be backported as far as 2.0 (and maybe to 1.8 too).

2 years agoBUG/MEDIUM: peers: Don't use resync timer when local resync is in progress
Christopher Faulet [Fri, 26 Aug 2022 16:40:46 +0000 (18:40 +0200)] 
BUG/MEDIUM: peers: Don't use resync timer when local resync is in progress

When a worker is stopped, the resync timer is used to limit in time the
connection stage to the new worker to perform the local resync. However,
this timer must be stopped when the resync is in progress and it must be
re-armed if the resync is interrupted (for instance because another
reload). Otherwise, if the resync is a bit long, an old worker may be killed
too early.

This bug was introduce by the commit 160fff665 ("BUG/MEDIUM: peers: limit
reconnect attempts of the old process on reload"). It must be backported as
far as 2.0.

2 years agoBUG/MEDIUM: peers: Add connect and server timeut to peers proxy
Christopher Faulet [Mon, 29 Aug 2022 09:32:26 +0000 (11:32 +0200)] 
BUG/MEDIUM: peers: Add connect and server timeut to peers proxy

Only the client timeout was set. Nothing prevent a peer applet to stall
during a connect or waiting a message from a remote peer. To avoid any
issue, it is important to also set connection and server timeouts. The
connect timeout is set to 1s and the server timeout is set to 5s.

This patch must be backported to all supported versions.

2 years agoBUG/MEDIUM: spoe: Properly update streams waiting for a ACK in async mode
Christopher Faulet [Thu, 25 Aug 2022 16:50:18 +0000 (18:50 +0200)] 
BUG/MEDIUM: spoe: Properly update streams waiting for a ACK in async mode

A bug was introduced by the commit b042e4f6f ("BUG/MAJOR: spoe: properly
detach all agents when releasing the applet"). The fix is not correct. We
really want to known if the released appctx is the last one or not. It is
important when async mode is used. If there are still running applets, we
just need to remove the reference on the current applet from streams in the
async waiting queue.

With the commit above, in async mode, if there are still running applets, it
will work as expected. Otherwise a processing timeout will be reported for
all these streams. So it is not too bad. But for other modes (sync and
pipelining), the async waiting queue is always empty. If at least one stream
is waiting to send a message, a new applet is created. It is an issue if the
SPOA is unhealthy because the number of running applets may explode.

However, the commit above tried to fix an issue. The bug is in fact when an
new SPOE applet is created. On success, we must remove reference on the
current appctx from the streams in the async waiting queue.

This patch must be backported as far as 1.8.

2 years agoBUG/MINOR: quic: Frames added to packets even if not built.
Frédéric Lécaille [Sat, 27 Aug 2022 13:51:30 +0000 (15:51 +0200)] 
BUG/MINOR: quic: Frames added to packets even if not built.

Several frames could remain as not build into <frm_list> built by qc_build_frms()
after having stopped at the first building error. So only one frame was reinserted in
the frame list passed as parameter to qc_do_build_pkt(). Then <frm_list> was
spliced to the packet frame list even its frames were not built, nor attached to
any packet. Such frames had their ->pkt member set to NULL, but considered as
built, then sent leading to a crash in qc_release_frm() where ->pkt is dereferenced.

This issue was again reported by useful traces provided by Tristan in GH #1808.

Must be backported to 2.6.

2 years agoBUG/MINOR: quic: Null packet dereferencing from qc_dup_pkt_frms() trace
Frédéric Lécaille [Sat, 27 Aug 2022 08:19:42 +0000 (10:19 +0200)] 
BUG/MINOR: quic: Null packet dereferencing from qc_dup_pkt_frms() trace

This function must duplicate frames be resent from packets. Some of
them are still in flight, others have already been detected as lost.
In this case the original frame ->pkt member is NULL.
Add a trace to distinguish these cases.

Thank you to Tristan for having reported this issue in GH #1808.

Must be backported to 2.6.

2 years agoDOC: configuration.txt: do-resolve must use host_only to remove its port.
William Lallemand [Fri, 26 Aug 2022 14:48:07 +0000 (16:48 +0200)] 
DOC: configuration.txt: do-resolve must use host_only to remove its port.

The do-resolve action does not support a port in its parameter string,
the host_only converter must be used.

Must be backported to 2.6.

2 years agoBUG/MINOR: httpclient: fix resolution with port
William Lallemand [Fri, 26 Aug 2022 14:45:13 +0000 (16:45 +0200)] 
BUG/MINOR: httpclient: fix resolution with port

Fix the resolution in the httpclient when a port is associated to a
domain. The do-resolve action doesn't support a port in its input.

Must be backported to 2.6. Require the "host_only" converter to be
backported.

2 years agoMINOR: sample: add the host_only and port_only converters
William Lallemand [Fri, 26 Aug 2022 14:21:28 +0000 (16:21 +0200)] 
MINOR: sample: add the host_only and port_only converters

Add 2 converters that can manipulate the value of an Host header.
host_only will return the host without any port, and port_only will
return the port.

2 years agoDOC: configuration: do-resolve doesn't work with a port in the string
William Lallemand [Fri, 26 Aug 2022 14:38:43 +0000 (16:38 +0200)] 
DOC: configuration: do-resolve doesn't work with a port in the string

Fix the documentation about do-resolve to handle the case where a port
is associated to the hostname in the Host header.

Must be backported as far as 2.0.

2 years agoRevert "MINOR: quic: Remove useless traces about references to TX packets"
Frédéric Lécaille [Thu, 25 Aug 2022 14:06:48 +0000 (16:06 +0200)] 
Revert "MINOR: quic: Remove useless traces about references to TX packets"

This reverts commit f61398a7caa35d780639a5961d2b1ea427f710b6.
After having checked a version with more traces and reproduced the issue
as reported by Tristan in GH #1808, there are remaining cases where
a duplicated but not already sent frame have to be marked as acked because
the frame it was copied from was acknowledeged before its copied was sent.

Must be backported to 2.6.

2 years agoMINOR: quic: Remove useless traces about references to TX packets
Frédéric Lécaille [Thu, 25 Aug 2022 05:13:33 +0000 (07:13 +0200)] 
MINOR: quic: Remove useless traces about references to TX packets

Since this commit:
    "BUG/MINOR: quic: Wrong list_for_each_entry() use when building packets from
     qc_do_build_pkt()"
there is no more reason that frames can be released without having been
sent, i.e. frames with non null ->pkt member. This ->pkt is the packet
the frame is attached to.

Must be backported to 2.6.

2 years agoCLEANUP: quic: Remove a useless check in qc_lstnr_pkt_rcv()
Frédéric Lécaille [Wed, 24 Aug 2022 16:59:23 +0000 (18:59 +0200)] 
CLEANUP: quic: Remove a useless check in qc_lstnr_pkt_rcv()

This function parses the QUIC packet from a UDP datagram. It was originally
supposed to be run by several thread. Here we remove a section of code
where the current thread checks there is not another thread which has already
inserted the new quic_conn it is trying to insert in the connections tree.

Must be backported to 2.6 to ease the future backports to come.

2 years agoCLEANUP: quic: No more use ->rx_list MT_LIST entry point (quic_rx_packet)
Frédéric Lécaille [Wed, 24 Aug 2022 16:17:13 +0000 (18:17 +0200)] 
CLEANUP: quic: No more use ->rx_list MT_LIST entry point (quic_rx_packet)

This quic_rx_packet is definitively no more used.

Should be backported to 2.6 to ease the future backports.

2 years agoBUG/MINOR: quic: Stalled connections (missing I/O handler wakeup)
Frédéric Lécaille [Wed, 24 Aug 2022 15:57:09 +0000 (17:57 +0200)] 
BUG/MINOR: quic: Stalled connections (missing I/O handler wakeup)

This was due to a missing I/O handler tasklet wakeup in process_timer() when
detecting packet loss. As, qc_release_lost_pkts() could remove the lost packets
from the in flight packets count, qc_set_timer() could cancel the timer used
to wakeup the connection I/O handler. Then the connection could remain idle
until it ends.

Must be backported to 2.6.

2 years agoBUG/MINOR: quic: Leak in qc_release_lost_pkts() for non in flight TX packets
Frédéric Lécaille [Wed, 24 Aug 2022 15:06:04 +0000 (17:06 +0200)] 
BUG/MINOR: quic: Leak in qc_release_lost_pkts() for non in flight TX packets

Packets with null "in flight" lengths are kept as the others packets as sent
but not already acknowledeged in the by packet number space trees.
But qc_release_lost_pkts() relied on this in fligh length to release the
memory allocated for this packets. We must release the memory allocated for
all the lost packets regardless of their in fligh lengths.

Modify this function to do nothing if the list of lost packets passed
as argument is empty. Stop using <lost_bytes> variable to decide if some packets
memory must be released or not.
Modify the callers to stop checking if this list is empty.

Should helping in fixing memory leak as reported by Tristan in GH #1801.

Must be backported to 2.6.

2 years agoRevert "BUG/MINOR: quix: Memleak for non in flight TX packets"
Frédéric Lécaille [Wed, 24 Aug 2022 14:23:44 +0000 (16:23 +0200)] 
Revert "BUG/MINOR: quix: Memleak for non in flight TX packets"

This reverts commit da9c441886dbfa02840a93e367f65fd6d312c835.

Indeed this commit prevented the ACK only packets to be used as other packets
when they are acknowledged. Even if not ack-eliciting packets they are
acknowledged alongside others packets. Such acknowledged ACK only packets
must be used for instance to compute the RTT.

Must be backported to 2.6 if da9c441 was backported to 2.6.

2 years agoMINOR: resolvers: shut the warning when "default" resolvers is implicit
William Lallemand [Wed, 24 Aug 2022 12:50:32 +0000 (14:50 +0200)] 
MINOR: resolvers: shut the warning when "default" resolvers is implicit

Shut the connect() warning of resolvers_finalize_config() when the
configuration was not emitted manually.

This shuts the warning for the "default" resolvers which is created
automatically for the httpclient.

Must be backported in 2.6.

2 years agoREGTESTS: Fix prometheus script to perform HTTP health-checks
Christopher Faulet [Wed, 24 Aug 2022 10:17:31 +0000 (12:17 +0200)] 
REGTESTS: Fix prometheus script to perform HTTP health-checks

TCP Health-checks are enabled on server "s2". However it expects to receive
an HTTP requests. So HAProxy configuration must be changed to perform HTTP
health-checks instead. Otherwise, depending on the timing, an error can be
triggered if a check is performed before the end of the script.

This scripts never failed because TCP_QUICKACK was disabled, adding some
latency on health-checks. But since the last fix, it is an issue.

This patch should be backported as far as 2.4.

2 years agoBUG/MINOR: tcpcheck: Disable QUICKACK only if data should be sent after connect
Christopher Faulet [Wed, 24 Aug 2022 09:38:03 +0000 (11:38 +0200)] 
BUG/MINOR: tcpcheck: Disable QUICKACK only if data should be sent after connect

It is only a real problem for agent-checks when there is no agent string to
send. The condition to disable TCP_QUICKACK was only based on the action
type following the connect one. But it is not always accurate. indeed, for
agent-checks, there is always a SEND action. But if there is no "agent-send"
string defined, nothing is sent. In this case, this adds 200ms of latency
with no reason.

To fix the bug, a flag is now used on the CONNECT action to instruct there
are data that should be sent after the connect. For health-checks, this flag
is set if the action following the connect is a SEND action. For
agent-checks, it is set if an "agent-send" string is defined.

This patch should fix the issue #1836. It must be backported as far as 2.2.

2 years agoBUG/MINOR: mworker: does not create the "default" resolvers in wait mode
William Lallemand [Wed, 24 Aug 2022 09:15:08 +0000 (11:15 +0200)] 
BUG/MINOR: mworker: does not create the "default" resolvers in wait mode

When doing a re-exec, the master was creating a "default" resolvers,
which could result in a warning emitted because the "default" resolvers
of the configuration file is not available anymore.

Skip the creating of the "default" resolvers in wait mode, this is not
useful in the master.

Must be backported as far as 2.6.

2 years agoBUG/MINOR: resolvers: return the correct value in resolvers_finalize_config()
William Lallemand [Wed, 24 Aug 2022 07:58:31 +0000 (09:58 +0200)] 
BUG/MINOR: resolvers: return the correct value in resolvers_finalize_config()

Patch c31577f ("MEDIUM: resolvers: continue startup if network is
unavailable") was not working correctly. Indeed
resolvers_finalize_config() was returning a ERR type, but a postparser
is supposed to return 0 or 1.

The return value was never right, however it was only a problem since c31577f.

Must be backported in every stable branch.

2 years agoBUILD: tcp_sample: fix build of get_tcp_info() on OpenBSD
Brad Smith [Sat, 13 Aug 2022 02:23:13 +0000 (22:23 -0400)] 
BUILD: tcp_sample: fix build of get_tcp_info() on OpenBSD

The build on OpenBSD is broken since commit 5c83e3a15 ("MINOR: tcp_sample:
clarifying samples support per os, for further expansion."), hence it
only affects 2.7 and 2.6.

It looks like this changed things in such a way that if TCP_INFO is added
but the OS is not added to the list of OS's it will not build.

Extend support for get_tcp_info to OpenBSD.

This must be backported to 2.6.

2 years agoMEDIUM: peers: limit the number of updates sent at once
Willy Tarreau [Tue, 19 Jul 2022 18:17:38 +0000 (20:17 +0200)] 
MEDIUM: peers: limit the number of updates sent at once

As seen in GH issue #1770, peers synchronization do not cope well with
very large buffers because by default the only two reasons for stopping
the processing of updates is either that the end was reached or that
the buffer is full. This can cause high latencies, and even rightfully
trigger the watchdog when the operations are numerous and slowed down
by competition on the stick-table lock.

This patch introduces a limit to the number of messages one may send
at once, which now defaults to 200, regardless of the buffer size. This
means taking and releasing the lock up to 400 times in a row, which is
costly enough to let some other parts work.

After some observation this could be backported to 2.6. If so, however,
previous commits "BUG/MEDIUM: applet: fix incorrect check for abnormal
return condition from handler" and "BUG/MINOR: applet: make the call_rate
only count the no-progress calls" must be backported otherwise the call
rate might trigger the looping protection.

2 years agoBUG/MINOR: applet: make the call_rate only count the no-progress calls
Willy Tarreau [Tue, 19 Jul 2022 18:36:15 +0000 (20:36 +0200)] 
BUG/MINOR: applet: make the call_rate only count the no-progress calls

This is very similar to what we did in commit 6c539c4b8 ("BUG/MINOR:
stream: make the call_rate only count the no-progress calls"), it's
better to only count the call rate with no progress than to count all
calls and try to figure if there's no progress, because a fast running
applet might once satisfy the whole condition and trigger the bug. This
typically happens when artificially limiting the number of messages sent
at once by an applet, but could happen with plenty of highly interactive
applets.

This patch could be backported to stable versions if there are any
indications that it might be useful there.

2 years agoBUG/MEDIUM: applet: fix incorrect check for abnormal return condition from handler
Willy Tarreau [Tue, 23 Aug 2022 07:01:30 +0000 (09:01 +0200)] 
BUG/MEDIUM: applet: fix incorrect check for abnormal return condition from handler

We have quite numerous checks for abnormal applet handler behavior which
are supposed to trigger the loop protection. However, consecutive to
commit 15252cd9c ("MEDIUM: stconn: move the RXBLK flags to the stream
connector") that was merged into 2.6-dev12, one flag was incorrectly
renamed, and the check for an applet waiting for a buffer that is present
mistakenly turned to a check for missing room in the buffer. This erroneous
test could mistakenly trigger on applets that perform intensive I/Os doing
small exchanges each (e.g. cache, peers or HTTP client) if the load would
be sustained (>100k iops). For the cache this could represent higher than
13 Gbps on an object at least 1.6 GB large for example, which is quite
unlikely but theoretically possible.

This fix needs to be backported to 2.6.

2 years agoMINOR: quic: Replace MT_LISTs by LISTs for RX packets.
Frédéric Lécaille [Tue, 23 Aug 2022 15:45:52 +0000 (17:45 +0200)] 
MINOR: quic: Replace MT_LISTs by LISTs for RX packets.

Replace ->rx.pqpkts quic_enc_level struct member MT_LIST by an LIST.
Same thing for ->list quic_rx_packet struct member MT_LIST.
Update the code consequently. This was a reminisence of the multithreading
support (several threads by connection).

Must be backported to 2.6

2 years agoBUG/MINOR: quic: Safer QUIC frame builders
Frédéric Lécaille [Tue, 23 Aug 2022 15:40:09 +0000 (17:40 +0200)] 
BUG/MINOR: quic: Safer QUIC frame builders

Do not rely on the fact the callers of qc_build_frm() handle their
buffer passed to function the correct way (without leaving garbage).
Make qc_build_frm() update the buffer passed as argument only if
the frame it builds is well formed.

As far as I sse, there is no such callers which does not handle
carefully such buffers.

Must be backported to 2.6.

2 years agoBUG/MINOR: quic: Wrong list_for_each_entry() use when building packets from qc_do_bui...
Frédéric Lécaille [Tue, 23 Aug 2022 09:42:48 +0000 (11:42 +0200)] 
BUG/MINOR: quic: Wrong list_for_each_entry() use when building packets from qc_do_build_pkt()

This is list_for_each_entry_safe() which must be used if we want to delete elements
inside its code block. This could explain that some frames which were not built were added
to packets with a NULL ->pkt member.

Thank you to Tristan for having reported this issue through backtraces in GH #1808

Must be backported to 2.6.

2 years agoBUG/MINOR: quix: Memleak for non in flight TX packets
Frédéric Lécaille [Mon, 22 Aug 2022 16:47:51 +0000 (18:47 +0200)] 
BUG/MINOR: quix: Memleak for non in flight TX packets

First, these packets must not be inserted in the tree of TX packets.
They are never explicitely acknowledged (for instance an ACK only
packet will never be acknowledged). Furthermore, if taken into an account
these packets may uselessly disturb the congestion control. We do not care
if they are lost or not. Furthermore as the ->in_fligh_len member value is null
they were not released by qc_release_lost_pkts() which rely on these values
to decide to release the allocated memory for such packets.

Must be backported to 2.6.