]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
3 years agoBUILD: makefile: enable both DEBUG_STRICT and DEBUG_MEMORY_POOLS by default
Willy Tarreau [Wed, 23 Feb 2022 16:50:37 +0000 (17:50 +0100)] 
BUILD: makefile: enable both DEBUG_STRICT and DEBUG_MEMORY_POOLS by default

The first one will enable all currently deployed BUG_ON() checks. These
ones are safe from a performance perspective and from a reliability
perspective. New ones may be added later with different categories
(hot path, detection of uncertain events, etc).

DEBUG_MEMORY_POOLS enables the "tag" pool debugging option by default,
so that pools may be better traced in dumps. This one alone results in
almost imperceptible performance difference, and 8 extra bytes per
allocated object.

Both options are safe for production use (they're among those enabled
all the time on haproxy.org) and allow to produce much more trustable
bug reports which should save a few round trips with the reporters.

3 years agoMINOR: pools: support setting debugging options using -dM
Willy Tarreau [Wed, 23 Feb 2022 14:20:53 +0000 (15:20 +0100)] 
MINOR: pools: support setting debugging options using -dM

The 9 currently available debugging options may now be checked, set, or
cleared using -dM. The directive now takes a comma-delimited list of
options after the optional poisonning byte. With "help", the list of
available options is displayed with a short help and their current
status.

The management doc was updated.

3 years agoMINOR: pools: delegate parsing of command line option -dM to a new function
Willy Tarreau [Fri, 18 Feb 2022 17:54:40 +0000 (18:54 +0100)] 
MINOR: pools: delegate parsing of command line option -dM to a new function

New function pool_parse_debugging() is now dedicated to parsing options
of -dM. For now it only handles the optional memory poisonning byte, but
the function may already return an informative message to be printed for
help, a warning or an error. This way we'll reuse it for the settings
that will be needed for configurable debugging options.

3 years agoMEDIUM: init: handle arguments earlier
Willy Tarreau [Wed, 23 Feb 2022 16:25:00 +0000 (17:25 +0100)] 
MEDIUM: init: handle arguments earlier

The argument parser runs too late, we'll soon need it before creating
pools, hence just after init_early(). No visible change is expected but
this part is sensitive enough to be placed into its own commit for easier
bisection later if needed.

3 years agoMINOR: init: extract args parsing to their own function
Willy Tarreau [Thu, 17 Feb 2022 17:10:36 +0000 (18:10 +0100)] 
MINOR: init: extract args parsing to their own function

The cmdline argument parsing was performed quite late, which prevents
from retrieving elements that can be used to initialize the pools and
certain sensitive areas. The goal is to improve this by parsing command
line arguments right after the early init stage. This is possible
because the cmdline parser already does very little beyond retrieving
config elements that are used later.

Doing so requires to move the parser code to a separate function and
to externalize a few variables out of the function as they're used
later in the boot process, in the original function.

This patch creates init_args() but doesn't move it upfront yet, it's
still executed just before init(), which essentially corresponds to
what was done before (only the trash buffers, ACLs and Lua were
initialized earlier and are not needed for this).

The rest is not modified and as expected no change is observed.

Note that the diff doesn't to justice to the change as it makes it
look like the early init() code was moved to a new function after
the function was renamed, while in fact it's clearly the parser
itself which moved.

3 years agoMEDIUM: init: split the early initialization in its own function
Willy Tarreau [Thu, 17 Feb 2022 16:45:58 +0000 (17:45 +0100)] 
MEDIUM: init: split the early initialization in its own function

There are some delicate chicken-and-egg situations in the initialization
code, because the init() function currently does way too much (it goes
as far as parsing the config) and due to this it must be started very
late. But it's also in charge of initializing a number of variables that
are needed in early boot (e.g. hostname/pid for error reporting, or
entropy for random generators).

This patch carefully extracts all the early code that depends on
absolutely nothing, and places it immediately after the STG_LOCK init
stage. The only possible failures at this stage are only allocation
errors and they continue to provoke an immediate exit().

Some environment variables, hostname, date, pid etc are retrieved at
this stage. The program's arguments are also copied there since they're
needed to be kept intact for the master process.

3 years agoMEDIUM: initcall: move STG_REGISTER earlier
Willy Tarreau [Fri, 18 Feb 2022 13:51:49 +0000 (14:51 +0100)] 
MEDIUM: initcall: move STG_REGISTER earlier

The STG_REGISTER init level is used to register known keywords and
protocol stacks. It must be called earlier because some of the init
code already relies on it to be known. For example, "haproxy -vv"
for now is constrained to start very late only because of this.

This patch moves it between STG_LOCK and STG_ALLOC, which is fine as
it's used for static registration.

3 years agoMINOR: pools: add a debugging flag for memory poisonning option
Willy Tarreau [Wed, 23 Feb 2022 13:15:18 +0000 (14:15 +0100)] 
MINOR: pools: add a debugging flag for memory poisonning option

Now -dM will set POOL_DBG_POISON for consistency with the rest of the
pool debugging options. As such now we only check for the new flag,
which allows the default value to be preset.

3 years agoMINOR: pools: replace DEBUG_MEMORY_POOLS with runtime POOL_DBG_TAG
Willy Tarreau [Wed, 23 Feb 2022 09:20:37 +0000 (10:20 +0100)] 
MINOR: pools: replace DEBUG_MEMORY_POOLS with runtime POOL_DBG_TAG

This option used to allow to store a marker at the end of the area, which
was used as a canary and detection against wrong freeing while the object
is used, and as a pointer to the last pool_free() caller when back in cache.
Now that we can compute the offsets at runtime, let's check it at run time
and continue the code simplification.

3 years agoMINOR: pools: replace DEBUG_POOL_TRACING with runtime POOL_DBG_CALLER
Willy Tarreau [Wed, 23 Feb 2022 09:10:33 +0000 (10:10 +0100)] 
MINOR: pools: replace DEBUG_POOL_TRACING with runtime POOL_DBG_CALLER

This option used to allow to store a pointer to the caller of the last
pool_alloc() or pool_free() at the end of the area. Now that we can
compute the offsets at runtime, let's check it at run time and continue
the code simplification. In __pool_alloc() we now always calculate the
return address (which is quite cheap), and the POOL_DEBUG_TRACE_CALLER()
calls are conditionned on the status of debugging option.

3 years agoMINOR: pools: get rid of POOL_EXTRA
Willy Tarreau [Wed, 23 Feb 2022 09:03:11 +0000 (10:03 +0100)] 
MINOR: pools: get rid of POOL_EXTRA

This macro is build-time dependent and is almost unused, yet where it
cannot easily be avoided. Now that we store the distinction between
pool->size and pool->alloc_sz, we don't need to maintain it and we
can instead compute it on the fly when creating a pool. This is what
this patch does. The variables are for now pretty static, but this is
sufficient to kill the macro and will allow to set them more dynamically.

3 years agoMINOR: pools: store the allocated size for each pool
Willy Tarreau [Wed, 23 Feb 2022 07:57:59 +0000 (08:57 +0100)] 
MINOR: pools: store the allocated size for each pool

The allocated size is the visible size plus the extra storage. Since
for now we can store up to two extra elements (mark and tracer), it's
convenient because now we know that the mark is always stored at
->size, and the tracer is always before ->alloc_sz.

3 years agoMEDIUM: pools: replace CONFIG_HAP_POOLS with a runtime "NO_CACHE" flag.
Willy Tarreau [Tue, 22 Feb 2022 15:23:09 +0000 (16:23 +0100)] 
MEDIUM: pools: replace CONFIG_HAP_POOLS with a runtime "NO_CACHE" flag.

Like previous patches, this replaces the build-time code paths that were
conditionned by CONFIG_HAP_POOLS with runtime paths conditionned by
!POOL_DBG_NO_CACHE. One trivial test had to be added in the hot path in
__pool_alloc() to refrain from calling pool_get_from_cache(), and another
one in __pool_free() to avoid calling pool_put_to_cache().

All cache-specific functions were instrumented with a BUG_ON() to make
sure we never call them with cache disabled. Additionally the cache[]
array was not initialized (remains NULL) so that we can later drop it
if not needed. It's particularly huge and should be turned to dynamic
with a pointer to a per-thread area where all the objects are located.
This will solve the memory usage issue and will improve locality, or
even help better deal with NUMA machines once each thread uses its own
arena.

3 years agoMINOR: pools: make the global pools a runtime option.
Willy Tarreau [Tue, 22 Feb 2022 08:21:13 +0000 (09:21 +0100)] 
MINOR: pools: make the global pools a runtime option.

There were very few functions left that were specific to global pools,
and even the checks they used to participate to are not directly on the
most critical path so they can suffer an extra "if".

What's done now is that pool_releasable() always returns 0 when global
pools are disabled (like the one before) so that pool_evict_last_items()
never tries to place evicted objects there. As such there will never be
any object in the free list. However pool_refill_local_from_shared() is
bypassed when global pools are disabled so that we even avoid the atomic
loads from this function.

The default global setting is still adjusted based on the original
CONFIG_NO_GLOBAL_POOLS that is set depending on threads and the allocator.
The global executable only grew by 1.1kB by keeping this code enabled,
and the code is simplified and will later support runtime options.

3 years agoMINOR: pools: add a new debugging flag POOL_DBG_INTEGRITY
Willy Tarreau [Mon, 21 Feb 2022 17:42:53 +0000 (18:42 +0100)] 
MINOR: pools: add a new debugging flag POOL_DBG_INTEGRITY

The test to decide whether or not to enforce integrity checks on cached
objects is now enabled at runtime and conditionned by this new debugging
flag. While previously it was not a concern to inflate the code size by
keeping the two functions static, they were moved to pool.c to limit the
impact. In pool_get_from_cache(), the fast code path remains fast by
having both flags tested at once to open a slower branch when either
POOL_DBG_COLD_FIRST or POOL_DBG_INTEGRITY are set.

3 years agoMINOR: pools: add a new debugging flag POOL_DBG_COLD_FIRST
Willy Tarreau [Mon, 21 Feb 2022 17:30:25 +0000 (18:30 +0100)] 
MINOR: pools: add a new debugging flag POOL_DBG_COLD_FIRST

When enabling pools integrity checks, we usually prefer to allocate cold
objects first in order to maximize the time the objects spend in the
cache. In order to make this configurable at runtime, let's introduce
a new debugging flag to control this allocation order. It is currently
preset by the DEBUG_POOL_INTEGRITY build-time setting.

3 years agoMINOR: pools: switch DEBUG_DONT_SHARE_POOLS to runtime
Willy Tarreau [Mon, 21 Feb 2022 16:31:50 +0000 (17:31 +0100)] 
MINOR: pools: switch DEBUG_DONT_SHARE_POOLS to runtime

This test used to appear at a single location in create_pool() to
enable a check on the pool name or unconditionally merge similarly
sized pools.

This patch introduces POOL_DBG_DONT_MERGE and conditions the test on
this new runtime flag, that is preset according to the aforementioned
debugging option.

3 years agoMINOR: pools: switch the fail-alloc test to runtime only
Willy Tarreau [Mon, 21 Feb 2022 16:16:22 +0000 (17:16 +0100)] 
MINOR: pools: switch the fail-alloc test to runtime only

The fail-alloc test used to be enabled/disabled at build time using
the DEBUG_FAIL_ALLOC macro, but it happens that the cost of the test
is quite cheap and that it can be enabled as one of the pool_debugging
options.

This patch thus introduces the first POOL_DBG_FAIL_ALLOC option, whose
default value depends on DEBUG_FAIL_ALLOC. The mem_should_fail() function
is now always built, but it was made static since it's never used outside.

3 years agoMINOR: pools: introduce a new pool_debugging global variable
Willy Tarreau [Fri, 18 Feb 2022 17:35:59 +0000 (18:35 +0100)] 
MINOR: pools: introduce a new pool_debugging global variable

This read-mostly variable will be used at runtime to enable/disable
certain pool-debugging features and will be set by the command-line
parser. A future option -dP will take a number of debugging features
as arguments to configure this variable's contents.

3 years agoMINOR: pools: disable redundant poisonning on pool_free()
Willy Tarreau [Wed, 23 Feb 2022 10:45:09 +0000 (11:45 +0100)] 
MINOR: pools: disable redundant poisonning on pool_free()

The poisonning performed on pool_free() used to help a little bit with
use-after-free detection, but usually did more harm than good in that
it was never possible to perform post-mortem analysis on released
objects once poisonning was enabled on allocation. Now that there is
a dedicated DEBUG_POOL_INTEGRITY, let's get rid of this annoyance
which is not even documented in the management manual.

3 years agoCLEANUP: init: remove the ifdef on HAPROXY_MEMMAX
Willy Tarreau [Thu, 17 Feb 2022 16:15:02 +0000 (17:15 +0100)] 
CLEANUP: init: remove the ifdef on HAPROXY_MEMMAX

It's ugly, let's move it to defaults.h with all other ones and preset
it to zero if not defined.

3 years agoCLEANUP: vars: move the per-process variables initialization to vars.c
Willy Tarreau [Thu, 17 Feb 2022 15:47:03 +0000 (16:47 +0100)] 
CLEANUP: vars: move the per-process variables initialization to vars.c

There's no point keeping the vars_init_head() call in init() when we
already have a vars_init() registered at the right time to do that,
and it complexifies the boot sequence, so let's move it there.

3 years agoCLEANUP: muxes: do not use a dynamic trash in list_mux_protos()
Willy Tarreau [Fri, 18 Feb 2022 10:07:40 +0000 (11:07 +0100)] 
CLEANUP: muxes: do not use a dynamic trash in list_mux_protos()

Let's not use a trash there anymore. The function is called at very
early boot (for "haproxy -vv"), and the need for a trash prevents the
arguments from being parsed earlier. Moreover, the function only uses
a FILE* on output with fprintf(), so there's not even any benefit in
using chunk_printf() on an intermediary variable, emitting the output
directly is both clearer and safer.

3 years agoCLEANUP: httpclient: initialize the client in stage INIT not REGISTER
Willy Tarreau [Fri, 18 Feb 2022 15:23:14 +0000 (16:23 +0100)] 
CLEANUP: httpclient: initialize the client in stage INIT not REGISTER

REGISTER is meant to only assemble static lists, not to initialize
code that may depend on some elements possibly initialized at this
level. For example the init code currently looks up transport protocols
such as XPRT_RAW and XPRT_SSL which ought to be themselves registered
from at REGISTER stage, and which currently work only because they're
still registered directly from a constructor. INIT is perfectly suited
for this level.

3 years agoDOC: httpclient/lua: fix the type of the dst parameter
William Lallemand [Wed, 23 Feb 2022 14:57:45 +0000 (15:57 +0100)] 
DOC: httpclient/lua: fix the type of the dst parameter

"dst" is of string type.

3 years agoBUG/MINOR: httpclient/lua: missing pop for new timeout parameter
William Lallemand [Wed, 23 Feb 2022 14:16:08 +0000 (15:16 +0100)] 
BUG/MINOR: httpclient/lua: missing pop for new timeout parameter

The lua timeout server lacks a lua_pop(), breaking the lua stack.

No backported needed.

3 years agoMINOR: httpclient/lua: ability to set a server timeout
William Lallemand [Wed, 23 Feb 2022 13:18:16 +0000 (14:18 +0100)] 
MINOR: httpclient/lua: ability to set a server timeout

Add the ability to set a "server timeout" on the httpclient with either
the httpclient_set_timeout() API or the timeout argument in a request.

Issue #1470.

3 years agoBUG/MEDIUM: stream: Abort processing if response buffer allocation fails
Christopher Faulet [Tue, 1 Feb 2022 17:53:53 +0000 (18:53 +0100)] 
BUG/MEDIUM: stream: Abort processing if response buffer allocation fails

In process_stream(), we force the response buffer allocation before any
processing to be able to return an error message. It is important because,
when an error is triggered, the stream is immediately closed. Thus we cannot
wait for the response buffer allocation.

When the allocation fails, the stream analysis is stopped and the expiration
date of the stream's task is updated before exiting process_stream(). But if
the stream was woken up because of a connection or an analysis timeout, the
expiration date remains blocked in the past. This means the stream is woken
up in loop as long as the response buffer is not properly allocated.

Alone, this behavior is already a bug. But because the mechanism to handle
buffer allocation failures is totally broken since a while, this bug becomes
more problematic. Because, most of time, the watchdog will kill HAProxy in
this case because it will detect a spinning loop.

To fix it, at least temporarily, an allocation failure at this stage is now
reported as an error and the processing is aborted. It's not satisfying but
it is better than nothing. If the buffers allocation mechanism is
refactored, this part will be reviewed.

This patch must be backported, probably as far as 2.0. It may be perceived
as a regression, but the actual behavior is probably even worse. And
because it was not reported, it is probably not a common situation.

3 years agoREGTESTS: fix the race conditions in 40be_2srv_odd_health_checks
Willy Tarreau [Mon, 21 Feb 2022 19:44:00 +0000 (20:44 +0100)] 
REGTESTS: fix the race conditions in 40be_2srv_odd_health_checks

This one started to randomly fail on me again and I could figure the
problem. It mixes one checked server with one unchecked on in each
backend, and tries to make sure that each checked server receives
exactly one request. But that doesn't work and is entirely time-
dependent because if the check starts before the client, a pure
TCP check is sent to the server, which sees an aborted connection
and makes the whole check fail.

Here what is done is that we make sure that only the second server
and not the first one is checked. The traffic is delivered to all
first servers, and each HTTP server must always receive a valid HTTP
request. In parallel, checks must not fail as they're delivered to
dummy servers. The check doesn't fail anymore, even when started on
a single thread at nice +5 while 8 processes are fighting on the same
core to inject HTTP traffic at 25 Gbps, which used to systematically
make it fail previously.

Since it took more than one hour to fix the "expect" line for the stats
output, I did it using a small script that I pasted into the vtc file
in case it's needed later. The relevance of this test is questionable
once its complexity is factored in. Let's keep it as long as it works
without too much effort.

3 years agoCLEANUP: pools: remove the now unused pool_is_crowded()
Willy Tarreau [Mon, 21 Feb 2022 18:06:07 +0000 (19:06 +0100)] 
CLEANUP: pools: remove the now unused pool_is_crowded()

This function was renderred obsolete by commit a0b5831ee ("MEDIUM: pools:
centralize cache eviction in a common function") which replaced its last
call inside the loop with a single call out of the loop to pool_releasable()
as introduced by commit 91a8e28f9 ("MINOR: pool: add a function to estimate
how many may be released at once"). Let's remove it before it becomes wrong
and used again.

3 years agoMINOR: pools: mark most static pool configuration variables as read-mostly
Willy Tarreau [Fri, 18 Feb 2022 17:31:53 +0000 (18:31 +0100)] 
MINOR: pools: mark most static pool configuration variables as read-mostly

The mem_poison_byte, mem_fail_rate, using_default_allocator and the
pools list are all only set once at boot time and never changed later,
while they're heavily used at run time. Let's optimize their usage from
all threads by marking them read-mostly so that them reside in a shared
cache line.

3 years agoMINOR: quic: fix handling of out-of-order received STREAM frames
Amaury Denoyelle [Mon, 21 Feb 2022 18:08:44 +0000 (19:08 +0100)] 
MINOR: quic: fix handling of out-of-order received STREAM frames

The recent changes was not complete.
  d1c76f24fdf1cfb85e574cb1ef0c773b74bee32a
  MINOR: quic: do not modify offset node if quic_rx_strm_frm in tree

The frame length and data pointer should incremented after the data
copy. A BUG_ON statement has been added to detect an incorrect decrement
operaiton.

3 years agoMINOR: mux-quic: fix uninitialized return on qc_send
Amaury Denoyelle [Mon, 21 Feb 2022 17:45:22 +0000 (18:45 +0100)] 
MINOR: mux-quic: fix uninitialized return on qc_send

This should fix the github issue #1562.

3 years agoMINOR: h3: fix compiler warning variable set but not used
Amaury Denoyelle [Mon, 21 Feb 2022 17:38:29 +0000 (18:38 +0100)] 
MINOR: h3: fix compiler warning variable set but not used

Some variables were only checked via BUG_ON macro. If compiling without
DEBUG_STRICT, this instruction is a noop. Fix this by using an explicit
condition + ABORT_NOW.

This should fix the github issue #1549.

3 years agoMINOR: quic: do not modify offset node if quic_rx_strm_frm in tree
Amaury Denoyelle [Mon, 21 Feb 2022 16:53:38 +0000 (17:53 +0100)] 
MINOR: quic: do not modify offset node if quic_rx_strm_frm in tree

qc_rx_strm_frm_cpy is unsafe because it updates the offset field of the
frame. This is not safe as the frame is inserted in the tree when
calling this function and offset serves as the key node.

To fix this, the API is modified so that qc_rx_strm_frm_cpy does not
update the frame parameter. The caller is responsible to update
offset/length in case of a partial copy.

The impact of this bug is not known. It can only happened with received
STREAM frames out-of-order. This might be triggered with large h3 POST
requests.

3 years agoDEBUG: stream-int: Check CS_FL_WANT_ROOM is not set with an empty input buffer
Christopher Faulet [Mon, 21 Feb 2022 15:12:00 +0000 (16:12 +0100)] 
DEBUG: stream-int: Check CS_FL_WANT_ROOM is not set with an empty input buffer

In si_cs_recv(), the mux must never set CS_FL_WANT_ROOM flag on the
conn-stream if the input buffer is empty and nothing was copied. It is
important because, there is nothing the app layer can do in this case to
make some room. If this happens, this will most probably lead to a ping-pong
loop between the mux and the stream.

With this BUG_ON(), it will be easier to spot such bugs.

3 years agoBUG/MAJOR: mux-h2: Be sure to always report HTX parsing error to the app layer
Christopher Faulet [Mon, 21 Feb 2022 14:12:54 +0000 (15:12 +0100)] 
BUG/MAJOR: mux-h2: Be sure to always report HTX parsing error to the app layer

If a parsing error is detected and the corresponding HTX flag is set
(HTX_FL_PARSING_ERROR), we must be sure to always report it to the app
layer. It is especially important when the error occurs during the response
parsing, on the server side. In this case, the RX buffer contains an empty
HTX message to carry the flag. And it remains in this state till the info is
reported to the app layer. This must be done otherwise, on the conn-stream,
the CS_FL_ERR_PENDING flag cannot be switched to CS_FL_ERROR and the
CS_FL_WANT_ROOM flag is always set when h2_rcv_buf() is called. The result
is a ping-pong loop between the mux and the stream.

Note that this patch fixes a bug. But it also reveals a design issue. The
error must not be reported at the HTX level. The error is already carried by
the conn-stream. There is no reason to duplicate it. In addition, it is
errorprone to have an empty HTX message only to report the error to the app
layer.

This patch should fix the issue #1561. It must be backported as far as 2.0
but the bug only affects HAProxy >= 2.4.

3 years agoBUG/MEDIUM: mux-h1: Don't wake h1s if mux is blocked on lack of output buffer
Christopher Faulet [Tue, 1 Feb 2022 17:25:06 +0000 (18:25 +0100)] 
BUG/MEDIUM: mux-h1: Don't wake h1s if mux is blocked on lack of output buffer

After sending some data, we try to wake the H1 stream to resume data
processing at the stream level, except if the output buffer is still
full. However we must also be sure the mux is not blocked because of an
allocation failure on this buffer. Otherwise, it may lead to a ping-pong
loop between the stream and the mux to send more data with an unallocated
output buffer.

Note there is a mechanism to queue buffers allocations when a failure
happens. However this mechanism is totally broken since the filters were
introducted in HAProxy 1.7. And it is worse now with the multiplexers. So
this patch fixes a possible loop needlessly consuming all the CPU. But
buffer allocation failures must remain pretty rare.

This patch must be backported as far as 2.0.

3 years agoBUG/MEDIUM: htx: Be sure to have a buffer to perform a raw copy of a message
Christopher Faulet [Tue, 1 Feb 2022 17:11:50 +0000 (18:11 +0100)] 
BUG/MEDIUM: htx: Be sure to have a buffer to perform a raw copy of a message

In htx_copy_msg(), if the destination buffer is empty, we perform a raw copy
of the message instead of a copy block per block. But we must be sure the
destianation buffer was really allocated. In other word, to perform a raw
copy, the HTX message must be empty _AND_ it must have some free space
available.

This function is only used to copy an HTTP reply (for instance, an error or
a redirect) in the buffer of the response channel. For now, we are sure the
buffer was allocated because it is a pre-requisite to call stream
analyzers. However, it may be a source of bug in future.

This patch may be backported as far as 2.3.

3 years agoMINOR: mux-quic: fix a possible null dereference in qc_timeout_task
Amaury Denoyelle [Mon, 21 Feb 2022 09:05:16 +0000 (10:05 +0100)] 
MINOR: mux-quic: fix a possible null dereference in qc_timeout_task

The qcc instance should be tested as it is implied by a previous test
that it may be NULL. In this case, qc_timeout_task can be stopped.

This should fix github issue #1559.

3 years agoDEBUG: buffer: check in __b_put_blk() whether the buffer room is respected
Willy Tarreau [Fri, 18 Feb 2022 16:33:27 +0000 (17:33 +0100)] 
DEBUG: buffer: check in __b_put_blk() whether the buffer room is respected

This adds a BUG_ON() to make sure we don't face other situations like the
one fixed by previous commit.

3 years agoBUG/MEDIUM: httpclient: limit transfers to the maximum available room
Willy Tarreau [Fri, 18 Feb 2022 16:28:25 +0000 (17:28 +0100)] 
BUG/MEDIUM: httpclient: limit transfers to the maximum available room

A bug was uncovered by commit fc5912914 ("MINOR: httpclient: Don't limit
data transfer to 1024 bytes"), it happens that callers of b_xfer() and
b_force_xfer() are expected to check for available room in the target
buffer. Previously it was unlikely to be full but now with full buffer-
sized transfers, it happens more often and in practice it is possible
to crash the process with the debug command "httpclient" on the CLI by
going beyond a the max buffer size. Other call places ought to be
rechecked by now and it might be time to rethink this API if it tends
to generalize.

This must be backported to 2.5.

3 years agoBUG/MINOR: tools: url2sa reads ipv4 too far
William Lallemand [Fri, 18 Feb 2022 15:13:12 +0000 (16:13 +0100)] 
BUG/MINOR: tools: url2sa reads ipv4 too far

The url2sa implementation is inconsitent when parsing an IPv4, indeed
url2sa() takes a <ulen> as a parameter where the call to url2ipv4() takes
a null terminated string. Which means url2ipv4 could try to read more
that it is supposed to.

This function is only used from a buffer so it never reach a unallocated
space. It can only cause an issue when used from the httpclient which
uses it with an ist.

This patch fixes the issue by copying everything in the trash and
null-terminated it.

Must be backported in all supported version.

3 years agoCLEANUP: httpclient/cli: fix indentation alignment of the help message
Willy Tarreau [Fri, 18 Feb 2022 15:26:36 +0000 (16:26 +0100)] 
CLEANUP: httpclient/cli: fix indentation alignment of the help message

The output was not aligned with other commands, let's fix it.

3 years agoBUG/MINOR: ssl: Missing return value check in ssl_ocsp_response_print
Remi Tricot-Le Breton [Wed, 16 Feb 2022 14:17:09 +0000 (15:17 +0100)] 
BUG/MINOR: ssl: Missing return value check in ssl_ocsp_response_print

When calling ssl_ocsp_response_print which is used to display an OCSP
response's details when calling the "show ssl ocsp-response" on the CLI,
we use the BIO_read function that copies an OpenSSL BIO into a trash.
The return value was not checked though, which could lead to some
crashes since BIO_read can return a negative value in case of error.

This patch should be backported to 2.5.

3 years agoBUG/MINOR: ssl: Fix leak in "show ssl ocsp-response" CLI command
Remi Tricot-Le Breton [Wed, 16 Feb 2022 14:03:51 +0000 (15:03 +0100)] 
BUG/MINOR: ssl: Fix leak in "show ssl ocsp-response" CLI command

When calling the "show ssl ocsp-response" CLI command some OpenSSL
objects need to be created in order to get some information related to
the OCSP response and some of them were not freed.

It should be backported to 2.5.

3 years agoBUG/MINOR: ssl: Add missing return value check in ssl_ocsp_response_print
Remi Tricot-Le Breton [Wed, 16 Feb 2022 13:42:22 +0000 (14:42 +0100)] 
BUG/MINOR: ssl: Add missing return value check in ssl_ocsp_response_print

The b_istput function called to append the last data block to the end of
an OCSP response's detailed output was not checked in
ssl_ocsp_response_print. The ssl_ocsp_response_print return value checks
were added as well since some of them were missing.
This error was raised by Coverity (CID 1469513).

This patch fixes GitHub issue #1541.
It can be backported to 2.5.

3 years agoMINOR: httpclient/lua: add 'dst' optionnal field
William Lallemand [Thu, 17 Feb 2022 19:00:23 +0000 (20:00 +0100)] 
MINOR: httpclient/lua: add 'dst' optionnal field

The 'dst' optionnal field on a httpclient request can be used to set an
alternative server address in the haproxy address format. Which means it
could be use with unix@, ipv6@ etc.

Should fix issue #1471.

3 years agoMINOR: httpclient: sets an alternative destination
William Lallemand [Thu, 17 Feb 2022 18:10:55 +0000 (19:10 +0100)] 
MINOR: httpclient: sets an alternative destination

httpclient_set_dst() allows to set an alternative destination address
using HAProxy addres format. This will ignore the address within the
URL.

3 years agoBUG/MINOR: mailers: negotiate SMTP, not ESMTP
Lukas Tribus [Thu, 17 Feb 2022 14:40:51 +0000 (15:40 +0100)] 
BUG/MINOR: mailers: negotiate SMTP, not ESMTP

As per issue #1552 the mailer code currently breaks on ESMTP multiline
responses. Let's negotiate SMTP instead.

Should be backported to 2.0.

3 years agoBUG/MINOR: httpclient: reinit flags in httpclient_start()
William Lallemand [Thu, 17 Feb 2022 11:52:09 +0000 (12:52 +0100)] 
BUG/MINOR: httpclient: reinit flags in httpclient_start()

When starting for the 2nd time a request from the same httpclient *hc
context, the flags are not reinitialized and the httpclient will stop
after the first call to the IO handler, because the END flag is always
present.

This patch also add a test before httpclient_start() to ensure we don't
start a client already started.

Must be backported in 2.5.

3 years agoBUG/MINOR: mux-h2: update the session's idle delay before creating the stream
Willy Tarreau [Fri, 4 Feb 2022 08:05:37 +0000 (09:05 +0100)] 
BUG/MINOR: mux-h2: update the session's idle delay before creating the stream

The idle connection delay calculation before a request is a bit tricky,
especially for multiplexed protocols. It changed between 2.3 and 2.4 by
the integration of the idle delay inside the session itself with these
commits:

  dd78921c6 ("MINOR: logs: Use session idle duration when no stream is provided")
  7a6c51324 ("MINOR: stream: Always get idle duration from the session")

and by then it was only set by the H1 mux. But over multiple changes, what
used to be a zero idle delay + a request delay for H2 became a bit odd, with
the idle time slipping into the request time measurement. The effect is that,
as reported in GH issue #1395, some H2 request times look huge.

This patch introduces the calculation of the session's idle time on the
H2 mux before creating the stream. This is made possible because the
stream_new() code immediately copies this value into the stream for use
at log time. Thus we don't care about changing something that will be
touched by every single request. The idle time is calculated as documented,
i.e. the delay from the previous request to the current one. This also
means that when a single stream is present on a connection, a part of
the server's response time may appear in the %Ti measurement, but this
reflects the reality since nothing would prevent the client from using
the connection to fetch more objects. In addition this shows how long
it takes a client to find references to objects in an HTML page and
start to fetch them.

A different approach could have consisted in counting from the last time
the connection was left without any request (i.e. really idle), but this
would at least require a documentation change and it's not certain this
would provide a more useful information.

Thanks to Bart Butler and Luke Seelenbinder for reporting enough elements
to diagnose this issue.

This should be backported to 2.4.

3 years agoBUG/MEDIUM: h2/hpack: fix emission of HPACK DTSU after settings change
Willy Tarreau [Wed, 16 Feb 2022 13:28:14 +0000 (14:28 +0100)] 
BUG/MEDIUM: h2/hpack: fix emission of HPACK DTSU after settings change

Sadly, despite particular care, commit 39a0a1e12 ("MEDIUM: h2/hpack: emit
a Dynamic Table Size Update after settings change") broke H2 when sending
DTSU. A missing negation on the flag caused the DTSU_EMITTED flag to be
lost and the DTSU to be sent again on the next stream, and possibly to
break flow control or a few other internal states.

This will have to be backported wherever the patch above was backported.

Thanks to Yves Lafon for notifying us with elements to reproduce the
issue!

3 years agoREGTESTS: peers: leave a bit more time to peers to synchronize
Willy Tarreau [Wed, 16 Feb 2022 10:28:09 +0000 (11:28 +0100)] 
REGTESTS: peers: leave a bit more time to peers to synchronize

tls_basic_sync_wo_stkt_backend fails once every 200 runs for me. This
seems to be because the startup delay doesn't always allow peers to
perform a simultaneous connect, close and new attempt. With 3s I can't
see it fail anymore. In addition the long "delay 0.2" are still way too
much since we do not really care about the startup order in practice.

3 years agoREGTESTS: server: close an occasional race on dynamic_server_ssl.vtc
Willy Tarreau [Wed, 16 Feb 2022 09:45:23 +0000 (10:45 +0100)] 
REGTESTS: server: close an occasional race on dynamic_server_ssl.vtc

Sometimes when sending commands to shut down a server, haproxy complains
that some connections remain, this is because the server-side connection
might not always be completely released at the moment the client leaves
and the operation is emitted. While shutting down server sessions work,
it seems cleaner to just use "option httpclose" which releases the server
earlier and avoids the race.

This can be backported to 2.5.

3 years agoBUG/MAJOR: spoe: properly detach all agents when releasing the applet
Willy Tarreau [Tue, 15 Feb 2022 15:49:37 +0000 (16:49 +0100)] 
BUG/MAJOR: spoe: properly detach all agents when releasing the applet

There's a bug in spoe_release_appctx() which checks the presence of items
in the wrong list rt[tid].agents to run over rt[tid].waiting_queue and
zero their spoe_appctx. The effect is that these contexts are not zeroed
and if spoe_stop_processing() is called, "sa->cur_fpa--" will be applied
to one of these recently freed contexts and will corrupt random memory
locations, as found at least in bugs #1494 and #1525.

This must be backported to all stable versions.

Many thanks to Christian Ruppert from Babiel for exchanging so many
useful traces over the last two months, testing debugging code and
helping set up a similar environment to reproduce it!

3 years agoBUG/MAJOR: http/htx: prevent unbounded loop in http_manage_server_side_cookies
Andrew McDermott [Fri, 11 Feb 2022 18:26:49 +0000 (18:26 +0000)] 
BUG/MAJOR: http/htx: prevent unbounded loop in http_manage_server_side_cookies

Ensure calls to http_find_header() terminate. If a "Set-Cookie2"
header is found then the while(1) loop in
http_manage_server_side_cookies() will never terminate, resulting in
the watchdog firing and the process terminating via SIGABRT.

The while(1) loop becomes unbounded because an unmatched call to
http_find_header("Set-Cookie") will leave ctx->blk=NULL. Subsequent
calls to check for "Set-Cookie2" will now enumerate from the beginning
of all the blocks and will once again match on subsequent
passes (assuming a match first time around), hence the loop becoming
unbounded.

This issue was introduced with HTX and this fix should be backported
to all versions supporting HTX.

Many thanks to Grant Spence (gspence@redhat.com) for working through
this issue with me.

3 years agoMINOR: h3: remove unused return value on decode_qcs
Amaury Denoyelle [Wed, 16 Feb 2022 13:35:10 +0000 (14:35 +0100)] 
MINOR: h3: remove unused return value on decode_qcs

This should fix 1470806 coverity report from github issue #1550.

3 years agoBUG/MINOR: httpclient/cli: display junk characters in vsn
William Lallemand [Wed, 16 Feb 2022 10:37:02 +0000 (11:37 +0100)] 
BUG/MINOR: httpclient/cli: display junk characters in vsn

ist are not ended by '\0', leading to junk characters being displayed
when using %s for printing the HTTP start line.

Fix the issue by replacing %s by %.*s + istlen.

Must be backported in 2.5.

3 years agoBUG/MINOR: jwt: Memory leak if same key is used in multiple jwt_verify calls
Remi Tricot-Le Breton [Fri, 4 Feb 2022 13:24:15 +0000 (14:24 +0100)] 
BUG/MINOR: jwt: Memory leak if same key is used in multiple jwt_verify calls

If the same filename was specified in multiple calls of the jwt_verify
converter, we would have parsed the contents of the file every time it
was used instead of checking if the entry already existed in the tree.
This lead to memory leaks because we would not insert the duplicated
entry and we would not free it (as well as the EVP_PKEY it referenced).
We now check the return value of ebst_insert and free the current entry
if it is a duplicate of an existing entry.
The order in which the tree insert and the pkey parsing happen was also
switched in order to avoid parsing key files in case of duplicates.

Should be backported to 2.5.

3 years agoBUG/MINOR: jwt: Missing pkey free during cleanup
Remi Tricot-Le Breton [Fri, 4 Feb 2022 13:21:02 +0000 (14:21 +0100)] 
BUG/MINOR: jwt: Missing pkey free during cleanup

When emptying the jwt_cert_tree during deinit, the entries are freed but
not the EVP_PKEY reference they kept, leading in a memory leak.

Should be backported in 2.5.

3 years agoBUG/MINOR: jwt: Double free in deinit function
Remi Tricot-Le Breton [Fri, 4 Feb 2022 13:06:34 +0000 (14:06 +0100)] 
BUG/MINOR: jwt: Double free in deinit function

The node pointer was not moving properly along the jwt_cert_tree during
the deinit which ended in a double free during cleanup (or when checking
a configuration that used the jwt_verify converter with an explicit
certificate specified).

This patch fixes GitHub issue #1533.
It should be backported to 2.5.

3 years agoMINOR: h3: report error on HEADERS/DATA parsing
Amaury Denoyelle [Tue, 15 Feb 2022 16:30:27 +0000 (17:30 +0100)] 
MINOR: h3: report error on HEADERS/DATA parsing

Inspect return code of HEADERS/DATA parsing functions and use a BUG_ON
to signal an error. The stream should be closed to handle the error
in a more clean fashion.

3 years agoMINOR: quic: Move quic_rxbuf_pool pool out of xprt part
Frédéric Lécaille [Tue, 15 Feb 2022 15:59:48 +0000 (16:59 +0100)] 
MINOR: quic: Move quic_rxbuf_pool pool out of xprt part

This pool could be confuse with that of the RX buffer pool for the connection
(quic_conn_rxbuf).

3 years agoMINOR: quic: Do not retransmit too much packets.
Frédéric Lécaille [Tue, 15 Feb 2022 11:00:55 +0000 (12:00 +0100)] 
MINOR: quic: Do not retransmit too much packets.

We retranmist at most one datagram and possibly one more with only PING frame
as ack-eliciting frame.

3 years agoMINOR: quic: Possible frame parsers array overrun
Frédéric Lécaille [Tue, 15 Feb 2022 09:27:34 +0000 (10:27 +0100)] 
MINOR: quic: Possible frame parsers array overrun

This should fix CID 1469663 for GH #1546.

3 years agoMINOR: quic: Non checked returned value for cs_new() in h3_decode_qcs()
Frédéric Lécaille [Tue, 15 Feb 2022 08:25:06 +0000 (09:25 +0100)] 
MINOR: quic: Non checked returned value for cs_new() in h3_decode_qcs()

This should fix CID 1469664 for GH #1546

3 years agoMINOR: h3: Dead code in h3_uqs_init()
Frédéric Lécaille [Tue, 15 Feb 2022 08:15:47 +0000 (09:15 +0100)] 
MINOR: h3: Dead code in h3_uqs_init()

This should fix CID 1469657 for GH #1546.

3 years agoMINOR: quic: Non checked returned value for cs_new() in hq_interop_decode_qcs()
Frédéric Lécaille [Tue, 15 Feb 2022 08:13:05 +0000 (09:13 +0100)] 
MINOR: quic: Non checked returned value for cs_new() in hq_interop_decode_qcs()

This should fix CID 1469657 for GH #1546

3 years agoMINOR: quic: Useless test in quic_lstnr_dghdlr()
Frédéric Lécaille [Tue, 15 Feb 2022 07:58:07 +0000 (08:58 +0100)] 
MINOR: quic: Useless test in quic_lstnr_dghdlr()

This statement is useless. This should fix CID 1469651 for GH #1546.

3 years agoMINOR: quic: Avoid warning about NULL pointer dereferences
Frédéric Lécaille [Mon, 14 Feb 2022 18:01:21 +0000 (19:01 +0100)] 
MINOR: quic: Avoid warning about NULL pointer dereferences

This is the same fixe as for this commit:
    "BUILD: tree-wide: avoid warnings caused by redundant checks of obj_types"
Should fix CID 1469649 for GH #1546

3 years agoMINOR: quic: ha_quic_set_encryption_secrets without server specific code
Frédéric Lécaille [Mon, 14 Feb 2022 16:54:04 +0000 (17:54 +0100)] 
MINOR: quic: ha_quic_set_encryption_secrets without server specific code

Remove this server specific code section. It is useless, not tested. Furthermore
this is really not the good place to retrieve the peer transport parameters.

3 years agoMINOR: quic: Code never reached in qc_ssl_sess_init()
Frédéric Lécaille [Mon, 14 Feb 2022 16:32:50 +0000 (17:32 +0100)] 
MINOR: quic: Code never reached in qc_ssl_sess_init()

There was a remaining useless statement in this code block.
This fixes CID 1469648 for GH #1546

3 years agoMINOR: quic: Wrong loss delay computation
Frédéric Lécaille [Mon, 14 Feb 2022 12:56:42 +0000 (13:56 +0100)] 
MINOR: quic: Wrong loss delay computation

I really do not know where does this statement come from even after having
checked several drafts.

3 years agoMINOR: quic: Wrong smoothed rtt initialization
Frédéric Lécaille [Mon, 14 Feb 2022 09:17:01 +0000 (10:17 +0100)] 
MINOR: quic: Wrong smoothed rtt initialization

In ->srtt we store 8*srtt to ease the srtt computations with this formula:
    srtt = 7/8 * srtt + 1/8 * adjusted_rtt
But its initialization was wrong.

3 years agoMINOR: h3: implement DATA parsing
Amaury Denoyelle [Mon, 14 Feb 2022 16:14:59 +0000 (17:14 +0100)] 
MINOR: h3: implement DATA parsing

Add a new function h3_data_to_htx. This function is used to parse a H3
DATA frame and copy it in the mux stream HTX buffer. This is required to
support HTTP POST data.

Note that partial transfers if the HTX buffer is fulled is not properly
handle. This causes large DATA transfer to fail at the moment.

3 years agoMINOR: h3: extract HEADERS parsing in a dedicated function
Amaury Denoyelle [Mon, 14 Feb 2022 16:13:55 +0000 (17:13 +0100)] 
MINOR: h3: extract HEADERS parsing in a dedicated function

Move the HEADERS parsing code outside of generic h3_decode_qcs to a new
dedicated function h3_headers_to_htx. The benefit will be visible when
other H3 frames parsing will be implemented such as DATA.

3 years agoMINOR: h3: report frames bigger than rx buffer
Amaury Denoyelle [Tue, 15 Feb 2022 15:59:39 +0000 (16:59 +0100)] 
MINOR: h3: report frames bigger than rx buffer

If a frame is bigger than the qcs buffer, it can not be parsed at the
moment. Add a TODO comment to signal that a fix is required.

3 years agoMINOR: h3: set CS_FL_NOT_FIRST
Amaury Denoyelle [Mon, 14 Feb 2022 16:14:35 +0000 (17:14 +0100)] 
MINOR: h3: set CS_FL_NOT_FIRST

When creating a new conn-stream on H3 HEADERS parsing, the flag
CS_FL_NOT_FIRST must be set. This is identical to the mux-h2.

3 years agoMINOR: mux-quic: set EOS on rcv_buf
Amaury Denoyelle [Mon, 14 Feb 2022 16:11:32 +0000 (17:11 +0100)] 
MINOR: mux-quic: set EOS on rcv_buf

Flags EOI/EOS must be set on conn-stream when transfering the last data
of a stream in rcv_buf. This is activated if qcs HTX buffer has the EOM
flag and has been fully transfered.

3 years agoMINOR: mux-quic: implement rcv_buf
Amaury Denoyelle [Mon, 14 Feb 2022 16:11:09 +0000 (17:11 +0100)] 
MINOR: mux-quic: implement rcv_buf

Implement the stream rcv_buf operation on QUIC mux.

A new buffer is stored in qcs structure named app_buf. This new buffer
will contains HTX and will be filled for example on H3 DATA frame
parsing.

The rcv_buf operation transfer as much as possible data from the HTX
from app_buf to the conn-stream buffer. This is mainly identical to
mux-h2. This is required to support HTTP POST data.

3 years agoMINOR: h3: set properly HTX EOM/BODYLESS on HEADERS parsing
Amaury Denoyelle [Mon, 14 Feb 2022 14:49:53 +0000 (15:49 +0100)] 
MINOR: h3: set properly HTX EOM/BODYLESS on HEADERS parsing

Adjust the method to detect that a H3 HEADERS frame is the last one of
the stream. If this is true, the flags EOM and BODYLESS must be set on
the HTX message.

3 years agoMINOR: h3: add documentation on h3_decode_qcs
Amaury Denoyelle [Tue, 15 Feb 2022 10:05:46 +0000 (11:05 +0100)] 
MINOR: h3: add documentation on h3_decode_qcs

Specify the purpose of the fin argument on h3_decode_qcs.

3 years agoMINOR: h3: remove transfer-encoding header
Amaury Denoyelle [Tue, 15 Feb 2022 15:10:42 +0000 (16:10 +0100)] 
MINOR: h3: remove transfer-encoding header

According to HTTP/3 specification, transfer-encoding header must not be
used in HTTP/3 messages. Remove it when converting HTX responses to
HTTP/3.

3 years agoBUG/MINOR: h3: fix the header length for QPACK decoding
Amaury Denoyelle [Mon, 14 Feb 2022 13:38:55 +0000 (14:38 +0100)] 
BUG/MINOR: h3: fix the header length for QPACK decoding

Pass the H3 frame length to QPACK decoding instead of the length of the
whole buffer.

Without this fix, if there is multiple H3 frames starting with a
HEADERS, QPACK decoding will be erroneously applied over all of them,
most probably leading to a decoding error.

3 years agoBUG/MINOR: quic: fix FIN stream signaling
Amaury Denoyelle [Tue, 15 Feb 2022 09:57:16 +0000 (10:57 +0100)] 
BUG/MINOR: quic: fix FIN stream signaling

If the last frame is not entirely copied and must be buffered, FIN
must not be signaled to the upper layer.

This might fix a rare bug which could cause the request channel to be
closed too early leading to an incomplete request.

3 years agoMINOR: qpack: fix typo in trace
Amaury Denoyelle [Mon, 14 Feb 2022 13:45:10 +0000 (14:45 +0100)] 
MINOR: qpack: fix typo in trace

hanme -> hname

3 years agoBUG/MEDIUM: quic: fix crash on CC if mux not present
Amaury Denoyelle [Tue, 15 Feb 2022 10:06:15 +0000 (11:06 +0100)] 
BUG/MEDIUM: quic: fix crash on CC if mux not present

If a CONNECTION_CLOSE is received during handshake or after mux release,
a segfault happens due to invalid dereferencement of qc->qcc. Check
mux_state first to prevent this.

3 years agoMINOR: quic: use a global dghlrs for each thread
Amaury Denoyelle [Tue, 8 Feb 2022 14:03:40 +0000 (15:03 +0100)] 
MINOR: quic: use a global dghlrs for each thread

Move the QUIC datagram handlers oustide of the receivers. Use a global
handler per-thread which is allocated on post-config. Implement a free
function on process deinit to avoid a memory leak.

3 years agoBUG/MAJOR: sched: prevent rare concurrent wakeup of multi-threaded tasks
Willy Tarreau [Mon, 14 Feb 2022 09:18:51 +0000 (10:18 +0100)] 
BUG/MAJOR: sched: prevent rare concurrent wakeup of multi-threaded tasks

Since the relaxation of the run-queue locks in 2.0 there has been a
very small but existing race between expired tasks and running tasks:
a task might be expiring and being woken up at the same time, on
different threads. This is protected against via the TASK_QUEUED and
TASK_RUNNING flags, but just after the task finishes executing, it
releases it TASK_RUNNING bit an only then it may go to task_queue().
This one will do nothing if the task's ->expire field is zero, but
if the field turns to zero between this test and the call to
__task_queue() then three things may happen:
  - the task may remain in the WQ until the 24 next days if it's in
    the future;
  - the task may prevent any other task after it from expiring during
    the 24 next days once it's queued
  - if DEBUG_STRICT is set on 2.4 and above, an abort may happen
  - since 2.2, if the task got killed in between, then we may
    even requeue a freed task, causing random behaviour next time
    it's found there, or possibly corrupting the tree if it gets
    reinserted later.

The peers code is one call path that easily reproduces the case with
the ->expire field being reset, because it starts by setting it to
TICK_ETERNITY as the first thing when entering the task handler. But
other code parts also use multi-threaded tasks and rightfully expect
to be able to touch their expire field without causing trouble. No
trivial code path was found that would destroy such a shared task at
runtime, which already limits the risks.

This must be backported to 2.0.

3 years agoDEBUG: pools: replace the link pointer with the caller's address on pool_free()
Willy Tarreau [Wed, 9 Feb 2022 15:49:16 +0000 (16:49 +0100)] 
DEBUG: pools: replace the link pointer with the caller's address on pool_free()

Along recent evolutions of the pools, we've lost the ability to reliably
detect double-frees because while in the past the same pointer was being
used to chain the objects in the cache and to store the pool's address,
since 2.0 they're different so the pool's address is never overwritten on
free() and a double-free will rarely be detected.

This patch sets the caller's return address there. It can never be equal
to a pool's address and will help guess what was the previous call path.
It will not work on exotic architectures nor with very old compilers but
these are not the environments where we're trying to get detailed bug
reports, and this is not done by default anyway so we don't care about
this limitation. Note that depending on the inlining status of the
function, the result may differ but that's no big deal either.

A test by placing a double free of an appctx inside the release handler
itself successfully reported the trouble during appctx_free() and showed
that the return address was in stream_int_shutw_applet() (this one calls
the release handler).

3 years agoDEBUG: pools: let's add reverse mapping from cache heads to thread and pool
Willy Tarreau [Wed, 9 Feb 2022 15:33:22 +0000 (16:33 +0100)] 
DEBUG: pools: let's add reverse mapping from cache heads to thread and pool

During global eviction we're visiting nodes from the LRU tail and we
determine their pool cache head and their pool. In order to make sure
we never mess up, let's add some backwards pointer to the thread number
and pool from the pool_cache_head. It's 64-byte aligned anyway so we're
not wasting space and it helps for debugging and will prevent memory
corruption the earliest possible.

3 years agoDEBUG: pools: add extra sanity checks when picking objects from a local cache
Willy Tarreau [Wed, 9 Feb 2022 15:23:55 +0000 (16:23 +0100)] 
DEBUG: pools: add extra sanity checks when picking objects from a local cache

These few checks are added to make sure we never try to pick an object from
an empty list, which would have a devastating effect.

3 years agoCLEANUP: pools: don't needlessly set a call mark during refilling of caches
Willy Tarreau [Mon, 14 Feb 2022 08:26:59 +0000 (09:26 +0100)] 
CLEANUP: pools: don't needlessly set a call mark during refilling of caches

When refilling caches from the shared cache, it's pointless to set the
pointer to the local pool since it may be overwritten immediately after
by the LIST_INSERT(). This is a leftover from the pre-2.4 code in fact.
It didn't hurt, though.

3 years agoBUG/MINOR: pools: always flush pools about to be destroyed
Willy Tarreau [Wed, 9 Feb 2022 15:19:24 +0000 (16:19 +0100)] 
BUG/MINOR: pools: always flush pools about to be destroyed

When destroying a pool (e.g. at exit or when resizing buffers), it's
important to try to free all their local objects otherwise we can leave
some in the cache. This is particularly visible when changing "bufsize",
because "show pools" will then show two "trash" pools, one of which
contains a single object in cache (which is fortunately not reachable).
In all cases this happens while single-threaded so that's easy to do,
we just have to do it on the current thread.

The easiest way to do this is to pass an extra argument to function
pool_evict_from_local_cache() to force a full flush instead of a
partial one.

This can probably be backported to about all branches where this
applies, but at least 2.4 needs it.

3 years agoBUG/MEDIUM: pools: ensure items are always large enough for the pool_cache_item
Willy Tarreau [Mon, 7 Feb 2022 09:32:00 +0000 (10:32 +0100)] 
BUG/MEDIUM: pools: ensure items are always large enough for the pool_cache_item

With the introduction of DEBUG_POOL_TRACING in 2.6-dev with commit
add43fa43 ("DEBUG: pools: add new build option DEBUG_POOL_TRACING"), small
pools might be too short to store both the pool_cache_item struct and the
caller location, resulting in memory corruption and crashes when this debug
option is used.

What happens here is that the way the size is calculated is by considering
that the POOL_EXTRA part is only used while the object is in use, but this
is not true anymore for the caller's pointer which must absolutely be placed
after the pool_cache_item.

This patch makes sure that the caller part will always start after the
pool_cache_item and that the allocation will always be sufficent. This is
only tagged medium because the debug option is new and unlikely to be used
unless requested by a developer.

No backport is needed.

3 years agoMINOR: quic: Useless statement in quic_crypto_data_cpy()
Frédéric Lécaille [Wed, 2 Feb 2022 14:57:22 +0000 (15:57 +0100)] 
MINOR: quic: Useless statement in quic_crypto_data_cpy()

This should fix Coverity CID 375057 in GH #1526 where a useless assignment
was detected.

3 years agoMINOR: quic: Possible memleak in qc_new_conn()
Frédéric Lécaille [Wed, 2 Feb 2022 14:39:55 +0000 (15:39 +0100)] 
MINOR: quic: Possible memleak in qc_new_conn()

This should fix Coverity CID 375047 in GH #1536 where <buf_area> could leak because
not always freed by by quic_conn_drop(), especially when not stored in <qc> variable.

3 years agoCLEANUP: h3: Unreachable target in h3_uqs_init()
Frédéric Lécaille [Wed, 2 Feb 2022 14:21:00 +0000 (15:21 +0100)] 
CLEANUP: h3: Unreachable target in h3_uqs_init()

This should fix Coverity CID 375045 in GH #1536 which detects a no more
use "err" target in h3_uqs_init()

3 years agoMINOR: quic: Possible overflow in qpack_get_varint()
Frédéric Lécaille [Wed, 2 Feb 2022 13:56:23 +0000 (14:56 +0100)] 
MINOR: quic: Possible overflow in qpack_get_varint()

This should fix CID 375051 in GH 1536 where a signed integer expression (1 << bit)
which could overflow was compared to a uint64_t.