haproxy-2.3.git
4 years agoBUG/MEDIUM: peers: stop considering ack messages teaching a full resync
Emeric Brun [Thu, 4 Mar 2021 09:27:10 +0000 (10:27 +0100)]
BUG/MEDIUM: peers: stop considering ack messages teaching a full resync

The re-con cursor was updated receiving any ack message
even if we are pushing a complete resync to a peer. This cursor
is reset at the end of the resync but if the connection is broken
during resync, we could re-start at an unwanted point.

With this patch, the peer stops to consider ack messages pushing
a resync since the resync process has is own acknowlegement and
is always restarted from the beginning in case of broken connection.

This patch should be backported on all supported branches ( >= 1.6 )

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

4 years agoBUG/MEDIUM: peers: register last acked value as origin receiving a resync req
Emeric Brun [Wed, 28 Apr 2021 07:49:33 +0000 (09:49 +0200)]
BUG/MEDIUM: peers: register last acked value as origin receiving a resync req

Receiving a resync request, the origins to start the full sync and
to reset after the full resync are mistakenly computed based on
the last update on the table instead of computed based on the
the last update acked by the node requesting the resync.

It could result in disordered or missing updates pushing to the
requester

This patch sets correctly those origins.

This patch should be backported on all supported branches ( >= 1.6 )

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

4 years agoBUG/MEDIUM: peers: initialize resync timer to get an initial full resync
Emeric Brun [Wed, 21 Apr 2021 14:06:35 +0000 (16:06 +0200)]
BUG/MEDIUM: peers: initialize resync timer to get an initial full resync

If a reload is performed and there is no incoming connections
from the old process to push a full resync, the new process
can be stuck waiting indefinitely for this conn and it never tries a
fallback requesting a full resync from a remote peer because the resync
timer was init to TICK_ETERNITY.

This patch forces a reset of the resync timer to default value (5 secs)
if we detect value is TICK_ETERNITY.

This patch should be backported on all supported branches ( >= 1.6 )

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

4 years agoBUG/MINOR: applet: Notify the other side if data were consumed by an applet
Christopher Faulet [Tue, 27 Apr 2021 15:08:10 +0000 (17:08 +0200)]
BUG/MINOR: applet: Notify the other side if data were consumed by an applet

If an applet consumed output data (the amount of output data has changed
between before and after the call to the applet), the producer is
notified. It means CF_WRITE_PARTIAL and CF_WROTE_DATA are set on the output
channel and the opposite stream interface is notified some room was made in
its input buffer. This way, it is no longer the applet responsibility to
take care of it. However, it doesn't matter if the applet does the same.

Said like that, it looks like an improvement not a bug. But it really fixes
a bug in the lua, for HTTP applets. Indeed, applet:receive() and
applet:getline() are buggy for HTTP applets. Data are consumed but the
producer is not notified. It means if the payload is not fully received in
one time, the applet may be blocked because the producer remains blocked (it
is time dependent).

This patch must be backported as far as 2.0 (only for the HTX part).

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

4 years agoBUG/MINOR: htx: Preserve HTX flags when draining data from an HTX message
Christopher Faulet [Thu, 22 Apr 2021 07:43:47 +0000 (09:43 +0200)]
BUG/MINOR: htx: Preserve HTX flags when draining data from an HTX message

When all data of an HTX message are drained, we rely on htx_reset() to
reinit the message state. However, the flags must be preserved. It is, among
other things, important to preserve processing or parsing errors.

This patch must be backported as far as 2.0.

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

4 years agoBUG/MINOR: mux-fcgi: Don't send normalized uri to FCGI application
Christopher Faulet [Mon, 26 Apr 2021 07:38:55 +0000 (09:38 +0200)]
BUG/MINOR: mux-fcgi: Don't send normalized uri to FCGI application

A normalized URI is the internal term used to specify an URI is stored using
the absolute format (scheme + authority + path). For now, it is only used
for H2 clients. It is the default and recommended format for H2 request.
However, it is unusual for H1 servers to receive such URI. So in this case,
we only send the path of the absolute URI. It is performed for H1 servers,
but not for FCGI applications. This patch fixes the difference.

Note that it is not a real bug, because FCGI applications should support
abosolute URI.

Note also a normalized URI is only detected for H2 clients when a request is
received. There is no such test on the H1 side. It means an absolute URI
received from an H1 client will be sent without modification to an H1 server
or a FCGI application.

To make it possible, a dedicated function has been added to get the H1
URI. This function is called by the H1 and the FCGI multiplexer when a
request is sent to a server.

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

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

4 years ago[RELEASE] Released version 2.3.10 v2.3.10
Christopher Faulet [Fri, 23 Apr 2021 16:51:30 +0000 (18:51 +0200)]
[RELEASE] Released version 2.3.10

Released version 2.3.10 with the following main changes :
    - BUILD: backend: fix build breakage in idle conn locking fix
    - BUG/MINOR: tcp: fix silent-drop workaround for IPv6
    - BUILD: tcp: use IPPROTO_IPV6 instead of SOL_IPV6 on FreeBSD/MacOS
    - BUG/MINOR: ssl: Fix update of default certificate
    - BUG/MINOR: ssl: Prevent removal of crt-list line if the instance is a default one
    - BUG/MINOR: http_fetch: make hdr_ip() resistant to empty fields
    - BUG/MINOR: ssl: Add missing free on SSL_CTX in ckch_inst_free
    - REGTESTS: ssl: "set ssl cert" and multi-certificates bundle
    - DOC: Explicitly state only IPv4 are supported by forwardfor/originalto options
    - REGTESTS: ssl: mark set_ssl_cert_bundle.vtc as broken
    - CONTRIB: halog: fix issue with array of type char
    - BUG/MINOR: tools: fix parsing "us" unit for timers
    - DOC: clarify that compression works for HTTP/2
    - MINOR: No longer rely on deprecated sample fetches for predefined ACLs
    - BUG/MEDIUM: sample: Fix adjusting size in field converter
    - DOC: ssl: Certificate hot update only works on fronted certificates
    - BUG/MEDIUM: threads: Ignore current thread to end its harmless period
    - BUG/MINOR: checks: Set missing id to the dummy checks frontend
    - MINOR: logs: Add support of checks as session origin to format lf strings
    - BUG/MINOR: connection: Fix fc_http_major and bc_http_major for TCP connections
    - MINOR: connection: Make bc_http_major compatible with tcp-checks
    - BUG/MINOR: ssl-samples: Fix ssl_bc_* samples when called from a health-check
    - BUG/MINOR: http-fetch: Make method smp safe if headers were already forwarded
    - BUG/MINOR: http_htx: Remove BUG_ON() from http_get_stline() function
    - BUG/MINOR: logs: Report the true number of retries if there was no connection
    - BUG/MINOR: mux-h1: Release idle server H1 connection if data are received
    - BUG/MINOR: server: free srv.lb_nodes in free_server
    - BUG/MAJOR: mux-h2: Properly detect too large frames when decoding headers
    - BUG/MEDIUM: mux-h2: Fix dfl calculation when merging CONTINUATION frames
    - BUG/MEDIUM: config: fix cpu-map notation with both process and threads
    - BUG/MINOR: mworker/init: don't reset nb_oldpids in non-mworker cases
    - BUG/MINOR: mworker: don't use oldpids[] anymore for reload
    - BUG/MEDIUM: mux-h2: Properly handle shutdowns when received with data
    - BUG/MINOR: peers: remove useless table check if initial resync is finished
    - BUG/MEDIUM: peers: re-work connection to new process during reload.
    - BUG/MEDIUM: peers: re-work refcnt on table to protect against flush

4 years agoBUG/MEDIUM: peers: re-work refcnt on table to protect against flush
Emeric Brun [Fri, 23 Apr 2021 15:02:22 +0000 (17:02 +0200)]
BUG/MEDIUM: peers: re-work refcnt on table to protect against flush

In proxy.c, when process is stopping we try to flush tables content
using 'stktable_trash_oldest'. A check on a counter "table->syncing" was
made to verify if there is no pending resync in progress.
But using multiple threads this counter can be increased by an other thread
only after some delay, so the content of some tables can be trashed earlier and
won't be pushed to the new process (after reload, some tables appear reset and
others don't).

This patch re-names the counter "table->syncing" to "table->refcnt" and
the counter is increased during configuration parsing (registering a table to
a peer section) to protect tables during runtime and until resync of a new
process has succeeded or failed.

The inc/dec operations are now made using atomic operations
because multiple peer sections could refer to the same table in futur.

This fix addresses github #1216.

This patch should be backported on all branches multi-thread support (v >= 1.8)

(cherry picked from commit 2cc201f97e409f926b9221fdafe7431877ba1dc6)
Signed-off-by: Emeric Brun <ebrun@haproxy.com>

4 years agoBUG/MEDIUM: peers: re-work connection to new process during reload.
Emeric Brun [Thu, 22 Apr 2021 16:20:37 +0000 (18:20 +0200)]
BUG/MEDIUM: peers: re-work connection to new process during reload.

The peers task handling the "stopping" could wake up multiple
times in stopping state with WOKEN_SIGNAL: the connection to the
local peer initiated on the first processing was immediatly
shutdown by the next processing of the task and the old process
exits considering it is unable to connect. It results on
empty stick-tables after a reload.

This patch checks the flag 'PEERS_F_DONOTSTOP' to know if the
signal is considered and if remote peers connections shutdown
is already done or if a connection to the local peer must be
established.

This patch should be backported on all supported branches (v >= 1.6)

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

4 years agoBUG/MINOR: peers: remove useless table check if initial resync is finished
Emeric Brun [Thu, 22 Apr 2021 16:13:13 +0000 (18:13 +0200)]
BUG/MINOR: peers: remove useless table check if initial resync is finished

The old process checked each table resync status even if
the resync process is finished. This behavior had no known impact
except useless processing and was discovered during debugging on
an other issue.

This patch could be backported in all supported branches (v >= 1.6)
but once again, it has no impact except avoid useless processing.

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

4 years agoBUG/MEDIUM: mux-h2: Properly handle shutdowns when received with data
Christopher Faulet [Fri, 23 Apr 2021 10:25:18 +0000 (12:25 +0200)]
BUG/MEDIUM: mux-h2: Properly handle shutdowns when received with data

The H2_CF_RCVD_SHUT flag is used to report a read0 was encountered. It is
used by the H2 mux to properly handle shutdowns. However, this flag is only
set when no data are received. If it is detected at the socket level when
some data are received, it is not handled. And because the event was
reported on the connection, any other read attempts are blocked. In this
case, we are unable to close the connection and release the mux
immediately. We must wait the mux timeout expires.

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

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

4 years agoBUG/MINOR: mworker: don't use oldpids[] anymore for reload
William Lallemand [Wed, 21 Apr 2021 14:55:34 +0000 (16:55 +0200)]
BUG/MINOR: mworker: don't use oldpids[] anymore for reload

Since commit 3f12887 ("MINOR: mworker: don't use children variable
anymore"), the oldpids array is not used anymore to generate the new -sf
parameters. So we don't need to set nb_oldpids to 0 during the first
start of the master process.

This patch fixes a bug when 2 masters process tries to synchronize their
peers, there is a small chances that it won't work because nb_oldpids
equals 0.

Should be backported as far as 2.0.

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

4 years agoBUG/MINOR: mworker/init: don't reset nb_oldpids in non-mworker cases
William Lallemand [Wed, 21 Apr 2021 14:42:18 +0000 (16:42 +0200)]
BUG/MINOR: mworker/init: don't reset nb_oldpids in non-mworker cases

This bug affects the peers synchronisation code which rely on the
nb_oldpids variable to synchronize the peer from the old PID.

In the case the process is not started in master-worker mode and tries
to synchronize using the peers, there is a small chance that won't work
because nb_oldpids equals 0.

Fix the bug by setting the variable to 0 only in the case of the
master-worker when not reloaded.

It could also be a problem when trying to synchronize the peers between
2 masters process which should be fixed in another patch.

Bug exists since commit 8a361b5 ("BUG/MEDIUM: mworker: don't reuse PIDs
passed to the master").

Sould be backported as far as 1.8.

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

4 years agoBUG/MEDIUM: config: fix cpu-map notation with both process and threads
Amaury Denoyelle [Thu, 15 Apr 2021 14:29:58 +0000 (16:29 +0200)]
BUG/MEDIUM: config: fix cpu-map notation with both process and threads

The application of a cpu-map statement with both process and threads
is broken (P-Q/1 or 1/P-Q notation).

For example, before the fix, when using P-Q/1, proc_t1 would be updated.
Then it would be AND'ed with thread which is still 0 and thus does
nothing.

Another problem is when using 1/1[-Q], thread[0] is defined. But if
there is multiple processes, every processes will use this define
affinity even if it should be applied only to 1st process.

The solution to the fix is a little bit too complex for my taste and
there is maybe a simpler solution but I did not wish to break the
storage of global.cpu_map, as it is quite painful to test all the
use-cases. Besides, this code will probably be clean up when
multiprocess support removed on the future version.

Let's try to explain my logic.

* either haproxy runs in multiprocess or multithread mode. If on
  multiprocess, we should consider proc_t1 (P-Q/1 notation). If on
  multithread, we should consider thread (1/P-Q notation). However
  during parsing, the final number of processes or threads is unknown,
  thus we have to consider the two possibilities.

* there is a special case for the first thread / first process which is
  present in both execution modes. And as a matter of fact cpu-map 1 or
  1/1 notation represents the same thing. Thus, thread[0] and proc_t1[0]
  represents the same thing. To solve this problem, only thread[0] is
  used for this special case.

This fix must be backported up to 2.0.

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

4 years agoBUG/MEDIUM: mux-h2: Fix dfl calculation when merging CONTINUATION frames
Christopher Faulet [Wed, 21 Apr 2021 09:11:21 +0000 (11:11 +0200)]
BUG/MEDIUM: mux-h2: Fix dfl calculation when merging CONTINUATION frames

When header are splitted over several frames, payload of HEADERS and
CONTINUATION frames are merged to form a unique HEADERS frame before
decoding the payload. To do so, info about the current frame are updated
(dff, dfl..) with info of the next one. Here there is a bug when the frame
length (dfl) is update. We must add the next frame length (hdr.dfl) and not
only the amount of data found in the buffer (clen). Because HEADERS frames
are decoded in one pass, dfl value is the whole frame length or 0. nothing
intermediary.

This patch must be backported as far as 2.0.

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

4 years agoBUG/MAJOR: mux-h2: Properly detect too large frames when decoding headers
Christopher Faulet [Wed, 21 Apr 2021 08:39:53 +0000 (10:39 +0200)]
BUG/MAJOR: mux-h2: Properly detect too large frames when decoding headers

In the function decoding payload of HEADERS frames, an internal error is
returned if the frame length is too large. it cannot exceed the buffer
size. The same is true when headers are splitted on several frames. The
payload of HEADERS and CONTINUATION frames are merged and the overall size
must not exceed the buffer size.

However, there is a bug when the current frame is big enough to only have
the space for a part of the header of the next frame. Because, in this case,
we wait for more data, to have the whole frame header. We don't properly
detect that the headers are too large to be stored in one buffer. In fact
the test to trigger this error is not accurate. When the buffer is full, the
error is reported if the frame length exceeds the amount of data in the
buffer. But in reality, an error must be reported when we are unable to
decode the current frame while the buffer is full. Because, in this case, we
know there is no way to change this state.

When the bug happens, the H2 connection is woken up in loop, consumming all
the CPU. But the traffic is not blocked for all that.

This patch must be backported as far as 2.0.

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

4 years agoBUG/MINOR: server: free srv.lb_nodes in free_server
Amaury Denoyelle [Tue, 20 Apr 2021 14:48:22 +0000 (16:48 +0200)]
BUG/MINOR: server: free srv.lb_nodes in free_server

lb_nodes is allocated for servers using lb_chash (balance random or
hash-type consistent).

It can be backported up to 1.8.

(cherry picked from commit fb247946a1b0a6dc6b1e6b27631adf1ef40c0ee3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
[Cf: Patch applied on haproxy.c because servers are freed in deinit()]

4 years agoBUG/MINOR: mux-h1: Release idle server H1 connection if data are received
Christopher Faulet [Mon, 19 Apr 2021 17:57:43 +0000 (19:57 +0200)]
BUG/MINOR: mux-h1: Release idle server H1 connection if data are received

When data are received on an idle H1 connection on server side, we must take
care to release the connection. Most of time, it will be a
408-Request-Time-out response. But, in all cases, these data should never be
processed as a response to a client.

There is no upstream ID because the 2.4 is not affected by this bug. It must
be backported as far as 2.0.

4 years agoBUG/MINOR: logs: Report the true number of retries if there was no connection
Christopher Faulet [Fri, 16 Apr 2021 09:24:20 +0000 (11:24 +0200)]
BUG/MINOR: logs: Report the true number of retries if there was no connection

When the session is aborted before any connection attempt to any server, the
number of connection retries reported in the logs is wrong. It happens
because when the retries counter is not strictly positive, we consider the
max number of retries was reached and the backend retries value is used. It
is obviously wrong when no connectioh was performed.

In fact, at this stage, the retries counter is initialized to 0. But the
backend stream-interface is in the INI state. Once it is set to SI_ST_REQ,
the counter is set to the backend value. And it is the only possible state
transition from INI state. Thus it is safe to rely on it to fix the bug.

This patch must be backported to all stable versions.

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

4 years agoBUG/MINOR: http_htx: Remove BUG_ON() from http_get_stline() function
Christopher Faulet [Thu, 15 Apr 2021 08:25:35 +0000 (10:25 +0200)]
BUG/MINOR: http_htx: Remove BUG_ON() from http_get_stline() function

The http_get_stline() was designed to be called from HTTP analyzers. Thus
before any data forwarding. To prevent any invalid usage, two BUG_ON()
statements were added. However, it is not a good idea because it is pretty
hard to be sure no HTTP sample fetch will never be called outside the
analyzers context. Especially because there is at least one possible area
where it may happens. An HTTP sample fetch may be used inside the unique-id
format string. On the normal case, it is generated in AN_REQ_HTTP_INNER
analyzer. But if an error is reported too early, the id is generated when
the log is emitted.

So, it is safer to remove the BUG_ON() statements and consider the normal
behavior is to return NULL if the first block is not a start-line. Of
course, this means all calling functions must test the return value or be
sure the start-line is really there.

This patch must be backported as far as 2.0.

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

4 years agoBUG/MINOR: http-fetch: Make method smp safe if headers were already forwarded
Christopher Faulet [Thu, 15 Apr 2021 07:28:02 +0000 (09:28 +0200)]
BUG/MINOR: http-fetch: Make method smp safe if headers were already forwarded

When method sample fetch is called, if an exotic method is found
(HTTP_METH_OTHER), when smp_prefetch_htx() is called, we must be sure the
start-line is still there. Otherwise, HAproxy may crash because of a NULL
pointer dereference, for instance if the method sample fetch is used inside
a unique-id format string. Indeed, the unique id may be generated when the
log message is emitted. At this stage, the request channel is empty.

This patch must be backported as far as 2.0. But the bug exists in all
stable versions for the legacy HTTP mode too. Thus it must be adapted to the
legacy HTTP mode and backported to all other stable versions.

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

4 years agoBUG/MINOR: ssl-samples: Fix ssl_bc_* samples when called from a health-check
Christopher Faulet [Wed, 14 Apr 2021 13:49:41 +0000 (15:49 +0200)]
BUG/MINOR: ssl-samples: Fix ssl_bc_* samples when called from a health-check

For all ssl_bc_* sample fetches, the test on the keyword when called from a
health-check is inverted. We must be sure the 5th charater is a 'b' to
retrieve a connection.

This patch must be backported as far as 2.2.

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

4 years agoMINOR: connection: Make bc_http_major compatible with tcp-checks
Christopher Faulet [Wed, 14 Apr 2021 13:46:49 +0000 (15:46 +0200)]
MINOR: connection: Make bc_http_major compatible with tcp-checks

bc_http_major sample fetch now works when it is called from a
tcp-check. When it happens, the session origin is a check. The backend
connection is retrieved from the conn-stream attached to the check.

If required, this path may easily be backported as far as 2.2.

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

4 years agoBUG/MINOR: connection: Fix fc_http_major and bc_http_major for TCP connections
Christopher Faulet [Wed, 14 Apr 2021 13:40:30 +0000 (15:40 +0200)]
BUG/MINOR: connection: Fix fc_http_major and bc_http_major for TCP connections

fc_http_major and bc_http_major sample fetches return the major digit of the
HTTP version used, respectively, by the frontend and the backend
connections, based on the mux. However, in reality, "2" is returned if the
H2 mux is detected, otherwise "1" is inconditionally returned, regardless
the mux used. Thus, if called for a raw TCP connection, "1" is returned.

To fix this bug, we now get the multiplexer flags, if there is one, to be
sure MX_FL_HTX is set.

I guess it was made this way on purpose when the H2 multiplexer was
introduced in the 1.8 and with the legacy HTTP mode there is no other
solution at the connection level. Thus this patch should be backported as
far as 2.2. For the 2.0, it must be evaluated first because of the legacy
HTTP mode.

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

4 years agoMINOR: logs: Add support of checks as session origin to format lf strings
Christopher Faulet [Wed, 14 Apr 2021 12:01:41 +0000 (14:01 +0200)]
MINOR: logs: Add support of checks as session origin to format lf strings

When a log-format string is built from an health-check, the session origin
is the health-check itself and not a connection. In addition, there is no
stream. It means for now some formats are not supported: %s, %sc, %b, %bi,
%bp, %si and %sp.

Thanks to this patch, the session origin is converted to a check. So it is
possible to retrieve the backend and the backend connection. Note this
session have no listener, thus %ft format must be guarded.

This patch is light and standalone, thus it may be backported as far as 2.2
if required. However, because the error is human, it is probably better to
wait a bit to be sure everything is properly protected.

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

4 years agoBUG/MINOR: checks: Set missing id to the dummy checks frontend
Christopher Faulet [Fri, 16 Apr 2021 08:49:07 +0000 (10:49 +0200)]
BUG/MINOR: checks: Set missing id to the dummy checks frontend

The dummy frontend used to create the session of the tcp-checks is
initialized without identifier. However, it is required because this id may
be used without any guard, for instance in log-format string via "%f" or
when fe_name sample fetch is called. Thus, an unset id may lead to crashes.

This patch must be backported as far as 2.2.

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

4 years agoBUG/MEDIUM: threads: Ignore current thread to end its harmless period
Christopher Faulet [Wed, 14 Apr 2021 12:02:25 +0000 (14:02 +0200)]
BUG/MEDIUM: threads: Ignore current thread to end its harmless period

A previous patch was pushed to fix a deadlock when an isolated thread ends
its harmless period (a9a9e9aac ["BUG/MEDIUM: thread: Fix a deadlock if an
isolated thread is marked as harmless"]). But, unfortunately, the fix is
incomplete. The same must be done in the outer loop, in
thread_harmless_end() function. The current thread must be ignored when
threads_want_rdv_mask mask is tested.

This patch must also be backported as far as 2.0.

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

4 years agoDOC: ssl: Certificate hot update only works on fronted certificates
Remi Tricot-Le Breton [Wed, 14 Apr 2021 14:19:28 +0000 (16:19 +0200)]
DOC: ssl: Certificate hot update only works on fronted certificates

The CLI's "set ssl cert" command only works on frontend certificates but
the documentation did not specify this limitations yet.

This patch can be backported to all stable branches.

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

4 years agoBUG/MEDIUM: sample: Fix adjusting size in field converter
Thayne McCombs [Mon, 12 Apr 2021 05:26:59 +0000 (23:26 -0600)]
BUG/MEDIUM: sample: Fix adjusting size in field converter

Adjust the size of the sample buffer before we change the "area"
pointer. The change in size is calculated as the difference between the
original pointer and the new start pointer. But since the
`smp->data.u.str.area` assignment results in `smp->data.u.str.area` and
`start` being the same pointer, we always ended up substracting zero.
This changes it to change the size by the actual amount it changed.

I'm not entirely sure what the impact of this is, but the previous code
seemed wrong.

[wt: from what I can see the only harmful case is when the output is
 converted to a stick-table key, it could result in zeroing past the
 end of the buffer; other cases do not touch beyond ->data]

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

4 years agoMINOR: No longer rely on deprecated sample fetches for predefined ACLs
Christopher Faulet [Thu, 1 Apr 2021 15:24:04 +0000 (17:24 +0200)]
MINOR: No longer rely on deprecated sample fetches for predefined ACLs

Some predefined ACLs were still based on deprecated sample fetches, like
req_proto_http or req_ver. Now, they use non-deprecated sample fetches. In
addition, the usage lines in the configuration manual have been updated to
be more explicit.

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

4 years agoDOC: clarify that compression works for HTTP/2
Julien Pivotto [Mon, 29 Mar 2021 10:41:40 +0000 (12:41 +0200)]
DOC: clarify that compression works for HTTP/2

This patch clarifies that compression also works with HTTP/2. I have
picked the wording "HTTP/1.1 or above" because it is already used
elsewhere in the documentation.

I have tested that compression indeed works in HTTP/2.

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
(cherry picked from commit ff80c82877ee9765b1bde3b03a5d61be8d3bf9d2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

4 years agoBUG/MINOR: tools: fix parsing "us" unit for timers
Thayne McCombs [Fri, 2 Apr 2021 20:12:43 +0000 (14:12 -0600)]
BUG/MINOR: tools: fix parsing "us" unit for timers

Commit c20ad0d8dbd1bb5707bbfe23632415c3062e046c (BUG/MINOR: tools:  make
parse_time_err() more strict on the timer validity) broke parsing the "us"
unit in timers. It caused `parse_time_err()` to return the string "s",
which indicates an error.

Now if the "u" is followed by an "s" we properly continue processing the
time instead of immediately failing.

This fixes #1209. It must be backported to all stable versions.

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

4 years agoCONTRIB: halog: fix issue with array of type char
Willy Tarreau [Fri, 2 Apr 2021 15:47:21 +0000 (17:47 +0200)]
CONTRIB: halog: fix issue with array of type char

I just noticed this in the windows build after moving the file to dev/:

   In file included from include/import/ist.h:32,
                   from include/haproxy/connection-t.h:32,
                   from dev/flags/flags.c:5:
  dev/flags/flags.c: In function `main':
  dev/flags/flags.c:442:20: error: array subscript has type `char' [-Werror=char-subscripts]
    442 |           (isalnum(*err) && toupper(*err) != 'U' && toupper(*err) != 'L'))
        |                    ^~~~
    LD      haproxy
  cc1: all warnings being treated as errors
  make: *** [Makefile:932: dev/flags/flags.o] Error 1
  make: *** Waiting for unfinished jobs....
  Error: Process completed with exit code 2.

Let's just cast it to uchar as is done everywhere else.

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

4 years agoREGTESTS: ssl: mark set_ssl_cert_bundle.vtc as broken
William Lallemand [Fri, 2 Apr 2021 15:01:25 +0000 (17:01 +0200)]
REGTESTS: ssl: mark set_ssl_cert_bundle.vtc as broken

set_ssl_cert_bundle.vtc requires at least OpenSSL 1.1.0 and we don't
have any way to check this when launching the reg-tests suite.

Mark the reg-test as broken since it will fails on old versions of
openSSL and libreSSL.

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

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.

4 years agoREGTESTS: ssl: "set ssl cert" and multi-certificates bundle
William Lallemand [Fri, 2 Apr 2021 13:23:14 +0000 (15:23 +0200)]
REGTESTS: ssl: "set ssl cert" and multi-certificates bundle

This test loads a configuration which uses multi-certificates bundle and
tries to change them over the CLI.

Could be backported as far as 2.2, however the 2.2 version must be
adapted to commit the bundle and not each certificate individually.

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

4 years agoBUG/MINOR: ssl: Add missing free on SSL_CTX in ckch_inst_free
Remi Tricot-Le Breton [Wed, 31 Mar 2021 09:49:27 +0000 (11:49 +0200)]
BUG/MINOR: ssl: Add missing free on SSL_CTX in ckch_inst_free

The ckch instance keeps a reference to its SSL_CTX since commit 358a822
("BUG/MINOR: ssl: Fix update of default certificate") but this instance
was not freed during the instance's deletion.

It can be backported as far as 2.2 and where 358a822 is backported.

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>

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

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

It can be backported as far as 2.2.

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

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

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

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

This should fix GitHub issue #1143.

It can be backported as far as 2.2.

(cherry picked from commit 8218aed90e862ed01c2a8bb9bf81e6d1a90f14db)
[wla: added the ctx field in the ckch_inst which is needed]
Signed-off-by: William Lallemand <wlallemand@haproxy.org>

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>

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>

4 years agoBUILD: backend: fix build breakage in idle conn locking fix
Willy Tarreau [Tue, 30 Mar 2021 16:51:07 +0000 (18:51 +0200)]
BUILD: backend: fix build breakage in idle conn locking fix

The IDLE_CONNS_LOCK is called OTHER_LOCK in 2.3, but as it's only used
by developers when -DDEBUG_THREAD is used, it didn't show up when commit
5e22cf3a9 ("MEDIUM: backend: use a trylock to grab a connection on high
FD counts as well") was backported to 2.3.

There's no upstream commit since it's a pure 2.3 fix.

4 years ago[RELEASE] Released version 2.3.9 v2.3.9
Willy Tarreau [Tue, 30 Mar 2021 16:38:07 +0000 (18:38 +0200)]
[RELEASE] Released version 2.3.9

Released version 2.3.9 with the following main changes :
    - BUG/MEDIUM: mux-h1: make h1_shutw_conn() idempotent
    - MEDIUM: backend: use a trylock to grab a connection on high FD counts as well
    - BUG/MINOR: payload: Wait for more data if buffer is empty in payload/payload_lv
    - BUG/MINOR: stats: Apply proper styles in HTML status page.
    - BUG/MEDIUM: time: make sure to always initialize the global tick

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>

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>

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

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

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

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

instead of :

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

This patch must be backported as far as 2.2.

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

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

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

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

(cherry picked from commit 9b9f8477f8c751e366a526e2177a8aab34c80f6d)
[backported for the same reason as the first one above -- managed to
 crash on a 8c16t Xeon without it; minor ctx adjustments (list vs tree)
 and lock name]
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>

4 years ago[RELEASE] Released version 2.3.8 v2.3.8
Willy Tarreau [Thu, 25 Mar 2021 15:05:47 +0000 (16:05 +0100)]
[RELEASE] Released version 2.3.8

Released version 2.3.8 with the following main changes :
    - MINOR: time: export the global_now variable
    - BUG/MINOR: freq_ctr/threads: make use of the last updated global time
    - BUG/MEDIUM: mux-fcgi: Fix locking of idle_conns lock in the FCGI I/O callback
    - MINOR: time: also provide a global, monotonic global_now_ms timer
    - BUG/MEDIUM: freq_ctr/threads: use the global_now_ms variable
    - BUG/MINOR: protocol: add missing support of dgram unix socket.
    - MINOR/BUG: mworker/cli: do not use the unix_bind prefix for the master CLI socket
    - MEDIUM: lua: Use a per-thread counter to track some non-reentrant parts of lua
    - BUG/MEDIUM: debug/lua: Don't dump the lua stack if not dumpable
    - BUG/MINOR: ssl: Prevent disk access when using "add ssl crt-list"
    - BUILD: ssl: guard ecdh functions with SSL_CTX_set_tmp_ecdh macro
    - MINOR: lua: Slightly improve function dumping the lua traceback
    - BUG/MEDIUM: debug/lua: Use internal hlua function to dump the lua traceback
    - BUG/MEDIUM: lua: Always init the lua stack before referencing the context
    - MINOR: fd: make fd_clr_running() return the remaining running mask
    - MINOR: fd: remove the unneeded running bit from fd_insert()
    - BUG/MEDIUM: fd: do not wait on FD removal in fd_delete()
    - CLEANUP: fd: remove unused fd_set_running_excl()
    - BUG/MEDIUM: fd: Take the fd_mig_lock when closing if no DWCAS is available.
    - BUG/MEDIUM: thread: Fix a deadlock if an isolated thread is marked as harmless
    - MINOR: tools: make url2ipv4 return the exact number of bytes parsed
    - BUG/MINOR: http_fetch: make hdr_ip() reject trailing characters

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>

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>

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>

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

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

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

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

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

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

(cherry picked from commit 6cf13119e2adeeed2520c66d2fc0cea6f6f23acb)
[wt: not strictly needed but will catch offenders if any out-of-tree
 code were to rely on it]
Signed-off-by: Willy Tarreau <w@1wt.eu>

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

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

          thread1                             thread2

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

(cherry picked from commit 6e8e10b4159f4a31e6aaa535e38dc035f8711787)
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>

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>

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>

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

let us use feature macro SSL_CTX_set_tmp_ecdh instead of comparing openssl
version

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

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

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

It can be backported as far as 2.2.

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

4 years agoBUG/MEDIUM: debug/lua: Don't dump the lua stack if not dumpable
Christopher Faulet [Fri, 19 Mar 2021 14:41:08 +0000 (15:41 +0100)]
BUG/MEDIUM: debug/lua: Don't dump the lua stack if not dumpable

When we try to dump the stack of a lua context, if it is not dumpable,
nothing is performed and a message is emitted instead. This happens when a
lua execution was interrupted inside a non-reentrant part.

This patch depends on following commit :

 * MEDIUM: lua: Use a per-thread counter to track some non-reentrant parts of lua

Thanks to this patch, we avoid a possible deadllock if the lua is
interrupted by the watchdog in the lua memory allocator, because realloc()
is not async-signal-safe.

Both patches must be backported as far as 2.0.

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

4 years agoMEDIUM: lua: Use a per-thread counter to track some non-reentrant parts of lua
Christopher Faulet [Fri, 19 Mar 2021 14:16:28 +0000 (15:16 +0100)]
MEDIUM: lua: Use a per-thread counter to track some non-reentrant parts of lua

Some parts of the Lua are non-reentrant. We must be sure to carefully track
these parts to not dump the lua stack when it is interrupted inside such
parts. For now, we only identified the custom lua allocator. If the thread
is interrupted during the memory allocation, we must not try to print the
lua stack wich also allocate memory. Indeed, realloc() is not
async-signal-safe.

In this patch we introduce a thread-local counter. It is incremented before
entering in a non-reentrant part and decremented when exiting. It is only
performed in hlua_alloc() for now.

(cherry picked from commit a61789a1d62fd71c751189faf5371740dd375f33)
[wt: the code uses malloc/realloc/free depending on the inputs in 2.3,
 let's place the counter around these 3 calls]
Signed-off-by: Willy Tarreau <w@1wt.eu>

4 years agoMINOR/BUG: mworker/cli: do not use the unix_bind prefix for the master CLI socket
Eric Salama [Tue, 16 Mar 2021 14:12:17 +0000 (15:12 +0100)]
MINOR/BUG: mworker/cli: do not use the unix_bind prefix for the master CLI socket

If the configuration file contains a 'unix-bind prefix' directive, and
if we use the -S option and specify a UNIX socket path, the path of the
socket will be prepended with the value of the unix-bind prefix.

For instance, if we have 'unix-bind prefix /tmp/sockets/' and we use
'-S /tmp/master-socket' on the command line, we will get this error:

Starting proxy MASTER:
cannot bind UNIX socket (No such file or directory) [/tmp/sockets/tmp/master-socket]

So this patch adds an exception, and will ignore the unix-bind prefix
for the master CLI socket.

This patch can be backported as far as 1.9.

(cherry picked from commit 1b8dacc858d7cb43488fb2ffca053274c571da1d)
[wt: cli_fe was called stats_fe in 2.3 and older]
Signed-off-by: Willy Tarreau <w@1wt.eu>

4 years agoBUG/MINOR: protocol: add missing support of dgram unix socket.
Emeric Brun [Thu, 18 Mar 2021 15:52:17 +0000 (16:52 +0100)]
BUG/MINOR: protocol: add missing support of dgram unix socket.

The proto "uxdg" (UNIX DGRAM) was not declared, causing an error trying
to put a socket unix on "dgram-bind" into a log-forward section.

This patch introduces the missing "uxdg" protocol by adding proto_uxdg.c
which was fully created based on the code available for the other
protocols.

This patch should be backported to version 2.3 and above.

(cherry picked from commit 8af3bb0abf1b808fde2c944d768431377e6a1526)
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>

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>

4 years agoBUG/MEDIUM: mux-fcgi: Fix locking of idle_conns lock in the FCGI I/O callback
Christopher Faulet [Mon, 22 Mar 2021 12:29:52 +0000 (13:29 +0100)]
BUG/MEDIUM: mux-fcgi: Fix locking of idle_conns lock in the FCGI I/O callback

When the commit e388f2fbc ("MEDIUM: muxes: mark idle conns tasklets with
TASK_F_USR1") was backported (commit id b360bb88 on the 2.3), a call to
HA_SPIN_UNLOCK() was missed and not moved in the right code block. Thus it
is possible to unlock the idle_conns lock while it was never acquired.

This patch should fix the issue #1191. It is 2.3 specific, thus there is no
upstream commid id. No backport is needed except if commit b360bb88 is
backported.

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>

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>

4 years ago[RELEASE] Released version 2.3.7 v2.3.7
Christopher Faulet [Tue, 16 Mar 2021 14:49:34 +0000 (15:49 +0100)]
[RELEASE] Released version 2.3.7

Released version 2.3.7 with the following main changes :
    - BUG/MINOR: backend: fix condition for reuse on mode HTTP
    - 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
    - REORG: atomic: reimplement pl_cpu_relax() from atomic-ops.h
    - BUG/MINOR: mt-list: always perform a cpu_relax call on failure
    - MINOR: atomic: add armv8.1-a atomics variant for cas-dw
    - MINOR: atomic: implement a more efficient arm64 __ha_cas_dw() using pairs
    - BUG/MEDIUM: session: NULL dereference possible when accessing the listener
    - MINOR: tasks: refine the default run queue depth
    - MINOR: listener: refine the default MAX_ACCEPT from 64 to 4
    - OPTIM: server: switch the actconn list to an mt-list
    - MINOR: server: move actconns to the per-thread structure
    - MINOR: lb/api: let callers of take_conn/drop_conn tell if they have the lock
    - OPTIM: lb-first: do not take the server lock on take_conn/drop_conn
    - OPTIM: lb-leastconn: do not take the server lock on take_conn/drop_conn
    - OPTIM: lb-leastconn: do not unlink the server if it did not change
    - MINOR: dynbuf: make the buffer wait queue per thread
    - MINOR: dynbuf: use regular lists instead of mt_lists for buffer_wait
    - MINOR: dynbuf: pass offer_buffers() the number of buffers instead of a threshold
    - MINOR: stream: add an "epoch" to figure which streams appeared when
    - MINOR: cli/streams: make "show sess" dump all streams till the new epoch
    - MINOR: streams: use one list per stream instead of a global one
    - MEDIUM: streams: do not use the streams lock anymore
    - MEDIUM: pools: add CONFIG_HAP_NO_GLOBAL_POOLS and CONFIG_HAP_GLOBAL_POOLS
    - MINOR: pools: double the local pool cache size to 1 MB
    - MEDIUM: backend: use a trylock when trying to grab an idle connection
    - MINOR: task: limit the number of subsequent heavy tasks with flag TASK_HEAVY
    - MINOR: ssl: mark the SSL handshake tasklet as heavy
    - BUG/MEDIUM: ssl: properly remove the TASK_HEAVY flag at end of handshake
    - MINOR: task: add an application specific flag to the state: TASK_F_USR1
    - MEDIUM: muxes: mark idle conns tasklets with TASK_F_USR1
    - MINOR: xprt: add new xprt_set_idle and xprt_set_used methods
    - MEDIUM: ssl: implement xprt_set_used and xprt_set_idle to relax context checks
    - MEDIUM: task: remove the tasks_run_queue counter and have one per thread
    - MINOR: task: give the scheduler a bit more flexibility in the runqueue size
    - OPTIM: task: automatically adjust the default runqueue-depth to the threads
    - BUG/MEDIUM: stick-tables: fix ref counter in table entry using multiple http tracksc.
    - BUILD: atomic/arm64: force the register pairs to use in __ha_cas_dw()
    - BUG/MEDIUM: filters: Set CF_FL_ANALYZE on channels when filters are attached
    - BUG/MINOR: tcpcheck: Update .health threshold of agent inside an agent-check
    - 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
    - BUG/MINOR: tcpcheck: Fix double free on error path when parsing tcp/http-check
    - CLEANUP: tcp-rules: add missing actions in the tcp-request error message
    - Revert "BUG/MINOR: resolvers: Only renew TTL for SRV records with an additional record"
    - 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: Unlink DNS resolution to set RMAINT on SRV resolution
    - BUG/MEDIUM: resolvers: Don't set an address-less server as UP
    - BUG/MEDIUM: resolvers: Fix the loop looking for an existing ADD item
    - MINOR: resolvers: new function find_srvrq_answer_record()
    - BUG/MINOR; resolvers: Ignore DNS resolution for expired SRV item
    - BUG/MEDIUM: resolvers: Trigger a DNS resolution if an ADD item is obsolete
    - MINOR: resolvers: Use a function to remove answers attached to a resolution
    - MINOR: resolvers: Purge answer items when a SRV resolution triggers an error
    - MINOR: resolvers: Add function to change the srv status based on SRV resolution
    - MINOR: resolvers: Directly call srvrq_update_srv_state() when possible
    - BUG/MEDIUM: resolvers: Don't release resolution from a requester callbacks
    - BUG/MEDIUM: resolvers: Skip DNS resolution at startup if SRV resolution is set
    - MINOR: resolvers: Use milliseconds for cached items in resolver responses
    - MINOR: resolvers: Don't try to match immediatly renewed ADD items
    - BUG/MINOR: resolvers: Add missing case-insensitive comparisons of DNS hostnames

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>

4 years agoMINOR: resolvers: Don't try to match immediatly renewed ADD items
Christopher Faulet [Fri, 12 Mar 2021 15:42:45 +0000 (16:42 +0100)]
MINOR: resolvers: Don't try to match immediatly renewed ADD items

The loop looking for existing ADD items to renew their last_seen must ignore
the items already renewed in the same loop. To do so, we rely on the
last_seen time. because it is now based on now_ms, it is safe.

Doing so avoid to match several time the same ADD item when the same IP
address is found in several ADD item. This reduces the number of extra DNS
resolutions.

This patch depends on "MINOR: resolvers: Use milliseconds for cached items
in resolver responses". Both may be backported as far as 2.2 if necessary.

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

4 years agoMINOR: resolvers: Use milliseconds for cached items in resolver responses
Christopher Faulet [Thu, 11 Mar 2021 08:36:05 +0000 (09:36 +0100)]
MINOR: resolvers: Use milliseconds for cached items in resolver responses

The last time when an item was seen in a resolver responses is now stored in
milliseconds instead of seconds. This avoid some corner-cases at the
edges. This also simplifies time comparisons.

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

4 years agoBUG/MEDIUM: resolvers: Skip DNS resolution at startup if SRV resolution is set
Christopher Faulet [Fri, 12 Mar 2021 09:23:05 +0000 (10:23 +0100)]
BUG/MEDIUM: resolvers: Skip DNS resolution at startup if SRV resolution is set

At startup, if a SRV resolution is set for a server, no DNS resolution is
created. We must wait the first SRV resolution to know if it must be
triggered. It is important to do so for two reasons.

First, during a "classical" startup, a server based on a SRV resolution has
no hostname. Thus the created DNS resolution is useless. Best waiting the
first SRV resolution. It is not really a bug at this stage, it is just
useless.

Second, in the same situation, if the server state is loaded from a file,
its hosname will be set a bit later. Thus, if there is no additionnal record
for this server, because there is already a DNS resolution, it inhibits any
new DNS resolution. But there is no hostname attached to the existing DNS
resolution. So no resolution is performed at all for this server.

To avoid any problem, it is fairly easier to handle this special case during
startup. But this means we must be prepared to have no "resolv_requester"
field for a server at runtime.

This patch must be backported as far as 2.2.

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

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>

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>

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>

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>

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>

4 years agoBUG/MEDIUM: resolvers: Trigger a DNS resolution if an ADD item is obsolete
Christopher Faulet [Wed, 10 Mar 2021 17:38:37 +0000 (18:38 +0100)]
BUG/MEDIUM: resolvers: Trigger a DNS resolution if an ADD item is obsolete

When a ADD item attached to a SRV item is removed because it is obsolete, we
must trigger a DNS resolution to be sure the hostname still resolves or
not. There is no other way to be the entry is still valid. And we cannot set
the server in RMAINT immediatly, because a DNS server may be inconsitent and
may stop to add some additionnal records.

The opposite is also true. If a valid ADD item is still attached to a SRV
item, any DNS resolution must be stopped. There is no reason to perform
extra resolution in this case.

This patch must be backported as far as 2.2.

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

4 years agoBUG/MINOR; resolvers: Ignore DNS resolution for expired SRV item
Christopher Faulet [Wed, 10 Mar 2021 14:07:27 +0000 (15:07 +0100)]
BUG/MINOR; resolvers: Ignore DNS resolution for expired SRV item

If no ADD item is found for a SRV item in a SRV response, a DNS resolution
is triggered. When it succeeds, we must be sure the SRV item is still
alive. Otherwise the DNS resolution must be ignored.

This patch depends on the commit "MINOR: resolvers: Move last_seen time of
an ADD into its corresponding SRV item". Both must be backported as far as
2.2.

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

4 years agoMINOR: resolvers: new function find_srvrq_answer_record()
Baptiste Assmann [Wed, 10 Mar 2021 11:22:18 +0000 (12:22 +0100)]
MINOR: resolvers: new function find_srvrq_answer_record()

This function search for a SRV answer item associated to a requester
whose type is server.
This is mainly useful to "link" a server to its SRV record when no
additional record were found to configure the IP address.

This patch is required by a bug fix.

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

4 years agoBUG/MEDIUM: resolvers: Fix the loop looking for an existing ADD item
Christopher Faulet [Wed, 10 Mar 2021 14:19:57 +0000 (15:19 +0100)]
BUG/MEDIUM: resolvers: Fix the loop looking for an existing ADD item

For each ADD item found in a SRV response, we try to find a corresponding
ADD item already attached to an existing SRV item. If found, the ADD
last_seen time is updated, otherwise we try to find a SRV item with no ADD
to attached the new one.

However, the loop is buggy. Instead of comparing 2 ADD items, it compares
the new ADD item with the SRV item. Because of this bug, we are unable to
renew last_seen time of existing ADD.

This patch must be backported as far as 2.2.

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

4 years agoBUG/MEDIUM: resolvers: Don't set an address-less server as UP
Christopher Faulet [Wed, 10 Mar 2021 14:34:52 +0000 (15:34 +0100)]
BUG/MEDIUM: resolvers: Don't set an address-less server as UP

when a server status is updated based on a SRV item, it is always set to UP,
regardless it has an IP address defined or not. For instance, if only a SRV
item is received, with no additional record, only the server hostname is
defined. We must wait to have an IP address to set the server as UP.

This patch must be backported as far as 2.2.

(cherry picked from commit ab177ac1f31b5c6c2e4f7d023923dd668cce3076)
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>

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>

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>

4 years agoRevert "BUG/MINOR: resolvers: Only renew TTL for SRV records with an additional record"
Christopher Faulet [Wed, 10 Mar 2021 14:54:14 +0000 (15:54 +0100)]
Revert "BUG/MINOR: resolvers: Only renew TTL for SRV records with an additional record"

This reverts commit a331a1e8eb2ad4750711a477ca3e22d940495faf.

This commit fixes a real bug, but it also reveals some hidden bugs, mostly
because of some design issues. Thus, in itself, it create more problem than
it solves. So revert it for now. All known bugs will be addressed in next
commits.

This patch should be backported as far as 2.2.

(cherry picked from commit 5037c06d91e0ed748932fcfaae11eeadf5949957)
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>

4 years agoBUG/MINOR: tcpcheck: Fix double free on error path when parsing tcp/http-check
Christopher Faulet [Fri, 12 Mar 2021 11:00:14 +0000 (12:00 +0100)]
BUG/MINOR: tcpcheck: Fix double free on error path when parsing tcp/http-check

When a "tcp-check" or a "http-check" rule is parsed, we try to get the
previous rule in the ruleset to get its index. We must take care to reset
the pointer on this rule in case an error is triggered later on the
parsing. Otherwise, the same rule may be released twice. For instance, it
happens with such line :

    http-check meth GET uri / ## note there is no "send" parameter

This patch must be backported as far as 2.2.

(cherry picked from commit cd03be73d59ad2c5d9ad43b28a1b0fda9f32526c)
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.

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>

4 years agoBUG/MINOR: tcpcheck: Update .health threshold of agent inside an agent-check
Christopher Faulet [Fri, 12 Mar 2021 08:06:07 +0000 (09:06 +0100)]
BUG/MINOR: tcpcheck: Update .health threshold of agent inside an agent-check

If an agent-check is configured for a server, When the response is parsed,
the .health threshold of the agent must be updated on up/down/stopped/fail
command and not the threshold of the health-check. Otherwise, the
agent-check will compete with the health-check and may mark a DOWN server as
UP.

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

(cherry picked from commit 24ec9434271345857b42cc5bd9c6b497ab01a7e4)
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>

4 years agoBUILD: atomic/arm64: force the register pairs to use in __ha_cas_dw()
Willy Tarreau [Fri, 12 Mar 2021 05:06:14 +0000 (06:06 +0100)]
BUILD: atomic/arm64: force the register pairs to use in __ha_cas_dw()

Since commit f8fb4f75f ("MINOR: atomic: implement a more efficient arm64
__ha_cas_dw() using pairs"), on some modern arm64 (armv8.1+) compiled
with -march=armv8.1-a under gcc-7.5.0, a build error may appear on ev_poll.o :

  /tmp/ccHD2lN8.s:1771: Error: reg pair must start from even reg at operand 1 -- `casp x27,x28,x22,x23,[x12]'
  Makefile:927: recipe for target 'src/ev_poll.o' failed

It appears that the compiler cannot always assign register pairs there
for a structure made of two u64. It was possibly later addressed since
gcc-9.3 never caused this, but there's no trivially available info on
the subject in the changelogs. Unsuprizingly, using a u128 instead does
fix this, but it significantly inflates the code (+4kB for just 6 places,
very likely that it loaded some extra stubs) and the comparison is ugly,
involving two slower conditional jumps instead of a single one and a
conditional comparison. For example, ha_random64() grew from 144 bytes
to 232.

However, simply forcing the base register does work pretty well, and
makes the code even cleaner and more efficient by further reducing it
by about 4.5kB, possibly because it helps the compiler to pick suitable
registers for the pair there. And the perf on 64-cores looks steadily
0.5% above the previous one, so let's do this.

Note that the commit above was backported to 2.3 to fix scalability
issues on AWS Graviton2 platform, so this one will need to be as well.

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

4 years agoBUG/MEDIUM: stick-tables: fix ref counter in table entry using multiple http tracksc.
Emeric Brun [Wed, 10 Mar 2021 15:58:03 +0000 (16:58 +0100)]
BUG/MEDIUM: stick-tables: fix ref counter in table entry using multiple http tracksc.

Setting multiple http-request track-scX rules generates entries
which never expires.

If there was already an entry registered by a previous http rule
'stream_track_stkctr(&s->stkctr[rule->action], t, ts)' didn't
register the new 'ts' into the stkctr. And function is left
with no reference on 'ts' whereas refcount had been increased
by the '_get_entry'

The patch applies the same policy as the one showed on tcp track
rules and if there is successive rules the track counter keep the
first entry registered in the counter and nothing more is computed.

After validation this should be backported in all versions.

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

4 years agoOPTIM: task: automatically adjust the default runqueue-depth to the threads
Willy Tarreau [Wed, 10 Mar 2021 10:06:26 +0000 (11:06 +0100)]
OPTIM: task: automatically adjust the default runqueue-depth to the threads

The recent default runqueue size reduction appeared to have significantly
lowered performance on low-thread count configs. Testing various values
runqueue values on different workloads under thread counts ranging from
1 to 64, it appeared that lower values are more optimal for high thread
counts and conversely. It could even be drawn that the optimal value for
various workloads sits around 280/sqrt(nbthread), and probably has to do
with both the L3 cache usage and how to optimally interlace the threads'
activity to minimize contention. This is much easier to optimally
configure, so let's do this by default now.

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