]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
4 years agoMINOR: diag: create cfgdiag module
Amaury Denoyelle [Tue, 30 Mar 2021 15:34:24 +0000 (17:34 +0200)] 
MINOR: diag: create cfgdiag module

This module is intended to serve as a placeholder for various
diagnostics executed after the configuration file has been fully loaded.

4 years agoMINOR: server: diag for 0 weight server
Amaury Denoyelle [Tue, 30 Mar 2021 08:26:27 +0000 (10:26 +0200)] 
MINOR: server: diag for 0 weight server

Output a diagnostic report if a server has been configured with a null
weight.

4 years agoMINOR: cfgparse: diag for multiple nbthread statements
Amaury Denoyelle [Mon, 29 Mar 2021 08:41:15 +0000 (10:41 +0200)] 
MINOR: cfgparse: diag for multiple nbthread statements

Output a diagnostic report if the nbthread statement is defined on
several places in the configuration.

4 years agoMINOR: global: define diagnostic mode of execution
Amaury Denoyelle [Mon, 29 Mar 2021 08:29:07 +0000 (10:29 +0200)] 
MINOR: global: define diagnostic mode of execution

Define MODE_DIAG which is used to run haproxy in diagnostic mode. This
mode is used to output extra warnings about possible configuration
blunder or sub-optimal usage. It can be activated with argument '-dD'.

A new output function ha_diag_warning is implemented reserved for
diagnostic output. It serves to standardize the format of diagnostic
messages.

A macro HA_DIAG_WARN_COND is also available to automatically check if
diagnostic mode is on before executing the diagnostic check.

4 years agoCLEANUP: vars: always pre-initialize smp in vars_parse_cli_get_var()
Willy Tarreau [Thu, 1 Apr 2021 15:01:43 +0000 (17:01 +0200)] 
CLEANUP: vars: always pre-initialize smp in vars_parse_cli_get_var()

In issue #1200 Coverity believes we may use an uninitialized field
smp.sess here while it's not possible because the returned variable
necessarily matches SCOPE_PROC hence smp.sess is not used. But it
cannot see this and it could be confusing if the code later evolved
into something more complex. That's not a critical path so let's
first reset the sample.

4 years agoBUG/MINOR: http-fetch: Fix test on message state to capture the version
Christopher Faulet [Thu, 1 Apr 2021 14:00:29 +0000 (16:00 +0200)] 
BUG/MINOR: http-fetch: Fix test on message state to capture the version

A bug was introduced when the legacy HTTP mode was removed. To capture the
HTTP version of the request or the response, we rely on the message state to
be sure the status line was received. However, the test is inverted. The
version can be captured if message headers were received, not the opposite.

This patch must be backported as far as 2.2.

4 years agoREGTESTS: Add script to tests the wait-for-body HTTP action
Christopher Faulet [Thu, 1 Apr 2021 08:45:57 +0000 (10:45 +0200)] 
REGTESTS: Add script to tests the wait-for-body HTTP action

This script tests the wait-for-body HTTP action, on the request side and the
response side.

4 years agoMEDIUM: http-rules: Add wait-for-body action on request and response side
Christopher Faulet [Mon, 29 Mar 2021 08:46:38 +0000 (10:46 +0200)] 
MEDIUM: http-rules: Add wait-for-body action on request and response side

Historically, an option was added to wait for the request payload (option
http-buffer-request). This option has 2 drawbacks. First, it is an ON/OFF
option for the whole proxy. It cannot be enabled on demand depending on the
message. Then, as its name suggests, it only works on the request side. The
only option to wait for the response payload was to write a dedicated
filter. While it is an acceptable solution for complex applications, it is a
bit overkill to simply match strings in the body.

To make everyone happy, this patch adds a dedicated HTTP action to wait for
the message payload, for the request or the response depending it is used in
an http-request or an http-response ruleset. The time to wait is
configurable and, optionally, the minimum payload size to have before stop
to wait.

Both the http action and the old http analyzer rely on the same internal
function.

4 years agoMINOR: payload/config: Warn if a L6 sample fetch is used from an HTTP proxy
Christopher Faulet [Fri, 26 Mar 2021 09:02:46 +0000 (10:02 +0100)] 
MINOR: payload/config: Warn if a L6 sample fetch is used from an HTTP proxy

L6 sample fetches are now ignored when called from an HTTP proxy. Thus, a
warning is emitted during the startup if such usage is detected. It is true
for most ACLs and for log-format strings. Unfortunately, it is a bit painful
to do so for sample expressions.

This patch relies on the commit "MINOR: action: Use a generic function to
check validity of an action rule list".

4 years agoMINOR: action: Use a generic function to check validity of an action rule list
Christopher Faulet [Thu, 25 Mar 2021 16:19:04 +0000 (17:19 +0100)] 
MINOR: action: Use a generic function to check validity of an action rule list

The check_action_rules() function is now used to check the validity of an
action rule list. It is used from check_config_validity() function to check
L5/6/7 rulesets.

4 years agoMINOR: htx: Make internal.strm.is_htx an internal sample fetch
Christopher Faulet [Thu, 25 Mar 2021 16:29:38 +0000 (17:29 +0100)] 
MINOR: htx: Make internal.strm.is_htx an internal sample fetch

It is not really a context-less sample fetch, but it is internal. And it
only fails if no stream is attached to the sample. This way, it is still
possible to use it on an HTTP proxy (L6 sample fetches are ignored now for
HTTP proxies).

If the commit "BUG/MINOR: payload/htx: Ingore L6 sample fetches for HTX
streams/checks" is backported, it may be a good idea to backport this one
too. But only as far as 2.2.

4 years agoBUG/MINOR: payload/htx: Ingore L6 sample fetches for HTX streams/checks
Christopher Faulet [Thu, 25 Mar 2021 10:58:51 +0000 (11:58 +0100)] 
BUG/MINOR: payload/htx: Ingore L6 sample fetches for HTX streams/checks

Use a L6 sample fetch on an HTX streams or a HTX health-check is meaningless
because data are not raw but structured. So now, these sample fetches fail
when called from an HTTP proxy. In addition, a warning has been added in the
configuration manual, at the begining of the L6 sample fetches section.

Note that req.len and res.len samples return the HTX data size instead of
failing. It is not accurate because it does not reflect the buffer size nor
the raw data length. But we keep it for backward compatibility purpose.
However it remains a bit strange to use it on an HTTP proxy.

This patch may be backported to all versions supporting the HTX, i.e as far
as 2.0. But the part about the health-checks is only valid for the 2.2 and
upper.

4 years agoREGTESTS: Add script to tests TCP to HTTP upgrades
Christopher Faulet [Wed, 31 Mar 2021 15:22:31 +0000 (17:22 +0200)] 
REGTESTS: Add script to tests TCP to HTTP upgrades

This script tests various TCP>HTTP upgrade scenarios and also performs some
checks for invalid/forbidden upgrades.

4 years agoDOC: config: Add documentation about TCP to HTTP upgrades
Christopher Faulet [Fri, 26 Mar 2021 13:44:00 +0000 (14:44 +0100)] 
DOC: config: Add documentation about TCP to HTTP upgrades

This patch adds explanation about chaining a TCP frontend to an HTTP
backend. It also explain how the HTTP upgrades work in this context. A note
has also been added in "Fetching HTTP samples" section to warning about HTTP
content processing in TCP.

4 years agoMINOR: config/proxy: Warn if a TCP proxy without backend is upgradable to HTTP
Christopher Faulet [Wed, 31 Mar 2021 15:13:49 +0000 (17:13 +0200)] 
MINOR: config/proxy: Warn if a TCP proxy without backend is upgradable to HTTP

If a 'switch-mode http' tcp action is configured on a listener with no
backend, a warning is displayed to remember HTTP connections cannot be
routed to TCP servers. Indeed, backend connection is still established using
the proxy mode.

4 years agoMINOR: config/proxy: Don't warn for HTTP rules in TCP if 'switch-mode http' set
Christopher Faulet [Mon, 15 Mar 2021 14:10:38 +0000 (15:10 +0100)] 
MINOR: config/proxy: Don't warn for HTTP rules in TCP if 'switch-mode http' set

Warnings about ignored HTTP directives in a TCP proxy are inhibited if at
least one switch-mode tcp action is configured to perform HTTP upgraded.

4 years agoMEDIUM: Add tcp-request switch-mode action to perform HTTP upgrade
Christopher Faulet [Mon, 15 Mar 2021 11:03:44 +0000 (12:03 +0100)] 
MEDIUM: Add tcp-request switch-mode action to perform HTTP upgrade

It is now possible to perform HTTP upgrades on a TCP stream from the
frontend side. To do so, a tcp-request content rule must be defined with the
switch-mode action, specifying the mode (for now, only http is supported)
and optionnaly the proto (h1 or h2).

This way it could be possible to set HTTP directives on a TCP frontend which
will only be evaluated if an upgrade is performed. This new way to perform
HTTP upgrades should replace progressively the old way, consisting to route
the request to an HTTP backend. And it should be also a good start to remove
all HTTP processing from tcp-request content rules.

This action is terminal, it stops the ruleset evaluation. It is only
available on proxy with the frontend capability.

The configuration manual has been updated accordingly.

4 years agoMINOR: stream: Handle stream HTTP upgrade in a dedicated function
Christopher Faulet [Mon, 15 Mar 2021 09:42:02 +0000 (10:42 +0100)] 
MINOR: stream: Handle stream HTTP upgrade in a dedicated function

The code responsible to perform an HTTP upgrade from a TCP stream is moved
in a dedicated function, stream_set_http_mode().

The stream_set_backend() function is slightly updated, especially to
correctly set the request analysers.

4 years agoMINOR: http-ana: Simplify creation/destruction of HTTP transactions
Christopher Faulet [Mon, 8 Mar 2021 18:12:58 +0000 (19:12 +0100)] 
MINOR: http-ana: Simplify creation/destruction of HTTP transactions

Now allocation and initialization of HTTP transactions are performed in a
unique function. Historically, there were two functions because the same TXN
was reset for K/A connections in the legacy HTTP mode. Now, in HTX, K/A
connections are handled at the mux level. A new stream, and thus a new TXN,
is created for each request. In addition, the function responsible to end
the TXN is now also reponsible to release it.

So, now, http_create_txn() and http_destroy_txn() must be used to create and
destroy an HTTP transaction.

4 years agoMINOR: filters/http-ana: Decide to filter HTTP headers in HTTP analysers
Christopher Faulet [Mon, 8 Mar 2021 17:20:09 +0000 (18:20 +0100)] 
MINOR: filters/http-ana: Decide to filter HTTP headers in HTTP analysers

It is just a small cleanup. AN_REQ_FLT_HTTP_HDRS and AN_RES_FLT_HTTP_HDRS
analysers are now set in HTTP analysers at the same place
AN_REQ_HTTP_XFER_BODY and AN_RES_HTTP_XFER_BODY are set.

4 years agoMINOR: stream: Use stream type instead of proxy mode when appropriate
Christopher Faulet [Mon, 8 Mar 2021 16:57:53 +0000 (17:57 +0100)] 
MINOR: stream: Use stream type instead of proxy mode when appropriate

We now use the stream instead of the proxy to know if we are processing HTTP
data or not. If the stream is an HTX stream, it means we are dealing with
HTTP data. It is more accurate than the proxy mode because when an HTTP
upgrade is performed, the proxy is not changed and only the stream may be
used.

Note that it was not a problem to rely on the proxy because HTTP upgrades
may only happen when an HTTP backend was set. But, we will add the support
of HTTP upgrades on the frontend side, after te tcp-request rules
evaluation.  In this context, we cannot rely on the proxy mode.

4 years agoDOC: config: Improve documentation about proto/check-proto keywords
Christopher Faulet [Fri, 26 Mar 2021 13:44:18 +0000 (14:44 +0100)] 
DOC: config: Improve documentation about proto/check-proto keywords

This patch adds a description about information provided by "haproxy -vv"
command regarding the available protocols. The description is adapted
depending the context (bind line, server line or health-check).

4 years agoMINOR: muxes: Show muxes flags when the mux list is displayed
Christopher Faulet [Mon, 22 Mar 2021 16:06:24 +0000 (17:06 +0100)] 
MINOR: muxes: Show muxes flags when the mux list is displayed

When the mux list is displayed on "haproxy -vv" output, the mux flags are
now diplayed. The flags meaning may be found in the configuration manual.

4 years agoMEDIUM: mux-pt: Expose passthrough in the list of supported mux protocols
Christopher Faulet [Fri, 5 Feb 2021 15:44:46 +0000 (16:44 +0100)] 
MEDIUM: mux-pt: Expose passthrough in the list of supported mux protocols

Add "none" in the list of supported mux protocols. It relies on the
passthrough multiplexer and use almost the same mux_ops structure. Only the
flags differ because this "new" mux does not support the upgrades. "none"
was chosen to explicitly stated there is not processing at the mux level.

Thus it is now possible to set "proto none" or "check-proto none" on
bind/server lines, depending on the context. However, when set, no upgrade
to HTTP is performed. It may be a way to disable HTTP upgrades per bind
line.

4 years agoMEDIUM: mux-h1: Expose h1 in the list of supported mux protocols
Christopher Faulet [Fri, 5 Feb 2021 15:44:21 +0000 (16:44 +0100)] 
MEDIUM: mux-h1: Expose h1 in the list of supported mux protocols

Add "h1" in the list of supported mux protocols. It relies on the H1
multiplexer and use the almost the same mux_ops structure. Only the flags
differ because this "new" mux does not support the upgrades.

Thus it is now possible to set "proto h1" or "check-proto h1" on bind/server
lines, depending on the context. However, when set, no upgrade to HTTP/2 is
performed. It may be a way to disable implicit HTTP/2 upgrades per bind
line.

4 years agoMINOR: mux-pt: Don't perform implicit HTTP upgrade if not supported by mux
Christopher Faulet [Mon, 8 Mar 2021 14:32:28 +0000 (15:32 +0100)] 
MINOR: mux-pt: Don't perform implicit HTTP upgrade if not supported by mux

For now this tests is useless, but if the PT muliplexer is flagged to
explicitly not support the upgrades to HTTP, an error is returned.

4 years agoMINOR: mux-h1: Don't perform implicit HTTP/2 upgrade if not supported by mux
Christopher Faulet [Mon, 8 Mar 2021 14:32:03 +0000 (15:32 +0100)] 
MINOR: mux-h1: Don't perform implicit HTTP/2 upgrade if not supported by mux

For now this tests is useless, but if the H1 muliplexer is flagged to
explicitly not support the upgrades to HTTP/2, an error is returned.

4 years agoMINOR: muxes: Add a flag to notify a mux does not support any upgrade
Christopher Faulet [Mon, 8 Mar 2021 14:28:28 +0000 (15:28 +0100)] 
MINOR: muxes: Add a flag to notify a mux does not support any upgrade

MX_FL_NO_UPG flag may now be set on a multiplexer to explicitly disable
upgrades from this mux. For now, it is set on the FCGI multiplexer because
it is not supported and there is no upgrade on backend-only multiplexers. It
is also set on the H2 multiplexer because it is clearly not supported.

4 years agoBUG/MINOR: config: Add warning for http-after-response rules in TCP mode
Christopher Faulet [Mon, 15 Mar 2021 14:09:21 +0000 (15:09 +0100)] 
BUG/MINOR: config: Add warning for http-after-response rules in TCP mode

No warning is emitted if some http-after-response rules are configured on a
TCP proxy while such warning messages are emitted for other HTTP ruleset in
same condition. It is just an oversight.

This patch may be backported as far as 2.2.

4 years agoBUG/MINOR: stream: Properly handle TCP>H1>H2 upgrades in http_wait_for_request
Christopher Faulet [Mon, 15 Mar 2021 16:10:12 +0000 (17:10 +0100)] 
BUG/MINOR: stream: Properly handle TCP>H1>H2 upgrades in http_wait_for_request

When a TCP stream is first upgraded to H1 and then to H2, we must be sure to
inhibit any connect and to properly handle the TCP stream destruction.

When the TCP stream is upgraded to H1, the HTTP analysers are set. Thus
http_wait_for_request() is called. In this case, the server connection must
be blocked, waiting for the request analysis. Otherwise, a server may be
assigned to the stream too early. It is especially a problem if the stream
is finally destroyed because of an implicit upgrade to H2.

In this case, the stream processing must be properly aborted to not have a
stalled stream. Thus, if a shutdown is detected in http_wait_for_request()
when an HTTP upgrade is performed, the stream is aborted.

It is a 2.4-specific bug. No backport is needed.

4 years agoMINOR: stream: Be sure to set HTTP analysers when creating an HTX stream
Christopher Faulet [Mon, 15 Mar 2021 16:09:27 +0000 (17:09 +0100)] 
MINOR: stream: Be sure to set HTTP analysers when creating an HTX stream

Always set frontend HTTP analysers when an HTX stream is created. It is only
useful in case a destructive HTTP upgrades (TCP>H2) because the frontend is
a TCP proxy.

In fact, to be strict, we must only set these analysers when the upgrade is
performed before setting the backend (it is not supported yet, but this
patch is required to do so), in the frontend part. If the upgrade happens
when the backend is set, it means the HTTP processing is just the backend
buisness. But there is no way to make the difference when a stream is
created, at least for now.

4 years agoMINOR: frontend: Create HTTP txn for HTX streams
Christopher Faulet [Mon, 15 Mar 2021 16:08:08 +0000 (17:08 +0100)] 
MINOR: frontend: Create HTTP txn for HTX streams

When an HTX stream is created, be sure to always create the HTTP txn object,
regardless of the ".http_needed" value of the frontend. That happens when a
destructive HTTP upgrades is performed (TCP>H2). The frontend is a TCP
proxy. If there is no dependency on the HTTP part, the HTTP transaction is
not created at this stage but only when the backend is set. For now, it is
not a problem. But an HTTP txn will be mandatory to fully support TCP to
HTTP upgrades after frontend tcp-request rules evaluation.

4 years agoMINOR: stream: Don't trigger errors on destructive HTTP upgrades
Christopher Faulet [Mon, 22 Mar 2021 14:07:51 +0000 (15:07 +0100)] 
MINOR: stream: Don't trigger errors on destructive HTTP upgrades

When a TCP stream is upgraded to H2 stream, a destructive upgrade is
performed. It means the TCP stream is silently released while a new one is
created. It is of course more complicated but it is what we observe from the
stream point of view.

That was performed by returning an error when the backend was set. It is
neither really elegant nor accurate. So now, instead of returning an error
from stream_set_backend() in case of destructive HTTP upgrades, the TCP
stream processing is aborted and no error is reported. However, the result
is more or less the same.

4 years agoBUG/MINOR: mux-h2: Don't emit log twice if an error occurred on the preface
Christopher Faulet [Mon, 15 Mar 2021 16:54:31 +0000 (17:54 +0100)] 
BUG/MINOR: mux-h2: Don't emit log twice if an error occurred on the preface

sess_log() was called twice if an error occurred on the preface parsing, in
h2c_frt_recv_preface() and in h2_process_demux().

This patch must be backported as far as 2.0.

4 years agoBUG/MINOR: http_fetch: make hdr_ip() resistant to empty fields
Willy Tarreau [Wed, 31 Mar 2021 09:41:36 +0000 (11:41 +0200)] 
BUG/MINOR: http_fetch: make hdr_ip() resistant to empty fields

The fix in commit 7b0e00d94 ("BUG/MINOR: http_fetch: make hdr_ip() reject
trailing characters") made hdr_ip() more sensitive to empty fields, for
example if a trusted proxy incorrectly sends the header with an empty
value, we could return 0.0.0.0 which is not correct. Let's make sure we
only assign an IPv4 type here when a non-empty address was found.

This should be backported to all branches where the fix above was
backported.

4 years agoCLEANUP: socket: replace SOL_IP/IPV6/TCP with IPPROTO_IP/IPV6/TCP
Willy Tarreau [Wed, 31 Mar 2021 06:45:47 +0000 (08:45 +0200)] 
CLEANUP: socket: replace SOL_IP/IPV6/TCP with IPPROTO_IP/IPV6/TCP

Historically we've used SOL_IP/SOL_IPV6/SOL_TCP everywhere as the socket
level value in getsockopt() and setsockopt() but as we've seen over time
it regularly broke the build and required to have them defined to their
IPPROTO_* equivalent. The Linux ip(7) man page says:

   Using the SOL_IP socket options level isn't portable; BSD-based
   stacks use the IPPROTO_IP level.

And it indeed looks like a pure linuxism inherited from old examples and
documentation. strace also reports SOL_* instead of IPPROTO_*, which does
not help... A check to linux/in.h shows they have the same values. Only
SOL_SOCKET and other non-IP values make sense since there is no IPPROTO
equivalent.

Let's get rid of this annoying confusion by removing all redefinitions of
SOL_IP/IPV6/TCP and using IPPROTO_* instead, just like any other operating
system. This also removes duplicated tests for the same value.

Note that this should not result in exposing syscalls to other OSes
as the only ones that were still conditionned to SOL_IPV6 were for
IPV6_UNICAST_HOPS which already had an IPPROTO_IPV6 equivalent, and
IPV6_TRANSPARENT which is Linux-specific.

4 years agoBUILD: tcp: use IPPROTO_IPV6 instead of SOL_IPV6 on FreeBSD/MacOS
Willy Tarreau [Wed, 31 Mar 2021 06:29:27 +0000 (08:29 +0200)] 
BUILD: tcp: use IPPROTO_IPV6 instead of SOL_IPV6 on FreeBSD/MacOS

Lukas reported in issue #1203 that the previous fix for silent-drop in
commit ab79ee8b1 ("BUG/MINOR: tcp: fix silent-drop workaround for IPv6")
breaks the build on FreeBSD/MacOS due to SOL_IPV6 not being defined. On
these platforms, IPPROTO_IPV6 must be used instead, so this should fix
it.

This needs to be backported to whatever version the fix above is backported
to.

4 years agoBUG/MINOR: tcp: fix silent-drop workaround for IPv6
Willy Tarreau [Tue, 30 Mar 2021 15:23:50 +0000 (17:23 +0200)] 
BUG/MINOR: tcp: fix silent-drop workaround for IPv6

As reported in github issue #1203 the TTL-based workaround that is used
when permissions are insufficient for the TCP_REPAIR trick does not work
for IPv6 because we're using only SOL_IP with IP_TTL. In IPv6 we have to
use SOL_IPV6 and IPV6_UNICAST_HOPS. Let's pick the right one based on the
source address's family.

This may be backported to all versions.

4 years agoBUG/MEDIUM: time: make sure to always initialize the global tick
Willy Tarreau [Tue, 30 Mar 2021 16:13:26 +0000 (18:13 +0200)] 
BUG/MEDIUM: time: make sure to always initialize the global tick

The issue with non-rotating freq counters was addressed in commit 8cc586c73
("BUG/MEDIUM: freq_ctr/threads: use the global_now_ms variable") using the
global date. But an issue remained with the comparison of the most recent
time. Since the initial time in the structure is zero, the tick_is_lt()
works on half of the periods depending on the first date an entry is
touched. And the wrapping happened last night:

  $ date --date=@$(((($(date +%s) * 1000) & -0x8000000) / 1000))
  Mon Mar 29 23:59:46 CEST 2021

So users of the last fix (backported to 2.3.8) may experience again an
always increasing rate for the next 24 days if they restart their process.

Let's always update the time if the latest date was not updated yet. It
will likely be simplified once the function is reorganized but this will
do the job for now.

Note that since this timer is only used by freq counters, no other
sub-system is affected. The bug can easily be tested with this config
during the right time period (i.e. today to today+24 days + N*49.7 days):

  global
    stats socket /tmp/sock1

  frontend web
    bind :8080
    mode http
    http-request track-sc0 src
    stick-table type ip size 1m expire 1h store http_req_rate(2s)

Issuing 'socat - /tmp/sock1  <<< "show table web"' should show a stable
rate after 2 seconds.

The fix must be backported to 2.3 and any other version the fix above
goes into.

Thanks to Thomas SIMON and Sander Klein for quickly reporting this issue
with a working reproducer.

4 years agoBUG/MINOR: stats: Apply proper styles in HTML status page.
Florian Apolloner [Tue, 30 Mar 2021 11:28:35 +0000 (13:28 +0200)] 
BUG/MINOR: stats: Apply proper styles in HTML status page.

When a backend is in status DOWN and going UP it is currently displayed
as yellow ("active UP, going down") instead of orange ("active DOWN, going
UP"). This patches restyles the table rows to actually match the
legend.

This may be backported to any version, the issue appeared in 1.7-dev2
with commit 0c378efe8 ("MEDIUM: stats: compute the color code only in
the HTML form").

4 years agoBUG/MINOR: payload: Wait for more data if buffer is empty in payload/payload_lv
Christopher Faulet [Mon, 29 Mar 2021 09:09:45 +0000 (11:09 +0200)] 
BUG/MINOR: payload: Wait for more data if buffer is empty in payload/payload_lv

In payload() and payload_lv() sample fetches, if the buffer is empty, we
must wait for more data by setting SMP_F_MAY_CHANGE flag on the sample.
Otherwise, when it happens in an ACL, nothing is returned (because the
buffer is empty) and the ACL is considered as finished (success or failure
depending on the test).

As a workaround, the buffer length may be tested first. For instance :

    tcp-request inspect-delay 1s
    tcp-request content reject unless { req.len gt 0 } { req.payload(0,0),fix_is_valid }

instead of :

    tcp-request inspect-delay 1s
    tcp-request content reject if ! { req.payload(0,0),fix_is_valid }

This patch must be backported as far as 2.2.

4 years ago[RELEASE] Released version 2.4-dev14 v2.4-dev14
Willy Tarreau [Sat, 27 Mar 2021 08:42:09 +0000 (09:42 +0100)] 
[RELEASE] Released version 2.4-dev14

Released version 2.4-dev14 with the following main changes :
    - MEDIUM: quic: Fix build.
    - MEDIUM: quic: Fix build.
    - CI: codespell: whitelist "Dragan Dosen"
    - CLEANUP: assorted typo fixes in the code and comments
    - CI: github actions: update LibreSSL to 3.2.5
    - REGTESTS: revert workaround for a crash with recent libressl on http-reuse sni
    - CLEANUP: mark defproxy as const on parse tune.fail-alloc
    - REGTESTS: remove unneeded experimental-mode in cli add server test
    - REGTESTS: wait for proper return of enable server in cli add server test
    - MINOR: compression: use pool_alloc(), not pool_alloc_dirty()
    - MINOR: spoe: use pool_alloc(), not pool_alloc_dirty()
    - MINOR: fcgi-app: use pool_alloc(), not pool_alloc_dirty()
    - MINOR: cache: use pool_alloc(), not pool_alloc_dirty()
    - MINOR: ssl: use pool_alloc(), not pool_alloc_dirty()
    - MINOR: opentracing: use pool_alloc(), not pool_alloc_dirty()
    - MINOR: dynbuf: make b_alloc() always check if the buffer is allocated
    - CLEANUP: compression: do not test for buffer before calling b_alloc()
    - CLEANUP: l7-retries: do not test the buffer before calling b_alloc()
    - MINOR: channel: simplify the channel's buffer allocation
    - MEDIUM: dynbuf: remove last usages of b_alloc_margin()
    - CLEANUP: dynbuf: remove b_alloc_margin()
    - CLEANUP: dynbuf: remove the unused b_alloc_fast() function
    - CLEANUP: pools: remove the unused pool_get_first() function
    - MINOR: pools: make the pool allocator support a few flags
    - MINOR: pools: add pool_zalloc() to return a zeroed area
    - CLEANUP: connection: use pool_zalloc() in conn_alloc_hash_node()
    - CLEANUP: filters: use pool_zalloc() in flt_stream_add_filter()
    - CLEANUP: spoe: use pool_zalloc() instead of pool_alloc+memset
    - CLEANUP: frontend: use pool_zalloc() in frontend_accept()
    - CLEANUP: mailers: use pool_zalloc() in enqueue_one_email_alert()
    - CLEANUP: resolvers: use pool_zalloc() in resolv_link_resolution()
    - CLEANUP: ssl: use pool_zalloc() in ssl_init_keylog()
    - CLEANUP: tcpcheck: use pool_zalloc() instead of pool_alloc+memset
    - CLEANUP: quic: use pool_zalloc() instead of pool_alloc+memset
    - MINOR: time: also provide a global, monotonic global_now_ms timer
    - BUG/MEDIUM: freq_ctr/threads: use the global_now_ms variable
    - MINOR: tools: introduce new option PA_O_DEFAULT_DGRAM on str2sa_range.
    - BUILD: tools: fix build error with new PA_O_DEFAULT_DGRAM
    - BUG/MINOR: ssl: Prevent disk access when using "add ssl crt-list"
    - CLEANUP: ssl: remove unused definitions
    - BUILD: ssl: guard ecdh functions with SSL_CTX_set_tmp_ecdh macro
    - MINOR: lua: Slightly improve function dumping the lua traceback
    - BUG/MEDIUM: debug/lua: Use internal hlua function to dump the lua traceback
    - BUG/MEDIUM: lua: Always init the lua stack before referencing the context
    - MINOR: fd: make fd_clr_running() return the remaining running mask
    - MINOR: fd: remove the unneeded running bit from fd_insert()
    - BUG/MEDIUM: fd: do not wait on FD removal in fd_delete()
    - CLEANUP: fd: remove unused fd_set_running_excl()
    - CLEANUP: fd: slightly simplify up _fd_delete_orphan()
    - BUG/MEDIUM: fd: Take the fd_mig_lock when closing if no DWCAS is available.
    - BUG/MEDIUM: release lock on idle conn killing on reached pool high count
    - BUG/MEDIUM: thread: Fix a deadlock if an isolated thread is marked as harmless
    - MINOR: tools: make url2ipv4 return the exact number of bytes parsed
    - BUG/MINOR: http_fetch: make hdr_ip() reject trailing characters
    - BUG/MEDIUM: mux-h1: make h1_shutw_conn() idempotent
    - BUG/MINOR: ssl: Fix update of default certificate
    - BUG/MINOR: ssl: Prevent removal of crt-list line if the instance is a default one
    - BUILD: ssl: introduce fine guard for ssl random extraction functions
    - REORG: global: move initcall register code in a dedicated file
    - REORG: global: move free acl/action in their related source files
    - REORG: split proxy allocation functions
    - MINOR: proxy: implement a free_proxy function
    - MINOR: proxy: define cap PR_CAP_LUA
    - MINOR: lua: properly allocate the lua Socket proxy
    - MINOR: lua: properly allocate the lua Socket servers
    - MINOR: vars: make get_vars() allow the session to be null
    - MINOR: vars: make the var() sample fetch keyword depend on nothing
    - CLEANUP: sample: remove duplicate "stopping" sample fetch keyword
    - MINOR: sample: make smp_resolve_args() return an allocate error message
    - MINOR: sample: add a new SMP_SRC_CONST sample capability
    - MINOR: sample: mark the truly constant sample fetch keywords as such
    - MINOR: sample: add a new CFG_PARSER context for samples
    - MINOR: action: add a new ACT_F_CFG_PARSER origin designation
    - MEDIUM: vars: add support for a "set-var" global directive
    - REGTESTS: add a basic reg-test for some "set-var" commands
    - MINOR: sample: add a new CLI_PARSER context for samples
    - MINOR: action: add a new ACT_F_CLI_PARSER origin designation
    - MINOR: vars/cli: add a "get var" CLI command to retrieve global variables
    - MEDIUM: cli: add a new experimental "set var" command
    - MINOR: compat: add short aliases for a few very commonly used types
    - BUILD: ssl: use EVP_CIPH_GCM_MODE macro instead of HA_OPENSSL_VERSION
    - MEDIUM: backend: use a trylock to grab a connection on high FD counts as well

4 years agoMEDIUM: backend: use a trylock to grab a connection on high FD counts as well
Willy Tarreau [Fri, 26 Mar 2021 19:52:10 +0000 (20:52 +0100)] 
MEDIUM: backend: use a trylock to grab a connection on high FD counts as well

Commit b1adf03df ("MEDIUM: backend: use a trylock when trying to grab an
idle connection") solved a contention issue on the backend under normal
condition, but there is another one further, which only happens when the
number of FDs in use is considered too high, and which obviously causes
random crashes with just 16 threads once the number of FDs is about to be
exhausted.

Like the aforementioned patch, this one should be backported to 2.3.

4 years agoBUILD: ssl: use EVP_CIPH_GCM_MODE macro instead of HA_OPENSSL_VERSION
Ilya Shipitsin [Fri, 26 Mar 2021 18:35:31 +0000 (23:35 +0500)] 
BUILD: ssl: use EVP_CIPH_GCM_MODE macro instead of HA_OPENSSL_VERSION

EVP_CIPH_GCM_MODE was introduced in https://github.com/openssl/openssl/commit/bdaa54155cceb34846a202d0027054fd51493695
together with EVP support for AES-GCM.

4 years agoMINOR: compat: add short aliases for a few very commonly used types
Willy Tarreau [Fri, 26 Mar 2021 16:28:47 +0000 (17:28 +0100)] 
MINOR: compat: add short aliases for a few very commonly used types

Very often we use "int" where negative numbers are not needed (and can
further cause trouble) just because it's painful to type "unsigned int"
or "unsigned", or ugly to use in function arguments. Similarly sometimes
chars would absolutely need to be signed but nobody types "signed char".
Let's add a few aliases for such types and make them part of the standard
internal API so that over time we can get used to them and get rid of
horrible definitions. A comment also reminds some commonly available
types and their properties regarding other types.

4 years agoMEDIUM: cli: add a new experimental "set var" command
Willy Tarreau [Fri, 26 Mar 2021 14:19:50 +0000 (15:19 +0100)] 
MEDIUM: cli: add a new experimental "set var" command

set var <name> <expression>
  Allows to set or overwrite the process-wide variable 'name' with the result
  of expression <expression>. Only process-wide variables may be used, so the
  name must begin with 'proc.' otherwise no variable will be set. The
  <expression> may only involve "internal" sample fetch keywords and converters
  even though the most likely useful ones will be str('something') or int().
  Note that the command line parser doesn't know about quotes, so any space in
  the expression must be preceeded by a backslash. This command requires levels
  "operator" or "admin". This command is only supported on a CLI connection
  running in experimental mode (see "experimental-mode on").

Just like for "set-var" in the global section, the command uses a temporary
dummy proxy to create a temporary "set-var(name)" rule to assign the value.

The reg test was updated to verify that an updated global variable is properly
reflected in subsequent HTTP responses.

4 years agoMINOR: vars/cli: add a "get var" CLI command to retrieve global variables
Willy Tarreau [Fri, 26 Mar 2021 13:51:31 +0000 (14:51 +0100)] 
MINOR: vars/cli: add a "get var" CLI command to retrieve global variables

Process-wide variables can now be displayed from the CLI using "get var"
followed by the variable name. They must all start with "proc." otherwise
they will not be found. The output is very similar to the one of the
debug converter, with a type and value being reported for the embedded
sample.

This command is limited to clients with the level "operator" or higher,
since it can possibly expose traffic-related data.

4 years agoMINOR: action: add a new ACT_F_CLI_PARSER origin designation
Willy Tarreau [Fri, 26 Mar 2021 14:36:44 +0000 (15:36 +0100)] 
MINOR: action: add a new ACT_F_CLI_PARSER origin designation

In order to process samples from the command line interface we'll need
rules as well, and these rules will have to be marked as coming from
the CLI parser. This new origin is used for this.

4 years agoMINOR: sample: add a new CLI_PARSER context for samples
Willy Tarreau [Fri, 26 Mar 2021 14:29:35 +0000 (15:29 +0100)] 
MINOR: sample: add a new CLI_PARSER context for samples

In order to prepare for supporting calling sample expressions from the
CLI, let's create a new CLI_PARSER parsing context. This one supports
constants and internal samples only.

4 years agoREGTESTS: add a basic reg-test for some "set-var" commands
Willy Tarreau [Fri, 26 Mar 2021 13:03:57 +0000 (14:03 +0100)] 
REGTESTS: add a basic reg-test for some "set-var" commands

This reg-test tests "set-var" in the global section, with some overlapping
variables and using a few samples and converters, then at the TCP and HTTP
levels using proc/sess/req variables.

4 years agoMEDIUM: vars: add support for a "set-var" global directive
Willy Tarreau [Fri, 26 Mar 2021 10:38:08 +0000 (11:38 +0100)] 
MEDIUM: vars: add support for a "set-var" global directive

While we do support process-wide variables ("proc.<name>"), there was
no way to preset them from the configuration. This was particularly
limiting their usefulness since configs involving them always had to
first check if the variable was set prior to performing an operation.

This patch adds a new "set-var" directive in the global section that
supports setting the proc.<name> variables from an expression, like
other set-var actions do. The syntax however follows what is already
being done for setenv, which consists in having one argument for the
variable name and another one for the expression.

Only "constant" expressions are allowed here, such as "int", "str"
etc, combined with arithmetic or string converters, and variable
lookups. A few extra sample fetch keywords like "date", "rand" and
"uuid" are also part of the constant expressions and may make sense
to allow to create a random key or differentiate processes.

The way it was done consists in parsing a dummy rule an executing the
expression in the CFG_PARSE context, then releasing the expression.
This is safe because the sample that variables store does not hold a
back pointer to expression that created them.

4 years agoMINOR: action: add a new ACT_F_CFG_PARSER origin designation
Willy Tarreau [Fri, 26 Mar 2021 10:11:34 +0000 (11:11 +0100)] 
MINOR: action: add a new ACT_F_CFG_PARSER origin designation

In order to process samples from the config file we'll need rules as
well, and these rules will have to be marked as coming from the
config parser. This new origin is used for this.

4 years agoMINOR: sample: add a new CFG_PARSER context for samples
Willy Tarreau [Fri, 26 Mar 2021 10:09:38 +0000 (11:09 +0100)] 
MINOR: sample: add a new CFG_PARSER context for samples

We'd sometimes like to be able to process samples while parsing
the configuration based on purely internal thing but that's not
possible right now. Let's add a new CFG_PARSER context for samples
which only permits constant samples (i.e. those which do not change
in the process' life and which are stable during config parsing).

4 years agoMINOR: sample: mark the truly constant sample fetch keywords as such
Willy Tarreau [Fri, 26 Mar 2021 11:03:11 +0000 (12:03 +0100)] 
MINOR: sample: mark the truly constant sample fetch keywords as such

A number of keywords are really constant and safe to use at config
time. This is the case for str(), int() etc but also env(), hostname(),
nbproc() etc. By extension a few other ones which can be useful to
preset values in a configuration were enabled as well, like data(),
rand() or uuid(). At the moment this doesn't change anything as they
are still only usable from runtime rules.

The "var()" keyword was also marked as const as it can definitely
return stable stuff at boot time.

4 years agoMINOR: sample: add a new SMP_SRC_CONST sample capability
Willy Tarreau [Fri, 26 Mar 2021 10:56:11 +0000 (11:56 +0100)] 
MINOR: sample: add a new SMP_SRC_CONST sample capability

This level indicates that everything it constant in the expression during
the whole process' life and that it may safely be used at config parsing
time.

4 years agoMINOR: sample: make smp_resolve_args() return an allocate error message
Willy Tarreau [Fri, 26 Mar 2021 15:11:55 +0000 (16:11 +0100)] 
MINOR: sample: make smp_resolve_args() return an allocate error message

For now smp_resolve_args() complains on stderr via ha_alert(), but if we
want to make it a bit more dynamic, we need it to return errors in an
allocated message. Let's pass it an error pointer and have it fill it.
On return we indent the output if it contains more than one line.

4 years agoCLEANUP: sample: remove duplicate "stopping" sample fetch keyword
Willy Tarreau [Fri, 26 Mar 2021 11:01:39 +0000 (12:01 +0100)] 
CLEANUP: sample: remove duplicate "stopping" sample fetch keyword

The "stopping" sample fetch keyword was accidently duplicated in 1.9
by commit 70fe94419 ("MINOR: sample: add cpu_calls, cpu_ns_avg,
cpu_ns_tot, lat_ns_avg, lat_ns_tot"). This has no effect so no
backport is needed.

4 years agoMINOR: vars: make the var() sample fetch keyword depend on nothing
Willy Tarreau [Fri, 26 Mar 2021 11:08:06 +0000 (12:08 +0100)] 
MINOR: vars: make the var() sample fetch keyword depend on nothing

This sample fetch doesn't require any L4 client session in practice, as
get_var() now checks for the session. This is important to remove this
dependency in order to support accessing variables in scope "proc" from
anywhere.

4 years agoMINOR: vars: make get_vars() allow the session to be null
Willy Tarreau [Fri, 26 Mar 2021 10:27:59 +0000 (11:27 +0100)] 
MINOR: vars: make get_vars() allow the session to be null

In order to support manipulating variables from outside a session,
let's make get_vars() not assume that the session is always set.

4 years agoMINOR: lua: properly allocate the lua Socket servers
Amaury Denoyelle [Wed, 24 Mar 2021 16:57:47 +0000 (17:57 +0100)] 
MINOR: lua: properly allocate the lua Socket servers

Instantiate both lua Socket servers tcp/ssl using standard function
new_server. There is currently no need to tune their settings except to
activate the ssl mode with noverify for the second one. Both servers are
freed with the free_server function.

4 years agoMINOR: lua: properly allocate the lua Socket proxy
Amaury Denoyelle [Wed, 24 Mar 2021 09:22:03 +0000 (10:22 +0100)] 
MINOR: lua: properly allocate the lua Socket proxy

Replace static initialization of the lua Socket proxy with the standard
function alloc_new_proxy. The settings proxy are properly applied thanks
to PR_CAP_LUA. The proxy is freed with the free_proxy function.

4 years agoMINOR: proxy: define cap PR_CAP_LUA
Amaury Denoyelle [Wed, 24 Mar 2021 09:49:34 +0000 (10:49 +0100)] 
MINOR: proxy: define cap PR_CAP_LUA

Define a new cap PR_CAP_LUA. It can be used to allocate the internal
proxy for lua Socket class. This cap overrides default settings for
preferable values in the lua context.

4 years agoMINOR: proxy: implement a free_proxy function
Amaury Denoyelle [Wed, 24 Mar 2021 15:13:20 +0000 (16:13 +0100)] 
MINOR: proxy: implement a free_proxy function

Move all liberation code related to a proxy in a dedicated function
free_proxy in proxy.c. For now, this function is only called in
haproxy.c. In the future, it will be used to free the lua proxy.

This helps to clean up haproxy.c.

4 years agoREORG: split proxy allocation functions
Amaury Denoyelle [Tue, 23 Mar 2021 16:27:05 +0000 (17:27 +0100)] 
REORG: split proxy allocation functions

Create a new function parse_new_proxy specifically designed to allocate
a new proxy from the configuration file and copy settings from the
default proxy.

The function alloc_new_proxy is reduced to a minimal allocation. It is
used for default proxy allocation and could also be used for internal
proxies such as the lua Socket proxy.

4 years agoREORG: global: move free acl/action in their related source files
Amaury Denoyelle [Thu, 25 Mar 2021 16:15:52 +0000 (17:15 +0100)] 
REORG: global: move free acl/action in their related source files

Move deinit_acl_cond and deinit_act_rules from haproxy.c respectively in
acl.c and action.c. The name of the functions has been slightly altered,
replacing the prefix deinit_* by free_* to reflect their purpose more
clearly.

This change has been made in preparation to the implementation of a free
proxy function. As a side-effect, it helps to clean up haproxy.c.

4 years agoREORG: global: move initcall register code in a dedicated file
Amaury Denoyelle [Thu, 25 Mar 2021 14:09:38 +0000 (15:09 +0100)] 
REORG: global: move initcall register code in a dedicated file

Create a new module init which contains code related to REGISTER_*
macros for initcalls. init.h is included in api.h to make init code
available to all modules.

It's a step to clean up a bit haproxy.c/global.h.

4 years agoBUILD: ssl: introduce fine guard for ssl random extraction functions
Ilya Shipitsin [Wed, 24 Mar 2021 19:41:41 +0000 (00:41 +0500)] 
BUILD: ssl: introduce fine guard for ssl random extraction functions

SSL_get_{client,server}_random are supported in OpenSSL-1.1.0, BoringSSL,
LibreSSL-2.7.0

let us introduce HAVE_SSL_EXTRACT_RANDOM for that purpose

4 years agoBUG/MINOR: ssl: Prevent removal of crt-list line if the instance is a default one
Remi Tricot-Le Breton [Fri, 26 Mar 2021 09:47:50 +0000 (10:47 +0100)] 
BUG/MINOR: ssl: Prevent removal of crt-list line if the instance is a default one

If the first active line of a crt-list file is also the first mentioned
certificate of a frontend that does not have the strict-sni option
enabled, then its certificate will be used as the default one. We then
do not want this instance to be removable since it would make a frontend
lose its default certificate.
Considering that a crt-list file can be used by multiple frontends, and
that its first mentioned certificate can be used as default certificate
for only a subset of those frontends, we do not want the line to be
removable for some frontends and not the others. So if any of the ckch
instances corresponding to a crt-list line is a default instance, the
removal of the crt-list line will be forbidden.

It can be backported as far as 2.2.

4 years agoBUG/MINOR: ssl: Fix update of default certificate
Remi Tricot-Le Breton [Wed, 17 Mar 2021 13:56:54 +0000 (14:56 +0100)] 
BUG/MINOR: ssl: Fix update of default certificate

The default SSL_CTX used by a specific frontend is the one of the first
ckch instance created for this frontend. If this instance has SNIs, then
the SSL context is linked to the instance through the list of SNIs
contained in it. If the instance does not have any SNIs though, then the
SSL_CTX is only referenced by the bind_conf structure and the instance
itself has no link to it.
When trying to update a certificate used by the default instance through
a cli command, a new version of the default instance was rebuilt but the
default SSL context referenced in the bind_conf structure would not be
changed, resulting in a buggy behavior in which depending on the SNI
used by the client, he could either use the new version of the updated
certificate or the original one.

This patch adds a reference to the default SSL context in the default
ckch instances so that it can be hot swapped during a certificate
update.

This should fix GitHub issue #1143.

It can be backported as far as 2.2.

4 years agoBUG/MEDIUM: mux-h1: make h1_shutw_conn() idempotent
Willy Tarreau [Fri, 26 Mar 2021 08:09:42 +0000 (09:09 +0100)] 
BUG/MEDIUM: mux-h1: make h1_shutw_conn() idempotent

In issue #1197, Stéphane Graber reported a rare case of crash that
results from an attempt to close an already closed H1 connection. It
indeed looks like under some circumstances it should be possible to
call the h1_shutw_conn() function more than once, though these
conditions are not very clear.

Without going through a deep analysis of all possibilities, one
potential case seems to be a detach() called with pending output data,
causing H1C_F_ST_SHUTDOWN to be set on the connection, then h1_process()
being immediately called on I/O, causing h1_send() to flush these data
and call h1_shutw_conn(), and finally the upper stream calling cs_shutw()
hence h1_shutw(), which itself will call h1_shutw_conn() again while the
transport and control layers have already been released. But the whole
sequence is not certain as it's not very clear in which case it's
possible to leave h1_send() without the connection anymore (at least
the obuf is empty).

However what is certain is that a shutdown function must be idempotent,
so let's fix h1_shutw_conn() regarding this point. Stéphane reported the
issue as far back as 2.0, so this patch should be backported this far.

4 years agoBUG/MINOR: http_fetch: make hdr_ip() reject trailing characters
Willy Tarreau [Thu, 25 Mar 2021 13:12:29 +0000 (14:12 +0100)] 
BUG/MINOR: http_fetch: make hdr_ip() reject trailing characters

The hdr_ip() sample fetch function will try to extract IP addresses
from a header field. These IP addresses are parsed using url2ipv4()
and if it fails it will fall back to inet_pton(AF_INET6), otherwise
will fail.

There is a small problem there which is that if a field starts with
an IP address and is immediately followed by some garbage, the IP
address part is still returned. This is a problem with fields such
as x-forwarded-for because it prevents detection of accidental
corruption or bug along the chain. For example, the following string:

   x-forwarded-for: 1.2.3.4; 5.6.7.8

or this one:

   x-forwarded-for: 1.2.3.4O    ( the last one being the letter 'O')

would still return "1.2.3.4" despite the trailing characters. This is
bad because it will silently cover broken code running on intermediary
proxies and may even in some cases allow haproxy to pass improperly
formatted headers after they were apparently validated, for example,
if someone extracts the address from this field to place it into
another one.

This issue would only affect the IPv4 parser, because the IPv6 parser
already uses inet_pton() which fails at the first invalid character and
rejects trailing port numbers.

In strict compliance with RFC7239, let's make sure that if there are any
characters left in the string, the parsing fails and makes hdr_ip()
return nothing. However, a special case has to be handled to support
IPv4 addresses followed by a colon and a valid port number, because till
now the parser used to implicitly accept them and it appears that this
practice, though rare, does exist at least in Azure:
   https://docs.microsoft.com/en-us/azure/application-gateway/how-application-gateway-works

This issue has always been there so the fix may be backported to all
versions. It will need the following commit in order to work as expected:

    MINOR: tools: make url2ipv4 return the exact number of bytes parsed

Many thanks to https://twitter.com/melardev and the BitMEX Security Team
for their detailed report.

4 years agoMINOR: tools: make url2ipv4 return the exact number of bytes parsed
Willy Tarreau [Thu, 25 Mar 2021 10:34:40 +0000 (11:34 +0100)] 
MINOR: tools: make url2ipv4 return the exact number of bytes parsed

The function's return value is currently used as a boolean but we'll
need it to return the number of bytes parsed. Right now it returns
it minus one, unless the last char doesn't match what is permitted.
Let's update this to make it more usable.

4 years agoBUG/MEDIUM: thread: Fix a deadlock if an isolated thread is marked as harmless
Christopher Faulet [Thu, 25 Mar 2021 13:11:36 +0000 (14:11 +0100)] 
BUG/MEDIUM: thread: Fix a deadlock if an isolated thread is marked as harmless

If an isolated thread is marked as harmless, it will loop forever in
thread_harmless_till_end() waiting no threads are isolated anymore. It never
happens because the current thread is isolated. To fix the bug, we exclude
the current thread for the test. We now wait for all other threads to leave
the rendez-vous point.

This bug only seems to occurr if HAProxy is compiled with DEBUG_UAF, when
pool_gc() is called. pool_gc() isolates the current thread, while
pool_free_area() set the thread as harmless when munmap is called.

This patch must be backported as far as 2.0.

4 years agoBUG/MEDIUM: release lock on idle conn killing on reached pool high count
Amaury Denoyelle [Tue, 23 Mar 2021 09:44:43 +0000 (10:44 +0100)] 
BUG/MEDIUM: release lock on idle conn killing on reached pool high count

Release the lock before calling mux destroy in connect_server when
trying to kill an idle connection because the pool high count has been
reached.

The lock must be released because the mux destroy will call
srv_release_conn which also takes the lock to remove the connection from
the tree. As the connection was already deleted from the tree at this
stage, it is safe to release the lock, and the removal in
srv_release_conn will be a noop.

It does not need to be backported because it is only present in the
current release. It has been introduced by
    5c7086f6b06d546c5800486ed9e4bb8d8d471e09
    MEDIUM: connection: protect idle conn lists with locks

4 years agoBUG/MEDIUM: fd: Take the fd_mig_lock when closing if no DWCAS is available.
Olivier Houchard [Thu, 25 Mar 2021 00:38:54 +0000 (01:38 +0100)] 
BUG/MEDIUM: fd: Take the fd_mig_lock when closing if no DWCAS is available.

In fd_delete(), if we're running with no double-width cas, take the
fd_mig_lock before setting thread_mask to 0 to make sure that
another thread calling fd_set_running() won't miss the new value of
thread_mask and set its bit in running_mask after we checked it.

This should be backported to 2.2 as part of the series fixing fd_delete().

4 years agoCLEANUP: fd: slightly simplify up _fd_delete_orphan()
Willy Tarreau [Wed, 24 Mar 2021 14:34:25 +0000 (15:34 +0100)] 
CLEANUP: fd: slightly simplify up _fd_delete_orphan()

Let's release the port range earlier so that all zeroes are grouped
together and that the compiler can slightly simplify the code.

4 years agoCLEANUP: fd: remove unused fd_set_running_excl()
Willy Tarreau [Wed, 24 Mar 2021 10:34:09 +0000 (11:34 +0100)] 
CLEANUP: fd: remove unused fd_set_running_excl()

This one is no longer used and was the origin of the previously mentioned
deadlock.

4 years agoBUG/MEDIUM: fd: do not wait on FD removal in fd_delete()
Willy Tarreau [Wed, 24 Mar 2021 09:51:32 +0000 (10:51 +0100)] 
BUG/MEDIUM: fd: do not wait on FD removal in fd_delete()

Christopher discovered an issue mostly affecting 2.2 and to a less extent
2.3 and above, which is that it's possible to deadlock a soft-stop when
several threads are using a same listener:

          thread1                             thread2

      unbind_listener()                   fd_set_running()
          lock(listener)                  listener_accept()
          fd_delete()                          lock(listener)
             while (running_mask);  -----> deadlock
          unlock(listener)

This simple case disappeared from 2.3 due to the removal of some locked
operations at the end of listener_accept() on the regular path, but the
architectural problem is still here and caused by a lock inversion built
around the loop on running_mask in fd_clr_running_excl(), because there
are situations where the caller of fd_delete() may hold a lock that is
preventing other threads from dropping their bit in running_mask.

The real need here is to make sure the last user deletes the FD. We have
all we need to know the last one, it's the one calling fd_clr_running()
last, or entering fd_delete() last, both of which can be summed up as
the last one calling fd_clr_running() if fd_delete() calls fd_clr_running()
at the end. And we can prevent new threads from appearing in running_mask
by removing their bits in thread_mask.

So what this patch does is that it sets the running_mask for the thread
in fd_delete(), clears the thread_mask, thus marking the FD as orphaned,
then clears the running mask again, and completes the deletion if it was
the last one. If it was not, another thread will pass through fd_clr_running
and will complete the deletion of the FD.

The bug is easily reproducible in 2.2 under high connection rates during
soft close. When the old process stops its listener, occasionally two
threads will deadlock and the old process will then be killed by the
watchdog. It's strongly believed that similar situations do exist in 2.3
and 2.4 (e.g. if the removal attempt happens during resume_listener()
called from listener_accept()) but if so, they should be much harder to
trigger.

This should be backported to 2.2 as the issue appeared with the FD
migration. It requires previous patches "fd: make fd_clr_running() return
the remaining running mask" and "MINOR: fd: remove the unneeded running
bit from fd_insert()".

Notes for backport: in 2.2, the fd_dodelete() function requires an extra
argument "do_close" indicating whether we want to remove and close the FD
(fd_delete) or just delete it (fd_remove). While this information is not
conveyed along the chain, we know that late calls always imply do_close=1
become do_close=0 exclusively results from fd_remove() which is only used
by the config parser and the master, both of which are single-threaded,
hence are always the last ones in the running_mask. Thus it is safe to
assume that a postponed FD deletion always implies do_close=1.

Thanks to Olivier for his help in designing this optimal solution.

4 years agoMINOR: fd: remove the unneeded running bit from fd_insert()
Willy Tarreau [Wed, 24 Mar 2021 09:42:16 +0000 (10:42 +0100)] 
MINOR: fd: remove the unneeded running bit from fd_insert()

There's no point taking the running bit in fd_insert() since by
definition there will never be more than one thread inserting the FD,
and that fd_insert() may only be done after the fd was allocated by
the system, indicating the end of use by any other thread.

This will need to be backported to 2.2 to fix an issue.

4 years agoMINOR: fd: make fd_clr_running() return the remaining running mask
Willy Tarreau [Wed, 24 Mar 2021 09:27:56 +0000 (10:27 +0100)] 
MINOR: fd: make fd_clr_running() return the remaining running mask

We'll need to know that a thread is the last one to use an fd, so let's
make fd_clr_running() return the remaining bits after removal. Note that
in practice we're only interested in knowing if it's zero but the compiler
doesn't make use of the clags after the AND and emits a CMPXCHG anyway :-/

This will need to be backported to 2.2 to fix an issue.

4 years agoBUG/MEDIUM: lua: Always init the lua stack before referencing the context
Christopher Faulet [Wed, 24 Mar 2021 14:03:01 +0000 (15:03 +0100)] 
BUG/MEDIUM: lua: Always init the lua stack before referencing the context

When a lua context is allocated, its stack must be initialized to NULL
before attaching it to its owner (task, stream or applet).  Otherwise, if
the watchdog is fired before the stack is really created, that may lead to a
segfault because we try to dump the traceback of an uninitialized lua stack.

It is easy to trigger this bug if a lua script do a blocking call while
another thread try to initialize a new lua context. Because of the global
lua lock, the init is blocked before the stack creation. Of course, it only
happens if the script is executed in the shared global context.

This patch must be backported as far as 2.0.

4 years agoBUG/MEDIUM: debug/lua: Use internal hlua function to dump the lua traceback
Christopher Faulet [Wed, 24 Mar 2021 13:52:24 +0000 (14:52 +0100)] 
BUG/MEDIUM: debug/lua: Use internal hlua function to dump the lua traceback

The commit reverts following commits:
  * 83926a04 BUG/MEDIUM: debug/lua: Don't dump the lua stack if not dumpable
  * a61789a1 MEDIUM: lua: Use a per-thread counter to track some non-reentrant parts of lua

Instead of relying on a Lua function to print the lua traceback into the
debugger, we are now using our own internal function (hlua_traceback()).
This one does not allocate memory and use a chunk instead. This avoids any
issue with a possible deadlock in the memory allocator because the thread
processing was interrupted during a memory allocation.

This patch relies on the commit "BUG/MEDIUM: debug/lua: Use internal hlua
function to dump the lua traceback". Both must be backported wherever the
patches above are backported, thus as far as 2.0

4 years agoMINOR: lua: Slightly improve function dumping the lua traceback
Christopher Faulet [Wed, 24 Mar 2021 13:48:45 +0000 (14:48 +0100)] 
MINOR: lua: Slightly improve function dumping the lua traceback

The separator string is now configurable, passing it as parameter when the
function is called. In addition, the message have been slightly changed to
be a bit more readable.

4 years agoBUILD: ssl: guard ecdh functions with SSL_CTX_set_tmp_ecdh macro
Ilya Shipitsin [Sun, 21 Mar 2021 07:50:47 +0000 (12:50 +0500)] 
BUILD: ssl: guard ecdh functions with SSL_CTX_set_tmp_ecdh macro

let us use feature macro SSL_CTX_set_tmp_ecdh instead of comparing openssl
version

4 years agoCLEANUP: ssl: remove unused definitions
Ilya Shipitsin [Sat, 20 Mar 2021 17:30:59 +0000 (22:30 +0500)] 
CLEANUP: ssl: remove unused definitions

not need since  e7eb1fec2f2349359c752c8fbb82357b14c7e4cf

4 years agoBUG/MINOR: ssl: Prevent disk access when using "add ssl crt-list"
Remi Tricot-Le Breton [Tue, 23 Mar 2021 15:41:53 +0000 (16:41 +0100)] 
BUG/MINOR: ssl: Prevent disk access when using "add ssl crt-list"

If an unknown CA file was first mentioned in an "add ssl crt-list" CLI
command, it would result in a call to X509_STORE_load_locations which
performs a disk access which is forbidden during runtime. The same would
happen if a "ca-verify-file" or "crl-file" was specified. This was due
to the fact that the crt-list file parsing and the crt-list related CLI
commands parsing use the same functions.
The patch simply adds a new parameter to all the ssl_bind parsing
functions so that they know if the call is made during init or by the
CLI, and the ssl_store_load_locations function can then reject any new
cafile_entry creation coming from a CLI call.

It can be backported as far as 2.2.

4 years agoBUILD: tools: fix build error with new PA_O_DEFAULT_DGRAM
Willy Tarreau [Tue, 23 Mar 2021 17:36:37 +0000 (18:36 +0100)] 
BUILD: tools: fix build error with new PA_O_DEFAULT_DGRAM

Previous commit 69ba35146 ("MINOR: tools: introduce new option
PA_O_DEFAULT_DGRAM on str2sa_range.") managed to introduce a
parenthesis imbalance that broke the build. No backport is needed.

4 years agoMINOR: tools: introduce new option PA_O_DEFAULT_DGRAM on str2sa_range.
Emeric Brun [Mon, 22 Mar 2021 16:17:34 +0000 (17:17 +0100)] 
MINOR: tools: introduce new option PA_O_DEFAULT_DGRAM on str2sa_range.

str2sa_range function options PA_O_DGRAM and PA_O_STREAM are used to
define the supported address types but also to set the default type
if it is not explicit. If the used address support both STREAM and DGRAM,
the default was always set to STREAM.

This patch introduce a new option PA_O_DEFAULT_DGRAM to force the
default to DGRAM type if it is not explicit in the address field
and both STREAM and DGRAM are supported. If only DGRAM or only STREAM
is supported, it continues to be considered as the default.

4 years agoBUG/MEDIUM: freq_ctr/threads: use the global_now_ms variable
Willy Tarreau [Tue, 23 Mar 2021 07:58:22 +0000 (08:58 +0100)] 
BUG/MEDIUM: freq_ctr/threads: use the global_now_ms variable

In commit a1ecbca0a ("BUG/MINOR: freq_ctr/threads: make use of the last
updated global time"), for period-based counters, the millisecond part
of the global_now variable was used as the date for the new period. But
it's wrong, it only works with sub-second periods as it wraps every
second, and for other periods the counters never rotate anymore.

Let's make use of the newly introduced global_now_ms variable instead,
which contains the global monotonic time expressed in milliseconds.

This patch needs to be backported wherever the patch above is backported.
It depends on previous commit "MINOR: time: also provide a global,
monotonic global_now_ms timer".

4 years agoMINOR: time: also provide a global, monotonic global_now_ms timer
Willy Tarreau [Tue, 23 Mar 2021 07:45:42 +0000 (08:45 +0100)] 
MINOR: time: also provide a global, monotonic global_now_ms timer

The period-based freq counters need the global date in milliseconds,
so better calculate it and expose it rather than letting all call
places incorrectly retrieve it.

Here what we do is that we maintain a new globally monotonic timer,
global_now_ms, which ought to be very close to the global_now one,
but maintains the monotonic approach of now_ms between all threads
in that global_now_ms is always ahead of any now_ms.

This patch is made simple to ease backporting (it will be needed for
a subsequent fix), but it also opens the way to some simplifications
on the time handling: instead of computing the local time and trying
to force it to the global one, we should soon be able to proceed in
the opposite way, that is computing the new global time an making the
local one just the latest snapshot of it. This will bring the benefit
of making sure that the global time is always ahead of the local one.

4 years agoCLEANUP: quic: use pool_zalloc() instead of pool_alloc+memset
Willy Tarreau [Mon, 22 Mar 2021 20:13:05 +0000 (21:13 +0100)] 
CLEANUP: quic: use pool_zalloc() instead of pool_alloc+memset

Two places used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: tcpcheck: use pool_zalloc() instead of pool_alloc+memset
Willy Tarreau [Mon, 22 Mar 2021 20:11:45 +0000 (21:11 +0100)] 
CLEANUP: tcpcheck: use pool_zalloc() instead of pool_alloc+memset

Two places used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: ssl: use pool_zalloc() in ssl_init_keylog()
Willy Tarreau [Mon, 22 Mar 2021 20:10:12 +0000 (21:10 +0100)] 
CLEANUP: ssl: use pool_zalloc() in ssl_init_keylog()

This one used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: resolvers: use pool_zalloc() in resolv_link_resolution()
Willy Tarreau [Mon, 22 Mar 2021 20:08:50 +0000 (21:08 +0100)] 
CLEANUP: resolvers: use pool_zalloc() in resolv_link_resolution()

This one used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: mailers: use pool_zalloc() in enqueue_one_email_alert()
Willy Tarreau [Mon, 22 Mar 2021 20:07:52 +0000 (21:07 +0100)] 
CLEANUP: mailers: use pool_zalloc() in enqueue_one_email_alert()

This one used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: frontend: use pool_zalloc() in frontend_accept()
Willy Tarreau [Mon, 22 Mar 2021 20:06:21 +0000 (21:06 +0100)] 
CLEANUP: frontend: use pool_zalloc() in frontend_accept()

The capture buffers were allocated then zeroed, let's have the allocator
do it.

4 years agoCLEANUP: spoe: use pool_zalloc() instead of pool_alloc+memset
Willy Tarreau [Mon, 22 Mar 2021 20:04:50 +0000 (21:04 +0100)] 
CLEANUP: spoe: use pool_zalloc() instead of pool_alloc+memset

Two places used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: filters: use pool_zalloc() in flt_stream_add_filter()
Willy Tarreau [Mon, 22 Mar 2021 20:02:50 +0000 (21:02 +0100)] 
CLEANUP: filters: use pool_zalloc() in flt_stream_add_filter()

This one used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: connection: use pool_zalloc() in conn_alloc_hash_node()
Willy Tarreau [Mon, 22 Mar 2021 20:01:05 +0000 (21:01 +0100)] 
CLEANUP: connection: use pool_zalloc() in conn_alloc_hash_node()

This one used to alloc then zero the area, let's have the allocator do it.

4 years agoMINOR: pools: add pool_zalloc() to return a zeroed area
Willy Tarreau [Mon, 22 Mar 2021 21:05:05 +0000 (22:05 +0100)] 
MINOR: pools: add pool_zalloc() to return a zeroed area

It's like pool_alloc() but the output is zeroed before being returned
and is never poisonned.