]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
4 years agoBUG/MINOR: threads: work around a libgcc_s issue with chrooting
Willy Tarreau [Wed, 2 Sep 2020 06:04:35 +0000 (08:04 +0200)] 
BUG/MINOR: threads: work around a libgcc_s issue with chrooting

Sander Hoentjen reported another issue related to libgcc_s in issue #671.
What happens is that when the old process quits, pthread_exit() calls
something from libgcc_s.so after the process was chrooted, and this is
the first call to that library, causing an attempt to load it. In a
chroot, this fails, thus libthread aborts. The behavior widely differs
between operating systems because some decided to use a static build for
this library.

In 2.2 this was resolved as a side effect of a workaround for the same issue
with the backtrace() call, which is also in libgcc_s. This was in commit
0214b45 ("MINOR: debug: call backtrace() once upon startup"). But backtraces
are not necessarily enabled, and we need something for older versions.

By inspecting a significant number of ligcc_s on various gcc versions and
platforms, it appears that a few functions have been present since gcc 3.0,
one of which, _Unwind_Find_FDE() has no side effect (it only returns a
pointer). What this patch does is that in the thread initialization code,
if built with gcc >= 3.0, a call to this function is made in order to make
sure that libgcc_s is loaded at start up time and that there will be no
need to load it upon exit.

An easy way to check which libs are loaded under Linux is :

  $ strace -e trace=openat ./haproxy -v

With this patch applied, libgcc_s now appears during init.

Sander confirmed that this patch was enough to put an end to the core
dumps on exit in 2.0, so this patch should be backported there, and maybe
as far as 1.8.

4 years agoREGTEST: increase some short timeouts to make tests more reliable
Willy Tarreau [Wed, 2 Sep 2020 05:26:08 +0000 (07:26 +0200)] 
REGTEST: increase some short timeouts to make tests more reliable

A few regtests continue to regularly fail in highly loaded VMs because
they have very short timeouts. Actually the goal of running with short
timeouts was to make sure we do not uselessly wait during tests designed
to trigger them, but these timeouts here are never supposed to fire at
all, so they don't need to be kept in the 15-20ms range. They do not
pose any issue on any regular machine, but VMs are often suffering from
huge time jumps and cannot always produce responses in that short of a
time.

Just like with commit ce6fc25b1 ("REGTEST: increase timeouts on the
seamless-reload test"), let's raise these short timeouts to 1 second.
A few other ones remain set to 150-200ms and do not seem to cause any
issue. Some are actually expected to trigger so let's not touch them
for now.

4 years agoCLEANUP: http: silence a cppcheck warning in get_http_auth()
Willy Tarreau [Wed, 2 Sep 2020 05:08:47 +0000 (07:08 +0200)] 
CLEANUP: http: silence a cppcheck warning in get_http_auth()

In issue #777, cppcheck wrongly assumes a useless null pointer check
in the expression below while it's obvious that in a 3G/1G split on
32-bit, len can become positive if p is NULL:

     p = memchr(ctx.value.ptr, ' ', ctx.value.len);
     len = p - ctx.value.ptr;
     if (!p || len <= 0)
           return 0;

In addition, on 64 bits you never know given that len is a 32-bit signed
int thus the sign of the result in case of a null p will always be the
opposite of the 32th bit of ctx.value.ptr. Admittedly the test is ugly.

Tim proposed this fix consisting in checking for p == ctx.value.ptr
instead when checking for first character only, which Ilya confirmed is
enough to shut cppcheck up. No backport is needed.

4 years agoBUG/MEDIUM: contrib/spoa-server: Fix ipv4_address used instead of ipv6_address
Gilchrist Dadaglo [Mon, 24 Aug 2020 19:21:35 +0000 (19:21 +0000)] 
BUG/MEDIUM: contrib/spoa-server: Fix ipv4_address used instead of ipv6_address

Typo leads to checking the wrong object for memory issues

This patch must be backported as far as 2.0.

4 years agoBUG/MINOR: contrib/spoa-server: Updating references to free in case of failure
Gilchrist Dadaglo [Mon, 24 Aug 2020 19:21:34 +0000 (19:21 +0000)] 
BUG/MINOR: contrib/spoa-server: Updating references to free in case of failure

When we encounter a failure, all previously borrowed references should
be freed. Especially if the program is not failing immediately

This patch must be backported as far as 2.0.

4 years agoBUG/MINOR: contrib/spoa-server: Do not free reference to NULL
Gilchrist Dadaglo [Mon, 24 Aug 2020 19:21:33 +0000 (19:21 +0000)] 
BUG/MINOR: contrib/spoa-server: Do not free reference to NULL

As per https://docs.python.org/3/c-api/refcounting.html, Py_DECREF
should not be called on NULL objects.

This patch must be backported as far as 2.0.

4 years agoBUG/MINOR: contrib/spoa-server: Ensure ip address references are freed
Gilchrist Dadaglo [Mon, 24 Aug 2020 19:21:32 +0000 (19:21 +0000)] 
BUG/MINOR: contrib/spoa-server: Ensure ip address references are freed

IP addresses references passed in argument for ps_python are not freed after
they have been used. Leading to a small chance of mem leak if a lot of ip
addresses are passed around

This patch must be backported as far as 2.0.

4 years agoBUG/MAJOR: contrib/spoa-server: Fix unhandled python call leading to memory leak
Gilchrist Dadaglo [Mon, 24 Aug 2020 19:21:31 +0000 (19:21 +0000)] 
BUG/MAJOR: contrib/spoa-server: Fix unhandled python call leading to memory leak

The result from spoa evaluation of the user provided python code is
never passed back to the main spoa process nor freed.
Same for the keyword list passed.
This results into the elements never freed by Python as reference count
never goes down.
https://docs.python.org/3/extending/extending.html#reference-counting-in-python

This patch must be backported as far as 2.0.

4 years agoMINOR: contrib/spoa-server: allow MAX_FRAME_SIZE override
Bertrand Jacquin [Mon, 24 Aug 2020 19:21:30 +0000 (19:21 +0000)] 
MINOR: contrib/spoa-server: allow MAX_FRAME_SIZE override

MAX_FRAME_SIZE is forced to the default value of tune.bufsize, however
they don't necessarily have to be tight together.

4 years agoMINOR: http-htx: Handle an optional reason when replacing the response status
Christopher Faulet [Mon, 31 Aug 2020 14:43:34 +0000 (16:43 +0200)] 
MINOR: http-htx: Handle an optional reason when replacing the response status

When calling the http_replace_res_status() function, an optional reason may now
be set. It is ignored if it points to NULL and the original reason is
preserved. Only the response status is replaced. Otherwise both the status and
the reason are replaced.

It simplifies the API and most of time, avoids an extra call to
http_replace_res_reason().

4 years agoBUG/MINOR: http-rules: Replace path and query-string in "replace-path" action
Christopher Faulet [Mon, 31 Aug 2020 14:27:42 +0000 (16:27 +0200)] 
BUG/MINOR: http-rules: Replace path and query-string in "replace-path" action

The documentation stated the "replace-path" action replaces the path, including
the query-string if any is present. But in the code, only the path is
replaced. The query-string is preserved. So, now, instead of relying on the same
action code than "set-uri" action (1), a new action code (4) is used for
"replace-path" action. In http_req_replace_stline() function, when the action
code is 4, we call http_replace_req_path() setting the last argument (with_qs)
to 1. This way, the query-string is not skipped but included to the path to be
replaced.

This patch relies on the commit b8ce505c6 ("MINOR: http-htx: Add an option to
eval query-string when the path is replaced"). Both must be backported as far as
2.0. It should fix the issue #829.

4 years agoMINOR: http-htx: Add an option to eval query-string when the path is replaced
Christopher Faulet [Mon, 31 Aug 2020 14:11:57 +0000 (16:11 +0200)] 
MINOR: http-htx: Add an option to eval query-string when the path is replaced

The http_replace_req_path() function now takes a third argument to evaluate the
query-string as part of the path or to preserve it. If <with_qs> is set, the
query-string is replaced with the path. Otherwise, only the path is replaced.

This patch is mandatory to fix issue #829. The next commit depends on it. So be
carefull during backports.

4 years agoBUG/MEDIUM: http-ana: Don't wait to send 1xx responses received from servers
Christopher Faulet [Mon, 31 Aug 2020 09:07:07 +0000 (11:07 +0200)] 
BUG/MEDIUM: http-ana: Don't wait to send 1xx responses received from servers

When an informational response (1xx) is received, we must be sure to send it
ASAP. To do so, CF_SEND_DONTWAIT flag must be set on the response channel to
instruct the stream-interface to not set the CO_SFL_MSG_MORE flag on the
transport layer. Otherwise the response delivery may be delayed, because of the
commit 8945bb6c0 ("BUG/MEDIUM: stream-int: fix loss of CO_SFL_MSG_MORE flag in
forwarding").

Note that a previous patch (cf6898cd ["BUG/MINOR: http-ana: Don't wait to send
1xx responses generated by HAProxy"]) add this flag on 1xx responses generated
by HAProxy but not on responses coming from servers.

This patch must be backported to 2.2 and may be backported as far as 1.9, for
HTX part only. But this part has changed in the 2.2, so it may be a bit
tricky. Note it does not fix any known bug on 2.1 and below because the
CO_SFL_MSG_MORE flag is ignored by the h1 mux.

4 years agoBUILD: sock_unix: fix build issue with isdigit()
Willy Tarreau [Sat, 29 Aug 2020 04:44:37 +0000 (06:44 +0200)] 
BUILD: sock_unix: fix build issue with isdigit()

Commit 0d06df6 ("MINOR: sock: introduce sock_inet and sock_unix")
made use of isdigit() on the UNIX socket path without casting the
value to unsigned char, breaking the build on cygwin and possibly
other platforms. No backport is needed.

4 years agoMINOR: sock: distinguish dgram from stream types when retrieving old sockets
Willy Tarreau [Fri, 28 Aug 2020 17:20:23 +0000 (19:20 +0200)] 
MINOR: sock: distinguish dgram from stream types when retrieving old sockets

For now we still don't retrieve dgram sockets, but the code must be able
to distinguish them before we switch to receivers. This adds a new flag
to the xfer_sock_list indicating that a socket is of type SOCK_DGRAM. The
way to set the flag for now is by looking at the dummy address family which
equals AF_CUST_UDP{4,6} in this case (given that other dgram sockets are not
yet supported).

4 years agoMINOR: sock: do not use LI_O_* in xfer_sock_list anymore
Willy Tarreau [Fri, 28 Aug 2020 17:09:19 +0000 (19:09 +0200)] 
MINOR: sock: do not use LI_O_* in xfer_sock_list anymore

We'll want to store more info there and some info that are not represented
in listener options at the moment (such as dgram vs stream) so let's get
rid of these and instead use a new set of options (SOCK_XFER_OPT_*).

4 years agoREORG: sock: move get_old_sockets() from haproxy.c
Willy Tarreau [Fri, 28 Aug 2020 16:42:45 +0000 (18:42 +0200)] 
REORG: sock: move get_old_sockets() from haproxy.c

The new function was called sock_get_old_sockets() and was left as-is
except a minimum amount of style lifting to make it more readable. It
will never be awesome anyway since it's used very early in the boot
sequence and needs to perform socket I/O without any external help.

4 years agoMINOR: sock_inet: move the IPv4/v6 transparent mode code to sock_inet
Willy Tarreau [Fri, 28 Aug 2020 15:23:40 +0000 (17:23 +0200)] 
MINOR: sock_inet: move the IPv4/v6 transparent mode code to sock_inet

This code was highly redundant, existing for TCP clients, TCP servers
and UDP servers. Let's move it to sock_inet where it belongs. The new
functions are sock_inet4_make_foreign() and sock_inet6_make_foreign().

4 years agoMINOR: sock: implement sock_find_compatible_fd()
Willy Tarreau [Fri, 28 Aug 2020 14:49:41 +0000 (16:49 +0200)] 
MINOR: sock: implement sock_find_compatible_fd()

This is essentially a merge from tcp_find_compatible_fd() and
uxst_find_compatible_fd() that relies on a listener's address and
compare function and still checks for other variations. For AF_INET6
it compares a few of the listener's bind options. A minor change for
UNIX sockets is that transparent mode, interface and namespace used
to be ignored when trying to pick a previous socket while now if they
are changed, the socket will not be reused. This could be refined but
it's still better this way as there is no more risk of using a
differently bound socket by accident.

Eventually we should not pass a listener there but a set of binding
parameters (address, interface, namespace etc...) which ultimately will
be grouped into a receiver. For now this still doesn't exist so let's
stick to the listener to break dependencies in the rest of the code.

4 years agoMINOR: sock: add interface and namespace length to xfer_sock_list
Willy Tarreau [Fri, 28 Aug 2020 14:35:06 +0000 (16:35 +0200)] 
MINOR: sock: add interface and namespace length to xfer_sock_list

This will ease and speed up comparisons in FD lookups.

4 years agoREORG: listener: move xfer_sock_list to sock.{c,h}.
Willy Tarreau [Fri, 28 Aug 2020 14:29:53 +0000 (16:29 +0200)] 
REORG: listener: move xfer_sock_list to sock.{c,h}.

This will be used for receivers as well thus it is not specific to
listeners but to sockets.

4 years agoREORG: sock_inet: move default_tcp_maxseg from proto_tcp.c
Willy Tarreau [Fri, 28 Aug 2020 16:03:10 +0000 (18:03 +0200)] 
REORG: sock_inet: move default_tcp_maxseg from proto_tcp.c

Let's determine it at boot time instead of doing it on first use. It
also saves us from having to keep it thread local. It's been moved to
the new sock_inet_prepare() function, and the variables were renamed
to sock_inet_tcp_maxseg_default and sock_inet6_tcp_maxseg_default.

4 years agoREORG: sock_inet: move v6only_default from proto_tcp.c to sock_inet.c
Willy Tarreau [Fri, 28 Aug 2020 14:06:01 +0000 (16:06 +0200)] 
REORG: sock_inet: move v6only_default from proto_tcp.c to sock_inet.c

The v6only_default variable is not specific to TCP but to AF_INET6, so
let's move it to the right file. It's now immediately filled on startup
during the PREPARE stage so that it doesn't have to be tested each time.
The variable's name was changed to sock_inet6_v6only_default.

4 years agoREORG: inet: replace tcp_is_foreign() with sock_inet_is_foreign()
Willy Tarreau [Fri, 28 Aug 2020 13:40:33 +0000 (15:40 +0200)] 
REORG: inet: replace tcp_is_foreign() with sock_inet_is_foreign()

The function now makes it clear that it's independent on the socket
type and solely relies on the address family. Note that it supports
both IPv4 and IPv6 as we don't seem to need it per-family.

4 years agoMINOR: sock_inet: implement sock_inet_get_dst()
Willy Tarreau [Fri, 28 Aug 2020 13:19:45 +0000 (15:19 +0200)] 
MINOR: sock_inet: implement sock_inet_get_dst()

This one is common to the TCPv4 and UDPv4 code, it retrieves the
destination address of a socket, taking care of the possiblity that for
an incoming connection the traffic was possibly redirected. The TCP and
UDP definitions were updated to rely on it and remove duplicated code.

4 years agoMINOR: tcp/udp/unix: make use of proto->addrcmp() to compare addresses
Willy Tarreau [Fri, 28 Aug 2020 13:30:11 +0000 (15:30 +0200)] 
MINOR: tcp/udp/unix: make use of proto->addrcmp() to compare addresses

The new addrcmp() protocol member points to the function to be used to
compare two addresses of the same family.

When picking an FD from a previous process, we can now use the address
specific address comparison functions instead of having to rely on a
local implementation. This will help move that code to a more central
place.

4 years agoMINOR: sock: introduce sock_inet and sock_unix
Willy Tarreau [Fri, 28 Aug 2020 13:10:11 +0000 (15:10 +0200)] 
MINOR: sock: introduce sock_inet and sock_unix

These files will regroup everything specific to AF_INET, AF_INET6 and
AF_UNIX socket definitions and address management. Some code there might
be agnostic to the socket type and could later move to af_xxxx.c but for
now we only support regular sockets so no need to go too far.

The files are quite poor at this step, they only contain the address
comparison function for each address family.

4 years agoREORG: sock: start to move some generic socket code to sock.c
Willy Tarreau [Fri, 28 Aug 2020 10:07:22 +0000 (12:07 +0200)] 
REORG: sock: start to move some generic socket code to sock.c

The new file sock.c will contain generic code for standard sockets
relying on file descriptors. We currently have way too much duplication
between proto_uxst, proto_tcp, proto_sockpair and proto_udp.

For now only get_src, get_dst and sock_create_server_socket were moved,
and are used where appropriate.

4 years agoREORG: unix: move UNIX bind/server keywords from proto_uxst.c to cfgparse-unix.c
Willy Tarreau [Fri, 28 Aug 2020 09:54:59 +0000 (11:54 +0200)] 
REORG: unix: move UNIX bind/server keywords from proto_uxst.c to cfgparse-unix.c

Let's finish the cleanup and get rid of all bind and server keywords
parsers from proto_uxst.c. They're now moved to cfgparse-unix.c. Now
proto_uxst.c is clean and only contains code related to binding and
connecting.

4 years agoREORG: tcp: move TCP bind/server keywords from proto_tcp.c to cfgparse-tcp.c
Willy Tarreau [Fri, 28 Aug 2020 09:49:31 +0000 (11:49 +0200)] 
REORG: tcp: move TCP bind/server keywords from proto_tcp.c to cfgparse-tcp.c

Let's continue the cleanup and get rid of all bind and server keywords
parsers from proto_tcp.c. They're now moved to cfgparse-tcp.c, just as
was done for ssl before 2.2 release. Nothing has changed beyond this.
Now proto_tcp.c is clean and only contains code related to binding and
connecting.

4 years agoREORG: tcp: move TCP sample fetches from proto_tcp.c to tcp_sample.c
Willy Tarreau [Fri, 28 Aug 2020 09:37:21 +0000 (11:37 +0200)] 
REORG: tcp: move TCP sample fetches from proto_tcp.c to tcp_sample.c

Let's continue the cleanup and get rid of all sample fetch functions
from proto_tcp.c. They're now moved to tcp_sample.c, just as was done
for ssl before 2.2 release. Nothing has changed beyond this.

4 years agoCLEANUP: tcp: stop exporting smp_fetch_src()
Willy Tarreau [Fri, 28 Aug 2020 09:31:31 +0000 (11:31 +0200)] 
CLEANUP: tcp: stop exporting smp_fetch_src()

This is totally ugly, smp_fetch_src() is exported only so that stick_table.c
can (ab)use it in the {sc,src}_* sample fetch functions. It could be argued
that the sample could have been reconstructed there in place, but we don't
even need to duplicate the code. We'd rather simply retrieve the "src"
fetch's function from where it's used at init time and be done with it.

4 years agoREORG: tcp: move TCP actions from proto_tcp.c to tcp_act.c
Willy Tarreau [Fri, 28 Aug 2020 09:03:28 +0000 (11:03 +0200)] 
REORG: tcp: move TCP actions from proto_tcp.c to tcp_act.c

The file proto_tcp.c has become a real mess because it still contains
tons of definitions that have nothing to do with the TCP protocol setup.
This commit moves the ruleset actions "set-src-port", "set-dst-port",
"set-src", "set-dst", and "silent-drop" to a new file "tcp_act.c".
Nothing has changed beyond this.

4 years agoBUG/MINOR: reload: do not fail when no socket is sent
Willy Tarreau [Fri, 28 Aug 2020 16:45:01 +0000 (18:45 +0200)] 
BUG/MINOR: reload: do not fail when no socket is sent

get_old_sockets() mistakenly sets ret=0 instead of ret2=0 before leaving
when the old process announces zero FD. So it will return an error
instead of success. This must be particularly rare not to have a
single socket to offer though!

A few comments were added to make it more obvious what to expect in
return.

This must be backported to 1.8 since the bug has always been there.

4 years agoDOC: add description of pidfile in master-worker mode
MIZUTA Takeshi [Wed, 26 Aug 2020 04:46:19 +0000 (13:46 +0900)] 
DOC: add description of pidfile in master-worker mode

Previously, pidfile was only described for daemon mode. In the case of
master-worker mode, the handling of pidfile is different from daemon mode,
so the description has been added.

4 years agoMEDIUM: reload: pass all exportable FDs, not just listeners
Willy Tarreau [Wed, 19 Aug 2020 15:03:55 +0000 (17:03 +0200)] 
MEDIUM: reload: pass all exportable FDs, not just listeners

Now we don't limit ourselves to listeners found in proxies nor peers
anymore, we're instead scanning all known FDs for those marked with
".exported=1". Just doing so has significantly simplified the code,
and will later allow to yield while sending FDs if desired.

When it comes to retrieving a possible namespace name or interface
name, for now this is only performed on listeners since these are the
only ones carrying such info. Once this moves somewhere else, we'll
be able to also pass these info for UDP receivers for example, with
only tiny changes.

4 years agoMINOR: fd: add a new "exported" flag and use it for all regular listeners
Willy Tarreau [Wed, 19 Aug 2020 08:00:57 +0000 (10:00 +0200)] 
MINOR: fd: add a new "exported" flag and use it for all regular listeners

This new flag will be used to mark FDs that must be passed to any future
process across the CLI's "_getsocks" command.

The scheme here is quite complex and full of special cases:
  - FDs inherited from parent processes are *not* exported this way, as
    they are supposed to instead be passed by the master process itself
    across reloads. However such FDs ought never to be paused otherwise
    this would disrupt the socket in the parent process as well;

  - FDs resulting from a "bind" performed over a socket pair, which are
    in fact one side of a socket pair passed inside another control socket
    pair must not be passed either. Since all of them are used the same
    way, for now it's enough never to put this "exported" flag to FDs
    bound by the socketpair code.

  - FDs belonging to temporary listeners (e.g. a passive FTP data port)
    must not be passed either. Fortunately we don't have such FDs yet.

  - the rest of the listeners for now are made of TCP, UNIX stream, ABNS
    sockets and are exportable, so they get the flag.

  - UDP listeners were wrongly created as listeners and are not suitable
    here. Their FDs should be passed but for now they are not since the
    client doesn't even distinguish the SO_TYPE of the retrieved sockets.

In addition, it's important to keep in mind that:
  - inherited FDs may never be closed in master process but may be closed
    in worker processes if the service is shut down (useless since still
    bound, but technically possible) ;

  - inherited FDs may not be disabled ;

  - exported FDs may be disabled because the caller will perform the
    subsequent listen() on them. However that might not work for all OSes

  - exported FDs may be closed, it just means the service was shut down
    from the worker, and will be rebound in the new process. This implies
    that we have to disable exported on close().

=> as such, contrary to an apparently obvious equivalence, the "exported"
   status doesn't imply anything regarding the ability to close a
   listener's FD or not.

4 years agoCLEANUP: fd: remove fd_remove() and rename fd_dodelete() to fd_delete()
Willy Tarreau [Wed, 26 Aug 2020 09:54:06 +0000 (11:54 +0200)] 
CLEANUP: fd: remove fd_remove() and rename fd_dodelete() to fd_delete()

This essentially undoes what we did in fd.c in 1.8 to support seamless
reload. Since we don't need to remove an fd anymore we can turn
fd_delete() to the simple function it used to be.

4 years agoMEDIUM: fd: replace usages of fd_remove() with fd_stop_both()
Willy Tarreau [Wed, 26 Aug 2020 09:44:17 +0000 (11:44 +0200)] 
MEDIUM: fd: replace usages of fd_remove() with fd_stop_both()

We used to require fd_remove() to remove an FD from a poller when we
still had the FD cache and it was not possible to directly act on the
pollers. Nowadays we don't need this anymore as the pollers will
automatically unregister disabled FDs. The fd_remove() hack is
particularly problematic because it additionally hides the FD from
the known FD list and could make one think it's closed.

It's used at two places:
  - with the async SSL engine
  - with the listeners (when unbinding from an fd for another process)

Let's just use fd_stop_both() instead, which will propagate down the
stack to do the right thing, without removing the FD from the array
of known ones.

Now when dumping FDs using "show fd" on a process which still knows some
of the other workers' FDs, the FD will properly be listed with a listener
state equal to "ZOM" for "zombie". This guarantees that the FD is still
known and will properly be passed using _getsocks().

4 years agoBUG/MEDIUM: ssl: fix ssl_bind_conf double free w/ wildcards
William Lallemand [Wed, 26 Aug 2020 15:34:44 +0000 (17:34 +0200)] 
BUG/MEDIUM: ssl: fix ssl_bind_conf double free w/ wildcards

The fix 7df5c2d ("BUG/MEDIUM: ssl: fix ssl_bind_conf double free") was
not complete. The problem still occurs when using wildcards in
certificate, during the deinit.

This patch removes the free of the ssl_conf structure in
ssl_sock_free_all_ctx() since it's already done in the crtlist deinit.

It must be backported in 2.2.

4 years agoMEDIUM: reload: stop passing listener options along with FDs
Willy Tarreau [Wed, 26 Aug 2020 08:30:09 +0000 (10:30 +0200)] 
MEDIUM: reload: stop passing listener options along with FDs

During a reload operation, we used to send listener options associated
with each passed file descriptor. These were passed as binary contents
for the size of the "options" field in the struct listener. This means
that any flag value change or field size change would be problematic,
the former failing to properly grab certain options, the latter possibly
causing permanent failures during this operation.

Since these two previous commits:
  MINOR: reload: determine the foreing binding status from the socket
  BUG/MINOR: reload: detect the OS's v6only status before choosing an old socket

we don't need this anymore as the values are determined from the file
descriptor itself.

Let's just turn the previous 32 bits to vestigal space, send them as
zeroes and ignore them on receipt. The only possible side effect is if
someone would want to roll back from a 2.3 to 2.2 or earlier, such options
might be ignored during this reload. But other forthcoming changes might
make this fail as well anyway so that's not a reason for keeping this
behavior.

4 years agoMINOR: reload: determine the foreing binding status from the socket
Willy Tarreau [Wed, 26 Aug 2020 08:23:40 +0000 (10:23 +0200)] 
MINOR: reload: determine the foreing binding status from the socket

Let's not look at the listener options passed by the original process
and determine from the socket itself whether it is configured for
transparent mode or not. This is cleaner and safer, and doesn't rely
on flag values that could possibly change between versions.

4 years agoBUG/MINOR: reload: detect the OS's v6only status before choosing an old socket
Willy Tarreau [Wed, 26 Aug 2020 07:29:21 +0000 (09:29 +0200)] 
BUG/MINOR: reload: detect the OS's v6only status before choosing an old socket

The v4v6 and v6only options are passed as data during the socket transfer
between processes so that the new process can decide whether it wants to
reuse a socket or not. But this actually misses one point: if no such option
is set and the OS defaults are changed between the reloads, then the socket
will still be inherited and will never be rebound using the new options.

This can be seen by starting the following config:

  global
    stats socket /tmp/haproxy.sock level admin expose-fd listeners

  frontend testme
    bind :::1234
    timeout client          2000ms

Having a look at the OS settins, v6only is disabled:

  $ cat /proc/sys/net/ipv6/bindv6only
  0

A first check shows it's indeed bound to v4 and v6:

  $ ss -an -6|grep 1234
  tcp   LISTEN 0      2035                                   *:1234             *:*

Reloading the process doesn't change anything (which is expected). Now let's set
bindv6only:

  $ echo 1 | sudo tee /proc/sys/net/ipv6/bindv6only
  1
  $ cat /proc/sys/net/ipv6/bindv6only
  1

Reloading gives the same state:

  $ ss -an -6|grep 1234
  tcp   LISTEN 0      2035                                   *:1234             *:*

However a restart properly shows a correct bind:

  $ ss -an -6|grep 1234
  tcp   LISTEN 0      2035                                [::]:1234          [::]:*

This one doesn't change once bindv6only is reset, for the same reason.

This patch attacks this problem differently. Instead of passing the two
options at once for each listening fd, it ignores the options and reads
the socket's current state for the IPV6_V6ONLY flag and sets it only.
Then before looking for a compatible FD, it checks the OS's defaults
before deciding which of the v4v6 and v6only needs to be kept on the
listener. And the selection is only made on this.

First, it addresses this issue. Second, it also ensures that if such
options are changed between reloads to identical states, the socket
can still be inherited. For example adding v4v6 when bindv6only is not
set will allow the socket to still be usable. Third, it avoids an
undesired dependency on the LI_O_* bit values between processes across
a reload (for these ones at least).

It might make sense to backport this to some recent stable versions, but
quite frankly the likelyhood that anyone will ever notice it is extremely
faint.

4 years agoMINOR: tcp: don't try to set/clear v6only on inherited sockets
Willy Tarreau [Wed, 26 Aug 2020 08:21:06 +0000 (10:21 +0200)] 
MINOR: tcp: don't try to set/clear v6only on inherited sockets

If a socket was already bound (inherited from a parent or retrieved from
a previous process), there's no point trying to change its IPV6_V6ONLY
state since it will fail. This is visible in strace as an EINVAL during
a reload when passing FDs.

4 years agoMINOR: ssl: Support SAN extension for certificate generation
Shimi Gersner [Sun, 23 Aug 2020 10:58:13 +0000 (13:58 +0300)] 
MINOR: ssl: Support SAN extension for certificate generation

The use of Common Name is fading out in favor of the RFC recommended
way of using SAN extensions. For example, Chrome from version 58
will only match server name against SAN.

The following patch adds SAN extension by default to all generated certificates.
The SAN extension will be of type DNS and based on the server name.

4 years agoMEDIUM: ssl: Support certificate chaining for certificate generation
Shimi Gersner [Sun, 23 Aug 2020 10:58:12 +0000 (13:58 +0300)] 
MEDIUM: ssl: Support certificate chaining for certificate generation

haproxy supports generating SSL certificates based on SNI using a provided
CA signing certificate. Because CA certificates may be signed by multiple
CAs, in some scenarios, it is neccesary for the server to attach the trust chain
in addition to the generated certificate.

The following patch adds the ability to serve the entire trust chain with
the generated certificate. The chain is loaded from the provided
`ca-sign-file` PEM file.

4 years agoBUILD: task: work around a bogus warning in gcc 4.7/4.8 at -O1
Willy Tarreau [Fri, 21 Aug 2020 03:48:34 +0000 (05:48 +0200)] 
BUILD: task: work around a bogus warning in gcc 4.7/4.8 at -O1

As reported in issue #816, when building task.o at -O1 with gcc 4.7 or
4.8, we get the following warning:

    CC      src/task.o
  In file included from include/haproxy/proxy.h:31:0,
                   from include/haproxy/cfgparse.h:27,
                   from src/task.c:19:
  src/task.c: In function 'next_timer_expiry':
  include/haproxy/ticks.h:121:10: warning: 'key' may be used uninitialized in this function [-Wmaybe-uninitialized]
  src/task.c:349:2: note: 'key' was declared here

It is wrong since the condition to use 'key' is exactly the same as
the one used to set it. This warning disappears at -O2 and disappeared
from gcc 5 and above. Let's just initialize 'key' there, it only adds
16 bytes of code and remains cheap enough for this function.

This should be backported to 2.2.

4 years agoBUILD: tools: include auxv a bit later
Willy Tarreau [Thu, 20 Aug 2020 14:39:14 +0000 (16:39 +0200)] 
BUILD: tools: include auxv a bit later

As reported in https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=24745,
haproxy fails to build with TARGET=generic and without extra options due
to auxv.h not being included, since the __GLIBC__ macro is not yet defined.
Let's include it after other libc headers so that the __GLIBC__ definition
is known. Thanks to David and Tim for the diag.

This should be backported to 2.2.

4 years agoMINOR: stats: prevent favicon.ico requests for stats page
zurikus [Tue, 18 Aug 2020 08:16:05 +0000 (11:16 +0300)] 
MINOR: stats: prevent favicon.ico requests for stats page

Haproxy stats page don't have a favicon.ico, but browsers always makes a request for it.
This lead to errors during stats page requests:

Aug 18 08:46:41 somehost.example.net haproxy[1521534]: X.X.X.X:61403 [18/Aug/2020:08:46:41.437] stats stats/ -1/-1/-1/-1/0 503 222 - - SC-- 2/2/0/0/0 0/0 "GET /favicon.ico HTTP/1.1"
Aug 18 08:46:42 somehost.example.net haproxy[1521534]: X.X.X.X:61403 [18/Aug/2020:08:46:42.650] stats stats/ -1/-1/-1/-1/0 503 222 - - SC-- 2/2/0/0/0 0/0 "GET /favicon.ico HTTP/1.1"

Patch provided disables favicon.ico requests for haproxy stats page.

4 years agoREGTEST: remove stray leading spaces in converteers_ref_cnt_never_dec.vtc
Willy Tarreau [Wed, 19 Aug 2020 09:20:28 +0000 (11:20 +0200)] 
REGTEST: remove stray leading spaces in converteers_ref_cnt_never_dec.vtc

Since commit f92afb732 ("MEDIUM: cfgparse: Emit hard error on truncated lines")
we now produce parsing errors on truncated lines, in an effort to clean
up dangerous or broken config files. And it turns out that one of our own
regression tests was suffering from this, as diagnosed by William and Tim.
The cause is the leading spaces in front of "} -start" that vtest makes
part of the output file, so the file finishes with a partial line made
of spaces.

4 years agoMINOR: cache: Reject duplicate cache names
Tim Duesterhus [Tue, 18 Aug 2020 20:20:27 +0000 (22:20 +0200)] 
MINOR: cache: Reject duplicate cache names

Using a duplicate cache name most likely is the result of a misgenerated
configuration. There is no good reason to allow this, as the duplicate
caches can't be referred to.

This commit resolves GitHub issue #820.

It can be argued whether this is a fix for a bug or not. I'm erring on the
side of caution and marking this as a "new feature". It can be considered for
backporting to 2.2, but for other branches the risk of accidentally breaking
some working (but non-ideal) configuration might be too large.

4 years agoDOC: cache: Use '<name>' instead of '<id>' in error message
Tim Duesterhus [Tue, 18 Aug 2020 20:06:51 +0000 (22:06 +0200)] 
DOC: cache: Use '<name>' instead of '<id>' in error message

When the cache name is left out in 'filter cache' the error message refers
to a missing '<id>'. The name of the cache is called 'name' within the docs.

Adjust the error message for consistency.

The error message was introduced in 99a17a2d91f9044ea20bba6617048488aed80555.
This commit first appeared in 1.9, thus the patch must be backported to 1.9+.

4 years agoMEDIUM: cfgparse: Emit hard error on truncated lines
Tim Duesterhus [Tue, 18 Aug 2020 20:00:04 +0000 (22:00 +0200)] 
MEDIUM: cfgparse: Emit hard error on truncated lines

As announced within the emitted log message this is going to become a hard
error in 2.3. It's 2.3 time now, let's do this.

see 2fd5bdb439da29f15381aeb57c51327ba57674fc

4 years agoDOC: overhauling github issue templates
Lukas Tribus [Mon, 17 Aug 2020 18:17:11 +0000 (20:17 +0200)] 
DOC: overhauling github issue templates

as per the suggestions from Cyril and Willy on the mailing list:

Message-ID: <a0010c72-70b8-0647-c269-55c6614627c3@free.fr>

and with direct contributions from Tim, this changes large parts
of the bug issue template.

The Feature template is also updated as well as a new template for
Code Reports is introduced.

Co-authored-by: Tim Duesterhus <tim@bastelstu.be>
4 years agoBUG/MEDIUM: ssl: crt-list negative filters don't work
William Lallemand [Mon, 17 Aug 2020 12:31:19 +0000 (14:31 +0200)] 
BUG/MEDIUM: ssl: crt-list negative filters don't work

The negative filters which are supposed to exclude a SNI from a
wildcard, never worked. Indeed the negative filters were skipped in the
code.

To fix the issue, this patch looks for negative filters that are on the
same line as a the wildcard that just matched.

This patch should fix issue #818. It must be backported in 2.2.  The
problem also exists in versions > 1.8 but the infrastructure required to
fix this was only introduced in 2.1.  In older versions we should
probably change the documentation to state that negative filters are
useless.

4 years agoMINOR: hlua: Add error message relative to the Channel manipulation and HTTP mode
Thierry Fournier [Sat, 15 Aug 2020 12:35:51 +0000 (14:35 +0200)] 
MINOR: hlua: Add error message relative to the Channel manipulation and HTTP mode

When the developper try to manipulate HAProxy channels in HTTP mode,
an error throws without explanation. This patch adds an explanation.

4 years ago[RELEASE] Released version 2.3-dev3 v2.3-dev3
Willy Tarreau [Fri, 14 Aug 2020 16:54:05 +0000 (18:54 +0200)] 
[RELEASE] Released version 2.3-dev3

Released version 2.3-dev3 with the following main changes :
    - SCRIPTS: git-show-backports: make -m most only show the left branch
    - SCRIPTS: git-show-backports: emit the shell command to backport a commit
    - BUILD: Makefile: require SSL_LIB, SSL_INC to be explicitly set
    - CI: travis-ci: specify SLZ_LIB, SLZ_INC for travis builds
    - BUG/MEDIUM: mux-h1: Refresh H1 connection timeout after a synchronous send
    - CLEANUP: dns: typo in reported error message
    - BUG/MAJOR: dns: disabled servers through SRV records never recover
    - BUG/MINOR: spoa-server: fix size_t format printing
    - DOC: spoa-server: fix false friends `actually`
    - BUG/MINOR: ssl: fix memory leak at OCSP loading
    - BUG/MEDIUM: ssl: memory leak of ocsp data at SSL_CTX_free()
    - BUG/MEDIUM: map/lua: Return an error if a map is loaded during runtime
    - MINOR: arg: Add an argument type to keep a reference on opaque data
    - BUG/MINOR: converters: Store the sink in an arg pointer for debug() converter
    - BUG/MINOR: lua: Duplicate map name to load it when a new Map object is created
    - BUG/MINOR: arg: Fix leaks during arguments validation for fetches/converters
    - BUG/MINOR: lua: Check argument type to convert it to IPv4/IPv6 arg validation
    - BUG/MINOR: lua: Check argument type to convert it to IP mask in arg validation
    - MINOR: hlua: Don't needlessly copy lua strings in trash during args validation
    - BUG/MINOR: lua: Duplicate lua strings in sample fetches/converters arg array
    - MEDIUM: lua: Don't filter exported fetches and converters
    - MINOR: lua: Add support for userlist as fetches and converters arguments
    - MINOR: lua: Add support for regex as fetches and converters arguments
    - MINOR: arg: Use chunk_destroy() to release string arguments
    - BUG/MINOR: snapshots: leak of snapshots on deinit()
    - CLEANUP: ssl: ssl_sock_crt2der semicolon and spaces
    - MINOR: ssl: add ssl_{c,s}_chain_der fetch methods
    - CLEANUP: fix all duplicated semicolons
    - BUG/MEDIUM: ssl: fix the ssl-skip-self-issued-ca option
    - BUG/MINOR: ssl: ssl-skip-self-issued-ca requires >= 1.0.2
    - BUG/MINOR: stats: use strncmp() instead of memcmp() on health states
    - BUILD: makefile: don't disable -Wstringop-overflow anymore
    - BUG/MINOR: ssl: double free w/ smp_fetch_ssl_x_chain_der()
    - BUG/MEDIUM: htx: smp_prefetch_htx() must always validate the direction
    - BUG/MEDIUM: ssl: never generates the chain from the verify store
    - OPTIM: regex: PCRE2 use JIT match when JIT optimisation occured.
    - BUG/MEDIUM: ssl: does not look for all SNIs before chosing a certificate
    - CLEANUP: ssl: remove poorly readable nested ternary

4 years agoCLEANUP: ssl: remove poorly readable nested ternary
William Lallemand [Fri, 14 Aug 2020 13:30:13 +0000 (15:30 +0200)] 
CLEANUP: ssl: remove poorly readable nested ternary

Replace a four level nested ternary expression by an if/else expression
in ssl_sock_switchctx_cbk()

4 years agoBUG/MEDIUM: ssl: does not look for all SNIs before chosing a certificate
William Lallemand [Fri, 14 Aug 2020 12:43:35 +0000 (14:43 +0200)] 
BUG/MEDIUM: ssl: does not look for all SNIs before chosing a certificate

In bug #810, the SNI are not matched correctly, indeed when trying to
match a certificate type in ssl_sock_switchctx_cbk() all SNIs were not
looked up correctly.

In the case you have in a crt-list:

wildcard.subdomain.domain.tld.pem.rsa *.subdomain.domain.tld record.subdomain.domain.tld
record.subdomain.domain.tld.pem.ecdsa record.subdomain.domain.tld another-record.subdomain.domain.tld

If the client only supports RSA and requests
"another-record.subdomain.domain.tld", HAProxy will find the single
ECDSA certificate and won't try to look up for a wildcard RSA
certificate.

This patch fixes the code so we look for all single and
wildcard before chosing the certificate type.

This bug was introduced by commit 3777e3a ("BUG/MINOR: ssl: certificate
choice can be unexpected with openssl >= 1.1.1").

It must be backported as far as 1.8 once it is heavily tested.

4 years agoOPTIM: regex: PCRE2 use JIT match when JIT optimisation occured.
David Carlier [Thu, 13 Aug 2020 13:53:41 +0000 (14:53 +0100)] 
OPTIM: regex: PCRE2 use JIT match when JIT optimisation occured.

When a regex had been succesfully compiled by the JIT pass, it is better
 to use the related match, thanksfully having same signature, for better
 performance.

Signed-off-by: David Carlier <devnexen@gmail.com>
4 years agoBUG/MEDIUM: ssl: never generates the chain from the verify store
William Lallemand [Wed, 12 Aug 2020 18:02:10 +0000 (20:02 +0200)] 
BUG/MEDIUM: ssl: never generates the chain from the verify store

In bug #781 it was reported that HAProxy completes the certificate chain
using the verify store in the case there is no chain.

Indeed, according to OpenSSL documentation, when generating the chain,
OpenSSL use the chain store OR the verify store in the case there is no
chain store.

As a workaround, this patch always put a NULL chain in the SSL_CTX so
OpenSSL does not tries to complete it.

This must be backported in all branches, the code could be different,
the important part is to ALWAYS set a chain, and uses sk_X509_new_null()
if the chain is NULL.

4 years agoBUG/MEDIUM: htx: smp_prefetch_htx() must always validate the direction
Willy Tarreau [Wed, 12 Aug 2020 12:04:52 +0000 (14:04 +0200)] 
BUG/MEDIUM: htx: smp_prefetch_htx() must always validate the direction

It is possible to process a channel based on desynchronized info if a
request fetch is called from a response and conversely. However, the
code in smp_prefetch_htx() already makes sure the analysis has already
started before trying to fetch from a buffer, so the problem effectively
lies in response rules making use of request expressions only.

Usually it's not a problem as extracted data are checked against the
current HTTP state, except when it comes to the start line, which is
usually accessed directly from sample fetch functions such as status,
path, url, url32, query and so on. In this case, trying to access the
request buffer from the response path will lead to unpredictable
results. When building with DEBUG_STRICT, a process violating these
rules will simply die after emitting:

  FATAL: bug condition "htx->first == -1" matched at src/http_htx.c:67

But when this is not enabled, it may or may not crash depending on what
the pending request buffer data look like when trying to spot a start
line there. This is typically what happens in issue #806.

This patch adds a test in smp_prefetch_htx() so that it does not try
to parse an HTX buffer in a channel belonging to the wrong direction.

There's one special case on the "method" sample fetch since it can
retrieve info even without a buffer, from the other direction, as
long as the method is one of the well known ones. Three, we call
smp_prefetch_htx() only if needed.

This was reported in 2.0 and must be backported there (oldest stable
version with HTX).

4 years agoBUG/MINOR: ssl: double free w/ smp_fetch_ssl_x_chain_der()
William Lallemand [Tue, 11 Aug 2020 09:18:46 +0000 (11:18 +0200)] 
BUG/MINOR: ssl: double free w/ smp_fetch_ssl_x_chain_der()

smp_fetch_ssl_x_chain_der() uses the SSL_get_peer_cert_chain() which
does not increment the refcount of the chain, so it should not be free'd.

The bug was introduced by a598b50 ("MINOR: ssl: add ssl_{c,s}_chain_der
fetch methods"). No backport needed.

4 years agoBUILD: makefile: don't disable -Wstringop-overflow anymore
Willy Tarreau [Tue, 11 Aug 2020 08:31:18 +0000 (10:31 +0200)] 
BUILD: makefile: don't disable -Wstringop-overflow anymore

This basically reverts commit c4e6460f6 ("MINOR: build: Disable
-Wstringop-overflow.") which is no more needed after previous one.

4 years agoBUG/MINOR: stats: use strncmp() instead of memcmp() on health states
Willy Tarreau [Tue, 11 Aug 2020 08:26:36 +0000 (10:26 +0200)] 
BUG/MINOR: stats: use strncmp() instead of memcmp() on health states

The reports for health states are checked using memcmp() in order to
only focus on the first word and possibly ignore trailing %d/%d etc.
This makes gcc unhappy about a potential use of "" as the string, which
never happens since the string is always set. This resulted in commit
c4e6460f6 ("MINOR: build: Disable -Wstringop-overflow.") to silence
these messages. However some lengths are incorrect (though cannot cause
trouble), and in the end strncmp() is just safer and cleaner.

This can be backported to all stable branches as it will shut a warning
with gcc 8 and above.

4 years agoBUG/MINOR: ssl: ssl-skip-self-issued-ca requires >= 1.0.2
William Lallemand [Mon, 10 Aug 2020 15:28:23 +0000 (17:28 +0200)] 
BUG/MINOR: ssl: ssl-skip-self-issued-ca requires >= 1.0.2

The previous fix for ssl-skip-self-issued-ca requires the use of
SSL_CTX_build_cert_chain() which is only available starting from OpenSSL
1.0.2

4 years agoBUG/MEDIUM: ssl: fix the ssl-skip-self-issued-ca option
William Lallemand [Mon, 10 Aug 2020 14:18:45 +0000 (16:18 +0200)] 
BUG/MEDIUM: ssl: fix the ssl-skip-self-issued-ca option

In commit f187ce6, the ssl-skip-self-issued-ca option was accidentally
made useless by reverting the SSL_CTX reworking.

The previous attempt of making this feature was putting each certificate
of the chain in the SSL_CTX with SSL_CTX_add_extra_chain_cert() and was
skipping the Root CA.
The problem here is that doing it this way instead of doing a
SSL_CTX_set1_chain() break the support of the multi-certificate bundles.

The SSL_CTX_build_cert_chain() function allows one to remove the Root CA
with the SSL_BUILD_CHAIN_FLAG_NO_ROOT flag. Use it instead of doing
tricks with the CA.

Should fix issue #804.

Must be backported in 2.2.

4 years agoCLEANUP: fix all duplicated semicolons
William Dauchy [Fri, 7 Aug 2020 20:19:23 +0000 (22:19 +0200)] 
CLEANUP: fix all duplicated semicolons

trivial commit, does not change the code behaviour

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
4 years agoMINOR: ssl: add ssl_{c,s}_chain_der fetch methods
William Dauchy [Thu, 6 Aug 2020 16:11:38 +0000 (18:11 +0200)] 
MINOR: ssl: add ssl_{c,s}_chain_der fetch methods

Following work from Arjen and Mathilde, it adds ssl_{c,s}_chain_der
methods; it returns DER encoded certs from SSL_get_peer_cert_chain

Also update existing vtc tests to add random intermediate certificates

When getting the result through this header:
  http-response add-header x-ssl-chain-der %[ssl_c_chain_der,hex]
One can parse it with any lib accepting ASN.1 DER data, such as in go:
  bin, err := encoding/hex.DecodeString(cert)
  certs_parsed, err := x509.ParseCertificates(bin)

Cc: Arjen Nienhuis <arjen@zorgdoc.nl>
Signed-off-by: Mathilde Gilles <m.gilles@criteo.com>
Signed-off-by: William Dauchy <w.dauchy@criteo.com>
4 years agoCLEANUP: ssl: ssl_sock_crt2der semicolon and spaces
William Dauchy [Thu, 6 Aug 2020 16:11:37 +0000 (18:11 +0200)] 
CLEANUP: ssl: ssl_sock_crt2der semicolon and spaces

trivial commit, does not change the code behaviour

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
4 years agoBUG/MINOR: snapshots: leak of snapshots on deinit()
William Lallemand [Fri, 7 Aug 2020 12:48:37 +0000 (14:48 +0200)] 
BUG/MINOR: snapshots: leak of snapshots on deinit()

Free the snapshots on deinit() when they were initialized in a proxy
upon an error.

This was introduced by c55015e ("MEDIUM: snapshots: dynamically allocate
the snapshots").

Should be backported as far as 1.9.

4 years agoMINOR: arg: Use chunk_destroy() to release string arguments
Christopher Faulet [Fri, 7 Aug 2020 09:45:18 +0000 (11:45 +0200)] 
MINOR: arg: Use chunk_destroy() to release string arguments

This way, all fields of the buffer structure are reset when a string argument
(ARGT_STR) is released.  It is also a good way to explicitly specify this kind
of argument is a chunk. So .data and .size fields must be set.

This patch may be backported to ease backports.

4 years agoMINOR: lua: Add support for regex as fetches and converters arguments
Christopher Faulet [Thu, 6 Aug 2020 09:10:57 +0000 (11:10 +0200)] 
MINOR: lua: Add support for regex as fetches and converters arguments

It means now regsub() converter is now exported to the lua. Map converters based
on regex are not available because the map arguments are not supported.

4 years agoMINOR: lua: Add support for userlist as fetches and converters arguments
Christopher Faulet [Thu, 6 Aug 2020 09:04:46 +0000 (11:04 +0200)] 
MINOR: lua: Add support for userlist as fetches and converters arguments

It means now http_auth() and http_auth_group() sample fetches are now exported
to the lua.

4 years agoMEDIUM: lua: Don't filter exported fetches and converters
Christopher Faulet [Thu, 6 Aug 2020 08:32:28 +0000 (10:32 +0200)] 
MEDIUM: lua: Don't filter exported fetches and converters

Thanks to previous commits, it is now safe to use from lua the sample fetches
and sample converters that convert arguments, especially the strings
(ARGT_STR). So now, there are all exported to the lua. They was filtered on the
validation functions. Only fetches without validation functions or with val_hdr
or val_payload_lv functions were exported, and converters without validation
functions.

This patch depends on following commits :

  * aec27ef44 "BUG/MINOR: lua: Duplicate lua strings in sample fetches/converters arg array"
  * fdea1b631 "MINOR: hlua: Don't needlessly copy lua strings in trash during args validation"

It must be backported as far as 2.1 because the date() and http_date()
converters are no longer exported because of the filter on the validation
function, since the commit ae6f125c7 ("MINOR: sample: add us/ms support to
date/http_date)".

4 years agoBUG/MINOR: lua: Duplicate lua strings in sample fetches/converters arg array
Christopher Faulet [Thu, 6 Aug 2020 06:54:25 +0000 (08:54 +0200)] 
BUG/MINOR: lua: Duplicate lua strings in sample fetches/converters arg array

Strings in the argument array used by sample fetches and converters must be
duplicated. This is mandatory because, during the arguments validations, these
strings may be converted and released. It works this way during the
configuration parsing and there is no reason to adapt this behavior during the
runtime when a sample fetch or a sample converter is called from the lua. In
fact, there is a reason to not change the behavior. It must reamain simple for
everyone to add new fetches or converters.

Thus, lua strings are duplicated. It is only performed at the end of the
hlua_lua2arg_check() function, if the argument is still a ARGT_STR. Of course,
it requires a cleanup loop after the call or when an error is triggered.

This patch depends on following commits:

  * 959171376 "BUG/MINOR: arg: Fix leaks during arguments validation for fetches/converters"
  * fdea1b631 "MINOR: hlua: Don't needlessly copy lua strings in trash during args validation"

It may be backported to all supported versions, most probably as far as 2.1
only.

4 years agoMINOR: hlua: Don't needlessly copy lua strings in trash during args validation
Christopher Faulet [Thu, 6 Aug 2020 06:29:18 +0000 (08:29 +0200)] 
MINOR: hlua: Don't needlessly copy lua strings in trash during args validation

Lua strings are NULL terminated. So in the hlua_lua2arg_check() function, used
to check arguments against the sample fetches specification, there is no reason
to copy these strings in a trash to add a terminating null byte.

In addition, when the array of arguments is built from lua values, we must take
care to count this terminating null bytes in the size of the buffer where a
string is stored. The same must be done when a sample is built from a lua value.

This patch may be backported to easy backports.

4 years agoBUG/MINOR: lua: Check argument type to convert it to IP mask in arg validation
Christopher Faulet [Fri, 7 Aug 2020 07:11:22 +0000 (09:11 +0200)] 
BUG/MINOR: lua: Check argument type to convert it to IP mask in arg validation

In hlua_lua2arg_check() function, before converting an argument to an IPv4 or
IPv6 mask, we must be sure to have an integer or a string argument (ARGT_SINT or
ARGT_STR).

This patch must be backported to all supported versions.

4 years agoBUG/MINOR: lua: Check argument type to convert it to IPv4/IPv6 arg validation
Christopher Faulet [Fri, 7 Aug 2020 07:07:26 +0000 (09:07 +0200)] 
BUG/MINOR: lua: Check argument type to convert it to IPv4/IPv6 arg validation

In hlua_lua2arg_check() function, before converting a string to an IP address,
we must be to sure to have a string argument (ARGT_STR).

This patch must be backported to all supported versions.

4 years agoBUG/MINOR: arg: Fix leaks during arguments validation for fetches/converters
Christopher Faulet [Wed, 5 Aug 2020 21:07:07 +0000 (23:07 +0200)] 
BUG/MINOR: arg: Fix leaks during arguments validation for fetches/converters

Some sample fetches or sample converters uses a validation functions for their
arguments. In these function, string arguments (ARGT_STR) may be converted to
another type (for instance a regex, a variable or a integer). Because these
strings are allocated when the argument list is built, they must be freed after
a conversion. Most of time, it is done. But not always. This patch fixes these
minor memory leaks (only on few strings, during the configuration parsing).

This patch may be backported to all supported versions, most probably as far as
2.1 only. If this commit is backported, the previous one 73292e9e6 ("BUG/MINOR:
lua: Duplicate map name to load it when a new Map object is created") must also
be backported. Note that some validation functions does not exists on old
version. It should be easy to resolve conflicts.

4 years agoBUG/MINOR: lua: Duplicate map name to load it when a new Map object is created
Christopher Faulet [Thu, 6 Aug 2020 06:40:09 +0000 (08:40 +0200)] 
BUG/MINOR: lua: Duplicate map name to load it when a new Map object is created

When a new map is created, the sample_load_map() function is called. To do so,
an argument array is created with the name as first argument. Because it is a
lua string, owned by the lua, it must be duplicated. The sample_load_map()
function will convert this argument to a map. In theory, after the conversion,
it must release the original string. It is not performed for now and it is a bug
that will be fixed in the next commit.

This patch may be backported to all supported versions, most probably as far as
2.1 only. But it must be backported with the next commit "BUG/MINOR: arg: Fix
leaks during arguments validation for fetches/converters".

4 years agoBUG/MINOR: converters: Store the sink in an arg pointer for debug() converter
Christopher Faulet [Fri, 7 Aug 2020 12:00:23 +0000 (14:00 +0200)] 
BUG/MINOR: converters: Store the sink in an arg pointer for debug() converter

The debug() converter uses a string to reference the sink where to send debug
events. During the configuration parsing, this string is converted to a sink
object but it is still store as a string argument. It is a problem on deinit
because string arguments are released. So the sink pointer will be released
twice.

To fix the bug, we keep a reference on the sink using an ARGT_PTR argument. This
way, it will not be freed on the deinit.

This patch depends on the commit e02fc4d0d ("MINOR: arg: Add an argument type to
keep a reference on opaque data"). Both must be backported as far as 2.1.

4 years agoMINOR: arg: Add an argument type to keep a reference on opaque data
Christopher Faulet [Fri, 7 Aug 2020 11:56:00 +0000 (13:56 +0200)] 
MINOR: arg: Add an argument type to keep a reference on opaque data

The ARGT_PTR argument type may now be used to keep a reference to opaque data in
the argument array used by sample fetches and converters. It is a generic way to
point on data. I guess it could be used for some other arguments, like proxy,
server, map or stick-table.

4 years agoBUG/MEDIUM: map/lua: Return an error if a map is loaded during runtime
Christopher Faulet [Wed, 5 Aug 2020 21:23:37 +0000 (23:23 +0200)] 
BUG/MEDIUM: map/lua: Return an error if a map is loaded during runtime

In sample_load_map() function, the global mode is now tested to be sure to be in
the starting mode. If not, an error is returned.

At first glance, this patch may seem useless because maps are loaded during the
configuration parsing. But in fact, it is possible to load a map from the lua,
using Map:new() method. And, there is nothing to forbid to call this method at
runtime, during a script execution. It must never be done because it may perform
an filesystem access for unknown maps or allocation for known ones. So at
runtime, it means a blocking call or a memroy leak. Note it is still possible to
load a map from the lua, but in the global part of a script only. This part is
executed during the configuration parsing.

This patch must be backported in all stable versions.

4 years agoBUG/MEDIUM: ssl: memory leak of ocsp data at SSL_CTX_free()
William Lallemand [Tue, 4 Aug 2020 15:41:39 +0000 (17:41 +0200)] 
BUG/MEDIUM: ssl: memory leak of ocsp data at SSL_CTX_free()

This bug affects all version of HAProxy since the OCSP data is not free
in the deinit(), but leaking on exit() is not really an issue. However,
when doing dynamic update of certificates over the CLI, those data are
not free'd upon the free of the SSL_CTX.

3 leaks are happening, the first leak is the one of the ocsp_arg
structure which serves the purpose of containing the pointers in the
case of a multi-certificate bundle. The second leak is the one ocsp
struct. And the third leak is the one of the struct buffer in the
ocsp_struct.

The problem lies with SSL_CTX_set_tlsext_status_arg() which does not
provide a way to free the argument upon an SSL_CTX_free().

This fix uses ex index functions instead of registering a
tlsext_status_arg(). This is really convenient because it allows to
register a free callback which will free the ex index content upon a
SSL_CTX_free().

A refcount was also added to the ocsp_response structure since it is
stored in a tree and can be reused in another SSL_CTX.

Should fix part of the issue #746.

This must be backported in 2.2 and 2.1.

4 years agoBUG/MINOR: ssl: fix memory leak at OCSP loading
William Lallemand [Thu, 6 Aug 2020 22:44:32 +0000 (00:44 +0200)] 
BUG/MINOR: ssl: fix memory leak at OCSP loading

Fix a memory leak when loading an OCSP file when the file was already
loaded elsewhere in the configuration.

Indeed, if the OCSP file already exists, a useless chunk_dup() will be
done during the load.

To fix it we reverts "ocsp" to "iocsp" like it was done previously.

This was introduced by commit 246c024 ("MINOR: ssl: load the ocsp
in/from the ckch").

Should fix part of the issue #746.

It must be backported in 2.1 and 2.2.

4 years agoDOC: spoa-server: fix false friends `actually`
William Dauchy [Sat, 1 Aug 2020 14:28:52 +0000 (16:28 +0200)] 
DOC: spoa-server: fix false friends `actually`

it seems like `actually` was wrongly used instead of `currently`

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
4 years agoBUG/MINOR: spoa-server: fix size_t format printing
William Dauchy [Sat, 1 Aug 2020 14:28:51 +0000 (16:28 +0200)] 
BUG/MINOR: spoa-server: fix size_t format printing

From https://www.python.org/dev/peps/pep-0353/
"A new type Py_ssize_t is introduced, which has the same size as the
compiler's size_t type, but is signed. It will be a typedef for ssize_t
where available."

For integer types, causes printf to expect a size_t-sized integer
argument.

This should fix github issue #702

This should be backported to >= v2.2

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
4 years agoBUG/MAJOR: dns: disabled servers through SRV records never recover
Baptiste Assmann [Tue, 4 Aug 2020 08:57:21 +0000 (10:57 +0200)] 
BUG/MAJOR: dns: disabled servers through SRV records never recover

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

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

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

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

4 years agoCLEANUP: dns: typo in reported error message
Baptiste Assmann [Tue, 4 Aug 2020 08:54:14 +0000 (10:54 +0200)] 
CLEANUP: dns: typo in reported error message

"record" instead of "recrd".

This should be backported to 2.2.

4 years agoBUG/MEDIUM: mux-h1: Refresh H1 connection timeout after a synchronous send
Christopher Faulet [Wed, 5 Aug 2020 09:31:16 +0000 (11:31 +0200)] 
BUG/MEDIUM: mux-h1: Refresh H1 connection timeout after a synchronous send

The H1 multiplexer is able to perform synchronous send. When a large body is
transfer, if nothing is received and if no error or shutdown occurs, it is
possible to not go down at the H1 connection level to do I/O for a long
time. When this happens, we must still take care to refresh the H1 connection
timeout. Otherwise it is possible to hit the connection timeout during the
transfer while it should not expire.

This bug exists because only h1_process() refresh the H1 connection timeout. To
fix the bug, h1_snd_buf() must also refresh this timeout. To make things more
readable, a dedicated function has been introduced and called to refresh the
timeout.

This bug exists on all HTX versions. But it is harder to hit it on 2.1 and below
because when a H1 mux is initialized, we actively try to read data instead of
subscribing for receiving. So there is at least one call to h1_process().

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

4 years agoCI: travis-ci: specify SLZ_LIB, SLZ_INC for travis builds
Ilya Shipitsin [Tue, 4 Aug 2020 10:39:41 +0000 (15:39 +0500)] 
CI: travis-ci: specify SLZ_LIB, SLZ_INC for travis builds

These variables must be explicitly set as they are not inherited from
the environment. Till now they were ignored.

4 years agoBUILD: Makefile: require SSL_LIB, SSL_INC to be explicitly set
Ilya Shipitsin [Tue, 4 Aug 2020 10:36:24 +0000 (15:36 +0500)] 
BUILD: Makefile: require SSL_LIB, SSL_INC to be explicitly set

The SSL_INC and SSL_LIB variables were not initialized in the Makefile,
so they could be accidently inherited from the environment. We require
that any makefile variable is explicitly set on the command line so they
must be initialized.

Note that the Travis scripts used to rely only on these variables to be
exported, so it was adjusted as well.

4 years agoSCRIPTS: git-show-backports: emit the shell command to backport a commit
Willy Tarreau [Fri, 31 Jul 2020 14:47:44 +0000 (16:47 +0200)] 
SCRIPTS: git-show-backports: emit the shell command to backport a commit

It's cumbersome to copy-paste a commit ID into another window after having
typed "git cherry-pick -sx", so let's have the suggested output format of
git-show prepare this line just before the subject line, it remains at a
stable position on the terminal when searching for "/^commit". One just
has to copy-paste the line into another terminal will result in the commit
being properly picked.

4 years agoSCRIPTS: git-show-backports: make -m most only show the left branch
Willy Tarreau [Fri, 31 Jul 2020 14:38:57 +0000 (16:38 +0200)] 
SCRIPTS: git-show-backports: make -m most only show the left branch

We've never used the output of the rightmost branch with this tool,
and it systematically causes two identical outputs making the job
harder during backport sessions. Let's simply remove the right part
when it's identical to the left one. This also adds a few line feeds
to make the output more readable.

4 years ago[RELEASE] Released version 2.3-dev2 v2.3-dev2
Willy Tarreau [Fri, 31 Jul 2020 12:48:32 +0000 (14:48 +0200)] 
[RELEASE] Released version 2.3-dev2

Released version 2.3-dev2 with the following main changes :
    - DOC: ssl: req_ssl_sni needs implicit TLS
    - BUG/MEDIUM: arg: empty args list must be dropped
    - BUG/MEDIUM: resolve: fix init resolving for ring and peers section.
    - BUG/MAJOR: tasks: don't requeue global tasks into the local queue
    - MINOR: tasks/debug: make the thread affinity BUG_ON check a bit stricter
    - MINOR: tasks/debug: add a few BUG_ON() to detect use of wrong timer queue
    - MINOR: tasks/debug: add a BUG_ON() check to detect requeued task on free
    - BUG/MAJOR: dns: Make the do-resolve action thread-safe
    - BUG/MEDIUM: dns: Release answer items when a DNS resolution is freed
    - MEDIUM: htx: Add a flag on a HTX message when no more data are expected
    - BUG/MEDIUM: stream-int: Don't set MSG_MORE flag if no more data are expected
    - BUG/MEDIUM: http-ana: Only set CF_EXPECT_MORE flag on data filtering
    - CLEANUP: dns: remove 45 "return" statements from dns_validate_dns_response()
    - BUG/MINOR: htx: add two missing HTX_FL_EOI and remove an unexpected one
    - BUG/MINOR: mux-fcgi: Don't url-decode the QUERY_STRING parameter anymore
    - BUILD: tools: fix build with static only toolchains
    - DOC: Use gender neutral language
    - BUG/MINOR: debug: Don't dump the lua stack if it is not initialized
    - BUG/MAJOR: dns: fix null pointer dereference in snr_update_srv_status
    - BUG/MAJOR: dns: don't treat Authority records as an error
    - CI : travis-ci : prepare for using stock OpenSSL
    - CI: travis-ci : switch to stock openssl when openssl-1.1.1 is used
    - MEDIUM: lua: Add support for the Lua 5.4
    - BUG/MEDIUM: dns: Don't yield in do-resolve action on a final evaluation
    - BUG/MINOR: lua: Abort execution of actions that yield on a final evaluation
    - MINOR: tcp-rules: Return an internal error if an action yields on a final eval
    - BUG/MINOR: tcp-rules: Preserve the right filter analyser on content eval abort
    - BUG/MINOR: tcp-rules: Set the inspect-delay when a tcp-response action yields
    - MEDIUM: tcp-rules: Use a dedicated expiration date for tcp ruleset
    - MEDIUM: lua: Set the analyse expiration date with smaller wake_time only
    - BUG/MEDIUM: connection: Be sure to always install a mux for sync connect
    - MINOR: connection: Preinstall the mux for non-ssl connect
    - MINOR: stream-int: Be sure to have a mux to do sends and receives
    - BUG/MINOR: lua: Fix a possible null pointer deref on lua ctx
    - SCRIPTS: announce-release: add the link to the wiki in the announce messages
    - CI: travis-ci: use better name for Coverity scan job
    - CI: travis-ci: use proper linking flags for SLZ build
    - BUG/MEDIUM: backend: always attach the transport before installing the mux
    - BUG/MEDIUM: tcp-checks: always attach the transport before installing the mux
    - MINOR: connection: avoid a useless recvfrom() on outgoing connections
    - MINOR: mux-h1: do not even try to receive if the connection is not fully set up
    - MINOR: mux-h1: do not try to receive on backend before sending a request
    - CLEANUP: assorted typo fixes in the code and comments
    - BUG/MEDIUM: ssl: check OCSP calloc in ssl_sock_load_ocsp()

4 years agoBUG/MEDIUM: ssl: check OCSP calloc in ssl_sock_load_ocsp()
William Lallemand [Fri, 31 Jul 2020 09:43:20 +0000 (11:43 +0200)] 
BUG/MEDIUM: ssl: check OCSP calloc in ssl_sock_load_ocsp()

Check the return of the calloc in ssl_sock_load_ocsp() which could lead
to a NULL dereference.

This was introduced by commit be2774d ("MEDIUM: ssl: Added support for
Multi-Cert OCSP Stapling").

Could be backported as far as 1.7.

4 years agoCLEANUP: assorted typo fixes in the code and comments
Ilya Shipitsin [Wed, 22 Jul 2020 19:32:55 +0000 (00:32 +0500)] 
CLEANUP: assorted typo fixes in the code and comments

This is 12th iteration of typo fixes

4 years agoMINOR: mux-h1: do not try to receive on backend before sending a request
Willy Tarreau [Fri, 31 Jul 2020 07:16:23 +0000 (09:16 +0200)] 
MINOR: mux-h1: do not try to receive on backend before sending a request

There's no point trying to perform an recv() on a back connection if we
have a stream before having sent a request, as it's expected to fail.
It's likely that this may avoid some spurious subscribe() calls in some
keep-alive cases (the close case was already addressed at the connection
level by "MINOR: connection: avoid a useless recvfrom() on outgoing
connections").

4 years agoMINOR: mux-h1: do not even try to receive if the connection is not fully set up
Willy Tarreau [Fri, 31 Jul 2020 07:15:43 +0000 (09:15 +0200)] 
MINOR: mux-h1: do not even try to receive if the connection is not fully set up

If the connection is still waiting for L4/L6, there's no point even trying
to receive as it will fail, so better return zero in h1_recv_allowed().