]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
5 years agoBUG/MAJOR: listener: do not schedule a task-less proxy
Willy Tarreau [Wed, 8 Jan 2020 18:15:07 +0000 (19:15 +0100)] 
BUG/MAJOR: listener: do not schedule a task-less proxy

Apparently seamingless commit 0591bf7deb ("MINOR: listener: make the
wait paths cleaner and more reliable") caused a nasty regression and
revealed a rare race that hits regtest stickiness/lb-services.vtc
about 4% of the times for 8 threads.

The problem is that when a multi-threaded listener wakes up on an
incoming connection, several threads can receive the event, especially
when idle. And all of them will race to accept the connections in
parallel, adjusting the listener's nbconn and proxy's feconn until
one reaches the proxy's limit and declines. At this step the changes
are cancelled, the listener is marked "limited", and when the threads
exit the function, one of them will unlimit the listener/proxy again
so that it can accept incoming connections again.

The problem happens when many threads connect to a small peers section
because its maxconn is very limited (typically 6 for 2 peers), and it's
sometimes possible for enough competing threads to hit the limit and
one of them will limit the listener and queue the proxy's task... except
that peers do not initialize their proxy task since they do not use rate
limiting. Thus the process crashes when doing task_schedule(p->task).
Prior to the cleanup patch above, this didn't happen because the error
path that was dedicated to only limiting the listener did not call
task_schedule(p->task).

Given that the proxy's task is optional, and that the expire value
passed there is always TICK_ETERNITY, it's sufficient and reasonable to
avoid calling this task_schedule() when expire is not set. And for long
term safety we can also avoid to do it when the task is not set. A first
fix consisted in allocating a task for the peers proxies but it's never
used and would eat resources for reason.

No backport is needed as this commit was only merged into 2.2.

5 years agoBUILD: cirrus-ci: choose proper openssl package name
Ilya Shipitsin [Tue, 7 Jan 2020 18:41:24 +0000 (23:41 +0500)] 
BUILD: cirrus-ci: choose proper openssl package name

freebsd-11.3 and 12.1 comes with different openssl naming
let us add proper switch to cirrus-ci script

5 years agoCLEANUP: mux-h2: remove unused goto "out_free_h2s"
William Dauchy [Wed, 8 Jan 2020 14:16:53 +0000 (15:16 +0100)] 
CLEANUP: mux-h2: remove unused goto "out_free_h2s"

Since commit fa8aa867b915 ("MEDIUM: connections: Change struct
wait_list to wait_event.") we no longer use this section.

this should fix github issue #437

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
5 years agoMINOR: http: Add 404 to http-request deny
Florian Tham [Wed, 8 Jan 2020 12:35:30 +0000 (13:35 +0100)] 
MINOR: http: Add 404 to http-request deny

This patch adds http status code 404 Not Found to http-request deny. See
issue #80.

5 years agoMINOR: http: Add 410 to http-request deny
Florian Tham [Wed, 8 Jan 2020 09:19:05 +0000 (10:19 +0100)] 
MINOR: http: Add 410 to http-request deny

This patch adds http status code 410 Gone to http-request deny. See
issue #80.

5 years agoMINOR: raw_sock: make sure to disable polling once everything is sent
Willy Tarreau [Wed, 8 Jan 2020 08:54:02 +0000 (09:54 +0100)] 
MINOR: raw_sock: make sure to disable polling once everything is sent

Analysing traces revealed a rare but surprizing pattern :

    connect()  = -1 EAGAIN
    send()     = success
    epoll_ctl(ADD, EPOLLOUT)
    epoll_wait()
    recvfrom() = success
    close()

What happens is that the failed connect() creates an FD update for pollout,
but the successful synchronous send() doesn't disable it because polling was
only disabled in the FD handler. But a successful synchronous connect()
cancellation is a good opportunity to disable polling before it's effectively
enabled in the next loop, so better disable it when reaching the end. The
cost is very low if it was already disabled anyway (one atomic op).

This only affects local connections but with this the typical number of
epoll_ctl() calls per connection dropped from ~4.2 to ~3.8 for plain TCP
and 10k transfers.

5 years agoMEDIUM: dns: implement synchronous send
Willy Tarreau [Fri, 20 Dec 2019 10:18:54 +0000 (11:18 +0100)] 
MEDIUM: dns: implement synchronous send

In dns_send_query(), there's no point in first waking up the FD, to get
called back by the poller to send the request and sleep. Instead let's
simply send the request as soon as it's known and only subscribe to the
poller when the socket buffers are full and it's required to poll (i.e.
almost never).

This significantly reduces the number of calls to the poller. A large
config sees the number of epoll_ctl() calls reduced from 577 to 7 over
10 seconds, the number of recvfrom() from 1533 to 582 and the number of
sendto() from 369 to 162.

It also has the extra benefit of building each requests only once per
resolution and sending it to multiple resolvers instead of rebuilding
it for each and every resolver.

This will reduce the risk of seeing situations similar to bug #416 in
the future.

5 years agoBUG/MEDIUM: session: do not report a failure when rejecting a session
Willy Tarreau [Tue, 7 Jan 2020 17:03:09 +0000 (18:03 +0100)] 
BUG/MEDIUM: session: do not report a failure when rejecting a session

In session_accept_fd() we can perform a synchronous call to
conn_complete_session() and if it succeeds the connection is accepted
and turned into a session. If it fails we take it as an error while it
is not, in this case, it's just that a tcp-request rule has decided to
reject the incoming connection. The problem with reporting such an event
as an error is that the failed status is passed down to the listener code
which decides to disable accept() for 100ms in order to leave some time
for transient issues to vanish, and that's not what we want to do here.

This fix must be backported as far as 1.7. In 1.7 the code is a bit
different as tcp_exec_l5_rules() is called directly from within
session_new_fd() and ret=0 must be assigned there.

5 years agoBUG/MINOR: channel: inject output data at the end of output
Christopher Faulet [Tue, 7 Jan 2020 09:01:57 +0000 (10:01 +0100)] 
BUG/MINOR: channel: inject output data at the end of output

In co_inject(), data must be inserted at the end of output, not the end of
input. For the record, this function does not take care of input data which are
supposed to not exist. But the caller may reset input data after or before the
call. It is its own choice.

This bug, among other effects, is visible when a redirect is performed on
the response path, on legacy HTTP mode (so for HAProxy < 2.1). The redirect
response is appended after the server response when it should overwrite it.

Thanks to Kevin Zhu <ip0tcp@gmail.com> to report the bug. It must be backported
as far as 1.9.

5 years agoBUG/MEDIUM: http-ana: Truncate the response when a redirect rule is applied
Kevin Zhu [Tue, 7 Jan 2020 08:42:55 +0000 (09:42 +0100)] 
BUG/MEDIUM: http-ana: Truncate the response when a redirect rule is applied

When a redirect rule is executed on the response path, we must truncate the
received response. Otherwise, the redirect is appended after the response, which
is sent to the client. So it is obviously a bug because the redirect is not
performed. With bodyless responses, it is the "only" bug. But if the response
has a body, the result may be invalid. If the payload is not fully received yet
when the redirect is performed, an internal error is reported.

It must be backported as far as 1.9.

5 years agoBUG/MINOR: proxy: Fix input data copy when an error is captured
Christopher Faulet [Mon, 6 Jan 2020 10:37:00 +0000 (11:37 +0100)] 
BUG/MINOR: proxy: Fix input data copy when an error is captured

In proxy_capture_error(), input data are copied in the error snapshot. The copy
must take care of the data wrapping. But the length of the first block is
wrong. It should be the amount of contiguous input data that can be copied
starting from the input's beginning. But the mininum between the input length
and the buffer size minus the input length is used instead. So it is a problem
if input data are wrapping or if more than the half of the buffer is used by
input data.

This patch must be backported as far as 1.9.

5 years agoBUG/MINOR: h1: Report the right error position when a header value is invalid
Christopher Faulet [Mon, 6 Jan 2020 12:41:01 +0000 (13:41 +0100)] 
BUG/MINOR: h1: Report the right error position when a header value is invalid

During H1 messages parsing, when the parser has finished to parse a full header
line, some tests are performed on its value, depending on its name, to be sure
it is valid. The content-length is checked and converted in integer and the host
header is also checked. If an error occurred during this step, the error
position must point on the header value. But from the parser point of view, we
are already on the start of the next header. Thus the effective reported
position in the error capture is the beginning of the unparsed header line. It
is a bit confusing when we try to figure out why a message is rejected.

Now, the parser state is updated to point on the invalid value. This way, the
error position really points on the right position.

This patch must be backported as far as 1.9.

5 years agoMINOR: ssl: Remove unused variable "need_out".
Olivier Houchard [Sun, 5 Jan 2020 15:45:14 +0000 (16:45 +0100)] 
MINOR: ssl: Remove unused variable "need_out".

The "need_out" variable was used to let the ssl code know we're done
reading early data, and we should start the handshake.
Now that the handshake function is responsible for taking care of reading
early data, all that logic has been removed from ssl_sock_to_buf(), but
need_out was forgotten, and left. Remove it know.
This patch was submitted by William Dauchy <w.dauchy@criteo.com>, and should
fix github issue #434.
This should be backported to 2.0 and 2.1.

5 years agoMINOR: config: disable busy polling on old processes
William Dauchy [Sat, 28 Dec 2019 14:36:02 +0000 (15:36 +0100)] 
MINOR: config: disable busy polling on old processes

in the context of seamless reload and busy polling, older processes will
create unecessary cpu conflicts; we can assume there is no need for busy
polling for old processes which are waiting to be terminated.

This patch is not a bug fix itself but might be a good stability
improvment when you are un the context of frequent seamless reloads with
a high "hard-stop-after" value; for that reasons I think this patch
should be backported in all 2.x versions.

Signed-off-by: William Dauchy <w.dauchy@criteo.com>
5 years agoBUILD: CI: modernize cirrus-ci
Ilya Shipitsin [Thu, 26 Dec 2019 08:37:36 +0000 (13:37 +0500)] 
BUILD: CI: modernize cirrus-ci

use freebsd-12.1 instead of freebsd-12.0,
add freebsd-11.3 to build matrix,
install socat in order to run modern reg-tests

5 years agoBUG/MEDIUM: connections: Hold the lock when wanting to kill a connection.
Olivier Houchard [Mon, 30 Dec 2019 17:15:40 +0000 (18:15 +0100)] 
BUG/MEDIUM: connections: Hold the lock when wanting to kill a connection.

In connect_server(), when we decide we want to kill the connection of
another thread because there are too many idle connections, hold the
toremove_lock of the corresponding thread, othervise, there's a small race
condition where we could try to add the connection to the toremove_connections
list while it has already been free'd.

This should be backported to 2.0 and 2.1.

5 years agoBUG/MEDIUM: checks: Only attempt to do handshakes if the connection is ready.
Olivier Houchard [Mon, 30 Dec 2019 14:13:42 +0000 (15:13 +0100)] 
BUG/MEDIUM: checks: Only attempt to do handshakes if the connection is ready.

When creating a new check connection, only attempt to add an handshake
connection if the connection has fully been initialized. It can not be the
case if a DNS resolution is still pending, and thus we don't yet have the
address for the server, as the handshake code assumes the connection is fully
initialized and would otherwise crash.
This is not ideal, the check shouldn't probably run until we have an address,
as it leads to check failures with "Socket error".
While I'm there, also add an xprt handshake if we're using socks4, otherwise
checks wouldn't be able to use socks4 properly.
This should fix github issue #430

This should be backported to 2.0 and 2.1.

5 years agoOPTIM: polling: do not create update entries for FD removal
Willy Tarreau [Fri, 27 Dec 2019 14:52:34 +0000 (15:52 +0100)] 
OPTIM: polling: do not create update entries for FD removal

In order to reduce the number of poller updates, we can benefit from
the fact that modern pollers use sampling to report readiness and that
under load they rarely report the same FD multiple times in a row. As
such it's not always necessary to disable such FDs especially when we're
almost certain they'll be re-enabled again and will require another set
of syscalls.

Now instead of creating an update for a (possibly temporary) removal,
we only perform this removal if the FD is reported again as ready while
inactive. In addition this is performed via another update so that
alternating workloads like transfers have a chance to re-enable the
FD without any syscall during the loop (typically after the data that
filled a buffer have been sent). However we only do that for single-
threaded FDs as the other ones require a more complex setup and are not
on the critical path.

This does cause a few spurious wakeups but almost totally eliminates the
calls to epoll_ctl() on connections seeing intermitent traffic like HTTP/1
to a server or client.

A typical example with 100k requests for 4 kB objects over 200 connections
shows that the number of epoll_ctl() calls doesn't depend on the number
of requests anymore but most exclusively on the number of established
connections:

Before:
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 57.09    0.499964           0    654361    321190 recvfrom
 38.33    0.335741           0    369097         1 epoll_wait
  4.56    0.039898           0     44643           epoll_ctl
  0.02    0.000211           1       200       200 connect
------ ----------- ----------- --------- --------- ----------------
100.00    0.875814               1068301    321391 total

After:
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 59.25    0.504676           0    657600    323630 recvfrom
 40.68    0.346560           0    374289         1 epoll_wait
  0.04    0.000370           0       620           epoll_ctl
  0.03    0.000228           1       200       200 connect
------ ----------- ----------- --------- --------- ----------------
100.00    0.851834               1032709    323831 total

As expected there is also a slight increase of epoll_wait() calls since
delaying de-activation of events can occasionally cause one spurious
wakeup.

5 years agoOPTIM: epoll: always poll for recv if neither active nor ready
Willy Tarreau [Thu, 26 Dec 2019 15:40:24 +0000 (16:40 +0100)] 
OPTIM: epoll: always poll for recv if neither active nor ready

The cost of enabling polling in one direction with epoll is very high
because it requires one syscall per FD and per direction change. In
addition we don't know about input readiness until we either try to
receive() or enable polling and watch the result. With HTTP keep-alive,
both are equally expensive as it's very uncommon to see the server
instantly respond (unless it's a second stage of the same process on
localhost, which has become much less common with threads).

But when a connection is established it's also quite usual to have to
poll for sending (except on localhost or UNIX sockets where it almost
always instantly works). So this cost of polling could be factored out
with the second step if both were enabled together.

This is the idea behind this patch. What it does is to always enable
polling for Rx if it's not ready and at least one direction is active.
This means that if it's not explicitly disabled, or if it was but in a
state that causes the loss of the information (rx ready cannot be
guessed), then let's take any opportunity for a polling change to
enable it at the same time, and learn about rx readiness for free.

In addition the FD never gets unregistered for Rx unless it's ready
and was blocked (buffer full). This avoids a lot of the flip-flop
behaviour at beginning and end of requests.

On a test with 10k requests in keep-alive, the difference is quite
noticeable:

Before:
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 83.67    0.010847           0     20078           epoll_ctl
 16.33    0.002117           0      2231           epoll_wait
  0.00    0.000000           0        20        20 connect
------ ----------- ----------- --------- --------- ----------------
100.00    0.012964                 22329        20 total

After:
% time     seconds  usecs/call     calls    errors syscall
------ ----------- ----------- --------- --------- ----------------
 96.35    0.003351           1      2644           epoll_wait
  2.36    0.000082           4        20        20 connect
  1.29    0.000045           0        66           epoll_ctl
------ ----------- ----------- --------- --------- ----------------
100.00    0.003478                  2730        20 total

It may also save a recvfrom() after connect() by changing the following
sequence, effectively saving one epoll_ctl() and one recvfrom() :

           before              |            after
  -----------------------------+----------------------------
  - connect()                  |  - connect()
  - epoll_ctl(add,out)         |  - epoll_ctl(add, in|out)
  - sendto()                   |  - epoll_wait() = out
  - epoll_ctl(mod,in|out)      |  - send()
  - epoll_wait() = out         |  - epoll_wait() = in|out
  - recvfrom() = EAGAIN        |  - recvfrom() = OK
  - epoll_ctl(mod,in)          |  - recvfrom() = EAGAIN
  - epoll_wait() = in          |  - epoll_ctl(mod, in)
  - recvfrom() = OK            |  - epoll_wait()
  - recvfrom() = EAGAIN        |
  - epoll_wait()               |
    (...)

Now on a 10M req test on 16 threads with 2k concurrent conns and 415kreq/s,
we see 190k updates total and 14k epoll_ctl() only.

5 years agoMINOR: poller: do not call the IO handler if the FD is not active
Willy Tarreau [Thu, 26 Dec 2019 10:09:43 +0000 (11:09 +0100)] 
MINOR: poller: do not call the IO handler if the FD is not active

For now this almost never happens but with subsequent patches it will
become more important not to uselessly call the I/O handlers if the FD
is not active.

5 years agoCLEANUP: connection: merge CO_FL_NOTIFY_DATA and CO_FL_NOTIFY_DONE
Willy Tarreau [Fri, 27 Dec 2019 13:57:45 +0000 (14:57 +0100)] 
CLEANUP: connection: merge CO_FL_NOTIFY_DATA and CO_FL_NOTIFY_DONE

Both flags became equal in commit 82967bf9 ("MINOR: connection: adjust
CO_FL_NOTIFY_DATA after removal of flags"), which already predicted the
overlap between xprt_done_cb() and wake() after the removal of the DATA
specific flags in 1.8. Let's simply remove CO_FL_NOTIFY_DATA since the
"_DONE" version already covers everything and explains the intent well
enough.

5 years agoMINOR: connection: remove the double test on xprt_done_cb()
Willy Tarreau [Fri, 27 Dec 2019 13:49:19 +0000 (14:49 +0100)] 
MINOR: connection: remove the double test on xprt_done_cb()

The conn_fd_handler used to have one possible call to this function to
notify about end of handshakes, and another one to notify about connection
setup or error. But given that we're now only performing wakeup calls
after connection validation, we don't need to keep two places to run
this test since the conditions do not change in between.

This patch merges the two tests into a single one and moves the
CO_FL_CONNECTED test appropriately as well so that it's called even
on the error path if needed.

5 years agoMINOR: connection: check for connection validation earlier
Willy Tarreau [Fri, 27 Dec 2019 09:54:22 +0000 (10:54 +0100)] 
MINOR: connection: check for connection validation earlier

In conn_fd_handler() we used to first give a chance to the send()
callback to try to send data and validate the connection at the same
time. But since 1.9 we do not call this callback anymore inline, it's
scheduled. So let's validate the connection ealier so that all other
decisions can be taken based on this confirmation. This may notably
be useful to the xprt_done_cb() to know that the connection was
properly validated.

5 years agoREORG: connection: move tcp_connect_probe() to conn_fd_check()
Willy Tarreau [Fri, 27 Dec 2019 09:40:21 +0000 (10:40 +0100)] 
REORG: connection: move tcp_connect_probe() to conn_fd_check()

The function is not TCP-specific at all, it covers all FD-based sockets
so let's move this where other similar functions are, in connection.c,
and rename it conn_fd_check().

5 years agoMEDIUM: tcp: make tcp_connect_probe() consider ERR/HUP
Willy Tarreau [Fri, 27 Dec 2019 09:25:29 +0000 (10:25 +0100)] 
MEDIUM: tcp: make tcp_connect_probe() consider ERR/HUP

Now that we know what pollers can return ERR/HUP, we can take this
into account to save one syscall: with such a poller, if neither are
reported, then we know the connection succeeded and we don't need to
go with getsockopt() nor connect() to validate this. In addition, for
the remaining cases (select() or suspected errors), we'll always go
through the extra connect() attempt and enumerate possible "in progress",
"connected" or "failed" status codes and take action solely based on
this.

This results in one saved syscall on modern pollers, only a second
connect() still being used on select() and the server's address never
being needed anymore.

Note that we cannot safely replace connect() with getsockopt() as the
latter clears the error on the socket without saving it, and health
checks rely on it for their reporting. This would be OK if the error
was saved in the connection itself.

5 years agoMINOR: pollers: add a new flag to indicate pollers reporting ERR & HUP
Willy Tarreau [Thu, 28 Nov 2019 17:17:33 +0000 (18:17 +0100)] 
MINOR: pollers: add a new flag to indicate pollers reporting ERR & HUP

In practice it's all pollers except select(). It turns out that we're
keeping some legacy code only for select and enforcing it on all
pollers, let's offer the pollers the ability to declare that they
do not need that.

5 years agoCLEANUP: connection: conn->xprt is never NULL
Willy Tarreau [Thu, 28 Nov 2019 17:08:49 +0000 (18:08 +0100)] 
CLEANUP: connection: conn->xprt is never NULL

Let's remove this outdated test that's been there since 1.5. For quite
some time now xprt hasn't been NULL anymore on an initialized connection.

5 years agoBUG/MINOR: connection: only wake send/recv callbacks if the FD is active
Willy Tarreau [Thu, 28 Nov 2019 16:34:58 +0000 (17:34 +0100)] 
BUG/MINOR: connection: only wake send/recv callbacks if the FD is active

Since commit c3df4507fa ("MEDIUM: connections: Wake the upper layer even
if sending/receiving is disabled.") the send/recv callbacks are called
on I/O if the FD is ready and not just if it's active. This means that
in some situations (e.g. send ready but nothing to send) we may
needlessly enter the if() block, notice we're not subscribed, set
io_available=1 and call the wake() callback even if we're just called
for read activity. Better make sure we only do this when the FD is
active in that direction..

This may be backported as far as 2.0 though it should remain under
observation for a few weeks first as the risk of harm by a mistake
is higher than the trouble it should cause.

5 years agoBUG/MINOR: checks: refine which errno values are really errors.
Willy Tarreau [Fri, 27 Dec 2019 11:03:27 +0000 (12:03 +0100)] 
BUG/MINOR: checks: refine which errno values are really errors.

Two regtest regularly fail in a random fashion depending on the machine's
load (one could really wonder if it's really worth keeping such
unreproducible tests) :
  - tcp-check_multiple_ports.vtc
  - 4be_1srv_smtpchk_httpchk_layer47errors.vtc

It happens that one of the reason is the time it takes to connect to
the local socket (hence the load-dependent aspect): if connect() on the
loopback returns EINPROGRESS then this status is reported instead of a
real error. Normally such a test is expected to see the error cleaned
by tcp_connect_probe() but it really depends on the timing and instead
we may very well send() first and see this error. The problem is that
everything is collected based on errno, hoping it won't get molested
in the way from the last unsuccesful syscall to wake_srv_chk(), which
obviously is hard to guarantee.

This patch at least makes sure that a few non-errors are reported as
zero just like EAGAIN. It doesn't fix the root cause but makes it less
likely to report incorrect failures.

This fix could be backported as far as 1.9.

5 years agoBUILD: travis-ci: reenable address sanitizer for clang builds
Ilya Shipitsin [Sat, 21 Dec 2019 13:10:28 +0000 (18:10 +0500)] 
BUILD: travis-ci: reenable address sanitizer for clang builds

address sanitizer was temporarily disabled. after getting rid of
LD_LIBRARY_PATH manipulation it works again, so let us enable it

5 years agoBUILD: travis-ci: link with ssl libraries using rpath instead of LD_LIBRARY_PATH...
Ilya Shipitsin [Fri, 20 Dec 2019 18:55:01 +0000 (23:55 +0500)] 
BUILD: travis-ci: link with ssl libraries using rpath instead of LD_LIBRARY_PATH/DYLD_LIBRARY_PATH

modifying LD_LIBRARY_PATH/DYLD_LIBRARY_PATH also affects other utilities like curl
to avoid side effects let us use rpath for ssl library linking

Fixes #418

5 years agoBUILD: ssl: improve SSL_CTX_set_ecdh_auto compatibility
Lukas Tribus [Fri, 20 Dec 2019 17:47:18 +0000 (18:47 +0100)] 
BUILD: ssl: improve SSL_CTX_set_ecdh_auto compatibility

SSL_CTX_set_ecdh_auto() is not defined when OpenSSL 1.1.1 is compiled
with the no-deprecated option. Remove existing, incomplete guards and
add a compatibility macro in openssl-compat.h, just as OpenSSL does:

https://github.com/openssl/openssl/blob/bf4006a6f9be691ba6eef0e8629e63369a033ccf/include/openssl/ssl.h#L1486

This should be backported as far as 2.0 and probably even 1.9.

5 years agoBUG/MEDIUM: stream: Be sure to never assign a TCP backend to an HTX stream
Christopher Faulet [Fri, 20 Dec 2019 14:59:20 +0000 (15:59 +0100)] 
BUG/MEDIUM: stream: Be sure to never assign a TCP backend to an HTX stream

With a TCP frontend, it is possible to upgrade a connection to HTTP when the
backend is in HTTP mode. Concretly the upgrade install a new mux. So, once it is
done, the downgrade to TCP is no longer possible. So we must take care to never
assign a TCP backend to a stream on this connection. Otherwise, HAProxy crashes
because raw data from the server are handled as structured data on the client
side.

This patch fixes the issue #420. It must be backported to all versions
supporting the HTX.

5 years agoBUG/MAJOR: mux-h1: Don't pretend the input channel's buffer is full if empty
Christopher Faulet [Fri, 20 Dec 2019 16:33:24 +0000 (17:33 +0100)] 
BUG/MAJOR: mux-h1: Don't pretend the input channel's buffer is full if empty

A regression was introduced by the commit 76014fd1 ("MEDIUM: h1-htx: Add HTX EOM
block when the message is in H1_MSG_DONE state"). When nothing is copied in the
channel's buffer when the input message is parsed, we erroneously pretend it is
because there is not enough room by setting the CS_FL_WANT_ROOM flag on the
conn-stream. This happens when a partial request is parsed. Because of this
flag, we never try anymore to get input data from the mux because we first wait
for more room in the channel's buffer, which is empty. Because of this bug, it
is pretty easy to freeze a h1 connection.

To fix the bug, we must obsiously set the CS_FL_WANT_ROOM flag only when there
are still data to transfer while the channel's buffer is not empty.

This patch must be backported if the patch 76014fd1 is backported too. So for
now, no backport needed.

5 years agoBUG/MINOR: state-file: do not leak memory on parse errors
Willy Tarreau [Fri, 20 Dec 2019 16:26:27 +0000 (17:26 +0100)] 
BUG/MINOR: state-file: do not leak memory on parse errors

Issue #417 reports a possible memory leak in the state-file loading code.
There's one such place in the loop which corresponds to parsing errors
where the curreently allocated line is not freed when dropped. In any
case this is very minor in that no more than the file's length may be
lost in the worst case, considering that the whole file is kept anyway
in case of success. This fix addresses this.

It should be backported to 2.1.

5 years agoBUG/MINOR: state-file: do not store duplicates in the global tree
Willy Tarreau [Fri, 20 Dec 2019 16:23:40 +0000 (17:23 +0100)] 
BUG/MINOR: state-file: do not store duplicates in the global tree

The global state file tree isn't configured for unique keys, so if an
entry appears multiple times, e.g. due to a bogus script that concatenates
entries multiple times, this will needlessly eat memory. Let's just drop
duplicates.

This should be backported to 2.1.

5 years agoBUG/MEDIUM: state-file: do not allocate a full buffer for each server entry
Willy Tarreau [Fri, 20 Dec 2019 16:18:13 +0000 (17:18 +0100)] 
BUG/MEDIUM: state-file: do not allocate a full buffer for each server entry

Starting haproxy with a state file of 700k servers eats 11.2 GB of RAM
due to a mistake in the function that loads the strings into a tree: it
allocates a full buffer for each backend+server name instead of allocating
just the required string. By just fixing this we're down to 80 MB.

This should be backported to 2.1.

5 years agoBUG/MINOR: ssl: openssl-compat: Fix getm_ defines
Rosen Penev [Thu, 19 Dec 2019 20:54:13 +0000 (12:54 -0800)] 
BUG/MINOR: ssl: openssl-compat: Fix getm_ defines

LIBRESSL_VERSION_NUMBER evaluates to 0 under OpenSSL, making the condition
always true. Check for the define before checking it.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
[wt: to be backported as far as 1.9]

5 years agoREGTEST: make the "set ssl cert" require version 2.1
Willy Tarreau [Fri, 20 Dec 2019 13:35:18 +0000 (14:35 +0100)] 
REGTEST: make the "set ssl cert" require version 2.1

This test fails on 2.0 and earlier since the feature was introduced in 2.1,
let's add the REQUIRE_VERSION tag.

5 years agoBUG/MEDIUM: fd/threads: fix a concurrency issue between add and rm on the same fd
Olivier Houchard [Thu, 19 Dec 2019 17:33:08 +0000 (18:33 +0100)] 
BUG/MEDIUM: fd/threads: fix a concurrency issue between add and rm on the same fd

There's a very hard-to-trigger bug in the FD list code where the
fd_add_to_fd_list() function assumes that if the FD it's trying to add
is already locked, it's in the process of being added. Unfortunately, it
can also be in the process of being removed. It is very hard to trigger
because it requires that one thread is removing the FD while another one
is adding it. First very few FDs run on multiple threads (listeners and
DNS), and second, it does not make sense to add and remove the FD at the
same time.

In practice the DNS code built on the older callback-only model does
perform bursts of fd_want_send() for all resolvers at once when it wants
to send a new query (dns_send_query()). And this is more likely to happen
when here are lots of resolutions in parallel and many resolvers, because
the dns_response_recv() callback can also trigger a series of queries on
all resolvers for each invalid response it receives. This means that it
really is perfectly possible to both stop and start in parallel during
short periods of time there.

This issue was not reported before 2.1, but 2.1 had the FD cache, built
on the exact same code base. It's very possible that the issue caused
exactly the opposite situation, where an event was occasionally lost,
causing a DNS retry that worked, and nobody noticing the problem in the
end. In 2.1 the lost entries are the updates asking for not polling for
writes anymore, and the effect is that the poller contiuously reports
writability on the socket when the issue happens.

This patch fixes bug #416 and must be backported as far as 1.8, and
absolutely requires that previous commit "MINOR: fd/threads: make
_GET_NEXT()/_GET_PREV() use the volatile attribute" is backported as
well otherwise it will make the issue worse.

Special thanks to Julien Pivotto for setting up a reliable reproducer
for this difficult issue.

5 years agoMINOR: fd/threads: make _GET_NEXT()/_GET_PREV() use the volatile attribute
Willy Tarreau [Fri, 20 Dec 2019 06:20:00 +0000 (07:20 +0100)] 
MINOR: fd/threads: make _GET_NEXT()/_GET_PREV() use the volatile attribute

These macros are either used between atomic ops which cause the volatile
to be implicit, or with an explicit volatile cast. However not having it
in the macro causes some traps in the code because certain loop paths
cannot safely be used without risking infinite loops if one isn't careful
enough.

Let's place the volatile attribute inside the macros and remove them from
the explicit places to avoid this. It was verified that the output executable
remains exactly the same byte-wise.

5 years agoBUG/MEDIUM: ssl: Revamp the way early data are handled.
Olivier Houchard [Thu, 19 Dec 2019 14:02:39 +0000 (15:02 +0100)] 
BUG/MEDIUM: ssl: Revamp the way early data are handled.

Instead of attempting to read the early data only when the upper layer asks
for data, allocate a temporary buffer, stored in the ssl_sock_ctx, and put
all the early data in there. Requiring that the upper layer takes care of it
means that if for some reason the upper layer wants to emit data before it
has totally read the early data, we will be stuck forever.

This should be backported to 2.1 and 2.0.
This may fix github issue #411.

5 years agoBUG/MAJOR: task: add a new TASK_SHARED_WQ flag to fix foreing requeuing
Willy Tarreau [Thu, 19 Dec 2019 06:39:06 +0000 (07:39 +0100)] 
BUG/MAJOR: task: add a new TASK_SHARED_WQ flag to fix foreing requeuing

Since 1.9 with commit b20aa9eef3 ("MAJOR: tasks: create per-thread wait
queues") a task bound to a single thread will not use locks when being
queued or dequeued because the wait queue is assumed to be the owner
thread's.

But there exists a rare situation where this is not true: the health
check tasks may be running on one thread waiting for a response, and
may in parallel be requeued by another thread calling health_adjust()
after a detecting a response error in traffic when "observe l7" is set,
and "fastinter" is lower than "inter", requiring to shorten the running
check's timeout. In this case, the task being requeued was present in
another thread's wait queue, thus opening a race during task_unlink_wq(),
and gets requeued into the calling thread's wait queue instead of the
running one's, opening a second race here.

This patch aims at protecting against the risk of calling task_unlink_wq()
from one thread while the task is queued on another thread, hence unlocked,
by introducing a new TASK_SHARED_WQ flag.

This new flag indicates that a task's position in the wait queue may be
adjusted by other threads than then one currently executing it. This means
that such WQ manipulations must be performed under a lock. There are two
types of such tasks:
  - the global ones, using the global wait queue (technically speaking,
    those whose thread_mask has at least 2 bits set).
  - some local ones, which for now will be placed into the global wait
    queue as well in order to benefit from its lock.

The flag is automatically set on initialization if the task's thread mask
indicates more than one thread. The caller must also set it if it intends
to let other threads update the task's expiration delay (e.g. delegated
I/Os), or if it intends to change the task's affinity over time as this
could lead to the same situation.

Right now only the situation described above seems to be affected by this
issue, and it is very difficult to trigger, and even then, will often have
no visible effect beyond stopping the checks for example once the race is
met. On my laptop it is feasible with the following config, chained to
httpterm:

    global
        maxconn 400 # provoke FD errors, calling health_adjust()

    defaults
        mode http
        timeout client 10s
        timeout server 10s
        timeout connect 10s

    listen px
        bind :8001
        option httpchk /?t=50
        server sback 127.0.0.1:8000 backup
        server-template s 0-999 127.0.0.1:8000 check port 8001 inter 100 fastinter 10 observe layer7

This patch will automatically address the case for the checks because
check tasks are created with multiple threads bound and will get the
TASK_SHARED_WQ flag set.

If in the future more tasks need to rely on this (multi-threaded muxes
for example) and the use of the global wait queue becomes a bottleneck
again, then it should not be too difficult to place locks on the local
wait queues and queue the task on its bound thread.

This patch needs to be backported to 2.1, 2.0 and 1.9. It depends on
previous patch "MINOR: task: only check TASK_WOKEN_ANY to decide to
requeue a task".

Many thanks to William Dauchy for providing detailed traces allowing to
spot the problem.

5 years agoMINOR: task: only check TASK_WOKEN_ANY to decide to requeue a task
Willy Tarreau [Thu, 19 Dec 2019 06:34:20 +0000 (07:34 +0100)] 
MINOR: task: only check TASK_WOKEN_ANY to decide to requeue a task

After processing a task, its RUNNING bit is cleared and at the same time
we check for other bits to decide whether to requeue the task or not. It
happens that we only want to check the TASK_WOKEN_* bits, because :
  - TASK_RUNNING was just cleared
  - TASK_GLOBAL and TASK_QUEUE cannot be set yet as the task was running,
    preventing it from being requeued

It's important not to catch yet undefined flags there because it would
prevent addition of new task flags. This also shows more clearly that
waking a task up with flags 0 is not something safe to do as the task
will not be woken up if it's already running.

5 years agoREGTEST: run-regtests: implement #REQUIRE_BINARIES
William Lallemand [Thu, 19 Dec 2019 13:30:00 +0000 (14:30 +0100)] 
REGTEST: run-regtests: implement #REQUIRE_BINARIES

Implement #REQUIRE_BINARIES for vtc files.

The run-regtests.sh script will check if the binary is available in the
environment, if not, it wil disable the vtc.

5 years agoREGTEST: ssl: test the "set ssl cert" CLI command
William Lallemand [Thu, 19 Dec 2019 10:25:19 +0000 (11:25 +0100)] 
REGTEST: ssl: test the "set ssl cert" CLI command

Add a reg-test which test the update of a certificate over the CLI. This
test requires socat and curl.

This commit also adds an ECDSA certificate in the ssl directory.

5 years agoMINOR: http: add a new "replace-path" action
Willy Tarreau [Tue, 17 Dec 2019 05:52:51 +0000 (06:52 +0100)] 
MINOR: http: add a new "replace-path" action

This action is very similar to "replace-uri" except that it only acts on the
path component. This is assumed to better match users' expectations when they
used to rely on "replace-uri" in HTTP/1 because mostly origin forms were used
in H1 while mostly absolute URI form is used in H2, and their rules very often
start with a '/', and as such do not match.

It could help users to get this backported to 2.0 and 2.1.

5 years agoMINOR: debug: support logging to various sinks
Willy Tarreau [Tue, 17 Dec 2019 09:07:25 +0000 (10:07 +0100)] 
MINOR: debug: support logging to various sinks

As discussed in the thread below [1], the debug converter is currently
not of much use given that it's only built when DEBUG_EXPR is set, and
it is limited to stderr only.

This patch changes this to make it take an optional prefix and an optional
target sink so that it can log to stdout, stderr or a ring buffer. The
default output is the "buf0" ring buffer, that can be consulted from the
CLI.

[1] https://www.mail-archive.com/haproxy@formilux.org/msg35671.html

Note: if this patch is backported, it also requires the following commit to
work: 46dfd78cbf ("BUG/MINOR: sample: always check converters' arguments").

5 years agoBUG/MINOR: ssl/cli: fix build for openssl < 1.0.2
William Lallemand [Wed, 18 Dec 2019 19:36:01 +0000 (20:36 +0100)] 
BUG/MINOR: ssl/cli: fix build for openssl < 1.0.2

Commit d4f946c ("MINOR: ssl/cli: 'show ssl cert' give information on the
certificates") introduced a build issue with openssl version < 1.0.2
because it uses the certificate bundles.

5 years agoMINOR: ssl/cli: 'show ssl cert' give information on the certificates
William Lallemand [Thu, 5 Dec 2019 09:26:40 +0000 (10:26 +0100)] 
MINOR: ssl/cli: 'show ssl cert' give information on the certificates

Implement the 'show ssl cert' command on the CLI which list the frontend
certificates. With a certificate name in parameter it will show more
details.

5 years agoBUG/MEDIUM: ssl: Don't set the max early data we can receive too early.
Olivier Houchard [Tue, 17 Dec 2019 14:39:54 +0000 (15:39 +0100)] 
BUG/MEDIUM: ssl: Don't set the max early data we can receive too early.

When accepting the max early data, don't set it on the SSL_CTX while parsing
the configuration, as at this point global.tune.maxrewrite may still be -1,
either because it was not set, or because it hasn't been set yet. Instead,
set it for each connection, just after we created the new SSL.
Not doing so meant that we could pretend to accept early data bigger than one
of our buffer.

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

5 years agoMINOR: sample: Validate the number of bits for the sha2 converter
Tim Duesterhus [Tue, 17 Dec 2019 11:31:20 +0000 (12:31 +0100)] 
MINOR: sample: Validate the number of bits for the sha2 converter

Instead of failing the conversion when an invalid number of bits is
given the sha2 converter now fails with an appropriate error message
during startup.

The sha2 converter was introduced in d4376302377e4f51f43a183c2c91d929b27e1ae3,
which is in 2.1 and higher.

5 years agoBUG/MINOR: sample: always check converters' arguments
Willy Tarreau [Tue, 17 Dec 2019 09:25:29 +0000 (10:25 +0100)] 
BUG/MINOR: sample: always check converters' arguments

In 1.5-dev20, sample-fetch arguments parsing was addresse by commit
689a1df0a1 ("BUG/MEDIUM: sample: simplify and fix the argument parsing").
The issue was that argument checks were not run for sample-fetches if
parenthesis were not present. Surprisingly, the fix was mde only for
sample-fetches and not for converters which suffer from the exact same
problem. There are even a few comments in the code mentioning that some
argument validation functions are not called when arguments are missing.

This fix applies the exact same method as the one above. The impact of
this bug is limited because over the years the code has learned to work
around this issue instead of fixing it.

This may be backported to all maintained versions.

5 years agoBUG/MINOR: sample: fix the closing bracket and LF in the debug converter
Willy Tarreau [Tue, 17 Dec 2019 08:00:15 +0000 (09:00 +0100)] 
BUG/MINOR: sample: fix the closing bracket and LF in the debug converter

The closing bracket was emitted for the "debug" converter even when the
opening one was not sent, and the new line was not always emitted. Let's
fix this. This is harmless since this converter is not built by default.

5 years agoDOC: clarify the fact that replace-uri works on a full URI
Willy Tarreau [Tue, 17 Dec 2019 05:51:20 +0000 (06:51 +0100)] 
DOC: clarify the fact that replace-uri works on a full URI

With H2 deployments becoming more common, replace-uri starts to hit
users by not always matching absolute URIs due to rules expecting the
URI to start with a '/'.

5 years agoREGTEST: Add an HTX reg-test to check an edge case
Christopher Faulet [Wed, 11 Dec 2019 15:26:11 +0000 (16:26 +0100)] 
REGTEST: Add an HTX reg-test to check an edge case

This test checks that an HTTP message is properly processed when we failed to
add the HTX EOM block in an HTX message during the parsing because the buffer is
full. Some space must be released in the buffer to make it possible. This
requires an extra pass in the H1 multiplexer. Here, we must be sure the mux is
called while there is no more incoming data.

It is a "devel" test because conditions to run the test successfully is highly
dependent on the implementation. So if it fail, it is not necessarily a bug. It
may be due of an internal change. It relies on internal HTX sample fetches.

5 years agoMINOR: http-htx: Add some htx sample fetches for debugging purpose
Christopher Faulet [Wed, 11 Dec 2019 14:52:32 +0000 (15:52 +0100)] 
MINOR: http-htx: Add some htx sample fetches for debugging purpose

These sample fetches are internal and must be used for debugging purpose. Idea
is to have a way to add some checks on the HTX content from http rules. The main
purpose is to ease reg-tests writing.

5 years agoMEDIUM: h1-htx: Add HTX EOM block when the message is in H1_MSG_DONE state
Christopher Faulet [Tue, 10 Dec 2019 10:47:22 +0000 (11:47 +0100)] 
MEDIUM: h1-htx: Add HTX EOM block when the message is in H1_MSG_DONE state

During H1 parsing, the HTX EOM block is added before switching the message state
to H1_MSG_DONE. It is an exception in the way to convert an H1 message to
HTX. Except for this block, the message is first switched to the right state
before starting to add the corresponding HTX blocks. For instance, the message
is switched in H1_MSG_DATA state and then the HTX DATA blocks are added.

With this patch, the message is switched to the H1_MSG_DONE state when all data
blocks or trailers were processed. It is the caller responsibility to call
h1_parse_msg_eom() when the H1_MSG_DONE state is reached. This way, it is far
easier to catch failures when the HTX buffer is full.

The H1 and FCGI muxes have been updated accordingly.

This patch may eventually be backported to 2.1 if it helps other backports.

5 years agoBUILD/MINOR: unix sockets: silence an absurd gcc warning about strncpy()
Willy Tarreau [Wed, 11 Dec 2019 15:29:10 +0000 (16:29 +0100)] 
BUILD/MINOR: unix sockets: silence an absurd gcc warning about strncpy()

Apparently gcc developers decided that strncpy() semantics are no longer
valid and now deserve a warning, especially if used exactly as designed.
This results in issue #304. Let's just remove one to the target size to
please her majesty gcc, the God of C Compilers, who tries hard to make
users completely eliminate any use of string.h and reimplement it by
themselves at much higher risks. Pfff....

This can be backported to stable version, the fix is harmless since it
ignores the last zero that is already set on next line.

5 years agoBUG/MINOR: listener: fix off-by-one in state name check
Willy Tarreau [Wed, 11 Dec 2019 14:51:37 +0000 (15:51 +0100)] 
BUG/MINOR: listener: fix off-by-one in state name check

As reported in issue #380, the state check in listener_state_str() is
invalid as it allows state value 9 to report crap. We don't use such
a state value so the issue should never happen unless the memory is
already corrupted, but better clean this now while it's harmless.

This should be backported to all maintained branches.

5 years agoBUG/MINOR: server: make "agent-addr" work on default-server line
Willy Tarreau [Wed, 11 Dec 2019 14:43:45 +0000 (15:43 +0100)] 
BUG/MINOR: server: make "agent-addr" work on default-server line

As reported in issue #408, "agent-addr" doesn't work on default-server
lines. This is due to the transcription of the old "addr" option in commit
6e5e0d8f9e ("MINOR: server: Make 'default-server' support 'addr' keyword.")
which correctly assigns it to the check.addr and agent.addr fields, but
which also copies the default check.addr into both the check's and the
agent's addr fields. Thus the default agent's address is never used.

This fix makes sure to copy the check from the check and the agent from
the agent. However it's worth noting that if "addr" is specified on the
server line, it will still overwrite both the check and the agent's
addresses.

This must be backported as far as 1.8.

5 years agoBUG/MINOR: listener: do not immediately resume on transient error
Willy Tarreau [Wed, 11 Dec 2019 14:06:30 +0000 (15:06 +0100)] 
BUG/MINOR: listener: do not immediately resume on transient error

The listener supports a "transient error" situation, which corresponds
to those situations where accept fails badly but poll() reports an event.
This happens for example when a listener is paused, or on out of FD. The
same mechanism is used when facing a maxconn or maxsessrate limitation.
When this happens, the listener is disabled for up to 100ms and put back
into the global listener queue so that it automatically wakes up again
as soon as the conditions change from an existing connection releasing
one resource, or the system recovers from a transient issue.

The listener_accept() function has a bug in its exit path causing a
freshly limited listener to be immediately enabled again because all
the conditions are met (connection count < max). It doesn't take into
account the fact that the listener might have been queued and must
first wait for the timeout to expire before doing so. The impact is
that upon certain errors, the faulty process will busy loop on the
accept code without sleeping. This is the scenario reported and
diagnosed by @hedong0411 in issue #382.

This commit fixes it by verifying that the global queue's delay is
at least expired before deciding to resume the listener. Another
approach could consist in having an extra state like LI_DELAY for
situations where only a delay is acceptable, but this would probably
not bring anything except more complex code.

This issue was introduced with the lock-free listener accept code
(commits 3f0d02b and 82c9789a) that were backported to 1.8.20+ and
1.9.7+, so this fix must be backported to the relevant branches.

5 years agoBUG/MINOR: mworker: properly pass SIGTTOU/SIGTTIN to workers
Willy Tarreau [Wed, 11 Dec 2019 13:24:07 +0000 (14:24 +0100)] 
BUG/MINOR: mworker: properly pass SIGTTOU/SIGTTIN to workers

If a new process is started with -sf and it fails to bind, it may send
a SIGTTOU to the master process in hope that it will temporarily unbind.
Unfortunately this one doesn't catch it and stops to background instead
of forwarding the signal to the workers. The same is true for SIGTTIN.

This commit simply implements an extra signal handler for the master to
deal with such signals that must be passed down to the workers. It must
be backported as far as 1.8, though there the code differs in that it's
entirely in haproxy.c and doesn't require an extra sig handler.

5 years agoBUG/MINOR: log: fix minor resource leaks on logformat error path
Willy Tarreau [Wed, 11 Dec 2019 11:05:39 +0000 (12:05 +0100)] 
BUG/MINOR: log: fix minor resource leaks on logformat error path

As reported by Ilya in issue #392, Coverity found that we're leaking
allocated strings on error paths in parse_logformat(). Let's use a
proper exit label for failures instead of seeding return 0 everywhere.

This should be backported to all supported versions.

5 years agoDOC: remove references to the outdated architecture.txt
Willy Tarreau [Wed, 11 Dec 2019 10:55:52 +0000 (11:55 +0100)] 
DOC: remove references to the outdated architecture.txt

As mentionned in bug #405 we continue to reference architecture.txt from
places in the doc despite this file not being packaged for many years.
Better drop the reference if it's confusing.

5 years agoDOC: proxies: HAProxy only supports 3 connection modes
Julien Pivotto [Tue, 10 Dec 2019 12:11:17 +0000 (13:11 +0100)] 
DOC: proxies: HAProxy only supports 3 connection modes

The 4th one (forceclose) has been deprecated and deleted from the
documentation in 10c6c16cde0b0b473a1ab16e958a7d6b61ed36fc

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
5 years agoMINOR: tasks: split wake_expired_tasks() in two parts to avoid useless wakeups
Willy Tarreau [Wed, 11 Dec 2019 07:12:23 +0000 (08:12 +0100)] 
MINOR: tasks: split wake_expired_tasks() in two parts to avoid useless wakeups

We used to have wake_expired_tasks() wake up tasks and return the next
expiration delay. The problem this causes is that we have to call it just
before poll() in order to consider latest timers, but this also means that
we don't wake up all newly expired tasks upon return from poll(), which
thus systematically requires a second poll() round.

This is visible when running any scheduled task like a health check, as there
are systematically two poll() calls, one with the interval, nothing is done
after it, and another one with a zero delay, and the task is called:

  listen test
    bind *:8001
    server s1 127.0.0.1:1111 check

  09:37:38.200959 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=8696843}) = 0
  09:37:38.200967 epoll_wait(3, [], 200, 1000) = 0
  09:37:39.202459 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=8712467}) = 0
>> nothing run here, as the expired task was not woken up yet.
  09:37:39.202497 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=8715766}) = 0
  09:37:39.202505 epoll_wait(3, [], 200, 0) = 0
  09:37:39.202513 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=8719064}) = 0
>> now the expired task was woken up
  09:37:39.202522 socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 7
  09:37:39.202537 fcntl(7, F_SETFL, O_RDONLY|O_NONBLOCK) = 0
  09:37:39.202565 setsockopt(7, SOL_TCP, TCP_NODELAY, [1], 4) = 0
  09:37:39.202577 setsockopt(7, SOL_TCP, TCP_QUICKACK, [0], 4) = 0
  09:37:39.202585 connect(7, {sa_family=AF_INET, sin_port=htons(1111), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRESS (Operation now in progress)
  09:37:39.202659 epoll_ctl(3, EPOLL_CTL_ADD, 7, {EPOLLOUT, {u32=7, u64=7}}) = 0
  09:37:39.202673 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=8814713}) = 0
  09:37:39.202683 epoll_wait(3, [{EPOLLOUT|EPOLLERR|EPOLLHUP, {u32=7, u64=7}}], 200, 1000) = 1
  09:37:39.202693 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=8818617}) = 0
  09:37:39.202701 getsockopt(7, SOL_SOCKET, SO_ERROR, [111], [4]) = 0
  09:37:39.202715 close(7)                = 0

Let's instead split the function in two parts:
  - the first part, wake_expired_tasks(), called just before
    process_runnable_tasks(), wakes up all expired tasks; it doesn't
    compute any timeout.
  - the second part, next_timer_expiry(), called just before poll(),
    only computes the next timeout for the current thread.

Thanks to this, all expired tasks are properly woken up when leaving
poll, and each poll call's timeout remains up to date:

  09:41:16.270449 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=10223556}) = 0
  09:41:16.270457 epoll_wait(3, [], 200, 999) = 0
  09:41:17.270130 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=10238572}) = 0
  09:41:17.270157 socket(AF_INET, SOCK_STREAM, IPPROTO_TCP) = 7
  09:41:17.270194 fcntl(7, F_SETFL, O_RDONLY|O_NONBLOCK) = 0
  09:41:17.270204 setsockopt(7, SOL_TCP, TCP_NODELAY, [1], 4) = 0
  09:41:17.270216 setsockopt(7, SOL_TCP, TCP_QUICKACK, [0], 4) = 0
  09:41:17.270224 connect(7, {sa_family=AF_INET, sin_port=htons(1111), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRESS (Operation now in progress)
  09:41:17.270299 epoll_ctl(3, EPOLL_CTL_ADD, 7, {EPOLLOUT, {u32=7, u64=7}}) = 0
  09:41:17.270314 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=10337841}) = 0
  09:41:17.270323 epoll_wait(3, [{EPOLLOUT|EPOLLERR|EPOLLHUP, {u32=7, u64=7}}], 200, 1000) = 1
  09:41:17.270332 clock_gettime(CLOCK_THREAD_CPUTIME_ID, {tv_sec=0, tv_nsec=10341860}) = 0
  09:41:17.270340 getsockopt(7, SOL_SOCKET, SO_ERROR, [111], [4]) = 0
  09:41:17.270367 close(7)                = 0

This may be backported to 2.1 and 2.0 though it's unlikely to bring any
user-visible improvement except to clarify debugging.

5 years agoBUG/MINOR: tasks: only requeue a task if it was already in the queue
Willy Tarreau [Wed, 11 Dec 2019 08:11:58 +0000 (09:11 +0100)] 
BUG/MINOR: tasks: only requeue a task if it was already in the queue

Commit 0742c314c3 ("BUG/MEDIUM: tasks: Make sure we switch wait queues
in task_set_affinity().") had a slight side effect on expired timeouts,
which is that when used before a timeout is updated, it will cause an
existing task to be requeued earlier than its expected timeout when done
before being updated, resulting in the next poll wakup timeout too early
or even instantly if the previous wake up was done on a timeout. This is
visible in strace when health checks are enabled because there are two
poll calls, one of which has a short or zero delay. The correct solution
is to only requeue a task if it was already in the queue.

This can be backported to all branches having the fix above.

5 years agoDOC: listeners: add a few missing transitions
Willy Tarreau [Wed, 11 Dec 2019 05:38:14 +0000 (06:38 +0100)] 
DOC: listeners: add a few missing transitions

Some disable() transitions were missing, and the distinction between
multi-threaded and single-threaded transitions was not mentioned.

5 years agoBUG/MEDIUM: proto_udp/threads: recv() and send() must not be exclusive.
Willy Tarreau [Tue, 10 Dec 2019 17:12:04 +0000 (18:12 +0100)] 
BUG/MEDIUM: proto_udp/threads: recv() and send() must not be exclusive.

This is a complement to previous fix for bug #399. The exclusion between
the recv() and send() calls prevents send handlers from being called if
rx readiness is reported. The DNS code can trigger this situations with
threads where the fd_recv_ready() flag disappears between the test in
dgram_fd_handler() and the second test in dns_resolve_recv() while a
thread calls fd_cant_recv(), and this situation can sustain itself for
a while. With 8 threads and an error in the socket queue, placing a
printf on the return statement in dns_resolve_recv() scrolls very fast.

Simply removing the "else" in dgram_fd_handler() addresses the issue.

This fix must be backported as far as 1.6.

5 years agoBUG/MAJOR: dns: add minimalist error processing on the Rx path
Willy Tarreau [Tue, 10 Dec 2019 17:38:09 +0000 (18:38 +0100)] 
BUG/MAJOR: dns: add minimalist error processing on the Rx path

It was reported in bug #399 that the DNS sometimes enters endless loops
after hours working fine. The issue is caused by a lack of error
processing in the DNS's recv() path combined with an exclusive recv OR
send in the UDP layer, resulting in some errors causing CPU loops that
will never stop until the process is restarted.

The basic cause is that the FD_POLL_ERR and FD_POLL_HUP flags are sticky
on the FD, and contrary to a stream socket, receiving an error on a
datagram socket doesn't indicate that this socket cannot be used anymore.
Thus the Rx code must at least handle this situation and flush the error
otherwise it will constantly be reported. In theory this should not be a
big issue but in practise it is due to another bug in the UDP datagram
handler which prevents the send() callback from being called when Rx
readiness was reported, so the situation cannot go away. It happens way
more easily with threads enabled, so that there is no dead time between
the moment the FD is disabled and another recv() is called, such as in
the example below where the request was sent to a closed port on the
loopback provoking an ICMP unreachable to be sent back:

  [pid 20888] 18:26:57.826408 sendto(29, ";\340\1\0\0\1\0\0\0\0\0\1\0031wt\2eu\0\0\34\0\1\0\0)\2\0\0\0\0\0\0\0", 35, 0, NULL, >
  [pid 20893] 18:26:57.826566 recvfrom(29, 0x7f97c54ef2f0, 513, 0, NULL, NULL) = -1 ECONNREFUSED (Connection refused)
  [pid 20889] 18:26:57.826601 recvfrom(29, 0x7f97c76182f0, 513, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
  [pid 20892] 18:26:57.826630 recvfrom(29, 0x7f97c5cf02f0, 513, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
  [pid 20891] 18:26:57.826684 recvfrom(29, 0x7f97c66162f0, 513, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
  [pid 20895] 18:26:57.826716 recvfrom(29, 0x7f97bffda2f0, 513, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
  [pid 20894] 18:26:57.826747 recvfrom(29, 0x7f97c4cee2f0, 513, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
  [pid 20888] 18:26:58.419838 recvfrom(29, 0x7ffcc8712c20, 513, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
  [pid 20893] 18:26:58.419900 recvfrom(29, 0x7f97c54ef2f0, 513, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
  (... hundreds before next sendto() ...)

This situation was handled by clearing HUP and ERR when recv()
returns <0.

A second case was handled, there was a control for a missing dgram
handler, but it does nothing, causing the FD to ring again if this
situation ever happens. After looking at the rest of the code, it
doesn't seem possible to face such a situation because these handlers
are registered during startup, but at least we need to handle it
properly.

A third case was handled, that's mainly a small optimization. With
threads and massive responses, due to the large lock around the loop,
it's likely that some threads will have seen fd_recv_ready() and will
wait at the lock(). But if they wait here, chances are that other
threads will have eliminated pending data and issued fd_cant_recv().
In this case, better re-check fd_recv_ready() before performing the
recv() call to avoid the huge amounts of syscalls that happen on
massively threaded setups.

This patch must be backported as far as 1.6 (the atomic AND just
needs to be turned to a regular AND).

5 years agoBUG/MEDIUM: kqueue: Make sure we report read events even when no data.
Olivier Houchard [Tue, 10 Dec 2019 17:22:55 +0000 (18:22 +0100)] 
BUG/MEDIUM: kqueue: Make sure we report read events even when no data.

When we have a EVFILT_READ event, an optimization was made, and the FD was
not reported as ready to receive if there were no data available. That way,
if the socket was closed by our peer (the EV8EOF flag was set), and there were
no remaining data to read, we would just close(), and avoid doing a recv().
However, it may be fine for TCP socket, but it is not for UDP.
If we send data via UDP, and we receive an error, the only way to detect it
is to attempt a recv(). However, in this case, kevent() will report a read
event, but with no data, so we'd just ignore that read event, nothing would be
done about it, and the poller would be woken up by it over and over.
To fix this, report read events if either we have data, or the EV_EOF flag
is not set.

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

5 years agoDOC: document the listener state transitions
Willy Tarreau [Tue, 10 Dec 2019 15:06:53 +0000 (16:06 +0100)] 
DOC: document the listener state transitions

This was done by reading all the code affecting a listener's state,
hopefully it will save some time in the future.

5 years agoREORG: listener: move the global listener queue code to listener.c
Willy Tarreau [Tue, 10 Dec 2019 10:18:41 +0000 (11:18 +0100)] 
REORG: listener: move the global listener queue code to listener.c

The global listener queue code and declarations were still lying in
haproxy.c while not needed there anymore at all. This complicates
the code for no reason. As a result, the global_listener_queue_task
and the global_listener_queue were made static.

5 years agoMINOR: listener: split dequeue_all_listener() in two
Willy Tarreau [Tue, 10 Dec 2019 13:10:52 +0000 (14:10 +0100)] 
MINOR: listener: split dequeue_all_listener() in two

We use it half times for the global_listener_queue and half times
for a proxy's queue and this requires the callers to take care of
these. Let's split it in two versions, the current one working only
on the global queue and another one dedicated to proxies for the
per-proxy queues. This cleans up quite a bit of code.

5 years agoMINOR: listener: make the wait paths cleaner and more reliable
Willy Tarreau [Tue, 10 Dec 2019 11:01:21 +0000 (12:01 +0100)] 
MINOR: listener: make the wait paths cleaner and more reliable

In listener_accept() there are several situations where we have to wait
for an event or a delay. These ones all implement their own call to
limit_listener() and the associated task_schedule(). In addition to
being ugly and confusing, one expire date computation is even wrong as
it doesn't take in account the fact that we're using threads and that
the value might change in the middle. Fortunately task_schedule() gets
it right for us.

This patch creates two jump locations, one for the global queue and
one for the proxy queue, allowing the rest of the code to only compute
the expire delay and jump to the right location.

5 years agoBUG/MEDIUM: listener/threads: fix a remaining race in the listener's accept()
Willy Tarreau [Tue, 10 Dec 2019 08:30:05 +0000 (09:30 +0100)] 
BUG/MEDIUM: listener/threads: fix a remaining race in the listener's accept()

Recent fix 4c044e274c ("BUG/MEDIUM: listener/thread: fix a race when
pausing a listener") is insufficient and moves the race slightly farther.
What now happens is that if we're limiting a listener due to a transient
error such as an accept() error for example, or because the proxy's
maxconn was reached, another thread might in the mean time have switched
again to LI_READY and at the end of the function we'll disable polling on
this FD, resulting in a listener that never accepts anything anymore. It
can more easily happen when sending SIGTTOU/SIGTTIN to temporarily pause
the listeners to let another process bind next to them.

What this patch does instead is to move all enable/disable operations at
the end of the function and condition them to the state. The listener's
state is checked under the lock and the FD's polling state adjusted
accordingly so that the listener's state and the FD always remain 100%
synchronized. It was verified with 16 threads that the cost of taking
that lock is not measurable so that's fine.

This should be backported to the same branches the patch above is
backported to.

5 years agoBUG/MINOR: listener: also clear the error flag on a paused listener
Willy Tarreau [Tue, 10 Dec 2019 07:42:21 +0000 (08:42 +0100)] 
BUG/MINOR: listener: also clear the error flag on a paused listener

When accept() fails because a listener is temporarily paused, the
FD might have both FD_POLL_HUP and FD_POLL_ERR bits set. While we do
not exploit FD_POLL_ERR here it's better to clear it because it is
reported on "show fd" and is confusing.

This may be backported to all versions.

5 years agoBUG/MINOR: listener/threads: always use atomic ops to clear the FD events
Willy Tarreau [Tue, 10 Dec 2019 07:37:04 +0000 (08:37 +0100)] 
BUG/MINOR: listener/threads: always use atomic ops to clear the FD events

There was a leftover of the single-threaded era when removing the
FD_POLL_HUP flag from the listeners. By not using an atomic operation
to clear the flag, another thread acting on the same listener might
have lost some events, though this would have resulted in that thread
to reprocess them immediately on the next loop pass.

This should be backported as far as 1.8.

5 years agoBUG/MINOR: proxy: make soft_stop() also close FDs in LI_PAUSED state
Willy Tarreau [Tue, 10 Dec 2019 06:11:35 +0000 (07:11 +0100)] 
BUG/MINOR: proxy: make soft_stop() also close FDs in LI_PAUSED state

The proxies' soft_stop() function closes the FDs in all opened states
except LI_PAUSED. This means that a transient error on a listener might
cause it to turn back to the READY state if it happens exactly when a
reload signal is received.

This must be backported to all supported versions.

5 years agoBUG/MEDIUM: mux-fcgi: Handle cases where the HTX EOM block cannot be inserted
Christopher Faulet [Fri, 6 Dec 2019 15:20:49 +0000 (16:20 +0100)] 
BUG/MEDIUM: mux-fcgi: Handle cases where the HTX EOM block cannot be inserted

During the HTTP response parsing, if there is not enough space in the channel's
buffer, it is possible to fail to add the HTX EOM block while all data in the
rxbuf were consumed. As for the h1 mux, we must notify the conn-stream the
buffer is full to have a chance to add the HTX EOM block later. In this case, we
must also be carefull to not report a server abort by setting too early the
CS_FL_EOS flag on the conn-stream.

To do so, the FCGI_SF_APPEND_EOM flag must be set on the FCGI stream to know the
HTX EOM block is missing.

This patch must be backported to 2.1.

5 years agoBUG/MINOR: mux-h1: Be sure to set CS_FL_WANT_ROOM when EOM can't be added
Christopher Faulet [Fri, 6 Dec 2019 14:59:05 +0000 (15:59 +0100)] 
BUG/MINOR: mux-h1: Be sure to set CS_FL_WANT_ROOM when EOM can't be added

During the message parsing, when the HTX buffer is full and only the HTX EOM
block cannot be added, it is important to notify the conn-stream that some
processing must still be done but it is blocked because there is not enough room
in the buffer. The way to do so is to set the CS_FL_WANT_ROOM flag on the
conn-stream. Otherwise, because all data are received and consumed, the mux is
not called anymore to add this last block, leaving the message unfinished from
the HAProxy point of view. The only way to unblock it is to receive a shutdown
for reads or to hit a timeout.

This patch must be backported to 2.1 and 2.0. The 1.9 does not seem to be
affected.

5 years agoMEDIUM: init: set NO_NEW_PRIVS by default when supported
Willy Tarreau [Fri, 6 Dec 2019 15:31:45 +0000 (16:31 +0100)] 
MEDIUM: init: set NO_NEW_PRIVS by default when supported

HAProxy doesn't need to call executables at run time (except when using
external checks which are strongly recommended against), and is even expected
to isolate itself into an empty chroot. As such, there basically is no valid
reason to allow a setuid executable to be called without the user being fully
aware of the risks. In a situation where haproxy would need to call external
checks and/or disable chroot, exploiting a vulnerability in a library or in
haproxy itself could lead to the execution of an external program. On Linux
it is possible to lock the process so that any setuid bit present on such an
executable is ignored. This significantly reduces the risk of privilege
escalation in such a situation. This is what haproxy does by default. In case
this causes a problem to an external check (for example one which would need
the "ping" command), then it is possible to disable this protection by
explicitly adding this directive in the global section. If enabled, it is
possible to turn it back off by prefixing it with the "no" keyword.

Before the option:

  $ socat - /tmp/sock1 <<< "expert-mode on; debug dev exec sudo /bin/id"
  uid=0(root) gid=0(root) groups=0(root

After the option:
  $ socat - /tmp/sock1 <<< "expert-mode on; debug dev exec sudo /bin/id"
  sudo: effective uid is not 0, is /usr/bin/sudo on a file system with the
        'nosuid' option set or an NFS file system without root privileges?

5 years agoMINOR: debug: replace popen() with pipe+fork() in "debug dev exec"
Willy Tarreau [Fri, 6 Dec 2019 16:18:28 +0000 (17:18 +0100)] 
MINOR: debug: replace popen() with pipe+fork() in "debug dev exec"

popen() is annoying because it doesn't catch stderr. The command was
implemented using it just by pure laziness, let's just redo it a bit
cleaner using normal syscalls. Note that this command is only enabled
when built with -DDEBUG_DEV.

5 years agoBUG/MEDIUM: checks: Make sure we set the task affinity just before connecting.
Olivier Houchard [Fri, 29 Nov 2019 15:18:51 +0000 (16:18 +0100)] 
BUG/MEDIUM: checks: Make sure we set the task affinity just before connecting.

In process_chk_conn(), make sure we set the task affinity to the current
thread as soon as we're attempting a connection (and reset the affinity to
"any thread" if we detect a failure).
We used to only set the task affinity if connect_conn_chk() returned
SF_ERR_NONE, however for TCP checks, SF_ERR_UP is returned, so for those
checks, the task could still run on any thread, and this could lead to a
race condition where the connection runs on one thread, while the task runs
on another one, which could create random memory corruption and/or crashes.
This may fix github issue #369.

This should be backported to 2.1, 2.0 and 1.9.

5 years agoBUG/MEDIUM: tasks: Make sure we switch wait queues in task_set_affinity().
Olivier Houchard [Thu, 5 Dec 2019 14:11:19 +0000 (15:11 +0100)] 
BUG/MEDIUM: tasks: Make sure we switch wait queues in task_set_affinity().

In task_set_affinity(), leave the wait_queue if any before changing the
affinity, and re-enter a wait queue once it is done. If we don't do that,
the task may stay in the wait queue of another thread, and we later may
end up modifying that wait queue while holding no lock, which could lead
to memory corruption.

THis should be backported to 2.1, 2.0 and 1.9.

5 years agoBUG/MINOR: mux-h1: Fix conditions to know whether or not we may receive data
Christopher Faulet [Thu, 5 Dec 2019 11:30:55 +0000 (12:30 +0100)] 
BUG/MINOR: mux-h1: Fix conditions to know whether or not we may receive data

The h1_recv_allowed() function is inherited from the h2 multiplexer. But for the
h1, conditions to know if we may receive data are less complex because there is
no multiplexing and because data are not parsed when received. So now, following
rules are respected :

 * if an error or a shutdown for reads was detected on the connection we must
   not attempt to receive
 * if the input buffer failed to be allocated or is full, we must not try to
   receive
 * if the input processing is busy waiting for the output side, we may attempt
   to receive
 * otherwise must may not attempt to receive

This patch must be backported as far as 1.9.

5 years agoBUG/MINOR: mux-h1: Don't rely on CO_FL_SOCK_RD_SH to set H1C_F_CS_SHUTDOWN
Christopher Faulet [Thu, 5 Dec 2019 10:18:31 +0000 (11:18 +0100)] 
BUG/MINOR: mux-h1: Don't rely on CO_FL_SOCK_RD_SH to set H1C_F_CS_SHUTDOWN

The CO_FL_SOCK_RD_SH flag is only set when a read0 is received. So we must not
rely on it to set the H1 connection in shutdown state (H1C_F_CS_SHUTDOWN). In
fact, it is suffisant to set the connection in shutdown state when the shutdown
for writes is forwared to the sock layer.

This patch must be backported as far as 1.9.

5 years agoBUG/MEDIUM: mux-h1: Never reuse H1 connection if a shutw is pending
Christopher Faulet [Thu, 5 Dec 2019 09:23:37 +0000 (10:23 +0100)] 
BUG/MEDIUM: mux-h1: Never reuse H1 connection if a shutw is pending

On the server side, when a H1 stream is detached from the connection, if the
connection is not reusable but some outgoing data remain, the connection is not
immediatly released. In this case, the connection is not inserted in any idle
connection list. But it is still attached to the session. Because of that, it
can be erroneously reused. h1_avail_streams() always report a free slot if no
stream is attached to the connection, independently on the connection's
state. It is obviously a bug. If a second request is handled by the same session
(it happens with H2 connections on the client side), this connection is reused
before we close it.

There is small window to hit the bug, but it may lead to very strange
behaviors. For instance, if a first h2 request is quickly aborted by the client
while it is blocked in the mux on the server side (so before any response is
received), a second request can be processed and sent to the server. Because the
connection was not closed, the possible reply to the first request will be
interpreted as a reply to the second one. It is probably the bug described by
Peter Fröhlich in the issue #290.

To fix the bug, a new flag has been added to know if an H1 connection is idle or
not. So now, H1C_F_CS_IDLE is set when a connection is idle and useable to
handle a new request. If it is set, we try to add the connection in an idle
connection list. And h1_avail_streams() only relies on this flag
now. Concretely, this flag is set when a K/A stream is detached and both the
request and the response are in DONE state. It is exclusive to other H1C_F_CS
flags.

This patch must be backported as far as 1.9.

5 years agoBUG/MINOR: ssl: certificate choice can be unexpected with openssl >= 1.1.1
Emmanuel Hocdet [Wed, 6 Nov 2019 15:05:34 +0000 (16:05 +0100)] 
BUG/MINOR: ssl: certificate choice can be unexpected with openssl >= 1.1.1

It's regression from 9f9b0c6 "BUG/MEDIUM: ECC cert should work with
TLS < v1.2 and openssl >= 1.1.1". Wilcard EC certifcate could be selected
at the expense of specific RSA certificate.
In any case, specific certificate should always selected first, next wildcard.
Reflect this rule in a loop to avoid any bug in certificate selection changes.

Fix issue #394.

It should be backported as far as 1.8.

5 years agoBUG/MEDIUM: listener/thread: fix a race when pausing a listener
Willy Tarreau [Thu, 5 Dec 2019 06:40:32 +0000 (07:40 +0100)] 
BUG/MEDIUM: listener/thread: fix a race when pausing a listener

There exists a race in the listener code where a thread might disable
receipt on a listener's FD then turn it to LI_PAUSED while at the same
time another one faces EAGAIN on accept() and enables it again via
fd_cant_recv(). The result is that the FD is in LI_PAUSED state with
its polling still enabled. listener_accept() does not do anything then
and doesn't disable the FD either, resulting in a thread eating all the
CPU as reported in issue #358. A solution would be to take the listener's
lock to perform the fd_cant_recv() call and do it only if the FD is still
in LI_READY state, but this would be totally overkill while in practice
the issue only happens during shutdown.

Instead what is done here is that when leaving we recheck the state and
disable polling if the listener is not in LI_READY state, which never
happens except when being limited. In the worst case there could be one
extra check per thread for the time required to converge, which is
absolutely nothing.

This fix was successfully tested, and should be backported to all
versions using the lock-free listeners, which means all those containing
commit 3f0d02bb ("MAJOR: listener: do not hold the listener lock in
listener_accept()"), hence 2.1, 2.0, 1.9.7+, 1.8.20+.

5 years agoBUG/MINOR: ssl/cli: don't overwrite the filters variable
William Lallemand [Wed, 4 Dec 2019 14:33:01 +0000 (15:33 +0100)] 
BUG/MINOR: ssl/cli: don't overwrite the filters variable

When a crt-list line using an already used ckch_store does not contain
filters, it will overwrite the ckchs->filters variable with 0.
This problem will generate all sni_ctx of this ckch_store without
filters. Filters generation mustn't be allowed in any case.

Must be backported in 2.1.

5 years agoBUG/MINOR: stream-int: avoid calling rcv_buf() when splicing is still possible
Willy Tarreau [Tue, 3 Dec 2019 17:13:04 +0000 (18:13 +0100)] 
BUG/MINOR: stream-int: avoid calling rcv_buf() when splicing is still possible

In si_cs_recv(), we can end up with a partial splice() call that will be
followed by an attempt to us rcv_buf(). Sometimes this works and places
data into the buffer, which then prevent splicing from being used, and
this causes splice() and recvfrom() calls to alternate. Better simply
refrain from calling rcv_buf() when there are data in the pipe and still
data to be forwarded. Usually this indicates that we've ate everything
available and that we still want to use splice() on subsequent calls.

This should be backported to 2.1 and 2.0.

5 years agoBUG/MEDIUM: stream-int: don't subscribed for recv when we're trying to flush data
Willy Tarreau [Tue, 3 Dec 2019 17:08:45 +0000 (18:08 +0100)] 
BUG/MEDIUM: stream-int: don't subscribed for recv when we're trying to flush data

If we cannot splice incoming data using rcv_pipe() due to remaining data
in the buffer, we must not subscribe to the mux but instead tag the
stream-int as blocked on missing Rx room. Otherwise when data are
flushed, calling si_chk_rcv() will have no effect because the WAIT_EP
flag remains present, and we'll end in an rx timeout. This case is very
hard to reproduce, and requires an inversion of the polling side in the
middle of a transfer. This can only happen when the client and the server
are using similar links and when splicing is enabled. It typically takes
hundreds of MB to GB for the problem to happen, and tends to be magnified
by the use of option contstats which causes process_stream() to be called
every 5s and to try again to recv.

This fix must be backported to 2.1, 2.0, and possibly 1.9.

5 years agoBUG/MINOR: ssl/cli: 'ssl cert' cmd only usable w/ admin rights
William Lallemand [Tue, 3 Dec 2019 12:32:54 +0000 (13:32 +0100)] 
BUG/MINOR: ssl/cli: 'ssl cert' cmd only usable w/ admin rights

The 3 commands 'set ssl cert', 'abort ssl cert' and 'commit ssl cert'
must be only usable with admin rights over the CLI.

Must be backported in 2.1.

5 years agoMEDIUM: init: prevent process and thread creation at runtime
Willy Tarreau [Tue, 3 Dec 2019 06:07:36 +0000 (07:07 +0100)] 
MEDIUM: init: prevent process and thread creation at runtime

Some concerns are regularly raised about the risk to inherit some Lua
files which make use of a fork (e.g. via os.execute()) as well as
whether or not some of bugs we fix might or not be exploitable to run
some code. Given that haproxy is event-driven, any foreground activity
completely stops processing and is easy to detect, but background
activity is a different story. A Lua script could very well discretely
fork a sub-process connecting to a remote location and taking commands,
and some injected code could also try to hide its activity by creating
a process or a thread without blocking the rest of the processing. While
such activities should be extremely limited when run in an empty chroot
without any permission, it would be better to get a higher assurance
they cannot happen.

This patch introduces something very simple: it limits the number of
processes and threads to zero in the workers after the last thread was
created. By doing so, it effectively instructs the system to fail on
any fork() or clone() syscall. Thus any undesired activity has to happen
in the foreground and is way easier to detect.

This will obviously break external checks (whose concept is already
totally insecure), and for this reason a new option
"insecure-fork-wanted" was added to disable this protection, and it
is suggested in the fork() error report from the checks. It is
obviously recommended not to use it and to reconsider the reasons
leading to it being enabled in the first place.

If for any reason we fail to disable forks, we still start because it
could be imaginable that some operating systems refuse to set this
limit to zero, but in this case we emit a warning, that may or may not
be reported since we're after the fork point. Ideally over the long
term it should be conditionned by strict-limits and cause a hard fail.

5 years agoDOC: move the "group" keyword at the right place
Willy Tarreau [Tue, 3 Dec 2019 07:29:22 +0000 (08:29 +0100)] 
DOC: move the "group" keyword at the right place

It looks like "hard-stop-after", "h1-case-adjust" and "h1-case-adjust-file"
were added before "group", breaking alphabetical ordering.

5 years agoDOC: Fix ordered list in summary
Julien Pivotto [Wed, 27 Nov 2019 14:49:54 +0000 (15:49 +0100)] 
DOC: Fix ordered list in summary

Section 6 about the cache was placed between 7 and 8. This should
be backported to 2.1.

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
5 years agoDOC: clarify matching strings on binary fetches
Mathias Weiersmueller [Mon, 2 Dec 2019 08:43:40 +0000 (09:43 +0100)] 
DOC: clarify matching strings on binary fetches

Add clarification and example to string matching on binary samples,
as comparison stops at first null byte due to strncmp behaviour.

Backporting all the way down to 1.5 is suggested as it might save
from headaches.

5 years agoBUG/MINOR: ssl: fix X509 compatibility for openssl < 1.1.0
Emmanuel Hocdet [Mon, 2 Dec 2019 10:41:23 +0000 (11:41 +0100)] 
BUG/MINOR: ssl: fix X509 compatibility for openssl < 1.1.0

Commit d4f9a60e "MINOR: ssl: deduplicate ca-file" uses undeclared X509
functions when build with openssl < 1.1.0. Introduce this functions
in openssl-compat.h .

Fix issue #385.