haproxy-2.5.git
3 years agoBUG/MINOR: mworker: does not erase the pidfile upon reload
William Lallemand [Mon, 14 Feb 2022 08:02:14 +0000 (09:02 +0100)]
BUG/MINOR: mworker: does not erase the pidfile upon reload

When started in master-worker mode combined with daemon mode, HAProxy
will open() with O_TRUNC the pidfile when switching to wait mode.

In 2.5, it happens  everytime after trying to load the configuration,
since we switch to wait mode.

In previous version this happens upon a failure of the configuration
loading.

Fixes bug #1545.

Must be backported in every supported branches.

(cherry picked from commit 7b820a6191736cdf4167d624b7bcab9957dd0697)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

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.

(cherry picked from commit 6c8babf6c4d8cb33ff0f5220177207610c22ffcb)
Signed-off-by: Willy Tarreau <w@1wt.eu>

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).

(cherry picked from commit 27c8da1fd5e4f906fd3a9f96a4284c886b559305)
[wt: simplified for 2.5 and older, just use POOL_LINK()]
Signed-off-by: Willy Tarreau <w@1wt.eu>

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.

(cherry picked from commit 49bb5d4268adaea64490ee20f4d73d94afe6a903)
[wt: context adjustment]
Signed-off-by: Willy Tarreau <w@1wt.eu>

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.

(cherry picked from commit e2830addda9baf42c682d7c1856a53e2875deea4)
[wt: adjusted context]
Signed-off-by: Willy Tarreau <w@1wt.eu>

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.

(cherry picked from commit c895c441c7579db652e4ed976c14c2f5b2de0c0e)
[wt: context adjustments as 2.5 doesn't use pool_evict_last_items()]
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MINOR: mworker: does not add the -sf in wait mode
William Lallemand [Wed, 24 Nov 2021 23:49:19 +0000 (00:49 +0100)]
BUG/MINOR: mworker: does not add the -sf in wait mode

Since the wait mode is automatically executed after charging the
configuration, -sf was shown in argv[] with the previous PID, which is
normal, but also the current one. This is only a visual problem when
listing the processes, because -sf does not do anything in wait mode.

Fix the issue by removing the whole "-sf" part in wait mode, but the
executed command can be seen in the argv[] of the latest worker forked.

Must be backported in 2.5.

(cherry picked from commit befab9ee4aa2dfe0405d7f1416991175be2fbe67)
[wla: in 2.5 we still need the -x in wait mode, because we don't know
anymore the stats socket in the configuration once we reexec in waitmode]
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

3 years agoBUG/MEDIUM: mworker: don't lose the stats socket on failed reload
Willy Tarreau [Tue, 1 Feb 2022 07:47:41 +0000 (08:47 +0100)]
BUG/MEDIUM: mworker: don't lose the stats socket on failed reload

In master mode, when the master reexecs itself, it passes the stats socket
address via "-x" on the command line. However, the address is first
retrieved from the configuration, and only later in the boot sequence
the one passed on the command line is considered. As such, some early
errors on reload like a missing config file will cause the process to
rexec itself with no stats socket address on the command line, preventing
the new master from retrieving previous listeners.

The right thing to do is to preset the value to the one found on the
command line so that it can be preserved across failed reloads.

There is no mainline equivalent for this commit because this code was
removed from 2.6, which now only uses the socket pair. The patch must
be backported to older versions where it applies.

3 years agoREGTESTS: ssl: Fix ssl_errors regtest with OpenSSL 1.0.2
Remi Tricot-Le Breton [Tue, 11 Jan 2022 16:29:24 +0000 (17:29 +0100)]
REGTESTS: ssl: Fix ssl_errors regtest with OpenSSL 1.0.2

This test was broken with OpenSSL 1.0.2 after commit a996763619d
(BUG/MINOR: ssl: Store client SNI in SSL context in case of ClientHello
error) because it expected the default TLS version to be 1.3 in some
cases (when it can't be the case with OpenSSL 1.0.2).

(cherry picked from commit aab8d255bc0fcbcc50884a4be4f69598ee08fe73)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoDEBUG: pools: add new build option DEBUG_POOL_INTEGRITY
Willy Tarreau [Fri, 21 Jan 2022 18:00:25 +0000 (19:00 +0100)]
DEBUG: pools: add new build option DEBUG_POOL_INTEGRITY

When enabled, objects picked from the cache are checked for corruption
by comparing their contents against a pattern that was placed when they
were inserted into the cache. Objects are also allocated in the reverse
order, from the oldest one to the most recent, so as to maximize the
ability to detect such a corruption. The goal is to detect writes after
free (or possibly hardware memory corruptions). Contrary to DEBUG_UAF
this cannot detect reads after free, but may possibly detect later
corruptions and will not consume extra memory. The CPU usage will
increase a bit due to the cost of filling/checking the area and for the
preference for cold cache instead of hot cache, though not as much as
with DEBUG_UAF. This option is meant to be usable in production.

(cherry picked from commit 0575d8fd760c6cd1de3d6ed66599d685a03c1873)
[wt: adjusted slightly since there is no batch refilling in 2.5; dropped
 the API doc parts; tested with/without option and works fine]
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUILD: debug/cli: condition test of O_ASYNC to its existence
Willy Tarreau [Tue, 25 Jan 2022 13:51:53 +0000 (14:51 +0100)]
BUILD: debug/cli: condition test of O_ASYNC to its existence

David Carlier reported a build breakage on Haiku since commit
5be7c198e ("DEBUG: cli: add a new "debug dev fd" expert command")
due to O_ASYNC not being defined. Ilya also reported it broke the
build on Cygwin. It's not that portable and sometimes defined as
O_NONBLOCK for portability. But here we don't even need that, as
we already condition other flags, let's just ignore it if it does
not exist.

(cherry picked from commit 410942b92a2687f690c886ae1fdc7ad69a3640e0)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoDEBUG: cli: add a new "debug dev fd" expert command
Willy Tarreau [Mon, 24 Jan 2022 19:26:09 +0000 (20:26 +0100)]
DEBUG: cli: add a new "debug dev fd" expert command

This command will scan the whole file descriptors space to look for
existing FDs that are unknown to haproxy's fdtab, and will try to dump
a maximum number of information about them (including type, mode, device,
size, uid/gid, cloexec, O_* flags, socket types and addresses when
relevant). The goal is to help detecting inherited FDs from parent
processes as well as potential leaks.

Some of those listed are actually known but handled so deep into some
systems that they're not in the fdtab (such as epoll FDs or inter-
thread pipes). This might be refined in the future so that these ones
become known and do not appear.

Example of output:

 $ socat - /tmp/sock1 <<< "expert-mode on;debug dev fd"

    0 type=tty. mod=0620 dev=0x8803 siz=0 uid=1000 gid=5 fs=0x16 ino=0x6 getfd=+0 getfl=O_RDONLY,O_APPEND
    1 type=tty. mod=0620 dev=0x8803 siz=0 uid=1000 gid=5 fs=0x16 ino=0x6 getfd=+0 getfl=O_RDONLY,O_APPEND
    2 type=tty. mod=0620 dev=0x8803 siz=0 uid=1000 gid=5 fs=0x16 ino=0x6 getfd=+0 getfl=O_RDONLY,O_APPEND
    3 type=pipe mod=0600 dev=0 siz=0 uid=1000 gid=100 fs=0xc ino=0x18112348 getfd=+0
    4 type=epol mod=0600 dev=0 siz=0 uid=0 gid=0 fs=0xd ino=0x3674 getfd=+0 getfl=O_RDONLY
   33 type=pipe mod=0600 dev=0 siz=0 uid=1000 gid=100 fs=0xc ino=0x24af8251 getfd=+0 getfl=O_RDONLY
   34 type=epol mod=0600 dev=0 siz=0 uid=0 gid=0 fs=0xd ino=0x3674 getfd=+0 getfl=O_RDONLY
   36 type=pipe mod=0600 dev=0 siz=0 uid=1000 gid=100 fs=0xc ino=0x24af8d1b getfd=+0 getfl=O_RDONLY
   37 type=epol mod=0600 dev=0 siz=0 uid=0 gid=0 fs=0xd ino=0x3674 getfd=+0 getfl=O_RDONLY
   39 type=pipe mod=0600 dev=0 siz=0 uid=1000 gid=100 fs=0xc ino=0x24afa04f getfd=+0 getfl=O_RDONLY
   41 type=pipe mod=0600 dev=0 siz=0 uid=1000 gid=100 fs=0xc ino=0x24af8252 getfd=+0 getfl=O_RDONLY
   42 type=epol mod=0600 dev=0 siz=0 uid=0 gid=0 fs=0xd ino=0x3674 getfd=+0 getfl=O_RDONLY

(cherry picked from commit 5be7c198e5991d5b601af3e648bccfd6f345c673)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MINOR: stream: make the call_rate only count the no-progress calls
Willy Tarreau [Thu, 20 Jan 2022 17:42:16 +0000 (18:42 +0100)]
BUG/MINOR: stream: make the call_rate only count the no-progress calls

We have an anti-looping protection in process_stream() that detects bugs
that used to affect a few filters like compression in the past which
sometimes forgot to handle a read0 or a particular error, leaving a
thread looping at 100% CPU forever. When such a condition is detected,
an alert it emitted and the process is killed so that it can be replaced
by a sane one:

  [ALERT]    (19061) : A bogus STREAM [0x274abe0] is spinning at 2057156
             calls per second and refuses to die, aborting now! Please
             report this error to developers [strm=0x274abe0,3 src=unix
             fe=MASTER be=MASTER dst=<MCLI> txn=(nil),0 txn.req=-,0
             txn.rsp=-,0 rqf=c02000 rqa=10000 rpf=88000021 rpa=8000000
             sif=EST,40008 sib=DIS,84018 af=(nil),0 csf=0x274ab90,8600
             ab=0x272fd40,1 csb=(nil),0
             cof=0x25d5d80,1300:PASS(0x274aaf0)/RAW((nil))/unix_stream(9)
             cob=(nil),0:NONE((nil))/NONE((nil))/NONE(0) filters={}]
    call trace(11):
    |       0x4dbaab [c7 04 25 01 00 00 00 00]: stream_dump_and_crash+0x17b/0x1b4
    |       0x4df31f [e9 bd c8 ff ff 49 83 7c]: process_stream+0x382f/0x53a3
    (...)

One problem with this detection is that it used to only count the call
rate because we weren't sure how to make it more accurate, but the
threshold was high enough to prevent accidental false positives.

There is actually one case that manages to trigger it, which is when
sending huge amounts of requests pipelined on the master CLI. Some
short requests such as "show version" are sufficient to be handled
extremely fast and to cause a wake up of an analyser to parse the
next request, then an applet to handle it, back and forth. But this
condition is not an error, since some data are being forwarded by
the stream, and it's easy to detect it.

This patch modifies the detection so that update_freq_ctr() only
applies to calls made without CF_READ_PARTIAL nor CF_WRITE_PARTIAL
set on any of the channels, which really indicates that nothing is
happening at all.

This is greatly sufficient and extremely effective, as the call above
is still caught (shutr being ignored by an analyser) while a loop on
the master CLI now has no effect. The "call_rate" field in the detailed
"show sess" output will now be much lower, except for bogus streams,
which may help spot them. This field is only there for developers
anyway so it's pretty fine to slightly adjust its meaning.

This patch could be backported to stable versions in case of reports
of such an issue, but as that's unlikely, it's not really needed.

(cherry picked from commit 6c539c4b8c24718177e7ff38af0d186ec84608ea)
[wt: bringing this to 2.5 only for now so that power users have at
 least a stable branch to upgrade to if they face this issue]
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MEDIUM: mcli: always realign wrapping buffers before parsing them
Willy Tarreau [Thu, 20 Jan 2022 07:47:35 +0000 (08:47 +0100)]
BUG/MEDIUM: mcli: always realign wrapping buffers before parsing them

Pipelined commands easily result in request buffers to wrap, and the
master-cli parser only deals with linear buffers since it needs contiguous
keywords to look for in a list. As soon as a buffer wraps, some commands
are ignored and the parser is called in loops because the wrapped data
do not leave the buffer.

Let's take the easiest path that's already used at the HTTP layer, we
simply realign the buffer if its input wraps. This rarely happens anyway
(typically once per buffer), remains reasonably cheap and guarantees this
cannot happen anymore.

This needs to be backported as far as 2.0.

(cherry picked from commit a4e4d66f701b58da87cad0a2a90dea7e7553bc71)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MEDIUM: mcli: do not try to parse empty buffers
Willy Tarreau [Thu, 20 Jan 2022 07:31:50 +0000 (08:31 +0100)]
BUG/MEDIUM: mcli: do not try to parse empty buffers

When pcli_parse_request() is called with an empty buffer, it still tries
to parse it and can go on believing it finds an empty request if the last
char before the beginning of the buffer is a '\n'. In this case it overwrites
it with a zero and processes it as an empty command, doing nothing but not
making the buffer progress. This results in an infinite loop that is stopped
by the watchdog. For a reason related to another issue (yet to be fixed),
this can easily be reproduced by pipelining lots of commands such as
"show version".

Let's add a length check after the search for a '\n'.

This needs to be backported as far as 2.0.

(cherry picked from commit 6cd93f52e974aca4dac7aa2756c4e17659ae5354)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MEDIUM: cli: Never wait for more data on client shutdown
Christopher Faulet [Tue, 18 Jan 2022 07:44:23 +0000 (08:44 +0100)]
BUG/MEDIUM: cli: Never wait for more data on client shutdown

When a shutdown is detected on the cli, we try to execute all pending
commands first before closing the connection. It is required because
commands execution is serialized. However, when the last part is a partial
command, the cli connection is not closed, waiting for more data. Because
there is no timeout for now on the cli socket, the connection remains
infinitely in this state. And because the maxconn is set to 10, if it
happens several times, the cli socket quickly becomes unresponsive because
all its slots are waiting for more data on a closed connections.

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

(cherry picked from commit 0f727dabf51d4ffead40fd43feb7c07193ebde99)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoMEDIUM: h2/hpack: emit a Dynamic Table Size Update after settings change
Willy Tarreau [Thu, 13 Jan 2022 15:00:12 +0000 (16:00 +0100)]
MEDIUM: h2/hpack: emit a Dynamic Table Size Update after settings change

As reported by @jinsubsim in github issue #1498, there is an
interoperability issue between nghttp2 as a client and a few servers
among which haproxy (in fact likely all those which do not make use
of the dynamic headers table in responses or which do not intend to
use a larger table), when reducing the header table size below 4096.
These are easily testable this way:

  nghttp -v -H":method: HEAD" --header-table-size=0 https://$SITE

It will result in a compression error for those which do not start
with an HPACK dynamic table size update opcode.

There is a possible interpretation of the H2 and HPACK specs that
says that an HPACK encoder must send an HPACK headers table update
confirming the new size it will be using after having acknowledged
it, because since it's possible for a decoder to advertise a late
SETTINGS and change it after transfers have begun, the initially
advertised value might very well be seen as a first change from the
initial setting, and the HPACK spec doesn't specify the side which
causes the change that triggers a DTSU update, which was essentially
summed up in this question from nghttp2's author when this issue
was already raised 6 years ago, but which didn't really find a solid
response by then:

  https://lists.w3.org/Archives/Public/ietf-http-wg/2015OctDec/0107.html

The ongoing consensus based on what some servers are doing and that aims
at limiting interoperability issues seems to be that a DTSU is expected
for each reduction from the current size, which should be reflected in
the next revision of the H2 spec:

  https://github.com/httpwg/http2-spec/pull/1005

Given that we do not make use of this table we can emit a DTSU of zero
before encoding any HPACK frame. However, some clients do not support
receiving DTSU with such values (e.g. VTest) so we cannot do it
inconditionnally!

The current patch aims at sticking as close to the spec as possible by
proceeding this way:
  - when a SETTINGS_HEADER_TABLE_SIZE is received, a flag is set
    indicating that the value changed
  - before sending any HPACK frame, this flag is checked to see if
    an update is wanted and if none was sent
  - in this case a DTSU of size zero is emitted and a flag is set
    to mention it was emitted so that it never has to be sent again

This addresses the problem with nghttp2 without affecting VTest.

More context is available here:
  https://github.com/nghttp2/nghttp2/issues/1660
  https://lists.w3.org/Archives/Public/ietf-http-wg/2021OctDec/0235.html

Many thanks to @jinsubsim for this report and participating to the issue
that led to an improvement of the H2 spec.

This should be backported to stable releases in a timely manner, ideally
as far as 2.4 once the h2spec update is merged, then to other versions
after a few months of observation or in case an issue around this is
reported.

(cherry picked from commit 39a0a1e12097a80d6707363792aee5caaa254f28)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MINOR: cli: avoid O(bufsize) parsing cost on pipelined commands
Willy Tarreau [Wed, 19 Jan 2022 16:23:52 +0000 (17:23 +0100)]
BUG/MINOR: cli: avoid O(bufsize) parsing cost on pipelined commands

Sending pipelined commands on the CLI using a semi-colon as a delimiter
has a cost that grows linearly with the buffer size, because co_getline()
is called for each word and looks up a '\n' in the whole buffer while
copying its contents into a temporary buffer.

This causes huge parsing delays, for example 3s for 100k "show version"
versus 110ms if parsed only once for a default 16k buffer.

This patch makes use of the new co_getdelim() function to support both
an LF and a semi-colon as delimiters so that it's no more needed to parse
the whole buffer, and that commands are instantly retrieved. We still
need to rely on co_getline() in payload mode as escapes and semi-colons
are not used there.

It should likely be backported where CLI processing speed matters, but
will require to also backport previous patch "MINOR: channel: add new
function co_getdelim() to support multiple delimiters". It's worth noting
that backporting it without "MEDIUM: cli: yield between each pipelined
command" would significantly increase the ratio of disconnections caused
by empty request buffers, for the sole reason that the currently slow
parsing grants more time to request data to come in. As such it would
be better to backport the patch above before taking this one.

(cherry picked from commit 0011c251448929180216761c760a552c81162226)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoMINOR: channel: add new function co_getdelim() to support multiple delimiters
Willy Tarreau [Wed, 19 Jan 2022 16:19:52 +0000 (17:19 +0100)]
MINOR: channel: add new function co_getdelim() to support multiple delimiters

For now we have co_getline() which reads a buffer and stops on LF, and
co_getword() which reads a buffer and stops on one arbitrary delimiter.
But sometimes we'd need to stop on a set of delimiters (CR and LF, etc).

This patch adds a new function co_getdelim() which takes a set of delimiters
as a string, and constructs a small map (32 bytes) that's looked up during
parsing to stop after the first delimiter found within the set. It also
supports an optional escape character that skips a delimiter (typically a
backslash). For the rest it works exactly like the two other variants.

(cherry picked from commit c5143653177368a40f3153dcf79d145284a5495e)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoMEDIUM: cli: yield between each pipelined command
Willy Tarreau [Wed, 19 Jan 2022 16:11:36 +0000 (17:11 +0100)]
MEDIUM: cli: yield between each pipelined command

Pipelining commands on the CLI is sometimes needed for batched operations
such as map deletion etc, but it causes two problems:
  - some possibly long-running commands will be run in series without
    yielding, possibly causing extremely long latencies that will affect
    quality of service and even trigger the watchdog, as seen in github
    issue #1515.

  - short commands that end on a buffer size boundary, when not run in
    interactive mode, will often cause the socket to be closed when
    the last command is parsed, because the buffer is empty.

This patch proposes a small change to this: by yielding in the CLI applet
after processing a command when there are data left, we significantly
reduce the latency, since only one command is executed per call, and
we leave an opportunity for the I/O layers to refill the request buffer
with more commands, hence to execute all of them much more often.

With this change there's no more watchdog triggered on long series of
"del map" on large map files, and the operations are much less disturbed.
It would be desirable to backport this patch to stable versions after some
period of observation in recent versions.

(cherry picked from commit fa7b4f6691646dc6bdebdb0d11c9cdf0382aeccb)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoDOC: management: mark "set server ssl" as deprecated
William Lallemand [Wed, 19 Jan 2022 14:17:08 +0000 (15:17 +0100)]
DOC: management: mark "set server ssl" as deprecated

This command was integrated in 2.4 when it was not possible to handle
SSL with dynamic servers, this is now possible so we should prefer this
way.

Must be backported in 2.5.

(cherry picked from commit 9998a33d3a027ff6863eab71bcc2f2d7158319b4)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
William Dauchy [Thu, 6 Jan 2022 15:57:15 +0000 (16:57 +0100)]
BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl

While giving a fresh try to `set server ssl` (which I wrote), I realised
the behavior is a bit inconsistent. Indeed when using this command over
a server with ssl enabled for the data path but also for the health
check path we have:

- data and health check done using tls
- emit `set server be_foo/srv0 ssl off`
- data path and health check path becomes plain text
- emit `set server be_foo/srv0 ssl on`
- data path becomes tls and health check path remains plain text

while I thought the end result would be:
- data path and health check path comes back in tls

In the current code we indeed erase all connections while deactivating,
but restore only the data path while activating.  I made this mistake in
the past because I was testing with a case where the health check plain
text by default.

There are several ways to solve this issue. The cleanest one would
probably be to avoid changing the health check connection when we use
`set server ssl` command, and create a new command `set server
ssl-check` to change this. For now I assumed this would be ok to simply
avoid changing the health check path and be more consistent.

This patch tries to address that and also update the documentation. It
should not break the existing usage with health check on plain text, as
in this case they should have `no-check-ssl` in defaults.  Without this
patch, it makes the command unusable in an env where you have a list of
server to add along the way with initial `server-template`, and all
using tls for data and healthcheck path.

For 2.6 we should probably reconsider and add `set server ssl-check`
command for better granularity of cases.

If this solution is accepted, this patch should be backported up to >=
2.4.

The alternative solution was to restore the previous state, but I
believe this will create even more confusion in the future.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
(cherry picked from commit a087f878753d73042ca9a69649d3f2668489e81d)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUILD/MINOR: fix solaris build with clang.
David Carlier [Thu, 13 Jan 2022 19:16:48 +0000 (19:16 +0000)]
BUILD/MINOR: fix solaris build with clang.

clang 9 sets the level to POSIX 2004.

(cherry picked from commit 4831db2f32077469472ebda03e176e6c77dc7472)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MINOR: httpclient/lua: don't pop the lua stack when getting headers
William Lallemand [Fri, 14 Jan 2022 16:59:01 +0000 (17:59 +0100)]
BUG/MINOR: httpclient/lua: don't pop the lua stack when getting headers

hlua_httpclient_table_to_hdrs() does a lua_pop(L, 1) at the end of the
function, this is supposed to be done in the caller and it is already be
done in hlua_httpclient_send().

This call has the consequence of poping the next parameter of the
httpclient, ignoring it.

This patch fixes the issue by removing the lua_pop(L, 1).

Must be backported in 2.5.

(cherry picked from commit 01e2be84d746c5203f7c5723bb6ca9e469026c3d)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MINOR: httpclient: set default Accept and User-Agent headers
William Lallemand [Fri, 14 Jan 2022 13:10:33 +0000 (14:10 +0100)]
BUG/MINOR: httpclient: set default Accept and User-Agent headers

Some servers require at least an Accept and a User-Agent header in the
request. This patch sets some default value.

Must be backported in 2.5.

(cherry picked from commit bad9c8cac411f11144236abe8f9342f1058c9344)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MINOR: httpclient: don't send an empty body
William Lallemand [Fri, 14 Jan 2022 13:08:34 +0000 (14:08 +0100)]
BUG/MINOR: httpclient: don't send an empty body

Forbid the httpclient to send an empty chunked client when there is no
data to send. It does happen when doing a simple GET too.

Must be backported in 2.5.

(cherry picked from commit e1e045f4d71f85462414d048449c9307886f7c36)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MEDIUM: htx: Adjust length to add DATA block in an empty HTX buffer
Christopher Faulet [Wed, 12 Jan 2022 13:03:42 +0000 (14:03 +0100)]
BUG/MEDIUM: htx: Adjust length to add DATA block in an empty HTX buffer

htx_add_data() is able to partially consume data. However there is a bug
when the HTX buffer is empty.  The data length is not properly
adjusted. Thus, if it exceeds the HTX buffer size, no block is added. To fix
the issue, the length is now adjusted first.

This patch must be backported as far as 2.0.

(cherry picked from commit 28e7ba86885d60a29036bc6fa3a89777148a3762)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MEDIUM: connection: properly leave stopping list on error
Willy Tarreau [Wed, 12 Jan 2022 16:24:26 +0000 (17:24 +0100)]
BUG/MEDIUM: connection: properly leave stopping list on error

The stopping-list management introduced by commit d3a88c1c3 ("MEDIUM:
connection: close front idling connection on soft-stop") missed two
error paths in the H1 and H2 muxes. The effect is that if a stream
or HPACK table couldn't be allocated for these incoming connections,
we would leave with the connection freed still attached to the
stopping_list and it would never leave it, resulting in use-after-free
hence either a crash or a data corruption.

This is marked as medium as it only happens under extreme memory pressure
or when playing with tune.fail-alloc. Other stability issues remain in
such a case so that abnormal behaviors cannot be explained by this bug
alone.

This must be backported to 2.4.

(cherry picked from commit 3b990fe0bee3072cb70c965c202707cdad0f29cd)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years ago[RELEASE] Released version 2.5.1 v2.5.1
Christopher Faulet [Tue, 11 Jan 2022 16:56:47 +0000 (17:56 +0100)]
[RELEASE] Released version 2.5.1

Released version 2.5.1 with the following main changes :
    - BUG/MINOR: cache: Fix loop on cache entries in "show cache"
    - BUG/MINOR: httpclient: allow to replace the host header
    - BUG/MINOR: lua: don't expose internal proxies
    - BUG/MINOR: lua: remove loop initial declarations
    - BUG/MEDIUM: cli: Properly set stream analyzers to process one command at a time
    - BUILD: evports: remove a leftover from the dead_fd cleanup
    - BUG/MINOR: vars: Fix the set-var and unset-var converters
    - BUG/MINOR: server: Don't rely on last default-server to init server SSL context
    - BUG/MEDIUM: resolvers: Detach query item on response error
    - BUG/MAJOR: segfault using multiple log forward sections.
    - BUG/MEDIUM: h1: Properly reset h1m flags when headers parsing is restarted
    - BUG/MEDIUM: mworker: FD leak of the eventpoll in wait mode
    - BUG/MINOR: mworker: deinit of thread poller was called when not initialized
    - MINOR: mux-h1: Improve H1 traces by adding info about http parsers
    - BUILD: bug: Fix error when compiling with -DDEBUG_STRICT_NOCRASH
    - BUG/MEDIUM: sample: Fix memory leak in sample_conv_jwt_member_query
    - MINOR: cli: "show version" displays the current process version
    - BUILD: tree-wide: avoid warnings caused by redundant checks of obj_types
    - IMPORT: slz: use the correct CRC32 instruction when running in 32-bit mode
    - MINOR: http-rules: Add capture action to http-after-response ruleset
    - BUG/MINOR: cli/server: Don't crash when a server is added with a custom id
    - DOC: spoe: Clarify use of the event directive in spoe-message section
    - DOC: config: Specify %Ta is only available in HTTP mode
    - DOC: config: retry-on list is space-delimited
    - DOC: config: fix error-log-format example
    - BUG/MEDIUM: mworker/cli: crash when trying to access an old PID in prompt mode
    - MINOR: ssl: Remove empty lines from "show ssl ocsp-response" output
    - MINOR: pools: work around possibly slow malloc_trim() during gc
    - BUG/MEDIUM: backend: fix possible sockaddr leak on redispatch
    - BUG/MEDIUM: peers: properly skip conn_cur from incoming messages
    - BUG/MEDIUM: mux-h1: Fix splicing by properly detecting end of message
    - BUG/MINOR: mux-h1: Fix splicing for messages with unknown length
    - BUILD: ssl: unbreak the build with newer libressl
    - DOC: fix misspelled keyword "resolve_retries" in resolvers
    - DEBUG: ssl: make sure we never change a servername on established connections
    - BUILD: opentracing: display warning in case of using OT_USE_VARS at compile time
    - BUG/MEDIUM: ssl: initialize correctly ssl w/ default-server
    - REGTESTS: ssl: fix ssl_default_server.vtc
    - MINOR: compat: detect support for dl_iterate_phdr()
    - MINOR: debug: add ability to dump loaded shared libraries
    - MINOR: debug: add support for -dL to dump library names at boot
    - MINOR: proxy: add option idle-close-on-response
    - MINOR: cpuset: switch to sched_setaffinity for FreeBSD 14 and above.
    - BUILD: makefile: add -Wno-atomic-alignment to work around clang abusive warning
    - CI: Github Actions: do not show VTest failures if build failed
    - BUG/MINOR: ssl: free the fields in srv->ssl_ctx
    - BUG/MEDIUM: ssl: free the ckch instance linked to a server
    - REGTESTS: ssl: update of a crt with server deletion
    - BUILD/MINOR: cpuset FreeBSD 14 build fix.
    - CI: github actions: update OpenSSL to 3.0.1
    - BUILD/MINOR: tools: solaris build fix on dladdr.
    - BUG/MINOR: cli: fix _getsocks with musl libc
    - BUG/MEDIUM: http-ana: Preserve response's FLT_END analyser on L7 retry
    - BUG/MEDIUM: mworker: don't use _getsocks in wait mode
    - BUG/MINOR: ssl: Store client SNI in SSL context in case of ClientHello error
    - BUG/MAJOR: mux-h1: Don't decrement .curr_len for unsent data
    - BUILD: cpuset: fix build issue on macos introduced by previous change
    - CI: github actions: clean default step conditions

3 years agoCI: github actions: clean default step conditions
Ilya Shipitsin [Fri, 7 Jan 2022 15:09:35 +0000 (20:09 +0500)]
CI: github actions: clean default step conditions

step condition "if: ${{ !failure() }}" was added in 2ef4c7c84363f5a9b80a2093df1370514319db28
during my experiments. As Tim Düsterhus mentioned, that condition is default and may be omitted.

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

3 years agoBUILD: cpuset: fix build issue on macos introduced by previous change
David CARLIER [Sat, 8 Jan 2022 09:59:38 +0000 (09:59 +0000)]
BUILD: cpuset: fix build issue on macos introduced by previous change

The build on macos was broken by recent commit df91cbd58 ("MINOR: cpuset:
switch to sched_setaffinity for FreeBSD 14 and above."), let's move the
variable declaration inside the ifdef.

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

3 years agoBUG/MAJOR: mux-h1: Don't decrement .curr_len for unsent data
Christopher Faulet [Mon, 10 Jan 2022 16:27:51 +0000 (17:27 +0100)]
BUG/MAJOR: mux-h1: Don't decrement .curr_len for unsent data

A regression was introduced by commit 140f1a58 ("BUG/MEDIUM: mux-h1: Fix
splicing by properly detecting end of message"). To detect end of the
outgoing message, when the content-length is announced, we count amount of
data already sent. But only data really sent must be counted.

If the output buffer is full, we can fail to send data (fully or
partially). In this case, we must take care to only count sent
data. Otherwise we may think too much data were sent and an internal error
may be erroneously reported.

This patch should fix issues #1510 and #1511. It must be backported as far
as 2.4.

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

3 years agoBUG/MINOR: ssl: Store client SNI in SSL context in case of ClientHello error
Remi Tricot-Le Breton [Fri, 7 Jan 2022 16:12:01 +0000 (17:12 +0100)]
BUG/MINOR: ssl: Store client SNI in SSL context in case of ClientHello error

If an error is raised during the ClientHello callback on the server side
(ssl_sock_switchctx_cbk), the servername callback won't be called and
the client's SNI will not be saved in the SSL context. But since we use
the SSL_get_servername function to return this SNI in the ssl_fc_sni
sample fetch, that means that in case of error, such as an SNI mismatch
with a frontend having the strict-sni option enabled, the sample fetch
would not work (making strict-sni related errors hard to debug).

This patch fixes that by storing the SNI as an ex_data in the SSL
context in case the ClientHello callback returns an error. This way the
sample fetch can fallback to getting the SNI this way. It will still
first call the SSL_get_servername function first since it is the proper
way of getting a client's SNI when the handshake succeeded.

In order to avoid memory allocations are runtime into this highly used
runtime function, a new memory pool was created to store those client
SNIs. Its entry size is set to 256 bytes since SNIs can't be longer than
255 characters.

This fixes GitHub #1484.

It can be backported in 2.5.

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

3 years agoBUG/MEDIUM: mworker: don't use _getsocks in wait mode
William Lallemand [Fri, 7 Jan 2022 17:19:42 +0000 (18:19 +0100)]
BUG/MEDIUM: mworker: don't use _getsocks in wait mode

Since version 2.5 the master is automatically re-executed in wait-mode
when the config is successfully loaded, puting corner cases of the wait
mode in plain sight.

When using the -x argument and with the right timing, the master will
try to get the FDs again in wait mode even through it's not needed
anymore, which will harm the worker by removing its listeners.

However, if it fails, (and it's suppose to, sometimes), the
master will exit with EXIT_FAILURE because it does not have the
MODE_MWORKER flag, but only the MODE_MWORKER_WAIT flag. With the
consequence of killing the workers.

This patch fixes the issue by restricting the use of _getsocks to some
modes.

This patch must be backported in every version supported, even through
the impact should me more harmless in version prior to 2.5.

(cherry picked from commit f82afbb9cd1515e9fa7f9e6df84904b832226304)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

3 years agoBUG/MEDIUM: http-ana: Preserve response's FLT_END analyser on L7 retry
Christopher Faulet [Tue, 4 Jan 2022 09:56:03 +0000 (10:56 +0100)]
BUG/MEDIUM: http-ana: Preserve response's FLT_END analyser on L7 retry

When a filter is attached on a stream, the FLT_END analyser must not be
removed from the response channel on L7 retry. It is especially important
because CF_FLT_ANALYZE flag is still set. This means the synchronization
between the two sides when the filter ends can be blocked. Depending on the
timing, this can freeze the stream infinitely or lead to a spinning loop.

Note that the synchronization between the two sides at the end of the
analysis was introduced because the stream was reused in HTTP between two
transactions. But, since the HTX was introduced, a new stream is created for
each transaction. So it is probably possible to remove this step for 2.2 and
higher.

This patch must be backported as far as 2.0.

(cherry picked from commit 7bf46bb9a972c1e9de50b31ce20811f2f59a6849)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MINOR: cli: fix _getsocks with musl libc
William Lallemand [Mon, 3 Jan 2022 18:43:41 +0000 (19:43 +0100)]
BUG/MINOR: cli: fix _getsocks with musl libc

In ticket #1413, the transfer of FDs couldn't correctly work on alpine
linux. After a few tests with musl on another distribution it seems to
be a limitation of this libc.

The number of FD that could be sent per sendmsg was set to 253, which
does not seem to work with musl, decreasing it 252 seems to work
better, so lets set this value everywhere since it does not have that
much impact.

This must be backported in every maintained version.

(cherry picked from commit 148d7a030156ca927e744013dd6fc32b473c1ef6)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUILD/MINOR: tools: solaris build fix on dladdr.
David Carlier [Fri, 31 Dec 2021 08:15:29 +0000 (08:15 +0000)]
BUILD/MINOR: tools: solaris build fix on dladdr.

dladdr takes a mutable address on this platform.

(cherry picked from commit ae5c42f4d0ce74af7554b9a94c8a3c43286efc35)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoCI: github actions: update OpenSSL to 3.0.1
Ilya Shipitsin [Sat, 25 Dec 2021 09:01:52 +0000 (14:01 +0500)]
CI: github actions: update OpenSSL to 3.0.1

OpenSSL-3.0.1 was released on 14 Dec 2021, let's switch to it

(cherry picked from commit 874c907a2e5ffae50599b1f368e0488d8eb2fe4f)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUILD/MINOR: cpuset FreeBSD 14 build fix.
David CARLIER [Fri, 31 Dec 2021 05:00:12 +0000 (05:00 +0000)]
BUILD/MINOR: cpuset FreeBSD 14 build fix.

The 14th release started to introduce api compatibility layer with Linux
for the cpuset part and doing so irrevocably change the CPU* macros as well.

(cherry picked from commit f64504716843f69a9b10582b2d9cce6a7919795a)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoREGTESTS: ssl: update of a crt with server deletion
William Lallemand [Thu, 30 Dec 2021 13:57:32 +0000 (14:57 +0100)]
REGTESTS: ssl: update of a crt with server deletion

This test verifies that a certificate is in a "Unused" state once every
server which uses it are dynamically removed.

(cherry picked from commit acd546b07cd68537d173dde38342d86fadd706d1)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MEDIUM: ssl: free the ckch instance linked to a server
William Lallemand [Thu, 30 Dec 2021 13:45:19 +0000 (14:45 +0100)]
BUG/MEDIUM: ssl: free the ckch instance linked to a server

This patch unlinks and frees the ckch instance linked to a server during
the free of this server.

This could have locked certificates in a "Used" state when removing
servers dynamically from the CLI. And could provoke a segfault once we
try to dynamically update the certificate after that.

This must be backported as far as 2.4.

(cherry picked from commit e69563fd8ed1a492ae4547451935908c0802e2c4)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MINOR: ssl: free the fields in srv->ssl_ctx
William Lallemand [Thu, 30 Dec 2021 10:25:43 +0000 (11:25 +0100)]
BUG/MINOR: ssl: free the fields in srv->ssl_ctx

A lot of free are missing in ssl_sock_free_srv_ctx(), this could result
in memory leaking when removing dynamically a server via the CLI.

This must be backported in every branches, by removing the fields that
does not exist in the previous branches.

(cherry picked from commit 231610ad9ccc2470930f7a728ba710a548677a65)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoCI: Github Actions: do not show VTest failures if build failed
Ilya Shipitsin [Sat, 25 Dec 2021 08:53:04 +0000 (13:53 +0500)]
CI: Github Actions: do not show VTest failures if build failed

this is mostly cleanup, issue is minor. If build failed, VTest execution
tried to be performed as well as VTest result show. This change ignores
those steps if build failed.

(cherry picked from commit 2ef4c7c84363f5a9b80a2093df1370514319db28)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUILD: makefile: add -Wno-atomic-alignment to work around clang abusive warning
Willy Tarreau [Fri, 7 Jan 2022 13:51:56 +0000 (14:51 +0100)]
BUILD: makefile: add -Wno-atomic-alignment to work around clang abusive warning

As reported in github issue #1502, clang, when building for i386, will
try to use CMPXCHG8B-based loops for 64-bit atomic operations, and emits
warnings for all 64-bit operands that are not 64-bit aligned, an alignment
that is *not* required by the ABI, that the compiler itself does not
enforce, and that the intel SDM clearly says is not required on this
32-bit platform for this operation. But this is likely an excessive
outcome of the same code being used in 64-bit for CMPXCHG16B which does
require proper alignment. Firefox already gave up on this one 3 years
ago, let's not waste our time arguing and just shut up the warning
instead. It might hide some real bugs in the future but till now
experience showed that overall it's unlikely.

This should be backported to all maintained branches that use 64-bit
atomic ops (e.g. for counters).

Thanks to Brad Smith for reporting it and confirming that shutting the
warning addresses it.

(cherry picked from commit 790169fe69c139e4fb6fc3046985696aa78c0569)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoMINOR: cpuset: switch to sched_setaffinity for FreeBSD 14 and above.
David CARLIER [Thu, 6 Jan 2022 18:53:50 +0000 (18:53 +0000)]
MINOR: cpuset: switch to sched_setaffinity for FreeBSD 14 and above.

Following up previous update on cpuset-t.h. Ultimately, at some point
 the cpuset_setaffinity code path could be removed.

(cherry picked from commit df91cbd5845eb7ccdd2586712cb62585fd55d7c0)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoMINOR: proxy: add option idle-close-on-response
William Dauchy [Wed, 5 Jan 2022 21:53:24 +0000 (22:53 +0100)]
MINOR: proxy: add option idle-close-on-response

Avoid closing idle connections if a soft stop is in progress.

By default, idle connections will be closed during a soft stop. In some
environments, a client talking to the proxy may have prepared some idle
connections in order to send requests later. If there is no proper retry
on write errors, this can result in errors while haproxy is reloading.
Even though a proper implementation should retry on connection/write
errors, this option was introduced to support back compat with haproxy <
v2.4. Indeed before v2.4, we were waiting for a last request to be able
to add a "connection: close" header and advice the client to close the
connection.

In a real life example, this behavior was seen in AWS using the ALB in
front of a haproxy. The end result was ALB sending 502 during haproxy
reloads.
This patch was tested on haproxy v2.4, with a regular reload on the
process, and a constant trend of requests coming in. Before the patch,
we see regular 502 returned to the client; when activating the option,
the 502 disappear.

This patch should help fixing github issue #1506.
In order to unblock some v2.3 to v2.4 migraton, this patch should be
backported up to v2.4 branch.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
[wt: minor edits to the doc to mention other options to care about]
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit a9dd901143385fef3a5113d0bf1681cd0536357a)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoMINOR: debug: add support for -dL to dump library names at boot
Willy Tarreau [Tue, 28 Dec 2021 14:43:11 +0000 (15:43 +0100)]
MINOR: debug: add support for -dL to dump library names at boot

This is a second help to dump loaded library names late at boot, once
external code has already been initialized. The purpose is to provide
a format that makes it easy to pass to "tar" to produce an archive
containing the executable and the list of dependencies. For example
if haproxy is started as "haproxy -f foo.cfg", a config check only
will suffice to quit before starting, "-q" will be used to disable
undesired output messages, and -dL will be use to dump libraries.
This will result in such a command to trivially produce a tarball
of loaded libraries:

   ./haproxy -q -c -dL -f foo.cfg | tar -T - -hzcf archive.tgz

(cherry picked from commit 654726db5ab1160ad5dc8d356e2965e69c163dcf)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoMINOR: debug: add ability to dump loaded shared libraries
Willy Tarreau [Tue, 28 Dec 2021 08:57:10 +0000 (09:57 +0100)]
MINOR: debug: add ability to dump loaded shared libraries

Many times core dumps reported by users who experience trouble are
difficult to exploit due to missing system libraries. Sometimes,
having just a list of loaded libraries and their respective addresses
can already provide some hints about some problems.

This patch makes a step in that direction by adding a new "show libs"
command that will try to enumerate the list of object files that are
loaded in memory, relying on the dynamic linker for this. It may also
be used to detect that some foreign code embarks other undesired libs
(e.g. some external Lua modules).

At the moment it's only supported on glibc when USE_DL is set, but it's
implemented in a way that ought to make it reasonably easy to be extended
to other platforms.

(cherry picked from commit 6ab7b21a1108c0f187d09196dfc20d40f654e6c9)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoMINOR: compat: detect support for dl_iterate_phdr()
Willy Tarreau [Tue, 28 Dec 2021 14:13:12 +0000 (15:13 +0100)]
MINOR: compat: detect support for dl_iterate_phdr()

We'll use this glibc function to dump loaded libs. It's been
available since glibc-2.2.4, and as it requires dlpi headers defined
in link.h, it implicitly relies on dlfcn, thus we condition it to
USE_DL. Other operating systems or libc might have different
dependencies so let's stick to the bare minimum for now.

(cherry picked from commit 3f3a56c9b07175f342f30c6271df24f783c892c6)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoREGTESTS: ssl: fix ssl_default_server.vtc
William Lallemand [Wed, 29 Dec 2021 17:16:27 +0000 (18:16 +0100)]
REGTESTS: ssl: fix ssl_default_server.vtc

Patch 2c776f1 ("BUG/MEDIUM: ssl: initialize correctly ssl w/
default-server") added tests that are not relevant anymore and broke the
reg-test. revert them.

(cherry picked from commit 0387632ac0db520402550cf19a96d41f8c1661de)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MEDIUM: ssl: initialize correctly ssl w/ default-server
William Lallemand [Tue, 28 Dec 2021 17:47:17 +0000 (18:47 +0100)]
BUG/MEDIUM: ssl: initialize correctly ssl w/ default-server

This bug was introduced by d817dc73 ("MEDIUM: ssl: Load client
certificates in a ckch for backend servers") in which the creation of
the SSL_CTX for a server was moved to the configuration parser when
using a "crt" keyword instead of being done in ssl_sock_prepare_srv_ctx().

The patch 0498fa40 ("BUG/MINOR: ssl: Default-server configuration ignored by
server") made it worse by setting the same SSL_CTX for every servers
using a default-server. Resulting in any SSL option on a server applied
to every server in its backend.

This patch fixes the issue by reintroducing a string which store the
path of certificate inside the server structure, and loading the
certificate in ssl_sock_prepare_srv_ctx() again.

This is a quick fix to backport, a cleaner way can be achieve by always
creating the SSL_CTX in ssl_sock_prepare_srv_ctx() and splitting
properly the ssl_sock_load_srv_cert() function.

This patch fixes issue #1488.

Must be backported as far as 2.4.

(cherry picked from commit 2c776f1c30c85be11c9ba8ca8d9a7d62690d1a32)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUILD: opentracing: display warning in case of using OT_USE_VARS at compile time
Miroslav Zagorac [Mon, 27 Dec 2021 11:44:07 +0000 (12:44 +0100)]
BUILD: opentracing: display warning in case of using OT_USE_VARS at compile time

Please do not set the OT_USE_VARS configuration variable, as the source
will probably not be able to compile!  For now, this variable can only
be used for experimental purposes, and is not intended for wider use.

For further clarification, please see commit 4cb2c83f4.

Must be backported to 2.5.

(cherry picked from commit 6c9f7faa62a00a4d028704d7b11e290c83f8a49d)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoDEBUG: ssl: make sure we never change a servername on established connections
Willy Tarreau [Thu, 23 Dec 2021 10:12:13 +0000 (11:12 +0100)]
DEBUG: ssl: make sure we never change a servername on established connections

Since this case was already met previously with commit 655dec81b
("BUG/MINOR: backend: do not set sni on connection reuse"), let's make
sure that we don't change reused connection settings. This could be
generalized to most settings that are only in effect before the handshake
in fact (like set_alpn and a few other ones).

(cherry picked from commit 77bfa66124e8532a4ecbe5025657574bb43b7160)
[wt: it's not enabled in default builds thus is safe to backport; it may
 help sort out certain future bug reports in field]
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoDOC: fix misspelled keyword "resolve_retries" in resolvers
Thierry Fournier [Wed, 15 Dec 2021 18:03:52 +0000 (19:03 +0100)]
DOC: fix misspelled keyword "resolve_retries" in resolvers

"resolve_retries" was spelled "resolve_retires".

(cherry picked from commit 55c40ea17778b1afa3cf8f0f0a2cc42717c9364a)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUILD: ssl: unbreak the build with newer libressl
Daniel Jakots [Wed, 8 Dec 2021 01:34:39 +0000 (20:34 -0500)]
BUILD: ssl: unbreak the build with newer libressl

In LibreSSL 3.5.0, BIO is going to become opaque, so haproxy's
compat macros will no longer work. The functions they substitute
have been available since LibreSSL 2.7.0.

(cherry picked from commit d1a2e2b0d1da0dff726738343fbaed044fb93470)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MINOR: mux-h1: Fix splicing for messages with unknown length
Christopher Faulet [Fri, 26 Nov 2021 16:26:19 +0000 (17:26 +0100)]
BUG/MINOR: mux-h1: Fix splicing for messages with unknown length

Splicing was disabled fo Messages with an unknown length (no C-L or T-E
header) with no valid reason. So now, it is possible to use the kernel
splicing for such messages.

This patch should be backported as far as 2.4.

(cherry picked from commit f5ce320156c1a1233755440dad766e7b2b401c0a)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MEDIUM: mux-h1: Fix splicing by properly detecting end of message
Christopher Faulet [Fri, 26 Nov 2021 15:37:55 +0000 (16:37 +0100)]
BUG/MEDIUM: mux-h1: Fix splicing by properly detecting end of message

Since the 2.4.4, the splicing support in the H1 multiplexer is buggy because
end of the message is not properly detected.

On the 2.4, when the requests is spliced, there is no issue. But when the
response is spliced, the client connection is always closed at the end of the
message. Note the response is still fully sent.

On the 2.5 and higher, when the last requests on a connection is spliced, a
client abort is reported. For other requests there is no issue. In all cases,
the requests are fully sent. When the response is spliced, the server connection
hangs till the server timeout and a server abort is reported. The response is
fully sent with no delay.

The root cause is the EOM block suppression. There is no longer extra block to
be sure to call a last time rcv_buf()/snd_buf() callback functions. At the end,
to fix the issue, we must now detect end of the message in rcv_pipe() and
snd_pipe() callback functions. To do so, we rely on the announced message length
to know when the payload is finished. This works because the chunk-encoded
messages are not spliced.

This patch must be backported as far as 2.4 after an observation period.

(cherry picked from commit 140f1a585248a7da1fefbb5f20f260dfafb32a9a)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MEDIUM: peers: properly skip conn_cur from incoming messages
Willy Tarreau [Fri, 24 Dec 2021 12:38:49 +0000 (13:38 +0100)]
BUG/MEDIUM: peers: properly skip conn_cur from incoming messages

The approach used for skipping conn_cur in commit db2ab8218 ("MEDIUM:
stick-table: never learn the "conn_cur" value from peers") was wrong,
it only works with simple tables but as soon as frequency counters or
arrays are exchanged after conn_cur, the stream is desynchronized and
incorrect values are read. This is because the fields have a variable
length depending on their types and cannot simply be skipped by a
"continue" statement.

Let's change the approach to make sure we continue to completely parse
these local-only fields, and only drop the value at the moment we're
about to store them, since this is exactly the intent.

A simpler approach could consist in having two sets of stktable_data_ptr()
functions, one for retrieval and one for storage, and to make the store
function return a NULL pointer for local types. For now this doesn't
seem worth the trouble.

This fixes github issue #1497. Thanks to @brenc for the reproducer.

This must be backported to 2.5.

(cherry picked from commit b4ff6f4ae9267620827f7da9b519f4e1b28b10e9)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoBUG/MEDIUM: backend: fix possible sockaddr leak on redispatch
Willy Tarreau [Fri, 24 Dec 2021 10:27:53 +0000 (11:27 +0100)]
BUG/MEDIUM: backend: fix possible sockaddr leak on redispatch

A subtle change of target address allocation was introduced with commit
68cf3959b ("MINOR: backend: rewrite alloc of stream target address") in
2.4. Prior to this patch, a target address was allocated by function
assign_server_address() only if none was previously allocated. After
the change, the allocation became unconditional. Most of the time it
makes no difference, except when we pass multiple times through
connect_server() with SF_ADDR_SET cleared.

The most obvious fix would be to avoid allocating that address there
when already set, but the root cause is that since introduction of
dynamically allocated addresses, the SF_ADDR_SET flag lies. It can
be cleared during redispatch or during a queue redistribution without
the address being released.

This patch instead gives back all its correct meaning to SF_ADDR_SET
and guarantees that when not set no address is allocated, by freeing
that address at the few places the flag is cleared. The flag could
even be removed so that only the address is checked but that would
require to touch many areas for no benefit.

The easiest way to test it is to send requests to a proxy with l7
retries enabled, which forwards to a server returning 500:

  defaults
    mode http
    timeout client 1s
    timeout server 1s
    timeout connect 1s
    retry-on all-retryable-errors
    retries 1
    option redispatch

  listen proxy
    bind *:5000
    server app 0.0.0.0:5001

  frontend dummy-app
    bind :5001
    http-request return status 500

Issuing "show pools" on the CLI will show that pool "sockaddr" grows
as requests are redispatched, and remains stable with the fix. Even
"ps" will show that the process' RSS grows by ~160B per request.

This fix will need to be backported to 2.4. Note that before 2.5,
there's no strm->si[1].dst, strm->target_addr must be used instead.

This addresses github issue #1499. Special thanks to Daniil Leontiev
for providing a well-documented reproducer.

(cherry picked from commit 266d5405490050adeaf414158f7f4b9bad5298bc)
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoMINOR: pools: work around possibly slow malloc_trim() during gc
Willy Tarreau [Thu, 23 Dec 2021 08:26:30 +0000 (09:26 +0100)]
MINOR: pools: work around possibly slow malloc_trim() during gc

During 2.4-dev, support for malloc_trim() was implemented to ease
release of memory in a stopping process. This was found to be quite
effective and later backported to 2.3.7.

Then it was found that sometimes malloc_trim() could take a huge time
to complete it if was competing with other threads still allocating and
releasing memory, reason why it was decided in 2.5-dev to move
malloc_trim() under the thread isolation that was already in place in
the shared pool version of pool_gc() (this was commit 26ed1835).

However, other instances of pool_gc() that used to call malloc_trim()
were not updated since they were not using thread isolation. Currently
we have two other such instances, one for when there is absolutely no
pool and one for when there are only thread-local pools.

Christian Ruppert reported in GH issue #1490 that he's sometimes seeing
and old process die upon reload when upgrading from 2.3 to 2.4, and
that this happens inside malloc_trim(). The problem is that since
2.4-dev11 with commit 0bae07592 we detect modern libc that provide a
faster thread-aware allocator and do not maintain shared pools anymore.
As such we're using again the simpler pool_gc() implementations that do
not use thread isolation around the malloc_trim() call.

All this code was cleaned up recently and the call moved to a new
function trim_all_pools(). This patch implements explicit thread isolation
inside that function so that callers do not have to care about this
anymore. The thread isolation is conditional so that this doesn't affect
the one already in place in the larger version of pool_gc(). This way it
will solve the problem for all callers.

This patch must be backported as far as 2.3. It may possibly require
some adaptations. If trim_all_pools() is not present, copy-pasting the
tests in each version of pool_gc() will have the same effect.

Thanks to Christian for his detailed report and his testing.

(cherry picked from commit 0d93a8186331972d9c60624ed822e7c2c008d1d4)
[wt: dropped the 2.6-specific mallctl() context]
Signed-off-by: Willy Tarreau <w@1wt.eu>

3 years agoMINOR: ssl: Remove empty lines from "show ssl ocsp-response" output
Remi Tricot-Le Breton [Fri, 17 Dec 2021 17:53:23 +0000 (18:53 +0100)]
MINOR: ssl: Remove empty lines from "show ssl ocsp-response" output

There were empty lines in the output of the CLI's "show ssl
ocsp-response" command (after the certificate ID and between two
certificates). This patch removes them since an empty line should mark
the end of the output.

Must be backported in 2.5.

(cherry picked from commit cc750efbc5c2180ed63b222a51029609ea96d0f7)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

3 years agoBUG/MEDIUM: mworker/cli: crash when trying to access an old PID in prompt mode
William Lallemand [Fri, 10 Dec 2021 13:14:53 +0000 (14:14 +0100)]
BUG/MEDIUM: mworker/cli: crash when trying to access an old PID in prompt mode

The master process encounter a crash when trying to access an old
process which left from the master CLI.

To reproduce the problem, you need a prompt to a previous worker, then
wait for this worker to leave, once it left launch a command from this
prompt. The s->target is then filled with a NULL which is dereferenced
when trying to connect().

This patch fixes the problem by checking if s->target is NULL.

Must be backported as far as 2.0.

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

3 years agoDOC: config: fix error-log-format example
Lukas Tribus [Thu, 9 Dec 2021 00:27:14 +0000 (01:27 +0100)]
DOC: config: fix error-log-format example

In commit 6f7497616 ("MEDIUM: connection: rename fc_conn_err and
bc_conn_err to fc_err and bc_err"), fc_conn_err became fc_err, so
update this example.

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

3 years agoDOC: config: retry-on list is space-delimited
Lukas Tribus [Wed, 8 Dec 2021 10:33:01 +0000 (11:33 +0100)]
DOC: config: retry-on list is space-delimited

We are using comma-delimited list for init-addr for example, let's
document that this is space-delimited to avoid the guessing game.

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

3 years agoDOC: config: Specify %Ta is only available in HTTP mode
Christopher Faulet [Fri, 3 Dec 2021 09:48:36 +0000 (10:48 +0100)]
DOC: config: Specify %Ta is only available in HTTP mode

%Ta format can only be used in HTTP mode but it was not specify in the
configuration manual.

This patch should fix the issue #1317.

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

3 years agoDOC: spoe: Clarify use of the event directive in spoe-message section
Christopher Faulet [Fri, 3 Dec 2021 09:18:09 +0000 (10:18 +0100)]
DOC: spoe: Clarify use of the event directive in spoe-message section

Only one event is possible for a spoe-message section. If defined several
time, only the last one is considered. The documentation is now explicit on
this point.

This patch is related to the the issue #1351.

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

3 years agoBUG/MINOR: cli/server: Don't crash when a server is added with a custom id
Christopher Faulet [Tue, 7 Dec 2021 17:49:44 +0000 (18:49 +0100)]
BUG/MINOR: cli/server: Don't crash when a server is added with a custom id

When a server is dynamically added via the CLI with a custom id, the key
used to insert it in the backend's tree of used names is not initialized.
The server id must be used but it is only used when no custom id is
provided. Thus, with a custom id, HAProxy crashes.

Now, the server id is always used to init this key, to be able to insert the
server in the corresponding tree.

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

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

3 years agoMINOR: http-rules: Add capture action to http-after-response ruleset
Christopher Faulet [Mon, 6 Dec 2021 07:43:22 +0000 (08:43 +0100)]
MINOR: http-rules: Add capture action to http-after-response ruleset

It is now possible to perform captures on the response when
http-after-response rules are evaluated. It may be handy to capture headers
from responses generated by HAProxy.

This patch is trivial, it may be backported if necessary.

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

3 years agoIMPORT: slz: use the correct CRC32 instruction when running in 32-bit mode
Willy Tarreau [Fri, 3 Dec 2021 16:38:42 +0000 (17:38 +0100)]
IMPORT: slz: use the correct CRC32 instruction when running in 32-bit mode

Many ARMv8 processors also support Aarch32 and can run armv7 and even
thumb2 code. While armv8 compilers will not emit these instructions,
armv7 compilers that are aware of these processors will do. For
example, using gcc built for an armv7 target and passing it
"-mcpu=cortex-a72" or "-march=armv8-a+crc" will result in the CRC32
instruction to be used.

In this case the current assembly code fails because with the ARM and
Thumb2 instruction sets there is no such "%wX" half-registers. We need
to use "%X" instead as the native 32-bit register when running with a
32-bit instruction set, and use "%wX" when using the 64-bit instruction
set (A64).

This is slz upstream commit fab83248612a1e8ee942963fe916a9cdbf085097

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

3 years agoBUILD: tree-wide: avoid warnings caused by redundant checks of obj_types
Willy Tarreau [Mon, 6 Dec 2021 07:01:02 +0000 (07:01 +0000)]
BUILD: tree-wide: avoid warnings caused by redundant checks of obj_types

At many places we use construct such as:

   if (objt_server(blah))
       do_something(objt_server(blah));

At -O2 the compiler manages to simplify the operation and see that the
second one returns the same result as the first one. But at -O1 that's
not always the case, and the compiler is able to emit a second
expression and sees the potential null that results from it, and may
warn about a potential null deref (e.g. with gcc-6.5). There are two
solutions to this:
  - either the result of the first test has to be passed to a local
    variable
  - or the second reference ought to be unchecked using the __objt_*
    variant.

This patch fixes all occurrences at once by taking the second approach
(the least intrusive). For constructs like:

   objt_server(blah) ? objt_server(blah)->name : "no name"

a macro could be useful. It would for example take the object type
(server), the field name (name) and the default value. But there
are probably not enough occurrences across the whole code for this
to really matter.

This should be backported wherever it applies.

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

3 years agoMINOR: cli: "show version" displays the current process version
William Lallemand [Tue, 14 Dec 2021 14:22:29 +0000 (15:22 +0100)]
MINOR: cli: "show version" displays the current process version

This patch implements a simple "show version" command which returns
the version of the current process.

It's available from the master and the worker processes, so it is easy
to check if the master and the workers have the same version.

This is a minor patch that really improve compatibility checks
for scripts.

Could be backported in haproxy version as far as 2.0.

(cherry picked from commit 740629e2966641871b183d1c728fa7aeda45a1cc)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

3 years agoBUG/MEDIUM: sample: Fix memory leak in sample_conv_jwt_member_query
Tim Duesterhus [Wed, 1 Dec 2021 22:04:15 +0000 (23:04 +0100)]
BUG/MEDIUM: sample: Fix memory leak in sample_conv_jwt_member_query

The function leaked one full buffer per invocation. Fix this by simply removing
the call to alloc_trash_chunk(), the static chunk from get_trash_chunk() is
sufficient.

This bug was introduced in 0a72f5ee7c2a61bdb379436461269315c776b50a, which is
2.5-dev10. This fix needs to be backported to 2.5+.

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

3 years agoBUILD: bug: Fix error when compiling with -DDEBUG_STRICT_NOCRASH
Christopher Faulet [Fri, 3 Dec 2021 07:58:22 +0000 (08:58 +0100)]
BUILD: bug: Fix error when compiling with -DDEBUG_STRICT_NOCRASH

ha_backtrace_to_stderr() must be declared in CRASH_NOW() macro whe HAProxy
is compiled with DEBUG_STRICT_NOCRASH. Otherwise an error is reported during
compilation:

include/haproxy/bug.h:58:26: error: implicit declaration of function ‘ha_backtrace_to_stderr’ [-Werror=implicit-function-declaration]
   58 | #define CRASH_NOW() do { ha_backtrace_to_stderr(); } while (0)

This patch must be backported as far as 2.4.

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

3 years agoMINOR: mux-h1: Improve H1 traces by adding info about http parsers
Christopher Faulet [Fri, 26 Nov 2021 16:31:35 +0000 (17:31 +0100)]
MINOR: mux-h1: Improve H1 traces by adding info about http parsers

Info about the request and the response parsers are now displayed in H1
traces for advanced and complete verbosity only. This should help debugging.

This patch may be backported as far as 2.4.

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

3 years agoBUG/MINOR: mworker: deinit of thread poller was called when not initialized
William Lallemand [Fri, 26 Nov 2021 13:43:57 +0000 (14:43 +0100)]
BUG/MINOR: mworker: deinit of thread poller was called when not initialized

Commit 67e371e ("BUG/MEDIUM: mworker: FD leak of the eventpoll in wait
mode") introduced a regression. Upon a reload it tries to deinit the
poller per thread, but no poll loop was initialized after loading the
configuration.

This patch fixes the issue by moving this part of the code in
mworker_reload(), since this function will be called only when the
poller is fully initialized.

This patch must be backported in 2.5.

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

3 years agoBUG/MEDIUM: mworker: FD leak of the eventpoll in wait mode
William Lallemand [Thu, 25 Nov 2021 09:03:44 +0000 (10:03 +0100)]
BUG/MEDIUM: mworker: FD leak of the eventpoll in wait mode

Since 2.5, before re-executing in wait mode, the master can have a
working configuration loaded, with a eventpoll fd. This case was not
handled correctly and a new eventpoll FD is leaking in the master at
each reload, which is inherited by the new worker.

Must be backported in 2.5.

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

3 years agoBUG/MEDIUM: h1: Properly reset h1m flags when headers parsing is restarted
Christopher Faulet [Wed, 1 Dec 2021 17:01:48 +0000 (18:01 +0100)]
BUG/MEDIUM: h1: Properly reset h1m flags when headers parsing is restarted

If H1 headers are not fully received at once, the parsing is restarted a
last time when all headers are finally received. When this happens, the h1m
flags are sanitized to remove all value set during parsing.

But some flags where erroneously preserved. Among others, H1_MF_TE_CHUNKED
flag was not removed, what could lead to parsing error.

To fix the bug and make things easy, a mask has been added with all flags
that must be preserved. It will be more stable. This mask is used to
sanitize h1m flags.

This patch should fix the issue #1469. It must be backported to 2.5.

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

3 years agoBUG/MAJOR: segfault using multiple log forward sections.
Emeric Brun [Wed, 1 Dec 2021 11:08:42 +0000 (12:08 +0100)]
BUG/MAJOR: segfault using multiple log forward sections.

For each new log forward section, the proxy was added to the log forward
proxy list but the ref on the previous log forward section's proxy was
scratched using "init_new_proxy" which performs a memset. After configuration
parsing this list contains only the last section's proxy.

The post processing walk through this list to resolve "ring" names.
Since some section's proxies are missing in this list, the resolving
is not done for those ones and the pointer on the ring is kept to null
causing a segfault at runtime trying to write a log message
into the ring.

This patch shift the "init_new_proxy" before adding the ref on the
previous log forward section's proxy on currently parsed one.

This patch shoud fix github issue #1464

This patch should be backported to 2.3

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

3 years agoBUG/MEDIUM: resolvers: Detach query item on response error
Christopher Faulet [Wed, 1 Dec 2021 09:18:08 +0000 (10:18 +0100)]
BUG/MEDIUM: resolvers: Detach query item on response error

When a new response is parsed, it is unexpected to have an old query item
still attached to the resolution. And indeed, when the response is parsed
and validated, the query item is detached and used for a last check on its
dname. However, this is only true for a valid response. If an error is
detected, the query is not detached. This leads to undefined behavior (most
probably a crash) on the next response because the first element in the
query list is referencing an old response.

This patch must be backported as far as 2.0.

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

3 years agoBUG/MINOR: server: Don't rely on last default-server to init server SSL context
Christopher Faulet [Wed, 1 Dec 2021 08:50:41 +0000 (09:50 +0100)]
BUG/MINOR: server: Don't rely on last default-server to init server SSL context

During post-parsing stage, the SSL context of a server is initialized if SSL
is configured on the server or its default-server. It is required to be able
to enable SSL at runtime. However a regression was introduced, because the
last parsed default-server is used. But it is not necessarily the
default-server line used to configure the server. This may lead to
erroneously initialize the SSL context for a server without SSL parameter or
the skip it while it should be done.

The problem is the default-server used to configure a server is not saved
during configuration parsing. So, the information is lost during the
post-parsing. To fix the bug, the SRV_F_DEFSRV_USE_SSL flag is
introduced. It is used to know when a server was initialized with a
default-server using SSL.

For the record, the commit f63704488e ("MEDIUM: cli/ssl: configure ssl on
server at runtime") has introduced the bug.

This patch must be backported as far as 2.4.

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

3 years agoBUG/MINOR: vars: Fix the set-var and unset-var converters
Remi Tricot-Le Breton [Fri, 26 Nov 2021 17:08:39 +0000 (18:08 +0100)]
BUG/MINOR: vars: Fix the set-var and unset-var converters

In commit 3a4bedccc6 the variable logic was changed. Instead of
accessing variables by their name during runtime, the variable tables
are now indexed by a hash of the name. But the set-var and unset-var
converters try to access the correct variable by calculating a hash on
the sample instead of the already calculated variable hash.

It should be backported to 2.5.

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

3 years agoBUILD: evports: remove a leftover from the dead_fd cleanup
Willy Tarreau [Tue, 30 Nov 2021 08:32:21 +0000 (09:32 +0100)]
BUILD: evports: remove a leftover from the dead_fd cleanup

Commit b1f29bc62 ("MINOR: activity/fd: remove the dead_fd counter") got
rid of FD_UPDT_DEAD, but evports managed to slip through the cracks and
wasn't cleaned up, thus it doesn't build anymore, as reported in github
issue #1467. We just need to remove the related lines since the situation
is already handled by the remaining conditions.

Thanks to Dominik Hassler for reporting the issue and confirming the fix.

This must be backported to 2.5 only.

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

3 years agoBUG/MEDIUM: cli: Properly set stream analyzers to process one command at a time
Christopher Faulet [Mon, 18 Oct 2021 12:52:49 +0000 (14:52 +0200)]
BUG/MEDIUM: cli: Properly set stream analyzers to process one command at a time

The proxy used by the master CLI is an internal proxy and no filter are
registered on it. Thus, there is no reason to take care to set or unset
filter analyzers in the master CLI analyzers. AN_REQ_FLT_END was set on the
request channel to prevent the infinite forward and be sure to be able to
process one commande at a time. However, the only work because
CF_FLT_ANALYZE flag was used by error as a channel analyzer instead of a
channel flag. This erroneously set AN_RES_FLT_END on the request channel,
that really prevent the infinite forward, be side effet.

In fact, We must avoid this kind of trick because this only work by chance
and may be source of bugs in future. Instead, we must always keep the CLI
request analyzer and add an early return if the response is not fully
processed. It happens when the CLI response analyzer is set.

This patch must be backported as far as 2.0.

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

3 years agoBUG/MINOR: lua: remove loop initial declarations
Bertrand Jacquin [Wed, 24 Nov 2021 21:16:06 +0000 (21:16 +0000)]
BUG/MINOR: lua: remove loop initial declarations

HAProxy is documented to support gcc >= 3.4 as per INSTALL file, however
hlua.c makes use of c11 only loop initial declarations leading to build
failure when using gcc-4.9.4:

  x86_64-unknown-linux-gnu-gcc -Iinclude  -Wchar-subscripts -Wcomment -Wformat -Winit-self -Wmain -Wmissing-braces -Wno-pragmas -Wparentheses -Wreturn-type -Wsequence-point -Wstrict-aliasing -Wswitch -Wtrigraphs -Wuninitialized -Wunknown-pragmas -Wunused-label -Wunused-variable -Wunused-value -Wpointer-sign -Wimplicit -pthread -fdiagnostics-color=auto -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -O3 -msse -mfpmath=sse -march=core2 -g -fPIC -g -Wall -Wextra -Wundef -Wdeclaration-after-statement -fwrapv -Wno-unused-label -Wno-sign-compare -Wno-unused-parameter -Wno-clobbered -Wno-missing-field-initializers -Wtype-limits      -DUSE_EPOLL  -DUSE_NETFILTER   -DUSE_PCRE2 -DUSE_PCRE2_JIT -DUSE_POLL -DUSE_THREAD -DUSE_BACKTRACE   -DUSE_TPROXY -DUSE_LINUX_TPROXY -DUSE_LINUX_SPLICE -DUSE_LIBCRYPT -DUSE_CRYPT_H -DUSE_GETADDRINFO -DUSE_OPENSSL -DUSE_LUA -DUSE_ACCEPT4   -DUSE_SLZ -DUSE_CPU_AFFINITY -DUSE_TFO -DUSE_NS -DUSE_DL -DUSE_RT      -DUSE_PRCTL  -DUSE_THREAD_DUMP        -DUSE_PCRE2 -DPCRE2_CODE_UNIT_WIDTH=8  -I/usr/local/include -DCONFIG_HAPROXY_VERSION=\"2.5.0\" -DCONFIG_HAPROXY_DATE=\"2021/11/23\" -c -o src/connection.o src/connection.c
  src/hlua.c: In function 'hlua_config_prepend_path':
  src/hlua.c:11292:2: error: 'for' loop initial declarations are only allowed in C99 or C11 mode
    for (size_t i = 0; i < 2; i++) {
    ^
  src/hlua.c:11292:2: note: use option -std=c99, -std=gnu99, -std=c11 or -std=gnu11 to compile your code

This commit moves loop iterator to an explicit declaration.

Must be backported to 2.5 because this issue was introduced in
v2.5-dev10~69 with commit 9e5e586e35c5 ("BUG/MINOR: lua: Fix lua error
 handling in `hlua_config_prepend_path()`")

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

3 years agoBUG/MINOR: lua: don't expose internal proxies
William Lallemand [Wed, 24 Nov 2021 15:14:24 +0000 (16:14 +0100)]
BUG/MINOR: lua: don't expose internal proxies

Since internal proxies are now in the global proxy list, they are now
reachable from core.proxies, core.backends, core.frontends.

This patch fixes the issue by checking the PR_CAP_INT flag before
exposing them in lua, so the user can't have access to them.

This patch must be backported in 2.5.

(cherry picked from commit 82d5f013f96d38867985469bf47bd36ff199a256)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

3 years agoBUG/MINOR: httpclient: allow to replace the host header
William Lallemand [Wed, 24 Nov 2021 14:38:17 +0000 (15:38 +0100)]
BUG/MINOR: httpclient: allow to replace the host header

This patch allows to replace the host header generated by the
httpclient instead of adding a new one, resulting in the server replying
an error 400.

The host header is now generated from the uri only if it wasn't found in
the list of headers.

Also add a new request in the VTC file to test this.

This patch must be backported in 2.5.

(cherry picked from commit f03b53c81d52344c29d7a5434ac32f3127cebf75)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

3 years agoBUG/MINOR: cache: Fix loop on cache entries in "show cache"
Christopher Faulet [Tue, 23 Nov 2021 15:03:05 +0000 (16:03 +0100)]
BUG/MINOR: cache: Fix loop on cache entries in "show cache"

A regression was introduced in the commit da91842b6 ("BUG/MEDIUM: cache/cli:
make "show cache" thread-safe"). When cli_io_handler_show_cache() is called,
only one node is retrieved and is used to fill the output buffer in loop.
Once set, the "node" variable is never renewed. At the end, all nodes are
dumped but each one is duplicated several time into the output buffer.

This patch must be backported everywhere the above commit is. It means only
to 2.5 and 2.4.

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

3 years ago[RELEASE] Released version 2.5.0 v2.5.0
Willy Tarreau [Tue, 23 Nov 2021 14:40:21 +0000 (15:40 +0100)]
[RELEASE] Released version 2.5.0

Released version 2.5.0 with the following main changes :
    - BUILD: SSL: add quictls build to scripts/build-ssl.sh
    - BUILD: SSL: add QUICTLS to build matrix
    - CLEANUP: sock: Wrap `accept4_broken = 1` into additional parenthesis
    - BUILD: cli: clear a maybe-unused  warning on some older compilers
    - BUG/MEDIUM: cli: make sure we can report a warning from a bind keyword
    - BUG/MINOR: ssl: make SSL counters atomic
    - CLEANUP: assorted typo fixes in the code and comments
    - BUG/MINOR: ssl: free correctly the sni in the backend SSL cache
    - MINOR: version: mention that it's stable now

3 years agoMINOR: version: mention that it's stable now
Willy Tarreau [Tue, 23 Nov 2021 14:38:10 +0000 (15:38 +0100)]
MINOR: version: mention that it's stable now

This version will be maintained up to around Q1 2023. The INSTALL file
also mentions it.

3 years agoBUG/MINOR: ssl: free correctly the sni in the backend SSL cache
William Lallemand [Tue, 23 Nov 2021 14:15:09 +0000 (15:15 +0100)]
BUG/MINOR: ssl: free correctly the sni in the backend SSL cache

__ssl_sock_load_new_ckch_instance() does not free correctly the SNI in
the session cache, it only frees the one in the current tid.

This bug was introduced with e18d4e8 ("BUG/MEDIUM: ssl: backend TLS
resumption with sni and TLSv1.3").

This fix must be backported where the mentionned commit was backported.
(all maintained versions).

3 years agoCLEANUP: assorted typo fixes in the code and comments
Ilya Shipitsin [Sat, 20 Nov 2021 18:11:12 +0000 (23:11 +0500)]
CLEANUP: assorted typo fixes in the code and comments

This is 28th iteration of typo fixes

3 years agoBUG/MINOR: ssl: make SSL counters atomic
Willy Tarreau [Mon, 22 Nov 2021 16:46:13 +0000 (17:46 +0100)]
BUG/MINOR: ssl: make SSL counters atomic

SSL counters were added with commit d0447a7c3 ("MINOR: ssl: add counters
for ssl sessions") in 2.4, but their updates were not atomic, so it's
likely that under significant loads they are not correct.

This needs to be backported to 2.4.

3 years agoBUG/MEDIUM: cli: make sure we can report a warning from a bind keyword
Willy Tarreau [Sat, 20 Nov 2021 19:10:41 +0000 (20:10 +0100)]
BUG/MEDIUM: cli: make sure we can report a warning from a bind keyword

Since recent 2.5 commit c8cac04bd ("MEDIUM: listener: deprecate "process"
in favor of "thread" on bind lines"), the "process" bind keyword may
report a warning. However some parts like the "stats socket" parser
will call such bind keywords and do not expect to face warnings, so
this will instantly cause a fatal error to be reported. A concrete
effect is that "stats socket ... process 1" will hard-fail indicating
the keyword is deprecated and will be removed in 2.7.

We must relax this test, but the code isn't designed to report warnings,
it uses a single string and only supports reporting an error code (-1).

This patch makes a special case of the ERR_WARN code and uses ha_warning()
to report it, and keeps the rest of the existing error code for other
non-warning codes. Now "process" on the "stats socket" is properly
reported as a warning.

No backport is needed.

3 years agoBUILD: cli: clear a maybe-unused warning on some older compilers
Willy Tarreau [Sat, 20 Nov 2021 18:17:38 +0000 (19:17 +0100)]
BUILD: cli: clear a maybe-unused  warning on some older compilers

The SHOW_TOT() and SHOW_AVG() macros used in cli_io_handler_show_activity()
produce a warning on gcc 4.7 on MIPS with threads disabled because the
compiler doesn't know that global.nbthread is necessarily non-null, hence
that at least one iteration is performed. Let's just change the loop for
a do {} while () that lets the compiler know it's always initialized. It
also has the tiny benefit of making the code shorter.

3 years agoCLEANUP: sock: Wrap `accept4_broken = 1` into additional parenthesis
Tim Duesterhus [Sat, 20 Nov 2021 13:39:47 +0000 (14:39 +0100)]
CLEANUP: sock: Wrap `accept4_broken = 1` into additional parenthesis

This makes it clear to static analysis tools that this assignment is
intentional and not a mistyped comparison.

3 years agoBUILD: SSL: add QUICTLS to build matrix
Ilya Shipitsin [Thu, 18 Nov 2021 13:27:57 +0000 (18:27 +0500)]
BUILD: SSL: add QUICTLS to build matrix

It also enables QUIC when QUICTLS is used.

3 years agoBUILD: SSL: add quictls build to scripts/build-ssl.sh
Ilya Shipitsin [Thu, 18 Nov 2021 13:27:56 +0000 (18:27 +0500)]
BUILD: SSL: add quictls build to scripts/build-ssl.sh

script/build-ssl.sh is used mostly in CI, let us introduce QUIC
OpenSSL fork support

3 years ago[RELEASE] Released version 2.5-dev15 v2.5-dev15
Willy Tarreau [Fri, 19 Nov 2021 18:30:04 +0000 (19:30 +0100)]
[RELEASE] Released version 2.5-dev15

Released version 2.5-dev15 with the following main changes :
    - BUG/MINOR: stick-table/cli: Check for invalid ipv6 key
    - CLEANUP: peers: Remove useless test on peer variable in peer_trace()
    - DOC: log: Add comments to specify when session's listener is defined or not
    - BUG/MEDIUM: mux-h1: Handle delayed silent shut in h1_process() to release H1C
    - REGTESTS: ssl_crt-list_filters: feature cmd incorrectly set
    - DOC: internals: document the list API
    - BUG/MINOR: h3: ignore unknown frame types
    - MINOR: quic: redirect app_ops snd_buf through mux
    - MEDIUM: quic: inspect ALPN to install app_ops
    - MINOR: quic: support hq-interop
    - MEDIUM: quic: send version negotiation packet on unknown version
    - BUG/MEDIUM: mworker: cleanup the listeners when reexecuting
    - DOC: internals: document the scheduler API
    - BUG/MINOR: quic: fix version negotiation packet generation
    - CLEANUP: ssl: fix wrong #else commentary
    - MINOR: config: support default values for environment variables
    - SCRIPTS: run-regtests: reduce the number of processes needed to check options
    - SCRIPT: run-regtests: avoid several calls to grep to test for features
    - SCRIPT: run-regtests: avoid calling awk to compute the version
    - REGTEST: set retries count to zero for all tests that expect at 503
    - REGTESTS: make tcp-check_min-recv fail fast
    - REGTESTS: extend the default I/O timeouts and make them overridable
    - BUG/MEDIUM: ssl: backend TLS resumption with sni and TLSv1.3
    - BUG/MEDIUM: ssl: abort with the correct SSL error when SNI not found
    - REGTESTS: ssl: test the TLS resumption
    - BUILD: makefile: stop opening sub-shells for each and every command
    - BUILD: makefile: reorder objects by build time
    - BUG/MEDIUM: mux-h2: always process a pending shut read
    - MINOR: quic_sock: missing CO_FL_ADDR_TO_SET flag
    - MINOR: quic: Possible wrong connection identification
    - MINOR: quic: Correctly pad UDP datagrams
    - MINOR: quic: Support transport parameters draft TLS extension
    - MINOR: quic: Anti-amplification implementation
    - MINOR: quic: Wrong Initial packet connection initialization
    - MINOR: quic: Wrong ACK range building
    - MINOR: quic: Update some QUIC protocol errors
    - MINOR: quic: Send CONNECTION_CLOSE frame upon TLS alert
    - MINOR: quic: Wrong largest acked packet number parsing
    - MINOR: quic: Add minimalistic support for stream flow control frames
    - MINOR: quic: Wrong value for version negotiation packet 'Unused' field
    - MINOR: quic: Support draft-29 QUIC version
    - BUG/MINOR: quic: fix segfault on trace for version negotiation
    - BUG/MINOR: hq-interop: fix potential NULL dereference
    - BUILD: quic: fix potential NULL dereference on xprt_quic
    - DOC: lua: documentation about the httpclient API
    - BUG/MEDIUM: cache/cli: make "show cache" thread-safe
    - BUG/MEDIUM: shctx: leave the block allocator when enough blocks are found
    - BUG/MINOR: shctx: do not look for available blocks when the first one is enough
    - MINOR: shctx: add a few BUG_ON() for consistency checks

3 years agoMINOR: shctx: add a few BUG_ON() for consistency checks
Willy Tarreau [Fri, 19 Nov 2021 16:47:18 +0000 (17:47 +0100)]
MINOR: shctx: add a few BUG_ON() for consistency checks

The shctx code relies on sensitive conditions that are hard to infer
from the code itself, let's add some BUG_ON() to verify them. They
helped spot the previous bugs.

3 years agoBUG/MINOR: shctx: do not look for available blocks when the first one is enough
Willy Tarreau [Fri, 19 Nov 2021 16:42:49 +0000 (17:42 +0100)]
BUG/MINOR: shctx: do not look for available blocks when the first one is enough

In shctx_row_reserve_hot() we only leave if we've found the exact
requested size instead of at least as large, as is documented. This
results in extra lookups and free calls in the avail loop while it is
not needed, and participates to seeing a negative data_len early as
spotted in previous bugs.

It doesn't seem to have any other impact however, but it's better to
backport it to stable branches.