haproxy-2.1.git
4 years agoMINOR: task: remove __tasklet_remove_from_tasklet_list()
Willy Tarreau [Mon, 30 Nov 2020 13:52:11 +0000 (14:52 +0100)]
MINOR: task: remove __tasklet_remove_from_tasklet_list()

This function is only used at a single place directly within the
scheduler in run_tasks_from_lists() and it really ought not be called
by anything else, regardless of what its comment says. Let's delete
it, move the two lines directly into the call place, and take this
opportunity to factor the atomic decrement on tasks_run_queue. A comment
was added on the remaining one tasklet_remove_from_tasklet_list() to
mention the risks in using it.

(cherry picked from commit 2da4c316c2cb7b01f54ed1959e91d1799d13959c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 9a0374aaaafefa07b80e761f97f0fcd7688dabf4)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit e4480476c172b13e0bb072ef1afd31b359cc3917)
[wt: needed for next fix; context hanges in 2.1, no
     run_tasks_from_lists(), single LIST_DEL location]
Signed-off-by: Willy Tarreau <w@1wt.eu>

4 years agoBUG/MEDIUM: resolvers: Don't release resolution from a requester callbacks
Christopher Faulet [Thu, 11 Mar 2021 17:09:53 +0000 (18:09 +0100)]
BUG/MEDIUM: resolvers: Don't release resolution from a requester callbacks

Another way to say it: "Safely unlink requester from a requester callbacks".

Requester callbacks must never try to unlink a requester from a resolution, for
the current requester or another one. First, these callback functions are called
in a loop on a request list, not necessarily safe. Thus unlink resolution at
this place, may be unsafe. And it is useless to try to make these loops safe
because, all this stuff is placed in a loop on a resolution list. Unlink a
requester may lead to release a resolution if it is the last requester.

However, the unkink is necessary because we cannot reset the server state
(hostname and IP) with some pending DNS resolution on it. So, to workaround
this issue, we introduce the "safe" unlink. It is only performed from a
requester callback. In this case, the unlink function never releases the
resolution, it only reset it if necessary. And when a resolution is found
with an empty requester list, it is released.

This patch depends on the following commits :

 * MINOR: resolvers: Purge answer items when a SRV resolution triggers an error
 * MINOR: resolvers: Use a function to remove answers attached to a resolution
 * MINOR: resolvers: Directly call srvrq_update_srv_state() when possible
 * MINOR: resolvers: Add function to change the srv status based on SRV resolution

All the series must be backported as far as 2.2. It fixes a regression
introduced by the commit b4badf720 ("BUG/MINOR: resolvers: new callback to
properly handle SRV record errors").

don't release resolution from requester cb

(cherry picked from commit 0efc0993ec5210c8cf5a98eb9c0220014e030fd6)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 119ec5279df7241da25f2ffc1e43de4075717f35)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit f721fd209cb7f7126a548bf5301f4dd008952184)
[cf: Must be backported as far as 2.0 because of recent changes on resolvers]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: resolvers: Directly call srvrq_update_srv_state() when possible
Christopher Faulet [Thu, 11 Mar 2021 17:06:23 +0000 (18:06 +0100)]
MINOR: resolvers: Directly call srvrq_update_srv_state() when possible

When the server status must be updated from the result of a SRV resolution,
we can directly call srvrq_update_srv_state(). It is simpler and this avoid
a test on the server DNS resolution.

This patch is mandatory for the next commit. It also rely on "MINOR:
resolvers: Directly call srvrq_update_srv_state() when possible".

(cherry picked from commit 6b117aed492d9916a5c656b4fbfcac6159133c0f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 07cb0e13937c453682feae1bd7180cf41017660c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit a4fc79b63235bd7d6d288904e252789f577d4805)
[cf: Must be backported as far as 2.0 because of recent changes on resolvers]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: resolvers: Add function to change the srv status based on SRV resolution
Christopher Faulet [Thu, 11 Mar 2021 17:03:21 +0000 (18:03 +0100)]
MINOR: resolvers: Add function to change the srv status based on SRV resolution

srvrq_update_srv_status() update the server status based on result of SRV
resolution. For now, it is only used from snr_update_srv_status() when
appropriate.

(cherry picked from commit 5efdef24c1753d7a68d1f6c8dc8cb6b4b84a3361)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 6d70368ab146925a9b2eae3963c5d92c00abe05d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit d938bd528e3fa172f984cec4b8d7ae92dbb908a6)
[cf: Must be backported as far as 2.0 because of recent changes on resolvers]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: resolvers: Purge answer items when a SRV resolution triggers an error
Christopher Faulet [Wed, 10 Mar 2021 14:46:46 +0000 (15:46 +0100)]
MINOR: resolvers: Purge answer items when a SRV resolution triggers an error

When a SRV request trigger an error, if we decide to handle the error
because last_valid duration is expired, the answer list may be purged. All
items are considered as obsolete.

(cherry picked from commit 51d5e3bda7fb07a5f03cb878dd66f673c3ed1a59)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 720ed09b491f5005741115bd36855e05deb99481)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit f48a118811b27ee5dbdbe80603cc0fc7e9e4dbb3)
[cf: Must be backported as far as 2.0 because of recent changes on resolvers]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: resolvers: Use a function to remove answers attached to a resolution
Christopher Faulet [Wed, 10 Mar 2021 13:40:39 +0000 (14:40 +0100)]
MINOR: resolvers: Use a function to remove answers attached to a resolution

resolv_purge_resolution_answer_records() must be used to removed all answers
attached to a resolution. For now, it is only used when a resolution is
released.

(cherry picked from commit 1dec5c793474027ddffbfca3849e5ca4e9e51083)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 656b42714a05e578c0480f1751a065d8e96f8f3a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 67cc9a6dc2d6cdf0686c36178440a6c4236ac5f3)
[cf: Must be backported as far as 2.0 because of recent changes on resolvers]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: resolvers: Unlink DNS resolution to set RMAINT on SRV resolution
Christopher Faulet [Wed, 10 Mar 2021 20:33:21 +0000 (21:33 +0100)]
BUG/MINOR: resolvers: Unlink DNS resolution to set RMAINT on SRV resolution

When a server is set in RMAINT becaues of a SRV resolution failure, the
server DNS resolution, if any, must be unlink first. It is mandatory to
handle the change in the context of a SRV resolution.

This patch must be backported as far as 2.2.

(cherry picked from commit bca680ba90787fc57b9711e268f6fb0c74e7e49c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 833937da99caf3639e67a14409095a402ea2f0b2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit f586f823539583c5f637e239a90f6dc0a959b859)
[cf: Must be backported as far as 2.0 because of recent changes on resolvers]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MAJOR: dns: disabled servers through SRV records never recover
Baptiste Assmann [Tue, 4 Aug 2020 08:57:21 +0000 (10:57 +0200)]
BUG/MAJOR: dns: disabled servers through SRV records never recover

A regression was introduced by 13a9232ebc63fdf357ffcf4fa7a1a5e77a1eac2b
when I added support for Additional section of the SRV responses..

Basically, when a server is managed through SRV records additional
section and it's disabled (because its associated Additional record has
disappeared), it never leaves its MAINT state and so never comes back to
production.
This patch updates the "snr_update_srv_status()" function to clear the
MAINT status when the server now has an IP address and also ensure this
function is called when parsing Additional records (and associating them
to new servers).

This can cause severe outage for people using HAProxy + consul (or any
other service registry) through DNS service discovery).

This should fix issue #793.
This should be backported to 2.2.

(cherry picked from commit 87138c3524bc4242dc48cfacba82d34504958e78)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit e7811e2add39f0329502181ed2d43a52275f697d)
[cf: Must be backported as far as 2.0 because of recent changes on resolvers]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MAJOR: dns: fix null pointer dereference in snr_update_srv_status
Jerome Magnin [Tue, 28 Jul 2020 11:38:22 +0000 (13:38 +0200)]
BUG/MAJOR: dns: fix null pointer dereference in snr_update_srv_status

Since commit 13a9232eb ("MEDIUM: dns: use Additional records from SRV
responses"), a struct server can have a NULL dns_requester->resolution,
when SRV records are used and DNS answers contain an Additional section.

This is a problem when we call snr_update_srv_status() because it does
not check that resolution is NULL, and dereferences it. This patch
simply adds a test for resolution being NULL. When that happens, it means
we are using SRV records with Additional records, and an entry was removed.

This should fix issue #775.
This should be backported to 2.2.

(cherry picked from commit 012261ab34e2423df983c502b43b304f84b71c9e)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 7e7cb73d019c3907dcf74159c91b8f31e9c994d4)
[cf: Must be backported as far as 2.0 because of recent changes on resolvers]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

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

(cherry picked from commit 39272c28bffcf57c050703b356c9381df454dea7)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 4253ce683402dfbcc6f08b60e42803aafe13147b)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 001d8345357d245f93cad0477116a242acd82ec8)
Signed-off-by: Willy Tarreau <w@1wt.eu>

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.

(cherry picked from commit 1e8433f594de4b860e5205fdd6cb40d91ff58f17)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit f2e7362e0cf879b3c78b29d3d8406a28668c1e12)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 36cdbeed01a1106c6e1c8efd4f1161721bc51212)
Signed-off-by: Willy Tarreau <w@1wt.eu>

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

(cherry picked from commit cc2c4f8f4c1d8613b481d1b346e083a9d2462811)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 9fb7eb90289552de9de544412de3f178cf6a3723)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit f80140db77075d3dcca673c8a58ea23468676114)
[wt: minor context adjustment: older attempt was not there in 2.1]
Signed-off-by: Willy Tarreau <w@1wt.eu>

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.

(cherry picked from commit d09cc519bd0c04a0d1ec5bf23ed053c1e596851a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 5326d6a152e7001c325ce3b00636132f0fb419eb)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit c4e150a99d5eb31ab7fd6355b9f21060b9433ccb)
Signed-off-by: Willy Tarreau <w@1wt.eu>

4 years agoDOC: tune: explain the origin of block size for ssl.cachesize
William Dauchy [Fri, 12 Feb 2021 14:58:46 +0000 (15:58 +0100)]
DOC: tune: explain the origin of block size for ssl.cachesize

A user could eventually ask himself where those 200 bytes block size are
coming from. This patch tries to better explain the origin in case
people are curious or want to double check the reality.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
(cherry picked from commit 9a4bbfe151b8db72ef4f353b5a1c5e1d60b20646)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit dede64a73977ac306c4e73fa347571826d0164c8)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 94eca95c52ffee8c345da49c44c92f37366f4f2a)
Signed-off-by: Willy Tarreau <w@1wt.eu>

4 years agoDOC: Explicitly state only IPv4 are supported by forwardfor/originalto options
Christopher Faulet [Tue, 6 Apr 2021 07:01:09 +0000 (09:01 +0200)]
DOC: Explicitly state only IPv4 are supported by forwardfor/originalto options

IPv6 support was added in the 2.4. However, on previous versions, it is not
explicit that only IPv4 addresses are supported. In addition, a workaround
solution is proposed to add IPv6 addresses.

This patch is related to issue #1145. There is no upstream commit ID. It may
be backported to any stable versions.

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

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.

(cherry picked from commit 645dc08533531416b91ca74ff5aa03154dc0ee50)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 48b6abfdbb9eaba62797bc7af86392b53c67c19d)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 25b66ddf2cd7246aa9f79aa6dce40a5363ca9ffe)
Signed-off-by: Willy Tarreau <w@1wt.eu>

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.

(cherry picked from commit da2319578555e56eafab0141505ce943f954d859)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit e3647da097fbf23ca45fb065bb5c1cde98d6c40d)
[wt: this is in proto_tcp.c in 2.2]
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit a20e7c0171b965c07c2c6c3389c97c15203431cd)
Signed-off-by: Willy Tarreau <w@1wt.eu>

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.

(cherry picked from commit ab79ee8b117dbb2c2872747e8119492e70506392)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 64300c5118f0e2cd40ccf1b6aa9d5f19ada0cdc9)
[wt: this is in proto_tcp.c in 2.2]
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 9650f63fb150e0f1c1bcb7012d59eff0ee61a868)
Signed-off-by: Willy Tarreau <w@1wt.eu>

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.

(cherry picked from commit 62592ad967d6d24be2aabb664a5e1d594ab35415)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit c6eedcceef97f6a5a03e2b8b35e32ec31470483c)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 632a3e24bb474badf705fa356ab31110ba18fa32)
Signed-off-by: Willy Tarreau <w@1wt.eu>

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.

(cherry picked from commit 7b0e00d943feaa6320522c376fbbff18965b1345)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 7e2e49ede43d9d65304edaae8861477ce53faab2)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 7a7b8393f4b0e6ca7a4ad9fe250370340cb8bff0)
[wt: small adjustments to the doc]
Signed-off-by: Willy Tarreau <w@1wt.eu>

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.

(cherry picked from commit 12e1027aa6c7687b5638c0c9260696266c9e400b)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 4d61bdbe244f43d7c1c9356564ee5863542c005b)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 3fe1c0e6b86bd4647f1390769f54ce6a24e7fb58)
Signed-off-by: Willy Tarreau <w@1wt.eu>

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.

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

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

(cherry picked from commit 8cc586c73fefd96f4be1f7820e38a1263f6252ca)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 049602fff734664afbf75b5b28b1d9a028ed9f04)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 84c81577e52b7376e2c03cfd552d3023fa4349ec)
Signed-off-by: Willy Tarreau <w@1wt.eu>

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.

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

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.

(cherry picked from commit 6064b34be0e761de881a5bfca287d01f69122602)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 3a9bc55350e18a92538ffa7295a204420fdba683)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 29d0329b8af90af42d37007c8c89872203fba1a3)
[wt: minor adjustments to includes (common/ instead of haproxy/)]
Signed-off-by: Willy Tarreau <w@1wt.eu>

4 years ago[RELEASE] Released version 2.1.12 v2.1.12
Christopher Faulet [Thu, 18 Mar 2021 13:25:32 +0000 (14:25 +0100)]
[RELEASE] Released version 2.1.12

Released version 2.1.12 with the following main changes :
    - BUG/MINOR: sample: check alloc_trash_chunk return value in concat()
    - BUG/MINOR: sample: Memory leak of sample_expr structure in case of error
    - BUG/MINOR: init: enforce strict-limits when using master-worker
    - BUG/MINOR: init: Use a dynamic buffer to set HAPROXY_CFGFILES env variable
    - BUG/MINOR: threads: Fixes the number of possible cpus report for Mac.
    - BUG/MINOR: peers: Wrong "new_conn" value for "show peers" CLI command.
    - BUG/MINOR: mux_h2: missing space between "st" and ".flg" in the "show fd" helper
    - BUG/MINOR: mworker: define _GNU_SOURCE for strsignal()
    - BUG/MEDIUM: mux-h2: fix read0 handling on partial frames
    - BUILD/MINOR: lua: define _GNU_SOURCE for LLONG_MAX
    - BUG/MEDIUM: stats: add missing INF_BUILD_INFO definition
    - BUG/MEDIUM: filters/htx: Fix data forwarding when payload length is unknown
    - BUG/MINOR: config: fix leak on proxy.conn_src.bind_hdr_name
    - MINOR: contrib: Make the wireshark peers dissector compile for more distribs.
    - DOC: management: fix "show resolvers" alphabetical ordering
    - BUG/MINOR: stick-table: Always call smp_fetch_src() with a valid arg list
    - BUG/MEDIUM: ssl: check a connection's status before computing a handshake
    - BUG/MINOR: xxhash: make sure armv6 uses memcpy()
    - BUG/MINOR: ssl: init tmp chunk correctly in ssl_sock_load_sctl_from_file()
    - BUG/MEDIUM: ssl/cli: abort ssl cert is freeing the old store
    - BUILD: Makefile: move REGTESTST_TYPE default setting
    - BUG/MEDIUM: mux-h2: handle remaining read0 cases
    - BUG/MEDIUM: mux-h2: do not quit the demux loop before setting END_REACHED
    - BUG/MINOR: http-ana: Don't increment HTTP error counter on internal errors
    - BUG/MEDIUM: mux-h1: Always set CS_FL_EOI for response in MSG_DONE state
    - BUG/MINOR: server: re-align state file fields number
    - BUG/MINOR: tools: Fix a memory leak on error path in parse_dotted_uints()
    - BUG/MINOR: backend: hold correctly lock when killing idle conn
    - BUG/MINOR: server: Fix server-state-file-name directive
    - CLEANUP: deinit: release global and per-proxy server-state variables on deinit
    - BUG/MEDIUM: config: don't pick unset values from last defaults section
    - BUG/MINOR: stats: revert the change on ST_CONVDONE
    - BUG/MINOR: cfgparse: do not mention "addr:port" as supported on proxy lines
    - BUG/MINOR: server: Don't call fopen() with server-state filepath set to NULL
    - CLEANUP: channel: fix comment in ci_putblk.
    - BUG/MINOR: server: Remove RMAINT from admin state when loading server state
    - BUG/MINOR: session: atomically increment the tracked sessions counter
    - BUG/MINOR: checks: properly handle wrapping time in __health_adjust()
    - BUG/MINOR: sample: Always consider zero size string samples as unsafe
    - BUG/MINOR: server: Init params before parsing a new server-state line
    - BUG/MINOR: server: Be sure to cut the last parsed field of a server-state line
    - BUG/MEDIUM: mux-h1: Fix handling of responses to CONNECT other than 200-ok
    - BUG/MINOR: ssl/cli: potential null pointer dereference in "set ssl cert"
    - BUG/MINOR: sample: secure convs that accept base64 string and var name as args
    - BUG/MEDIUM: vars: make functions vars_get_by_{name,desc} thread-safe
    - BUG/MEDIUM: proxy: use thread-safe stream killing on hard-stop
    - BUG/MEDIUM: cli/shutdown sessions: make it thread-safe
    - BUG/MINOR: proxy: wake up all threads when sending the hard-stop signal
    - BUG/MINOR: resolvers: new callback to properly handle SRV record errors
    - BUG/MEDIUM: resolvers: Reset server address and port for obselete SRV records
    - BUG/MEDIUM: resolvers: Reset address for unresolved servers
    - BUG/MINOR: mux-h1: Immediately report H1C errors from h1_snd_buf()
    - BUG/MINOR: http-ana: Only consider dst address to process originalto option
    - BUG/MINOR: tcp-act: Don't forget to set the original port for IPv4 set-dst rule
    - BUG/MINOR: connection: Use the client's dst family for adressless servers
    - BUG/MEDIUM: spoe: Kill applets if there are pending connections and nbthread > 1
    - DOC: spoe: Add a note about fragmentation support in HAProxy
    - BUG/MINOR: mux-h2: Fix typo in scheme adjustment
    - BUG/MINOR: http-ana: Don't increment HTTP error counter on read error/timeout
    - BUG/MEDIUM: dns: Consider the fact that dns answers are case-insensitive
    - BUG/MEDIUM: lists: Lock the element while we check if it is in a list.
    - BUG/MEDIUM: lists: Avoid an infinite loop in MT_LIST_TRY_ADDQ().
    - BUG/MINOR: hlua: Don't strip last non-LWS char in hlua_pushstrippedstring()
    - BUG/MINOR: ssl: don't truncate the file descriptor to 16 bits in debug mode
    - BUG/MEDIUM: session: NULL dereference possible when accessing the listener
    - BUG/MEDIUM: filters: Set CF_FL_ANALYZE on channels when filters are attached
    - BUG/MINOR: proxy/session: Be sure to have a listener to increment its counters
    - BUG/MINOR: session: Add some forgotten tests on session's listener
    - CLEANUP: tcp-rules: add missing actions in the tcp-request error message
    - BUG/MINOR: resolvers: Consider server to have no IP on DNS resolution error
    - BUG/MINOR: resolvers: Reset server address on DNS error only on status change
    - BUG/MINOR: resolvers: Add missing case-insensitive comparisons of DNS hostnames
    - MINOR: time: export the global_now variable
    - BUG/MINOR: freq_ctr/threads: make use of the last updated global time
    - MINOR: version: Set the EOL of the 2.1 branch

4 years agoMINOR: version: Set the EOL of the 2.1 branch
Christopher Faulet [Thu, 4 Mar 2021 14:06:51 +0000 (15:06 +0100)]
MINOR: version: Set the EOL of the 2.1 branch

The end-of-life of the 2.1 branch was planned for Q1 2021. This patch makes
it official for the next coming release.

4 years agoBUG/MINOR: freq_ctr/threads: make use of the last updated global time
Willy Tarreau [Wed, 17 Mar 2021 18:10:23 +0000 (19:10 +0100)]
BUG/MINOR: freq_ctr/threads: make use of the last updated global time

The freq counters were using the thread's own time as the start of the
current period. The problem is that in case of contention, it was
occasionally possible to perform non-monotonic updates on the edge of
the next second, because if the upfront thread updates a counter first,
it causes a rotation, then the second thread loses the race from its
older time, and tries again, and detects a different time again, but
in the past so it only updates the counter, then a third thread on the
new date would detect a change again, thus provoking a rotation again.

The effect was triple:
  - rare loss of stored values during certain transitions from one
    period to the next one, causing counters to report 0
  - half of the threads forced to go through the slow path every second
  - difficult convergence when using many threads where the CAS can fail
    a lot and we can observe N(N-1) attempts for N threads to complete

This patch fixes this issue in two ways:
  - first, it now makes use og the monotonic global_now value which also
    happens to be volatile and to carry the latest known time; this way
    time will never jump backwards anymore and only the first thread
    updates it on transition, the other ones do not need to.

  - second, re-read the time in the loop after each failure, because
    if the date changed in the counter, it means that one thread knows
    a more recent one and we need to update. In this case if it matches
    the new current second, the fast path is usable.

This patch relies on previous patch "MINOR: time: export the global_now
variable" and must be backported as far as 1.8.

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

4 years agoMINOR: time: export the global_now variable
Willy Tarreau [Wed, 17 Mar 2021 17:52:18 +0000 (18:52 +0100)]
MINOR: time: export the global_now variable

This is the process-wide monotonic time that is used to update each
thread's own time. It may be required at a few places where a strictly
monotonic clock is required such as freq_ctr. It will be have to be
backported as a dependency of a forthcoming fix.

(cherry picked from commit 650f374f24c4dd14e90d336c72c2370242be1459)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit f520788e171f3393347da00358ae4ca2f30789a2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 8ff0c907c7f9edc3f778b51e6980b1cd0bcff5e6)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: resolvers: Add missing case-insensitive comparisons of DNS hostnames
Christopher Faulet [Tue, 16 Mar 2021 10:21:04 +0000 (11:21 +0100)]
BUG/MINOR: resolvers: Add missing case-insensitive comparisons of DNS hostnames

DNS hostname comparisons were fixed to be case-insensitive (see b17b88487
"BUG/MEDIUM: dns: Consider the fact that dns answers are
case-insensitive"). However 2 comparisons are still case-sensitive.

This patch must be backported as far as 1.8.

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

4 years agoBUG/MINOR: resolvers: Reset server address on DNS error only on status change
Christopher Faulet [Wed, 10 Mar 2021 19:31:40 +0000 (20:31 +0100)]
BUG/MINOR: resolvers: Reset server address on DNS error only on status change

When a DNS resolution error is detected, in snr_resolution_error_cb(), the
server address must be reset only if the server status has changed. It this
case, it means the server is set to RMAINT. Thus the server address may by
reset.

This patch fixes a bug introduced by commit d127ffa9f ("BUG/MEDIUM:
resolvers: Reset address for unresolved servers"). It must be backported as
far as 2.0.

(cherry picked from commit 5130c21fbb806a28ab4e535b700df872a80798d8)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 7c9201e08f03367db74f2fca795cb1eb35d14554)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 06623af1dc7088a2f48a3aeb53e252edfea66ec6)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: resolvers: Consider server to have no IP on DNS resolution error
Christopher Faulet [Wed, 10 Mar 2021 14:39:16 +0000 (15:39 +0100)]
BUG/MINOR: resolvers: Consider server to have no IP on DNS resolution error

When an error is received for a DNS resolution, for instance a NXDOMAIN
error, the server must be considered to have no address when its status is
updated, not the opposite.

Concretly, because this parameter is not used on error path in
snr_update_srv_status(), there is no impact.

This patch must be backported as far as 1.8.

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

4 years agoCLEANUP: tcp-rules: add missing actions in the tcp-request error message
Willy Tarreau [Fri, 12 Mar 2021 12:42:43 +0000 (13:42 +0100)]
CLEANUP: tcp-rules: add missing actions in the tcp-request error message

The tcp-request error message only mentions "accept", "reject" and
track-sc*, but there are a few other ones that were missing, so let's
add them.

This could be backported, though it's not likely that it will help anyone
with an existing config.

(cherry picked from commit 72d012fbd971bfd8b2bf6ab022572a816b759cbd)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 5834848252fbc7a6f192e600eb56f388de3258b1)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 02b2e0567d9b668549dd9e0a1d04424c3a3f29cf)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: session: Add some forgotten tests on session's listener
Christopher Faulet [Fri, 12 Mar 2021 10:26:15 +0000 (11:26 +0100)]
BUG/MINOR: session: Add some forgotten tests on session's listener

During backport of commit 36119de18 ("BUG/MEDIUM: session: NULL dereference
possible when accessing the listener"), I missed some tests on the session's
listener because of the context changes.

It is specific to the 2.3, thus there is no upstream commit ID. It must
backported with the above commit as far as 1.8.

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

4 years agoBUG/MINOR: proxy/session: Be sure to have a listener to increment its counters
Christopher Faulet [Fri, 12 Mar 2021 08:16:27 +0000 (09:16 +0100)]
BUG/MINOR: proxy/session: Be sure to have a listener to increment its counters

It is possible to have a session without a listener. It happens for applets
on the client side. Thus all accesses to the listener info from the session
must be guarded. It was the purpose of the commit 36119de18 ("BUG/MEDIUM:
session: NULL dereference possible when accessing the listener"). However,
some tests on the session's listener existence are missing in proxy_inc_*
functions.

This patch should fix the issues #1171, #1172, #1173, #1174 and #1175. It
must be backported with the above commit as far as 1.8.

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

4 years agoBUG/MEDIUM: filters: Set CF_FL_ANALYZE on channels when filters are attached
Christopher Faulet [Mon, 8 Mar 2021 12:40:30 +0000 (13:40 +0100)]
BUG/MEDIUM: filters: Set CF_FL_ANALYZE on channels when filters are attached

CF_FL_ANALYZE flag is used to know a channel is filtered. It is important to
synchronize request and response channels when the filtering ends.

However, it is possible to call all request analyzers before starting the
filtering on the response channel. This means flt_end_analyze() may be
called for the request channel before flt_start_analyze() on the response
channel. Thus because CF_FL_ANALYZE flag is not set on the response channel,
we consider the filtering is finished on both sides. The consequence is that
flt_end_analyze() is not called for the response and backend filters are
unregistered before their execution on the response channel.

It is possible to encounter this bug on TCP frontend or CONNECT request on
HTTP frontend if the client shutdown is reveiced with the first read.

To fix this bug, CF_FL_ANALYZE is set when filters are attached to the
stream. It means, on the request channel when the stream is created, in
flt_stream_start(). And on both channels when the backend is set, in
flt_set_stream_backend().

This patch must be backported as far as 1.7.

(cherry picked from commit 5647fbacdf822f2a6812a07dbdf27660f436f103)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 1b89c27781fce9ac83d9bc4d83bcc999bce4e5a0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit efa89a22c2badcb7dc03c18667509a9b0265e544)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: session: NULL dereference possible when accessing the listener
William Lallemand [Mon, 8 Mar 2021 14:26:48 +0000 (15:26 +0100)]
BUG/MEDIUM: session: NULL dereference possible when accessing the listener

When implementing a client applet, a NULL dereference was encountered on
the error path which increment the counters.

Indeed, the counters incremented are the one in the listener which does
not exist in the case of client applets, so in sess->listener->counters,
listener is NULL.

This patch fixes the access to the listener structure when accessing
from a sesssion, most of the access are the counters in error paths.

Must be backported as far as 1.8.

(cherry picked from commit 36119de182154b1f87e0cdf4bd1efba9e2e64113)
[wt: minor ctx adjustments in http_ana and mux_h1]
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 656730d92f1c3afaed0baf67911d4ee055528e2e)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 35054f1d6cc549dbd53832d551f7f9f446a0d18d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: ssl: don't truncate the file descriptor to 16 bits in debug mode
Willy Tarreau [Tue, 2 Mar 2021 18:32:39 +0000 (19:32 +0100)]
BUG/MINOR: ssl: don't truncate the file descriptor to 16 bits in debug mode

Errors reported by ssl_sock_dump_errors() to stderr would only report the
16 lower bits of the file descriptor because it used to be casted to ushort.
This can be backported to all versions but has really no importance in
practice since this is never seen.

(cherry picked from commit 566cebc1fc4f9908a47a1924c80ff32460543a49)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 07451aceab1ea0863b06f57629b618961edbb3ab)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 58da3d4ff2578ee6092c1acbfaa4b851f68d5475)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: hlua: Don't strip last non-LWS char in hlua_pushstrippedstring()
Christopher Faulet [Wed, 3 Mar 2021 18:36:51 +0000 (19:36 +0100)]
BUG/MINOR: hlua: Don't strip last non-LWS char in hlua_pushstrippedstring()

hlua_pushstrippedstring() function strips leading and trailing LWS
characters. But the result length it too short by 1 byte. Thus the last
non-LWS character is stripped. Note that a string containing only LWS
characters resulting to a stipped string with an invalid length (-1). This
leads to a lua runtime error.

This bug was reported in the issue #1155. It must be backported as far as
1.7.

(cherry picked from commit 2ec4e3c1acf95bcdc56028bbefe1a355c457b978)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 598ec14ca797b61f25f183216323d533a7c45c5e)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 50e5b6ec5e0e5122e71f49c2f537f8e20dbeae9d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: lists: Avoid an infinite loop in MT_LIST_TRY_ADDQ().
Olivier Houchard [Thu, 18 Feb 2021 22:55:30 +0000 (23:55 +0100)]
BUG/MEDIUM: lists: Avoid an infinite loop in MT_LIST_TRY_ADDQ().

In MT_LIST_TRY_ADDQ(), deal with the "prev" field of the element before the
"next". If the element is the first in the list, then its next will
already have been locked when we locked list->prev->next, so locking it
again will fail, and we'll start over and over.

This should be backported to 2.3.

(cherry picked from commit 5567f41d0ab61dd6843535edc8081407d599024d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 6f682bea6ab08830d17ef3e973be6cc4d2474e69)
[wt: in 2.2 the macro is there, it's MT_LIST_ADDQ() which was later renamed]
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit cd721385380410d9432d7fba9fe257415bfb09f2)
[wt: also present in 2.1]
Signed-off-by: Willy Tarreau <w@1wt.eu>

4 years agoBUG/MEDIUM: lists: Lock the element while we check if it is in a list.
Olivier Houchard [Wed, 25 Nov 2020 19:38:00 +0000 (20:38 +0100)]
BUG/MEDIUM: lists: Lock the element while we check if it is in a list.

In MT_LIST_TRY_ADDQ() and MT_LIST_TRY_ADD() we can't just check if the
element is already in a list, because there's a small race condition, it
could be added  between the time we checked, and the time we actually set
its next and prev, so we have to lock it first.

This is required to address issue #958.

This should be backported to 2.3, 2.2 and 2.1.

(cherry picked from commit 1f05324cbe92a7dde71f44dc740eb8240539746f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit c4f3013d9452707d4efbda455360c8d2af022411)
[wt: fix has been in 2.3 for 3 versions now]
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 4aebb4e88ed360003efacc7f75a48c1d39bda6bb)
[wt: updated the two macros to match the equivalent upstream patch
 because during the reorg they were cleaned up by trimming the unused
 do{}while(0), making the patch impossible to apply that way]
Signed-off-by: Willy Tarreau <w@1wt.eu>

4 years agoBUG/MEDIUM: dns: Consider the fact that dns answers are case-insensitive
Olivier Houchard [Wed, 1 Apr 2020 16:30:27 +0000 (18:30 +0200)]
BUG/MEDIUM: dns: Consider the fact that dns answers are case-insensitive

We can't expect the DNS answer to always match the case we used for the
request, so we can't just use memcmp() to compare the DNS answer with what
we are expected.
Instead, introduce dns_hostname_cmp(), which compares each string in a
case-insensitive way.
This should fix github issue #566.

This should be backported to 2.1, 2.0, 1.9 and 1.8.

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

4 years agoBUG/MINOR: http-ana: Don't increment HTTP error counter on read error/timeout
Christopher Faulet [Wed, 3 Mar 2021 10:24:10 +0000 (11:24 +0100)]
BUG/MINOR: http-ana: Don't increment HTTP error counter on read error/timeout

This should have been fixed when the commit "BUG/MINOR: http-ana: Don't
increment HTTP error counter on internal errors" was backported but I forgot
to do so. The HTTP error counter must not be incremented if a read error or
a read timeout is encountered. Parsing error are already reported by the
mux.

This patch must be backported as far as 2.0, on the HTX part only.

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

4 years agoBUG/MINOR: mux-h2: Fix typo in scheme adjustment
Tim Duesterhus [Sun, 28 Feb 2021 15:12:20 +0000 (16:12 +0100)]
BUG/MINOR: mux-h2: Fix typo in scheme adjustment

That comma should've been a semicolon. Fortunately, as it is now there
is no impact thanks to operators precedence, and all expressions are
properly evaluated. But this is troubling and the risk is high to
turn it into an effective bug with a minor change.

Introduced in b8ce8905cf63ecd06b36af39c05103fadf3cc347 which first
appeared in 2.1-dev3. This fix must be backported to 2.1+.

(cherry picked from commit a3298023b04923ba12429d79c559dc7a850ae122)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 31871d22827508fcbfe1064a831d220084e19d74)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 21e7aa3f57be44c2b41b4b40056e74374b229438)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoDOC: spoe: Add a note about fragmentation support in HAProxy
Christopher Faulet [Tue, 2 Mar 2021 09:05:03 +0000 (10:05 +0100)]
DOC: spoe: Add a note about fragmentation support in HAProxy

Add a note in SPOE.txt to make it clear that HAPRoxy does not support the
fragmentation. It can send fragmented frames if an agent supports it but it
cannot receives and handles fragmented frames.

This patch should fix the issue #659. It may be backported as far as 1.8.

(cherry picked from commit 9536ad707f390c8026777ec8e074a5edaa2f7c7a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 71ebab63e0d8062b8795ac616d99730bc16f89d3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 0f7a3a1d84d7df08c7cc2d2b2a7c95eefa0d9909)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: spoe: Kill applets if there are pending connections and nbthread > 1
Christopher Faulet [Mon, 1 Mar 2021 14:01:14 +0000 (15:01 +0100)]
BUG/MEDIUM: spoe: Kill applets if there are pending connections and nbthread > 1

When the processing stage is finished for a SPOE applet, before returning it
into the idle list, we check if the assigned server appears as full or if
there are some pending connections on the backend or the assigned server. If
yes, it means we reach a maxconn and we close the applet to free a
slot. Otherwise, the applet can be reused. This test is only performed if
there are more than one thread.

It is important to close SPOE applets when there are pending connections for
multithreaded instances because connections with the SPOE agents are
persistent and local to a thread (applets are local to a thread). If a
maxconn is configured, some threads may take all available slots for a
while, leaving remaining threads without any free slot to process SPOE
messages. It is especially true if the maxconn is low.

This patch should fix the issue #705. It must be backported as far as
1.8. However, the code in 1.8 is quite different, a test must be performed
to be sure it works well.

(cherry picked from commit 9e647e5af77faa6a95dd511d78ed8763781e764a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 1b28e0a57fe2393370c5f64450ce2f27a4fabeb7)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 02bfbacb84ceafc28dc79f416da51b97943f01e0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: connection: Use the client's dst family for adressless servers
Christopher Faulet [Mon, 1 Mar 2021 10:33:59 +0000 (11:33 +0100)]
BUG/MINOR: connection: Use the client's dst family for adressless servers

When the selected server has no address, the destination address of the
client is used. However, for now, only the address is set, not the
family. Thus depending on how the server is configured and the client's
destination address, the server address family may be wrong.

For instance, with such server :

   server srv 0.0.0.0:0

The server address family is AF_INET. The server connection will fail if a
client is asking for an IPv6 destination.

To fix the bug, we take care to set the rigth family, the family of the
client destination address.

This patch should fix the issue #202. It must be backported to all stable
versions.

(cherry picked from commit ae3056157c17b181c754556836a5d8336fb9823f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 63f4db3ffb9ccc5ed24c39118fee7a882529df3f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 123f1352303b9d7d3087504029c7bb3c5eb85429)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: tcp-act: Don't forget to set the original port for IPv4 set-dst rule
Christopher Faulet [Mon, 1 Mar 2021 10:21:14 +0000 (11:21 +0100)]
BUG/MINOR: tcp-act: Don't forget to set the original port for IPv4 set-dst rule

If an IPv4 is set via a TCP/HTTP set-dst rule, the original port must be
preserved or set to 0 if the previous family was neither AF_INET nor
AF_INET6. The first case is not an issue because the port remains the
same. But if the previous family was, for instance, AF_UNIX, the port is not
set to 0 and have an undefined value.

This patch must be backported as far as 1.7.

(cherry picked from commit e01ca0fbc9c72de95514816e016a58c5a28ab2a8)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 3a6133a3b7eff777f0c7bee0f000a763cf2ec971)
[cf: Changes applied in src/proto_tcp.c]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 7defd7020b188afb82cc20b7d63d5dba4ac756a4)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: http-ana: Only consider dst address to process originalto option
Christopher Faulet [Fri, 26 Feb 2021 11:45:56 +0000 (12:45 +0100)]
BUG/MINOR: http-ana: Only consider dst address to process originalto option

When an except parameter is used for originalto option, only the destination
address must be evaluated. Especially, the address family of the destination
must be tested and not the source one.

This patch must be backported to all stable versions. However be careful,
depending the versions the code may be slightly different.

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

4 years agoBUG/MINOR: mux-h1: Immediately report H1C errors from h1_snd_buf()
Christopher Faulet [Mon, 1 Mar 2021 16:46:32 +0000 (17:46 +0100)]
BUG/MINOR: mux-h1: Immediately report H1C errors from h1_snd_buf()

In case an H1 stream tries to send while on error occurred on its underlying
H1 connection, we must report the error. The way the stream-interface is
synchronously notified of the error. It seems to only be a problem on the
2.0. Probably because the scheduling has changed in upper versions. On the
2.0, it prevent the stream to be notified of errors, when for instance, a
payload is found in a response to a HEAD request. Not always though.

This patch must be backported as far as 2.0 because, on 2.0, it should fix the
issue #1101. There is no upstream ID for this commit because on the 2.4, this
fix already exists, it is part of non-backportable commit.

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

4 years agoBUG/MEDIUM: resolvers: Reset address for unresolved servers
Christopher Faulet [Tue, 23 Feb 2021 11:33:17 +0000 (12:33 +0100)]
BUG/MEDIUM: resolvers: Reset address for unresolved servers

If the DNS resolution failed for a server, its ip address must be
removed. Otherwise, the server is stopped but keeps its ip. This may be
confusing when the servers state are retrieved on the CLI and it may lead to
undefined behavior if HAproxy is configured to load its servers state from a
file.

This patch should be backported as far as 2.0.

(cherry picked from commit d127ffa9f439a4eb05455f371191694f22dd7a5b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 9b8c904fa1822fd8d4e46a99a5d81e1f9956bb78)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 9005c77e73730f6ee3cc082836d657cef11f55f8)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: resolvers: Reset server address and port for obselete SRV records
Christopher Faulet [Tue, 23 Feb 2021 11:24:09 +0000 (12:24 +0100)]
BUG/MEDIUM: resolvers: Reset server address and port for obselete SRV records

When a SRV record expires, the ip/port assigned to the associated server are
now removed. Otherwise, the server is stopped but keeps its ip/port while
the server hostname is removed. It is confusing when the servers state are
retrieve on the CLI and may be a problem if saved in a server-state
file. Because the reload may fail because of this inconsistency.

Here is an example:

 * Declare a server template in a backend, using the resolver <dns>

server-template test 2 _http._tcp.example.com resolvers dns check

 * 2 SRV records are announced with the corresponding additional
   records. Thus, 2 servers are filled. Here is the "show servers state"
   output :

2 frt 1 test1 192.168.1.1 2 64 0 1 2 15 3 4 6 0 0 0 http1.example.com 8001 _http._tcp.example.com 0 0 - - 0
2 frt 2 test2 192.168.1.2 2 64 0 1 1 15 3 4 6 0 0 0 http2.example.com 8002 _http._tcp.example.com 0 0 - - 0

 * Then, one additional record is removed (or a SRV record is removed, the
   result is the same). Here is the new "show servers state" output :

2 frt 1 test1 192.168.1.1 2 64 0 1 38 15 3 4 6 0 0 0 http1.example.com 8001 _http._tcp.example.com 0 0 - - 0
2 frt 2 test2 192.168.1.2 0 96 0 1 19 15 3 0 14 0 0 0 - 8002 _http._tcp.example.com 0 0 - - 0

On reload, if a server-state file is used, this leads to undefined behaviors
depending on the configuration.

This patch should be backported as far as 2.0.

(cherry picked from commit 52d4d3010991e851b8e9e4d9f923ad1f74d30d69)
[cf: Changes applied in src/dns.c]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 8427aa63d7664b985607d8623b6c9edcd823b1fa)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit a37808290d11fa43940b48c81d998448ba0c43e3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: resolvers: new callback to properly handle SRV record errors
Baptiste Assmann [Thu, 19 Nov 2020 21:38:33 +0000 (22:38 +0100)]
BUG/MINOR: resolvers: new callback to properly handle SRV record errors

When a SRV record was created, it used to register the regular server name
resolution callbacks. That said, SRV records and regular server name
resolution don't work the same way, furthermore on error management.

This patch introduces a new call back to manage DNS errors related to
the SRV queries.

this fixes github issue #50.

Backport status: 2.3, 2.2, 2.1, 2.0

(cherry picked from commit b4badf720ce484001f606011aee7cd216e5ce4e3)
[cf: Changes applied in src/dns.c and structures renamed]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 826060e383b34661a91bf3350f6e1137c603e9f5)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 8c0d185e01060f447bf94949a507a3aaf678a779)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: proxy: wake up all threads when sending the hard-stop signal
Willy Tarreau [Wed, 24 Feb 2021 10:13:59 +0000 (11:13 +0100)]
BUG/MINOR: proxy: wake up all threads when sending the hard-stop signal

The hard-stop event didn't wake threads up. In the past it wasn't an issue
as the poll timeout was limited to 1 second, but since commit 4f59d3861
("MINOR: time: increase the minimum wakeup interval to 60s") it has become
a problem because old processes can remain live for up to one minute after
the hard-stop-after delay. Let's just wake them up.

This may be backported to older releases, though before 2.4 the extra
delay was only one second.

(cherry picked from commit 0d03825b93cc59a289e838105f9d83d53ccdfc8b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 6b08479975a798d9ee929b512bbb0d0ffd0f02be)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 300b2e8ffb4403ee7ecb961115ce05b093521006)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: cli/shutdown sessions: make it thread-safe
Willy Tarreau [Wed, 24 Feb 2021 10:11:06 +0000 (11:11 +0100)]
BUG/MEDIUM: cli/shutdown sessions: make it thread-safe

There's no locking around the lookup of a stream nor its shutdown
when issuing "shutdown sessions" over the CLI so the risk of crashing
the process is particularly high.

Let's use a thread_isolate() there which is suitable for this task, and
there are not that many alternatives.

This must be backported to 1.8.

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

4 years agoBUG/MEDIUM: proxy: use thread-safe stream killing on hard-stop
Willy Tarreau [Wed, 24 Feb 2021 10:08:56 +0000 (11:08 +0100)]
BUG/MEDIUM: proxy: use thread-safe stream killing on hard-stop

When setting hard-stop-after, hard_stop() is called at the end to kill
last pending streams. Unfortunately there's no locking there while
walking over the streams list nor when shutting them down, so it's
very likely that some old processes have been crashing or gone wild
due to this. Let's use a thread_isolate() call for this as we don't
have much other choice (and it happens once in the process' life,
that's OK).

This must be backported to 1.8.

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

4 years agoBUG/MEDIUM: vars: make functions vars_get_by_{name,desc} thread-safe
Dragan Dosen [Mon, 22 Feb 2021 16:20:01 +0000 (17:20 +0100)]
BUG/MEDIUM: vars: make functions vars_get_by_{name,desc} thread-safe

This patch adds a lock to functions vars_get_by_name() and
vars_get_by_desc() to protect accesses to the list of variables.

After the variable is fetched, a sample data is duplicated by using
smp_dup() because the variable may be modified by another thread.

This should be backported to all versions supporting vars along with
"BUG/MINOR: sample: secure convs that accept base64 string and var name
as args" which this patch depends on.

(cherry picked from commit 14518f2305027dfd537c1be0f88350337b5fba23)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit bdea395d630bdb92e5d882e235439a1560e12296)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 6524481ea5ab6ca5888450d535d011046d7d2f80)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: sample: secure convs that accept base64 string and var name as args
Dragan Dosen [Mon, 22 Feb 2021 09:03:53 +0000 (10:03 +0100)]
BUG/MINOR: sample: secure convs that accept base64 string and var name as args

This patch adds a few improvements in order to secure the use of
converters that accept base64 string and variable name as arguments.

The first change is within related function sample_conv_var2smp_str()
which now flags the sample as SMP_F_CONST if the argument is of type
ARGT_STR. This makes the sample more safe for later use.

A new function sample_check_arg_base64() is added. It checks an argument
and fills it with a variable type if the argument string contains a
valid variable name. If failed, it tries to perform a base64 decode
operation on a non-empty string, and fills the argument with the decoded
content which can be used later, without any additional base64dec()
function calls during runtime. This means that haproxy configuration
check may fail if variable lookup fails and an invalid base64 encoded
string is specified as an argument for such converters.

Both converters, "aes_gcm_dec" and "hmac", now use alloc_trash_chunk()
in order to allocate additional buffers for various conversions, and
avoid the use of a pre-allocated trash chunks directly (usually returned
by get_trash_chunk()). The function sample_check_arg_base64() is used
for both converters in order to check their arguments specified within
the haproxy configuration.

This patch should be backported as far as 2.0. However, it is important
to keep in mind a few things. The "hmac" converter is only available
starting with 2.2. In versions prior to 2.2, the "aes_gcm_dec" converter
and sample_conv_var2smp_str() are implemented in src/ssl_sock.c. Thus
the patch will have to be adapted on these versions.

Note that this patch is required for a subsequent, more important fix.

(cherry picked from commit 9e8db138c9e50262f2aae898bbc9b9b0b9a93449)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 11a23edcf6f4ccaef80bf81c528412de513a0c8b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 17c744088999a0292593a10d97982c22b65745ff)
[cf: Changes applied in src/ssl_sock.c]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: ssl/cli: potential null pointer dereference in "set ssl cert"
William Lallemand [Tue, 23 Feb 2021 13:45:45 +0000 (14:45 +0100)]
BUG/MINOR: ssl/cli: potential null pointer dereference in "set ssl cert"

A potential null pointer dereference was reported with an old gcc
version (6.5)

    src/ssl_ckch.c: In function 'cli_parse_set_cert':
    src/ssl_ckch.c:838:7: error: potential null pointer dereference [-Werror=null-dereference]
      if (!ssl_sock_copy_cert_key_and_chain(src->ckch, dst->ckch))
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    src/ssl_ckch.c:838:7: error: potential null pointer dereference [-Werror=null-dereference]
    src/ssl_ckch.c: In function 'ckchs_dup':
    src/ssl_ckch.c:838:7: error: potential null pointer dereference [-Werror=null-dereference]
      if (!ssl_sock_copy_cert_key_and_chain(src->ckch, dst->ckch))
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    src/ssl_ckch.c:838:7: error: potential null pointer dereference [-Werror=null-dereference]
    cc1: all warnings being treated as errors

This case does not actually happen but it's better to fix the ckch API
with a NULL check.

Could be backported as far as 2.1.

(cherry picked from commit 6c0961442c5e19a1bfc706374f96cfbd42feaeb2)
Signed-off-by: William Lallemand <wlallemand@haproxy.org>
(cherry picked from commit 8f71298de2fa153fa9855711b992f52cfb8fb1ff)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 1e5d84df6510c2fbac974c3372e46e027b56922c)
[cf: Changes applied in src/ssl_sock.c]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: mux-h1: Fix handling of responses to CONNECT other than 200-ok
Christopher Faulet [Mon, 22 Feb 2021 07:11:59 +0000 (08:11 +0100)]
BUG/MEDIUM: mux-h1: Fix handling of responses to CONNECT other than 200-ok

For a CONNECT request, if the tunnel establishment is refused by the server,
the connection is always closed on the client side. This happen because we
fail to detect the end of the tunnel. Now, when a reponse other than 200-ok
is received, the request is switch back to MSG_DONE state and the end of the
transaction is handled as a classical request/response exchange.

This patch should fix the issue #1140. It must be backported as far as
2.0. There is no upstream commit ID because tunnel management was already
fixed in a non-backportable way in 2.4.

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

4 years agoBUG/MINOR: server: Be sure to cut the last parsed field of a server-state line
Christopher Faulet [Fri, 19 Feb 2021 15:57:20 +0000 (16:57 +0100)]
BUG/MINOR: server: Be sure to cut the last parsed field of a server-state line

If a line of a server-state file has too many fields, the last one is not
cut on the first following space, as all other fileds. It contains all the
end of the line. It is not the expected behavior. So, now, we cut it on the
next following space, if any. The parsing loop was slighly rewritten.

Note that for now there is no error reported if the line is too long.

This patch may be backported at least as far as 2.1. On 2.0 and prior the
code is not the same. The line parsing is inlined in apply_server_state()
function.

(cherry picked from commit 868a5757e584431fafe713546c8ef8e799865476)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 8c5b974f282729abefb617671e7c5910347555fa)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 61cdb0f1c4ddbe8b7b4882dda767f57d6cf5ecd3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: server: Init params before parsing a new server-state line
Christopher Faulet [Fri, 19 Feb 2021 15:47:11 +0000 (16:47 +0100)]
BUG/MINOR: server: Init params before parsing a new server-state line

Same static arrays of parameters are used to parse all server-state
lines. Thus it is important to reinit them to be sure to not get params from
the previous line, eventually from the previous loaded file.

This patch should be backported to all stable branches. However, in 2.0 and
prior, the parsing of server-state lines are inlined in apply_server_state()
function. Thus the patch will have to be adapted on these versions.

(cherry picked from commit 06cd2569786d31ebb15832cb1053be9aaaadc3f7)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit d119e5671ace438224c51e6603d1e146981a224b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 6018f67036b56c01c2a01f7054ac8128756373d5)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: sample: Always consider zero size string samples as unsafe
Christopher Faulet [Thu, 18 Feb 2021 09:22:48 +0000 (10:22 +0100)]
BUG/MINOR: sample: Always consider zero size string samples as unsafe

smp_is_safe() function is used to be sure a sample may be safely
modified. For string samples, a test is performed to verify if there is a
null-terminated byte. If not, one is added, if possible. It means if the
sample is not const and if there is some free space in the buffer, after
data. However, we must not try to read the null-terminated byte if the
string sample is too long (data >= size) or if the size is equal to
zero. This last test was not performed. Thus it was possible to consider a
string sample as safe by testing a byte outside the buffer.

Now, a zero size string sample is always considered as unsafe and is
duplicated when smp_make_safe() is called.

This patch must be backported in all stable versions.

(cherry picked from commit 8dd40fbde9d51cf7bf0ee622a5bc5c1f56048d84)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit f07249871c4e1f710322655bb0ff8bb089207283)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 01f6efed6ba55ff45b51829ad229a9e38be8ecab)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: checks: properly handle wrapping time in __health_adjust()
Willy Tarreau [Wed, 17 Feb 2021 14:15:15 +0000 (15:15 +0100)]
BUG/MINOR: checks: properly handle wrapping time in __health_adjust()

There's an issue when a server state changes, we use an integer comparison
to decide whether or not to reschedule a test instead of using a wrapping
timer comparison. This will cause some health-checks not to be immediately
triggered half of the time, and some unneeded calls to task_queue() to be
performed in other cases.

This bug has always been there as it was introduced with the commit that
added the feature, 97f07b832 ("[MEDIUM] Decrease server health based on
http responses / events, version 3"). This may be backported everywhere.

(cherry picked from commit 64ba5ebadcd5d98e00989d08dfaa3c94c15196c9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 4d45917e6e57c29e9cc7f1a3cb05f6e4212ee842)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 2e432f3a44a6730472732a56f78ed0c7bbb40684)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: session: atomically increment the tracked sessions counter
Willy Tarreau [Tue, 16 Feb 2021 17:08:12 +0000 (18:08 +0100)]
BUG/MINOR: session: atomically increment the tracked sessions counter

In session_count_new() the tracked counter was still incremented with
a "++" outside of any lock, resulting in occasional slightly off values
such as the following:

    # table: foo, type: string, size:1000, used:1
    0xb2a398: key=127.1.2.3 use=0 exp=86398318 sess_cnt=999959 http_req_cnt=1000004

Now with the correct atomic increment:

    # table: foo, type: string, size:1000, used:1
    0x7f82a4026d38: key=127.1.2.3 use=0 exp=86399294 sess_cnt=1000004 http_req_cnt=1000004

This can be backported to 1.8.

(cherry picked from commit 9805859f245f4f59fc3baa098cb349786e21aaba)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 86eb8369d3c584631e6f4f93a2e2cfcba945bee6)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit e78daa425262e64faf520784e318eb2e35e819e7)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: server: Remove RMAINT from admin state when loading server state
Christopher Faulet [Fri, 12 Feb 2021 16:36:08 +0000 (17:36 +0100)]
BUG/MINOR: server: Remove RMAINT from admin state when loading server state

The RMAINT admin state is dynamic and should be remove from the
srv_admin_state parameter when a server state is loaded from a server-state
file. Otherwise an erorr is reported, the server-state line is ignored and
the server state is not updated.

This patch should fix the issue #576. It must be backported as far as 1.8.

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

4 years agoCLEANUP: channel: fix comment in ci_putblk.
Emeric Brun [Mon, 11 Jan 2021 09:30:42 +0000 (10:30 +0100)]
CLEANUP: channel: fix comment in ci_putblk.

The comment is outdated and refer to an old code.

Should be backported until branch 1.5

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

4 years agoBUG/MINOR: server: Don't call fopen() with server-state filepath set to NULL
Christopher Faulet [Fri, 12 Feb 2021 15:31:03 +0000 (16:31 +0100)]
BUG/MINOR: server: Don't call fopen() with server-state filepath set to NULL

When a local server-state file is loaded, if its name is too long, the error
is not properly handled, resulting to a call to fopen() with the "filepath"
variable set to NULL. To fix the bug, when this error occurs, we jump to the
next proxy, via a "continue" statement. And we take case to set "filepath"
variable after the error handling to be sure.

This patch should fix the issue #1111. It must be backported as far as 1.6.

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

4 years agoBUG/MINOR: cfgparse: do not mention "addr:port" as supported on proxy lines
Willy Tarreau [Fri, 12 Feb 2021 12:28:22 +0000 (13:28 +0100)]
BUG/MINOR: cfgparse: do not mention "addr:port" as supported on proxy lines

The very old error message indicating that a proxy name is mandatory
still had a reference to the optional addr:port argument while this one
is explicitly rejected a few lines later since at least 1.9.

This is harmless but confusing. This can be backported to 2.0.

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

4 years agoBUG/MINOR: stats: revert the change on ST_CONVDONE
Willy Tarreau [Fri, 12 Feb 2021 10:49:25 +0000 (11:49 +0100)]
BUG/MINOR: stats: revert the change on ST_CONVDONE

In 2.1, commit ee4f5f83d ("MINOR: stats: get rid of the ST_CONVDONE flag")
introduced a subtle bug. By testing curproxy against defproxy in
check_config_validity(), it tried to eliminate the need for a flag
to indicate that stats authentication rules were already compiled,
but by doing so it left the issue opened for the case where a new
defaults section appears after the two proxies sharing the first
one:

      defaults
          mode http
          stats auth foo:bar

      listen l1
          bind :8080

      listen l2
          bind :8181

      defaults
          # just to break above

This config results in:
  [ALERT] 042/113725 (3121) : proxy 'f2': stats 'auth'/'realm' and 'http-request' can't be used at the same time.
  [ALERT] 042/113725 (3121) : Fatal errors found in configuration.

Removing the last defaults remains OK. It turns out that the cleanups
that followed that patch render it useless, so the best fix is to revert
the change (with the up-to-date flags instead). The flag was marked as
belonging to the config. It's not exact but it's the closest to the
reality, as it's not there to configure the behavior but ti mention
that the config parser did its job.

This could be backported as far as 2.1, but in practice it looks like
nobody ever hit it.

(cherry picked from commit 5bbc676608f654ae76c7a4cc5852a443bfe8bd41)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 6199db2715561bee77bd7c22cc144c6f44050d20)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit ef2c61682b6f8bf383f5c65a89a0e13d15f71eb3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: config: don't pick unset values from last defaults section
Willy Tarreau [Fri, 12 Feb 2021 10:14:35 +0000 (11:14 +0100)]
BUG/MEDIUM: config: don't pick unset values from last defaults section

Since commit 1.3.14 with commit 1fa3126ec ("[MEDIUM] introduce separation
between contimeout, and tarpit + queue"), check_config_validity() looks
at the last defaults section to update all proxies' queue and tarpit
timeouts if they were not set!

This was apparently an attempt to properly set them on the fallback values,
except that the fallback values were taken from the default proxy before
looking at the current proxy itself. The worst part of it is that it might
have randomly worked by accident for some configurations when there was a
single defaults section, but has certainly caused too short queue
expirations once another defaults section was added later in the file with
these explicitly defined.

Let's remove the defproxy part and keep only the curproxy ones. This could
be backported everywhere, the bug has been there for 13 years.

(cherry picked from commit 937c3ead34becd6851572a8280831d760f612a09)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 759b7a594d746271addcb214a1ed275b9dcfb128)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 29934fbb22598389a8b5fb0db817545173ed6550)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoCLEANUP: deinit: release global and per-proxy server-state variables on deinit
Christopher Faulet [Fri, 12 Feb 2021 08:28:13 +0000 (09:28 +0100)]
CLEANUP: deinit: release global and per-proxy server-state variables on deinit

The global server-state base directory and file name are now released on
deinit, as well as per-proxy server-state file name.

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

4 years agoBUG/MINOR: server: Fix server-state-file-name directive
Christopher Faulet [Fri, 12 Feb 2021 08:27:10 +0000 (09:27 +0100)]
BUG/MINOR: server: Fix server-state-file-name directive

Since the beginning, this directive is documented to accept an optional file
name. But it should also be possible to use it without any argument to use
the backend name as file name. However, when no argument is provided, an
error is reported during the configuration parsing requesting an argument, a
file name or "use-backend-name". And This last special argument is not
documented.

So, to respect the documentation and to avoid configuration breakages, all
modes are now supported. If this directive is called with no argument or
with "use-backend-name", the backend name is use as file name for the
server-state file. Otherwise, the provided string is used.

In addition, we take care to release any previously allocated file name in
case this directive is defines multiple times in the same backend. And an
error is reported if more than one argument are defined. Finally, the
documentation is updated accordingly. Sections supporting this directive are
also mentioned.

This patch should be backported as far as 1.6.

(cherry picked from commit 583b6de68aa1a1070ac3b9c5e21605916aed2de0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 05df94b374dbbaeff5a356525a4ad9b1be0ed217)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit efcbd8efd0ea65295a570642fee6d0376c32dcc8)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: backend: hold correctly lock when killing idle conn
Amaury Denoyelle [Thu, 28 Jan 2021 09:16:29 +0000 (10:16 +0100)]
BUG/MINOR: backend: hold correctly lock when killing idle conn

The wrong lock seems to be held when trying to remove another thread
connection if max fd limit has been reached (locking the current thread
instead of the target thread lock).

This could be backported up to 2.0.

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

4 years agoBUG/MINOR: tools: Fix a memory leak on error path in parse_dotted_uints()
Christopher Faulet [Thu, 11 Feb 2021 09:42:41 +0000 (10:42 +0100)]
BUG/MINOR: tools: Fix a memory leak on error path in parse_dotted_uints()

When an invalid character is found during parsing in parse_dotted_uints()
function, the allocated array of uint must be released. This patch fixes a
memory leak on error path during the configuration parsing.

This patch should fix the issue #1106. It should be backported as far as
2.0. Note that, for 2.1 and 2.0, the function is in src/standard.c

(cherry picked from commit 4b524124db9dc6e64b4e0f0882b5fc71d24970e0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 9027d3a6a34a9254bf4c3d642ac1fa886d5c4f23)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 94556d39f1b6d5e4f4ad8715a7b47104a0c636da)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: server: re-align state file fields number
William Dauchy [Mon, 8 Feb 2021 22:53:29 +0000 (23:53 +0100)]
BUG/MINOR: server: re-align state file fields number

Since commit 3169471964fdc49963e63f68c1fd88686821a0c4 ("MINOR: Add
server port field to server state file.") max_fields was not increased
on version number 1. So this patch aims to fix it. This should be
backported as far as v1.8, but the numbering should be adpated depending
on the version: simply increase the field by 1.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
(cherry picked from commit 38cd986c54975add4e14ef0f693dff494e36336d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit b8b2e8fbe57188d175ab56e57a199364d2546410)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit c87b69d2530f318f72e301c86f931c6e9c1ec587)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: mux-h1: Always set CS_FL_EOI for response in MSG_DONE state
Christopher Faulet [Mon, 8 Feb 2021 16:18:01 +0000 (17:18 +0100)]
BUG/MEDIUM: mux-h1: Always set CS_FL_EOI for response in MSG_DONE state

During the message parsing, if in MSG_DONE state, the CS_FL_EOI flag must
always be set on the conn-stream if following conditions are met :

  * It is a response or
  * It is a request but not a protocol upgrade nor a CONNECT.

For now, there is no test on the message type (request or response). Thus
the CS_FL_EOI flag is not set for a response with a "Connection: upgrade"
header but not a 101 response.

This bug was introduced by the commit 3e1748bbf ("BUG/MINOR: mux-h1: Don't
set CS_FL_EOI too early for protocol upgrade requests"). It was backported
as far as 2.0. Thus, this patch must also be backported as far as 2.0.

(cherry picked from commit a22782b597ee9a3bfecb18a66e29633c8e814216)
[cf: context adjustments]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit d50c18f9c78d0ba0ff357c9a8b51941a8cda3441)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 9da31b5bfc5716f123567293fd579e54aff540e1)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: http-ana: Don't increment HTTP error counter on internal errors
Christopher Faulet [Wed, 10 Feb 2021 13:58:01 +0000 (14:58 +0100)]
BUG/MINOR: http-ana: Don't increment HTTP error counter on internal errors

If internal error is reported by the mux during HTTP request parsing, the
HTTP error counter should not be incremented. It should only be incremented
on parsing error to reflect errors caused by clients.

This patch must be backported as far as 2.0. During the backport, the same
must be performed for 408-request-time-out errors.

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

4 years agoBUG/MEDIUM: mux-h2: do not quit the demux loop before setting END_REACHED
Willy Tarreau [Fri, 5 Feb 2021 11:16:01 +0000 (12:16 +0100)]
BUG/MEDIUM: mux-h2: do not quit the demux loop before setting END_REACHED

The demux loop could quit on missing data but the H2_CF_END_REACHED flag
would not be set in this case. This fixes a remaining situation where
previous commit f09612289 ("BUG/MEDIUM: mux-h2: handle remaining read0
cases") could not be sufficient and still leave CLOSE_WAIT. It's harder
to reproduce but was still observed in prod.

Now we quit via the end of the loop which already takes care of shutr.

This should be backported along with the patch above as far as 2.0.

(cherry picked from commit 133aaa9f110f5b78e57a23f9db0553e2978eca0e)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 02bd15b8db6990fd518544aa1a182afa5eaf7a2d)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit a89aea1a1638591dab4aca8d1c2c71571957c74c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: mux-h2: handle remaining read0 cases
Willy Tarreau [Fri, 5 Feb 2021 10:41:46 +0000 (11:41 +0100)]
BUG/MEDIUM: mux-h2: handle remaining read0 cases

Commit 3d4631fec ("BUG/MEDIUM: mux-h2: fix read0 handling on partial
frames") tried to address an issue introduced in commit aade4edc1 where
read0 wasn't properly handled in the middle of a frame. But the fix was
incomplete for two reasons:

  - first, it would set H2_CF_RCVD_SHUT in h2_recv() after detecting
    a read0 but the condition was guarded by h2_recv_allowed() which
    explicitly excludes read0 ;

  - second, h2_process would only call h2_process_demux() when there
    were still data in the buffer, but closing after a short pause to
    leave a buffer empty wouldn't be caught in this case.

This patch fixes this by properly taking care of the received shutdown
and by also waking up h2_process_demux() on an empty buffer if the demux
is not blocked.

Given the patches above were tagged for backporting to 2.0, this one
should be as well.

(cherry picked from commit f09612289f4a6e358524df385473323ea4254883)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 5554e636261a90f672e391e7e90c3089501a386d)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit f58701b2f2a4db22f0eaea6fc3e53e22dcbe11b0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUILD: Makefile: move REGTESTST_TYPE default setting
William Lallemand [Fri, 5 Feb 2021 10:27:54 +0000 (11:27 +0100)]
BUILD: Makefile: move REGTESTST_TYPE default setting

In patch 3bad3d5 ("BUILD: Makefile: exclude broken tests by default"),
the default setting of the REGTESTST_TYPE variable was set in the
Makefile instead of the run-regtests.sh script.

Doing it in the Makefile was breaking the use of this environment
varible with make ( REGTESTS_TYPES=slow,default make reg-tests )

This patch move the default setting from the Makefile to
run-regtests.sh. It also change the documentation in `make
reg-tests-help` about the default value.

This patch should be backported where 3bad3d5 is backported.

(cherry picked from commit c1ddcafdf9b425ff4ac2f1e3f1662a9133fc4cf6)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 99e4ce23d220f620b3bf2b8e2780a9b599ffba91)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 882debd98e6c385b31dad9bbbb0b597a8844f6b4)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: ssl/cli: abort ssl cert is freeing the old store
William Lallemand [Mon, 1 Feb 2021 14:31:00 +0000 (15:31 +0100)]
BUG/MEDIUM: ssl/cli: abort ssl cert is freeing the old store

The "abort ssl cert" command is buggy and removes the current ckch store,
and instances, leading to SNI removal. It must only removes the new one.

This patch also adds a check in set_ssl_cert.vtc and
set_ssl_server_cert.vtc.

Must be backported as far as 2.2.

(cherry picked from commit 8695ce0bae21238eba660438c819797a245be71e)
[wt: dropped reg-tests/ssl/set_ssl_server_cert.vtc]
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 3d6ebec8d3a957e191b70b67fe9682cb79c107c4)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 9b3473811b48e1e86f93004a48621c4637c0a809)
[cf: Backport to 2.1 also required; context adjustment]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: ssl: init tmp chunk correctly in ssl_sock_load_sctl_from_file()
William Lallemand [Wed, 27 Jan 2021 13:58:51 +0000 (14:58 +0100)]
BUG/MINOR: ssl: init tmp chunk correctly in ssl_sock_load_sctl_from_file()

Use chunk_inistr() for a chunk initialisation in
ssl_sock_load_sctl_from_file() instead of a manual initialisation which
was not initialising head.

Fix issue #1073.

Must be backported as far as 2.2

(cherry picked from commit 8d67394f6915c6d2db40bc1e9593fd392827da8d)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 7cdbb5643de32c97f9d7693f140b89f4a4cb9493)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 5df6c0815064a48111d6a35b9ce29677e087a0b0)
[cf: Backport to 2.1 also required; context adjustment]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: xxhash: make sure armv6 uses memcpy()
Willy Tarreau [Thu, 4 Feb 2021 16:02:39 +0000 (17:02 +0100)]
BUG/MINOR: xxhash: make sure armv6 uses memcpy()

There was a special case made to allow ARMv6 to use unaligned accesses
via a cast in xxHash when __ARM_FEATURE_UNALIGNED is defined. But while
ARMv6 (and v7) does support unaligned accesses, it's only for 32-bit
pointers, not 64-bit ones, leading to bus errors when the compiler emits
an ldrd instruction and the input (e.g. a pattern) is not aligned, as in
issue #1035.

Note that v7 was properly using the packed approach here and was safe,
however haproxy versions 2.3 and older use the old r39 xxhash code which
has the same issue for armv7. A slightly different fix is required there,
by using a different definition of packed for 32 and 64 bits.

The problem is really visible when running v7 code on a v8 kernel because
such kernels do not implement alignment trap emulation, and the process
dies when this happens. This is why in the issue above it was only detected
under lxc. The emulation could have been disabled on v7 as well by writing
zero to /proc/cpu/alignment though.

This commit is a backport of xxhash commit a470f2ef ("update default memory
access for armv6").

Thanks to @srkunze for the report and tests, @stgraber for his help on
setting up an easy reproducer outside of lxc, and @Cyan4973 for the
discussion around the best way to fix this. Details and alternate patches
available on https://github.com/Cyan4973/xxHash/issues/490.

(cherry picked from commit 4acb99f8672232753adb36e57b45e80e5bd87783)
[wt: used the different version suitable for backpotring, using the
 distinct packed settings]
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 59ad20e080aa9dd9a197c074b18850b99c94b050)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 5b1f60dd07e35539dacc048d9c4f37922c161ade)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: ssl: check a connection's status before computing a handshake
Willy Tarreau [Tue, 2 Feb 2021 14:42:25 +0000 (15:42 +0100)]
BUG/MEDIUM: ssl: check a connection's status before computing a handshake

As spotted in issue #822, we're having a problem with error detection in
the SSL layer. The problem is that on an overwhelmed machine, accepted
connections can start to pile up, each of them requiring a slow handshake,
and during all this time if the client aborts, the handshake will still be
calculated.

The error controls are properly placed, it's just that the SSL layer
reads records exactly of the advertised size, without having the ability
to encounter a pending connection error. As such if injecting many TLS
connections to a listener with a huge backlog, it's fairly possible to
meet this situation:

  12:50:48.236056 accept4(8, {sa_family=AF_INET, sin_port=htons(62794), sin_addr=inet_addr("127.0.0.1")}, [128->16], SOCK_NONBLOCK) = 1109
  12:50:48.236071 setsockopt(1109, SOL_TCP, TCP_NODELAY, [1], 4) = 0
  (process other connections' handshakes)

  12:50:48.257270 getsockopt(1109, SOL_SOCKET, SO_ERROR, [ECONNRESET], [4]) = 0
  (proof that error was detectable there but this code was added for the PoC)

  12:50:48.257297 recvfrom(1109, "\26\3\1\2\0", 5, 0, NULL, NULL) = 5
  12:50:48.257310 recvfrom(1109, "\1\0\1\3"..., 512, 0, NULL, NULL) = 512

  (handshake calculation taking 700us)

  12:50:48.258004 sendto(1109, "\26\3\3\0z"..., 1421, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = -1 EPIPE (Broken pipe)
  12:50:48.258036 close(1109)             = 0

The situation was amplified by the multi-queue accept code, as it resulted
in many incoming connections to be accepted long before they could be
handled. Prior to this they would have been accepted and the handshake
immediately started, which would have resulted in most of the connections
waiting in the the system's accept queue, and dying there when the client
aborted, thus the error would have been detected before even trying to
pass them to the handshake code.

As a result, with a listener running on a very large backlog, it's possible
to quickly accept tens of thousands of connections and waste time slowly
running their handshakes while they get replaced by other ones.

This patch adds an SO_ERROR check on the connection's FD before starting
the handshake. This is not pretty as it requires to access the FD, but it
does the job.

Some improvements should be made over the long term so that the transport
layers can report extra information with their ->rcv_buf() call, or at the
very least, implement a ->get_conn_status() function to report various
flags such as shutr, shutw, error at various stages, allowing an upper
layer to inquire for the relevance of engaging into a long operation if
it's known the connection is not usable anymore. An even simpler step
could probably consist in implementing this in the control layer.

This patch is simple enough to be backported as far as 2.0.

Many thanks to @ngaugler for his numerous tests with detailed feedback.

(cherry picked from commit 0630038e771d4d08ae726080e2ef240d5ddaba68)
[wt: context adjustments]
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit d287140ade12f0e5cc228929255b3d19c1f6f3cd)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 900dd4cec76daacd7a83fc423d0b8f14ff86e3e9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: stick-table: Always call smp_fetch_src() with a valid arg list
Christopher Faulet [Fri, 29 Jan 2021 09:27:47 +0000 (10:27 +0100)]
BUG/MINOR: stick-table: Always call smp_fetch_src() with a valid arg list

The sample fetch functions must always be called with a valid argument
list. When called by hand, if there is no argument to pass, empty_arg_list must
be used.

In the stick-table code, there are some calls to smp_fetch_src() with NULL as
argument list. It is changed to use empty_arg_list instead. It is not really a
bug because smp_fetch_src() does not use the argument list. But it is an API
bug.

This patch may be backported to all stable branches as a cleanup.

(cherry picked from commit bdbd5db2a50461fceb7fb89329e73d08f90e44fd)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 1945529933f00a37383990035d7257020efef520)
[cf: context adjustments]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 8e720d0fb09c93ca45076ccb23aa11e268ca0e16)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoDOC: management: fix "show resolvers" alphabetical ordering
Willy Tarreau [Fri, 29 Jan 2021 11:01:46 +0000 (12:01 +0100)]
DOC: management: fix "show resolvers" alphabetical ordering

Not sure why it was located between "show ssl" and "show table"...
This should be backported.

(cherry picked from commit 87ef32397101c7bc4783982af952da12c1750e88)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 84ee51a7c6266292bbcdc585eaa1fb4d43553e35)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit f7c201d1cd49ff40552c83e1861ce05d3d15d51d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoMINOR: contrib: Make the wireshark peers dissector compile for more distribs.
Frédéric Lécaille [Tue, 19 Jan 2021 13:33:24 +0000 (14:33 +0100)]
MINOR: contrib: Make the wireshark peers dissector compile for more distribs.

With a 2.6.8 wireshark, this module could not compile because of ws_version.h
missing header. This patch offers the possibility to compile this plugin without
having to include this header. Furthermore with my wireshark version a
"plugin_release" object is required to make it be loaded by wireshark. This is
a string which seems to have to match a dotted string made of you wireshark
major and minor versions.

(cherry picked from commit aab6f7c3e642aa5e298a9d8765b5063abf518bfb)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 9885f4b329dcb8aaacd41ebcffef520cb3d11fb1)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 5172cb0ebbed1ff82acd00a71891c02956025b17)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: config: fix leak on proxy.conn_src.bind_hdr_name
Amaury Denoyelle [Tue, 26 Jan 2021 13:35:22 +0000 (14:35 +0100)]
BUG/MINOR: config: fix leak on proxy.conn_src.bind_hdr_name

Leak for parsing of option usesrc of the source keyword.

This can be backported to 1.8.

(cherry picked from commit 69c5c3ab330584f9c53e2cf7c86af371a84f104d)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 7edf20eb55282d86e78fa1c642975b4b8ffcbec4)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 50c1fa2e4c59e097b10ee4d7fab699e8a8a5e73b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: filters/htx: Fix data forwarding when payload length is unknown
Christopher Faulet [Mon, 25 Jan 2021 11:02:00 +0000 (12:02 +0100)]
BUG/MEDIUM: filters/htx: Fix data forwarding when payload length is unknown

It is only a problem on the response path because the request payload length
it always known. But when a filter is registered to analyze the response
payload, the filtering may hang if the server closes just after the headers.

The root cause of the bug comes from an attempt to allow the filters to not
immediately forward the headers if necessary. A filter may choose to hold
the headers by not forwarding any bytes of the payload. For a message with
no payload but a known payload length, there is always a EOM block to
forward. Thus holding the EOM block for bodyless messages is a good way to
also hold the headers. However, messages with an unknown payload length,
there is no EOM block finishing the message, but only a SHUTR flag on the
channel to mark the end of the stream. If there is no payload when it
happens, there is no payload at all to forward. In the filters API, it is
wrongly detected as a condition to not forward the headers.

Because it is not the most used feature and not the obvious one, this patch
introduces another way to hold the message headers at the begining of the
forwarding. A filter flag is added to explicitly says the headers should be
hold. A filter may choose to set the STRM_FLT_FL_HOLD_HTTP_HDRS flag and not
forwad anything to hold the headers. This flag is removed at each call, thus
it must always be explicitly set by filters. This flag is only evaluated if
no byte has ever been forwarded because the headers are forwarded with the
first byte of the payload.

reg-tests/filters/random-forwarding.vtc reg-test is updated to also test
responses with unknown payload length (with and without payload).

This patch must be backported as far as 2.0.

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

4 years agoBUG/MEDIUM: stats: add missing INF_BUILD_INFO definition
Adis Nezirovic [Fri, 15 Jan 2021 12:12:33 +0000 (13:12 +0100)]
BUG/MEDIUM: stats: add missing INF_BUILD_INFO definition

commit 5a982a71656ce885be4b1d4b90b8db31204788a1 ("MINOR:
contrib/prometheus-exporter: export build_info") is breaking lua
`core.get_info()`.

This patch makes sure build_info is correctly initialised in all cases.

Reviewed-by: William Dauchy <wdauchy@gmail.com>
(cherry picked from commit b62b78be131de1848d71350d369deac07daf448a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit d918314545a1cd3e77447a2e57ede10d7e242a2e)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 3ddec3ee7d344112b4e4fbde317f8886a20d66a0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUILD/MINOR: lua: define _GNU_SOURCE for LLONG_MAX
Bertrand Jacquin [Thu, 21 Jan 2021 21:14:07 +0000 (21:14 +0000)]
BUILD/MINOR: lua: define _GNU_SOURCE for LLONG_MAX

Lua requires LLONG_MAX defined with __USE_ISOC99 which is set by
_GNU_SOURCE, not necessarely defined by default on old compiler/glibc.

  $ make V=1 TARGET=linux-glibc-legacy USE_THREAD= USE_ACCEPT4= USE_PCRE=1 USE_OPENSSL=1 USE_ZLIB=1  USE_LUA=1
  ..
cc -Iinclude -O2 -g -Wall -Wextra -Wdeclaration-after-statement -fwrapv -Wno-strict-aliasing -Wno-unused-label -Wno-sign-compare -Wno-unused-parameter -Wno-missing-field-initializers -DUSE_EPOLL -DUSE_NETFILTER -DUSE_PCRE -DUSE_POLL -DUSE_TPROXY -DUSE_LINUX_TPROXY -DUSE_LINUX_SPLICE -DUSE_LIBCRYPT -DUSE_CRYPT_H -DUSE_GETADDRINFO -DUSE_OPENSSL -DUSE_LUA -DUSE_FUTEX -DUSE_ZLIB -DUSE_CPU_AFFINITY -DUSE_DL -DUSE_RT -DUSE_PRCTL -DUSE_THREAD_DUMP -I/usr/include/openssl101e/ -DUSE_PCRE -I/usr/include -DCONFIG_HAPROXY_VERSION=\"2.4-dev5-73246d-83\" -DCONFIG_HAPROXY_DATE=\"2021/01/21\" -c -o src/hlua.o src/hlua.c
  In file included from /usr/local/include/lua.h:15,
                   from /usr/local/include/lauxlib.h:15,
                   from src/hlua.c:16:
  /usr/local/include/luaconf.h:581:2: error: #error "Compiler does not support 'long long'. Use option '-DLUA_32BITS'   or '-DLUA_C89_NUMBERS' (see file 'luaconf.h' for details)"
  ..
cc -Iinclude -O2 -g -Wall -Wextra -Wdeclaration-after-statement -fwrapv -Wno-strict-aliasing -Wno-unused-label -Wno-sign-compare -Wno-unused-parameter -Wno-missing-field-initializers -DUSE_EPOLL -DUSE_NETFILTER -DUSE_PCRE -DUSE_POLL -DUSE_TPROXY -DUSE_LINUX_TPROXY -DUSE_LINUX_SPLICE -DUSE_LIBCRYPT -DUSE_CRYPT_H -DUSE_GETADDRINFO -DUSE_OPENSSL -DUSE_LUA -DUSE_FUTEX -DUSE_ZLIB -DUSE_CPU_AFFINITY -DUSE_DL -DUSE_RT -DUSE_PRCTL -DUSE_THREAD_DUMP -I/usr/include/openssl101e/ -DUSE_PCRE -I/usr/include -DCONFIG_HAPROXY_VERSION=\"2.4-dev5-73246d-83\" -DCONFIG_HAPROXY_DATE=\"2021/01/21\" -c -o src/hlua_fcn.o src/hlua_fcn.c
  In file included from /usr/local/include/lua.h:15,
                   from /usr/local/include/lauxlib.h:15,
                   from src/hlua_fcn.c:17:
  /usr/local/include/luaconf.h:581:2: error: #error "Compiler does not support 'long long'. Use option '-DLUA_32BITS'   or '-DLUA_C89_NUMBERS' (see file 'luaconf.h' for details)"
  ..

Cc: Thierry Fournier <tfournier@arpalert.org>
(cherry picked from commit f4c12d4da25cd9da14215b0c4ee740d2b2ef762e)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 3dfbd27af6c8a71ee7b35839e2d3b81180107e38)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 81c18c34f4543309aa198116067d9b6d9f31100f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MEDIUM: mux-h2: fix read0 handling on partial frames
Willy Tarreau [Wed, 20 Jan 2021 09:53:13 +0000 (10:53 +0100)]
BUG/MEDIUM: mux-h2: fix read0 handling on partial frames

Since commit aade4edc1 ("BUG/MEDIUM: mux-h2: Don't handle pending read0
too early on streams"), we've met a few cases where an early connection
close wouldn't be properly handled if some data were pending in a frame
header, because the test now considers the buffer's contents before
accepting to report the close, but given that frame headers or preface
are consumed at once, the buffer cannot make progress when it's stuck
at intermediary lengths.

In order to address this, this patch introduces two flags in the h2c
connection to store any reported shutdown and failed parsing. The idea
is that we cannot rely on conn_xprt_read0_pending() in the parser since
it wouldn't consider data pending in the buffer nor intermediary layers,
but we know for certain that after a read0 is reported by the transport
layer in presence of an RD_SH on the connection, no more progress will
be made there. This alone is not sufficient to decide to end processing,
we can only do this once these final data have been submitted to a parser.
Therefore, now when a parser fails on missing data, we check if a read0
has already been reported on this connection, and if so we set a new
END_REACHED flag on the connection to indicate a failure to process the
final data. The h2c_read0_pending() function now simply reports this
flag's status. This way we're certain that the input shutdown is only
considered after the demux attempted to parse the last frame.

Maybe over the long term the subscribe() API should be improved to
synchronously fail when trying to subscribe for an even that will not
happen. This may be an elegant solution that could possibly work across
multiple layers and even muxes, and be usable at a few specific places
where that's needed.

Given the patch above was backported as far as 2.0, this one should be
backported there as well. It is possible that the fcgi mux has the same
issue, but this was not analysed yet.

Thanks to Pierre Cheynier for providing detailed traces allowing to
quickly narrow the problem down, and to Olivier for his analysis.

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

4 years agoBUG/MINOR: mworker: define _GNU_SOURCE for strsignal()
Bertrand Jacquin [Thu, 21 Jan 2021 01:31:46 +0000 (01:31 +0000)]
BUG/MINOR: mworker: define _GNU_SOURCE for strsignal()

glibc < 2.10 requires _GNU_SOURCE in order to make use of strsignal(),
otherwise leading to SEGV at runtime.

  $ make V=1 TARGET=linux-glibc-legacy USE_THREAD= USE_ACCEPT4=
  ..
  src/mworker.c: In function 'mworker_catch_sigchld':
  src/mworker.c:285: warning: implicit declaration of function 'strsignal'
  src/mworker.c:285: warning: pointer/integer type mismatch in conditional expression
  ..

  $ make V=1 reg-tests REGTESTS_TYPES=slow,default
  ..
  ###### Test case: reg-tests/mcli/mcli_start_progs.vtc ######
  ## test results in: "/tmp/haregtests-2021-01-19_15-18-07.n24989/vtc.29077.28f6153d"
  ---- h1    Bad exit status: 0x008b exit 0x0 signal 11 core 128
  ---- h1    Assert error in haproxy_wait(), src/vtc_haproxy.c line 792:  Condition(*(&h->fds[1]) >= 0) not true.  Errno=0 Success
  ..

  $ gdb ./haproxy /tmp/core.0.haproxy.30270
  ..
  Core was generated by `/root/haproxy/haproxy -d -W -S fd@8 -dM -f /tmp/haregtests-2021-01-19_15-18-07.'.
  Program terminated with signal 11, Segmentation fault.
  #0  0x00002aaaab387a10 in strlen () from /lib64/libc.so.6
  (gdb) bt
  #0  0x00002aaaab387a10 in strlen () from /lib64/libc.so.6
  #1  0x00002aaaab354b69 in vfprintf () from /lib64/libc.so.6
  #2  0x00002aaaab37788a in vsnprintf () from /lib64/libc.so.6
  #3  0x00000000004a76a3 in memvprintf (out=0x7fffedc680a0, format=0x5a5d58 "Current worker #%d (%d) exited with code %d (%s)\n", orig_args=0x7fffedc680d0)
      at src/tools.c:3868
  #4  0x00000000004bbd40 in print_message (label=0x58abed "ALERT", fmt=0x5a5d58 "Current worker #%d (%d) exited with code %d (%s)\n", argp=0x7fffedc680d0)
      at src/log.c:1066
  #5  0x00000000004bc07f in ha_alert (fmt=0x5a5d58 "Current worker #%d (%d) exited with code %d (%s)\n") at src/log.c:1109
  #6  0x0000000000534b7b in mworker_catch_sigchld (sh=<value optimized out>) at src/mworker.c:293
  #7  0x0000000000556af3 in __signal_process_queue () at src/signal.c:88
  #8  0x00000000004f6216 in signal_process_queue () at include/haproxy/signal.h:39
  #9  run_poll_loop () at src/haproxy.c:2859
  #10 0x00000000004f63b7 in run_thread_poll_loop (data=<value optimized out>) at src/haproxy.c:3028
  #11 0x00000000004faaac in main (argc=<value optimized out>, argv=0x7fffedc68498) at src/haproxy.c:904

See: https://man7.org/linux/man-pages/man3/strsignal.3.html

Must be backported as far as 2.0.

(cherry picked from commit 25439de1811aeda7bf472f70f40c3fac9725907e)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit c3f67cdac3c23db5d4487130121690ed2c332c68)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 23986696c9856a02b8ae87f52e4b45a441126779)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: mux_h2: missing space between "st" and ".flg" in the "show fd" helper
Willy Tarreau [Wed, 20 Jan 2021 14:50:03 +0000 (15:50 +0100)]
BUG/MINOR: mux_h2: missing space between "st" and ".flg" in the "show fd" helper

That was causing confusing outputs like this one whenan H2S is known:

   1030 : ... last_h2s=0x2ed8390 .id=775 .st=HCR.flg=0x4001 .rxbuf=...
                                                ^^^^

This was introduced by commit ab2ec4540 in 2.1-dev2 so the fix can be
backported as far as 2.1.

(cherry picked from commit ed4464e6c698bfb410610b7f3850cd0fa3331eb1)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 1c694698bc378224165860000f5e41b3301a9be7)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 663c62c31e45eac5b6b0c4b3d4b24d6a8151e5c8)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: peers: Wrong "new_conn" value for "show peers" CLI command.
Frédéric Lécaille [Mon, 18 Jan 2021 14:14:39 +0000 (15:14 +0100)]
BUG/MINOR: peers: Wrong "new_conn" value for "show peers" CLI command.

This counter could be hugely incremented by the peer task responsible of managing
peer synchronizations and reconnections, for instance when a peer is not reachable
there is a period where the appctx is not created. If we receive  stick-table
updates before the peer session (appctx) is instantiated, we reach the code
responsible of incrementing the "new_conn" counter.
With this patch we increment this counter only when we really instantiate a new
peer session thanks to peer_session_create().

May be backported as far as 2.0.

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

4 years agoBUG/MINOR: threads: Fixes the number of possible cpus report for Mac.
David CARLIER [Fri, 15 Jan 2021 08:09:56 +0000 (08:09 +0000)]
BUG/MINOR: threads: Fixes the number of possible cpus report for Mac.

There is no low level api to achieve same as Linux/FreeBSD, we rely
on CPUs available. Without this, the number of threads is just 1 for
Mac while having 8 cores in my M1.

Backporting to 2.1 should be enough if that's possible.

Signed-off-by: David CARLIER <devnexen@gmail.com>
(cherry picked from commit 6a9060189d66ca931984706d5e2a970ed913f457)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 64e563351414f6bdcc9e9f8bc41a13c496ff8ecc)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit c84718cab39b69ba743d927bf3d641bd3ff53fa3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: init: Use a dynamic buffer to set HAPROXY_CFGFILES env variable
Christopher Faulet [Tue, 12 Jan 2021 17:57:38 +0000 (18:57 +0100)]
BUG/MINOR: init: Use a dynamic buffer to set HAPROXY_CFGFILES env variable

The HAPROXY_CFGFILES env variable is built using a static trash chunk, via a
call to get_trash_chunk() function. This chunk is reserved during the whole
configuration parsing. It is far too large to guarantee it will not be
reused during the configuration parsing. And in fact, it happens in the lua
code since the commit f67442efd ("BUG/MINOR: lua: warn when registering
action, conv, sf, cli or applet multiple times"), when a lua script is
loaded.

To fix the bug, we now use a dynamic buffer instead. And we call memprintf()
function to handle both the allocation and the formatting. Allocation errors
at this stage are fatal.

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

(cherry picked from commit 4e36682d51f4e206c63d792a8cde3e669fb8a0d4)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 33edd5101136f9e0a7fecb07e1fb7a32d5c1e9e8)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 9f8149ff4ad693d6a4db2f1dbd9704e1ac1a97ba)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: init: enforce strict-limits when using master-worker
Jerome Magnin [Tue, 12 Jan 2021 19:19:38 +0000 (20:19 +0100)]
BUG/MINOR: init: enforce strict-limits when using master-worker

The strict-limits global option was introduced with commit 0fec3ab7b
("MINOR: init: always fail when setrlimit fails"). When used in
conjuction with master-worker, haproxy will not fail when a setrlimit
fails. This happens because we only exit() if master-worker isn't used.

This patch removes all tests for master-worker mode for all cases covered
by strict-limits scope.

This should be backported from 2.1 onward.
This should fix issue #1042.

Reviewed by William Dauchy <wdauchy@gmail.com>

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

4 years agoBUG/MINOR: sample: Memory leak of sample_expr structure in case of error
Remi Tricot-Le Breton [Tue, 12 Jan 2021 13:55:12 +0000 (14:55 +0100)]
BUG/MINOR: sample: Memory leak of sample_expr structure in case of error

If an errors occurs during the sample expression parsing, the alloced
sample_expr is not freed despite having its main pointer reset.

This fixes GitHub issue #1046.
It could be backported as far as 1.8.

(cherry picked from commit 22e0d9b39cfb238e7bf866b3d78aa9b393004508)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 54c88770d37ce28077dd4a20b8ffbb54a56cca8b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 780316e86b7170fe2951235f9bf214f26502fc14)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>