]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
5 years agoMINOR: trace: implement a very basic trace() function
Willy Tarreau [Mon, 12 Aug 2019 13:51:58 +0000 (15:51 +0200)] 
MINOR: trace: implement a very basic trace() function

For now it remains quite basic. It performs a few state checks, calls
the source's sink if defined, and performs the transitions between
RUNNING, STOPPED and WAITING when the configured events match.

5 years agoMINOR: trace/cli: add "show trace" to report trace state and statistics
Willy Tarreau [Mon, 12 Aug 2019 14:44:33 +0000 (16:44 +0200)] 
MINOR: trace/cli: add "show trace" to report trace state and statistics

The new "show trace" CLI command lists available trace sources and
indicates their status, their sink, and number of dropped packets.
When "show trace <source>" is used, the list of known events is
also listed with their status per action (report/start/stop/pause).

5 years agoMINOR: trace/cli: parse the "level" argument to configure the trace verbosity
Willy Tarreau [Mon, 12 Aug 2019 15:57:57 +0000 (17:57 +0200)] 
MINOR: trace/cli: parse the "level" argument to configure the trace verbosity

The "level" keyword allows to indicate the expected level of verbosity
in the traces, among "user" (least verbose, just synthetic info) to
"developer" (very detailed, including function entry/leaving). It's only
displayed and set but not used yet.

5 years agoMINOR: trace/cli: register the "trace" CLI keyword to list the sources
Willy Tarreau [Thu, 8 Aug 2019 16:48:12 +0000 (18:48 +0200)] 
MINOR: trace/cli: register the "trace" CLI keyword to list the sources

For now it lists the sources if one is not provided, and checks
for the source's existence. It lists the events if not provided,
checks for their existence if provided, and adjusts reported
events/start/stop/pause events, and performs state transitions.
It lists sinks and adjusts them as well. Filters, lock, and
level are not implemented yet.

5 years agoMINOR: trace: add allocation of buffer-sized trace buffers
Willy Tarreau [Mon, 19 Aug 2019 13:55:34 +0000 (15:55 +0200)] 
MINOR: trace: add allocation of buffer-sized trace buffers

This will be needed so that we can implement protocol decoders which will
have to emit their contents into such a buffer.

5 years agoMINOR: trace: start to create a new trace subsystem
Willy Tarreau [Thu, 8 Aug 2019 16:21:26 +0000 (18:21 +0200)] 
MINOR: trace: start to create a new trace subsystem

The principle of this subsystem will be to support taking live traces
at various places in the code with conditional triggers, filters, and
ability to lock on some elements. The traces will support typed events
and will be sent into sinks made of ring buffers, file descriptors or
remote servers.

5 years agoMINOR: sink: add a support for file descriptors
Willy Tarreau [Tue, 20 Aug 2019 09:57:52 +0000 (11:57 +0200)] 
MINOR: sink: add a support for file descriptors

This is the most basic type of sink. It pre-registers "stdout" and
"stderr", and is able to use writev() on them. The writev() operation
is locked to avoid mixing outputs. It's likely that the registration
should move somewhere else to take into account the fact that stdout
and stderr are still opened or are closed.

5 years agoMINOR: sink: create definitions a minimal code for event sinks
Willy Tarreau [Sun, 11 Aug 2019 14:38:56 +0000 (16:38 +0200)] 
MINOR: sink: create definitions a minimal code for event sinks

The principle will be to be able to dispatch events to various destinations
called "sinks". This is already done in part in logs where log servers can
be either a UDP socket or a file descriptor. This will be needed with the
new trace subsystem where we may also want to add ring buffers. And it turns
out that all such destinations make sense at all places. Logs may need to be
sent to a TCP server via a ring buffer, or consulted from the CLI. Trace
events may need to be sent to stdout/stderr as well as to remote log servers.

This patch creates a new structure "sink" aiming at addressing these similar
needs. The goal is to merge together what is common to all of them, such as
the output format, the dropped events count, etc, and also keep separately
the target identification (network address, file descriptor). Provisions
were made to have a "waiter" on the sink. For a TCP log server it will be
the task to wake up after writing to the log buffer. For a ring buffer, it
could be the list of watchers on the CLI running a "tail" operation and
waiting for new events. A lock was also placed in the struct since many
operations will require some locking, including the FD ones. The output
formats covers those in use by logs and two extra ones prepending the ISO
time in front of the message (convenient for stdio/buffer).

For now only the generic infrastructure is present, no type-specific
output is implemented. There's the sink_write() function which prepares
and formats a message to be sent, trying hard to avoid copies and only
using pointer manipulation, where the type-specific code just has to be
added. Dropped messages are already counted (for now 100% drop). The
message is put into an iovec array as it will be trivial to use with
file descriptors and sockets.

5 years agoREORG: trace: rename trace.c to calltrace.c and mention it's not thread-safe
Willy Tarreau [Thu, 8 Aug 2019 14:53:40 +0000 (16:53 +0200)] 
REORG: trace: rename trace.c to calltrace.c and mention it's not thread-safe

The function call tracing code is a quite old and was never ported to
support threads. It's not even sure whether it still works well, but
at least its presence creates confusion for future work so let's rename
it to calltrace.c and add a comment about its lack of thread-safety.

5 years agoMINOR: tools: add a DEFNULL() macro to use NULL for empty args
Willy Tarreau [Mon, 19 Aug 2019 15:39:33 +0000 (17:39 +0200)] 
MINOR: tools: add a DEFNULL() macro to use NULL for empty args

It's sometimes convenient for debugging macros not to be forced to
explicitly pass NULL in an unused argument. This macro does this, it
replaces a missing arg with NULL.

5 years agoMINOR: list: add LIST_SPLICE() to merge one list into another
Willy Tarreau [Fri, 16 Aug 2019 09:27:50 +0000 (11:27 +0200)] 
MINOR: list: add LIST_SPLICE() to merge one list into another

This will move the contents of list <old> at the beginning of list
<new>.

5 years agoBUG/MEDIUM: h1: Always try to receive more in h1_rcv_buf().
Olivier Houchard [Thu, 22 Aug 2019 16:34:25 +0000 (18:34 +0200)] 
BUG/MEDIUM: h1: Always try to receive more in h1_rcv_buf().

In h1_rcv_buf(), wake the h1c tasklet as long as we're not done reading the
request/response, and the h1c is not already subscribed for receiving. Now
that we no longer subscribe in h1_recv() if we managed to read data, we
rely on h1_rcv_buf() calling us again, but h1_process_input() may have
returned 0 if we only received part of the request, so we have to wake
the tasklet to be sure to get more data again.

5 years agoMEDIUM: debug: make the thread dump code show Lua backtraces
Willy Tarreau [Wed, 21 Aug 2019 12:16:02 +0000 (14:16 +0200)] 
MEDIUM: debug: make the thread dump code show Lua backtraces

When we dump a thread's state (show thread, panic) we don't know if
anything is happening in Lua, which can be problematic especially when
calling external functions. With this patch, the thread dump code can
now detect if we're running in a global Lua task (hlua_process_task),
or in a TCP or HTTP Lua service (task_run_applet and applet.fct ==
hlua_applet_tcp_fct or http_applet_http_fct), or a fetch/converter
from an analyser (s->hlua != NULL). In such situations, it's able to
append a formatted Lua backtrace of the Lua execution path with
function names, file names and line numbers.

Note that a shorter alternative could be to call "luaL_where(hlua->T,0)"
which only prints the current location, but it's not necessarily sufficient
for complex code.

5 years agoMINOR: lua: export applet and task handlers
Willy Tarreau [Wed, 21 Aug 2019 12:14:50 +0000 (14:14 +0200)] 
MINOR: lua: export applet and task handlers

The current functions are seen outside from the debugging code and are
convenient to export so that we can improve the thread dump output :

  void hlua_applet_tcp_fct(struct appctx *ctx);
  void hlua_applet_http_fct(struct appctx *ctx);
  struct task *hlua_process_task(struct task *task, void *context, unsigned short state);

Of course they are only available when USE_LUA is defined.

5 years agoMINOR: tools: add append_prefixed_str()
Willy Tarreau [Wed, 21 Aug 2019 11:17:37 +0000 (13:17 +0200)] 
MINOR: tools: add append_prefixed_str()

This is somewhat related to indent_msg() except that this one places a
known prefix at the beginning of each line, allows to replace the EOL
character, and not to insert a prefix on the first line if not desired.
It works with a normal output buffer/chunk so it doesn't need to allocate
anything nor to modify the input string. It is suitable for use in multi-
line backtraces.

5 years agoMINOR: debug: indicate the applet name when the task is task_run_applet()
Willy Tarreau [Wed, 21 Aug 2019 12:12:19 +0000 (14:12 +0200)] 
MINOR: debug: indicate the applet name when the task is task_run_applet()

This allows to figure what applet is currently being executed (and likely
hung).

5 years agoBUILD: travis-ci: trigger non-mainstream configurations only on daily builds.
Ilya Shipitsin [Mon, 19 Aug 2019 16:32:53 +0000 (21:32 +0500)] 
BUILD: travis-ci: trigger non-mainstream configurations only on daily builds.

Let us save some electricity of Travis-CI.

The following configurations are built on every push:

  - linux-glibc with threads+openssl 1.1.1 on x86 + clang
  - linux-glibc with threads+libressl 2.9.2 on x86 + clang
  - linux-glibc without SSL nor threads on x86 + clang
  - osx with openssl-1.1.1 + clang

The following configurations are built daily:

  - linux-ppc64le + openssl-1.0.2
  - linux + openssl-1.1.0 + 51d/tree
  - linux + libressl-2.8.3
  - linux + libressl-2.7.5 + prometheus
  - linux + boringssl
  - cygwin
  - coverity scan

5 years agoBUG/MEDIUM: mux_pt: Don't call unsubscribe if we did not subscribe.
Olivier Houchard [Sat, 10 Aug 2019 21:56:16 +0000 (23:56 +0200)] 
BUG/MEDIUM: mux_pt: Don't call unsubscribe if we did not subscribe.

In mux_pt_attach(), don't inconditionally call unsubscribe, and only do so
if we were subscribed. The idea was that at this point we would always be
subscribed, as for the mux_pt attach would only be called after at least one
request, after which the mux_pt would have subscribed, but this is wrong.
We can also be called if for some reason the connection failed before the
xprt was created. And with no xprt, attempting to call unsubscribe will
probably lead to a crash.

This should be backported to 2.0.

5 years agoMINOR: fd: make sure to mark the thread as not stuck in fd_update_events()
Willy Tarreau [Fri, 16 Aug 2019 14:06:14 +0000 (16:06 +0200)] 
MINOR: fd: make sure to mark the thread as not stuck in fd_update_events()

When I/O events are being processed, we want to make sure to mark the
thread as not stuck. The reason is that some pollers (like poll()) which
do not limit the number of FDs they report could possibly report a huge
amount of FD all having to perform moderately expensive operations in
the I/O callback (e.g. via mux-pt which forwards to the upper layers),
making the watchdog think the thread is stuck since it does not schedule.
Of course this must never happen but if it ever does we must be liberal
about it.

This should be backported to 2.0, where the situation may happen more
easily due to the FD cache which can start to collect a large amount of
events. It may be related to the report in issue #201 though nothing is
certain about it.

5 years agoBUG/MINOR: stats: Wait the body before processing POST requests
Christopher Faulet [Thu, 15 Aug 2019 20:26:48 +0000 (22:26 +0200)] 
BUG/MINOR: stats: Wait the body before processing POST requests

The stats applet waits to have a full body to process POST requests. Because
when it is waiting for the end of a request it does not produce anything, the
applet may be blocked. The client side is blocked because the stats applet does
not consume anything and the applet is waiting because all the body is not
received. Registering the analyzer AN_REQ_HTTP_BODY when a POST request is sent
for the stats applet solves the issue.

This patch must be backported to 2.0.

5 years agoBUG/MEDIUM: lua: Fix test on the direction to set the channel exp timeout
Christopher Faulet [Wed, 14 Aug 2019 21:19:45 +0000 (23:19 +0200)] 
BUG/MEDIUM: lua: Fix test on the direction to set the channel exp timeout

This bug was introduced by the commit bfab2ddd ("MINOR: hlua: Add a flag on the
lua txn to know in which context it can be used"). The wrong test was done. So
the timeout was always set on the response channel. It may lead to an infinite
loop.

This patch must be backported everywhere the commit bfab2ddd is. For now, at
least to 2.0, 1.9 and 1.8.

5 years agoBUG/MINOR: Fix prometheus '# TYPE' and '# HELP' headers
Anthonin Bonnefoy [Wed, 7 Aug 2019 15:45:25 +0000 (17:45 +0200)] 
BUG/MINOR: Fix prometheus '# TYPE' and '# HELP' headers

Prometheus protocol defines HELP and TYPE as a token after the '#' and
the space after the '#' is necessary.
This is expected in the prometheus python client for example
(https://github.com/prometheus/client_python/blob/a8f5c80f651ea570577c364203e0edbef67db727/prometheus_client/parser.py#L194)
and the missing space is breaking the parsing of metrics' type.

This patch must be backported to 2.0.

5 years agoBUG/MINOR: lua: fix setting netfilter mark
Lukas Tribus [Sun, 11 Aug 2019 16:03:45 +0000 (18:03 +0200)] 
BUG/MINOR: lua: fix setting netfilter mark

In the REORG of commit 1a18b5414 ("REORG: connection: centralize the
conn_set_{tos,mark,quickack} functions") a bug was introduced by
calling conn_set_tos instead of conn_set_mark.

This was reported in issue #212

This should be backported to 1.9 and 2.0.

5 years agoBUG/MEDIUM: proxy: Don't use cs_destroy() when freeing the conn_stream.
Olivier Houchard [Fri, 9 Aug 2019 16:01:15 +0000 (18:01 +0200)] 
BUG/MEDIUM: proxy: Don't use cs_destroy() when freeing the conn_stream.

When we upgrade the mux from TCP to H2/HTX, don't use cs_destroy() to free
the conn_stream, use cs_free() instead. Using cs_destroy() would call the
mux detach method, and at that point of time the mux would be the H2 mux,
which knows nothing about that conn_stream, so bad things would happen.
This should eventually make upgrade from TCP to H2/HTX work, and fix
the github issue #196.

This should be backported to 2.0.

5 years agoBUG/MEDIUM: proxy: Don't forget the SF_HTX flag when upgrading TCP=>H1+HTX.
Olivier Houchard [Fri, 9 Aug 2019 15:50:05 +0000 (17:50 +0200)] 
BUG/MEDIUM: proxy: Don't forget the SF_HTX flag when upgrading TCP=>H1+HTX.

In stream_end_backend(), if we're upgrading from TCP to H1/HTX, as we don't
destroy the stream, we have to add the SF_HTX flag on the stream, or bad
things will happen.
This was broken when attempting to fix github issue #196.

This should be backported to 2.0.

5 years agoCLEANUP: cli: replace all occurrences of manual handling of return messages
Willy Tarreau [Fri, 9 Aug 2019 09:21:01 +0000 (11:21 +0200)] 
CLEANUP: cli: replace all occurrences of manual handling of return messages

There were 221 places where a status message or an error message were built
to be returned on the CLI. All of them were replaced to use cli_err(),
cli_msg(), cli_dynerr() or cli_dynmsg() depending on what was expected.
This removed a lot of duplicated code because most of the times, 4 lines
are replaced by a single, safer one.

5 years agoMINOR: cli: add cli_msg(), cli_err(), cli_dynmsg(), cli_dynerr()
Willy Tarreau [Thu, 8 Aug 2019 17:09:21 +0000 (19:09 +0200)] 
MINOR: cli: add cli_msg(), cli_err(), cli_dynmsg(), cli_dynerr()

These functions perform all the boring filling of the appctx's
cli struct needed by CLI parsers to return a message or an error,
and they return 1 so that they can be used as a single-line return
statement. They may be used for const messages or dynamic messages.

5 years agoMINOR: cli: add two new states to print messages on the CLI
Willy Tarreau [Fri, 9 Aug 2019 07:57:36 +0000 (09:57 +0200)] 
MINOR: cli: add two new states to print messages on the CLI

Right now we used to have extremely inconsistent states to report output,
one is CLI_ST_PRINT which prints constant message cli->msg with the
assigned severity, and CLI_ST_PRINT_FREE which prints dynamically
allocated cli->err with severity LOG_ERR, and nothing in between,
eventhough it's useful to be able to report dynamically allocated
messages as well as constant error messages.

This patch adds two extra states, which are not particularly well named
given the constraints imposed by existing ones. One is CLI_ST_PRINT_ERR
which prints a constant error message. The other one is CLI_ST_PRINT_DYN
which prints a dynamically allocated message. By doing so we maintain
the compatibility with current code.

It is important to keep in mind that we cannot pre-initialize pointers
and automatically detect what message type it is based on the assigned
fields, because the CLI's context is in a union shared with all other
users, thus unused fields contain anything upon return. This is why we
have no choice but using 4 states. Keeping the two fields <msg> and
<err> remains useful because one is const and not the other one, and
this catches may copy-paste mistakes. It's just that <err> is pretty
confusing here, it should be renamed.

5 years agoCLEANUP: task: move the cpu_time field to the task-only part
Willy Tarreau [Thu, 8 Aug 2019 08:09:08 +0000 (10:09 +0200)] 
CLEANUP: task: move the cpu_time field to the task-only part

The CPU time accounting field called "cpu_time" is used only by tasks
and not tasklets, yet it used to be stored into the TASK_COMMON part,
which doesn't make sense and wastes tasklet memory. In addition, moving
it to tasks also helps better group the various parts in cache lines.

5 years agoCLEANUP: buffer: replace b_drop() with b_free()
Willy Tarreau [Thu, 8 Aug 2019 06:06:27 +0000 (08:06 +0200)] 
CLEANUP: buffer: replace b_drop() with b_free()

Since last commit there's no point anymore in having two variants of the
same function, let's switch to b_free() only. __b_drop() was renamed to
__b_free() for obvious consistency reasons.

5 years agoBUG/MINOR: buffers/threads: always clear a buffer's head before releasing it
Willy Tarreau [Thu, 8 Aug 2019 05:53:20 +0000 (07:53 +0200)] 
BUG/MINOR: buffers/threads: always clear a buffer's head before releasing it

A small race exists in buffers with "show sess all". This one wants to show
some information grabbed from the buffer (especially in HTX mode). But the
thread owning this buffer might just be releasing its area, right after a
free() or munmap() call, resulting in a head that is not seen as empty yet
though the area was released. It may then be dereferenced by "show sess all"
causing a crash. Note that in practice it only happens in debug mode with
UAF enabled, but it's tricky enough to fix it right now.

This should be backported to stable versions which support threads and a
store barrier. It's worth noting that by performing the clearing first,
b_free() and b_drop() now become two exact equivalent.

5 years agoBUG/MINOR: pools: don't mark the thread harmless if already isolated
Willy Tarreau [Thu, 8 Aug 2019 05:38:19 +0000 (07:38 +0200)] 
BUG/MINOR: pools: don't mark the thread harmless if already isolated

Commit 85b2cae63 ("MINOR: pools: make the thread harmless during the
mmap/munmap syscalls") was used to relax the pressure experienced by
other threads when running in debug mode with UAF enabled. It places
a pair of thread_harmless_now()/thread_harmless_end() around the call
to mmap(), assuming callers are not sensitive to parallel activity.
But there are a few cases like "show sess all" where this happens in
isolated threads, and marking the thread as harmless there is a very
bad idea, even worse when arriving to thread_harmless_end() which loops
forever.

Let's only do that when the thread is not isolated. No backport is
needed as the patch above was only in 2.1-dev.

5 years agoMINOR: ssl: ssl_fc_has_early should work for BoringSSL
Emmanuel Hocdet [Wed, 7 Aug 2019 12:44:49 +0000 (14:44 +0200)] 
MINOR: ssl: ssl_fc_has_early should work for BoringSSL

CO_FL_EARLY_SSL_HS/CO_FL_EARLY_DATA are removed for BoringSSL. Early
data can be checked via BoringSSL API and ssl_fc_has_early can used it.

This should be backported to all versions till 1.8.

5 years agoBUG/MINOR: ssl: fix 0-RTT for BoringSSL
Emmanuel Hocdet [Mon, 5 Aug 2019 16:04:16 +0000 (18:04 +0200)] 
BUG/MINOR: ssl: fix 0-RTT for BoringSSL

Since BoringSSL commit 777a2391 "Hold off flushing NewSessionTicket until write.",
0-RTT doesn't work. It appears that half-RTT data (response from 0-RTT) never
worked before the BoringSSL fix. For HAProxy the regression come from 010941f8
"BUG/MEDIUM: ssl: Use the early_data API the right way.": the problem is link to
the logic of CO_FL_EARLY_SSL_HS used for OpenSSL. With BoringSSL, handshake is
done before reading early data, 0-RTT data and half-RTT data are processed as
normal data: CO_FL_EARLY_SSL_HS/CO_FL_EARLY_DATA is not needed, simply remove
it.

This should be backported to all versions till 1.8.

5 years agoMINOR: cache: allow caching of OPTIONS request
Baptiste Assmann [Wed, 7 Aug 2019 10:24:36 +0000 (12:24 +0200)] 
MINOR: cache: allow caching of OPTIONS request

Allow HAProxy to cache responses to OPTIONS HTTP requests.
This is useful in the use case of "Cross-Origin Resource Sharing" (cors)
to cache CORS responses from API servers.

Since HAProxy does not support Vary header for now, this would be only
useful for "access-control-allow-origin: *" use case.

5 years agoMINOR: cache: add method to cache hash
Baptiste Assmann [Mon, 5 Aug 2019 14:55:32 +0000 (16:55 +0200)] 
MINOR: cache: add method to cache hash

Current HTTP cache hash contains only the Host header and the url path.
That said, request method should also be added to the mix to support
caching other request methods on the same URL. IE GET and OPTIONS.

5 years agoCLEANUP: mux-h2: move the demuxed frame check code in its own function
Willy Tarreau [Wed, 7 Aug 2019 12:25:20 +0000 (14:25 +0200)] 
CLEANUP: mux-h2: move the demuxed frame check code in its own function

The frame check code in the demuxer was moved to its own function to
keep the demux function clean enough. This also simplifies the test
case as we can now simply call this function once in H2_CS_FRAME_P
state.

5 years agoBUG/MEDIUM: stick-table: Wrong stick-table backends parsing.
Frédéric Lécaille [Wed, 7 Aug 2019 07:28:39 +0000 (09:28 +0200)] 
BUG/MEDIUM: stick-table: Wrong stick-table backends parsing.

When parsing references to stick-tables declared as backends, they are added to
a list of proxies (they are proxies!) which refer to this stick-tables.
Before this patch we added them to these list without checking they were already
present, making the silly hypothesis the actions/sample were checked/resolved in the same
order the proxies are parsed.

This patch implement a simple inline function to in_proxies_list() to test
the presence of a proxy in a list of proxies. We use this function when resolving
/checking samples/actions.

This bug was introduced by 015e4d7 commit.

Must be backported to 2.0.

5 years agoBUG/MEDIUM: checks: make sure to close nicely when we're the last to speak
Willy Tarreau [Tue, 6 Aug 2019 14:26:31 +0000 (16:26 +0200)] 
BUG/MEDIUM: checks: make sure to close nicely when we're the last to speak

In SMTP, MySQL and PgSQL checks, we're supposed to finish with a message
to politely quit the server, otherwise some of them will log some errors.
This is the case with Postfix as reported in GH issue #187. Since commit
fe4abe6 ("BUG/MEDIUM: connections: Don't call shutdown() if we want to
disable linger.") we are a bit more aggressive on outgoing connection
closure and checks were not prepared for this.

This patch makes the 3 checks above disable the linger_risk for these
checks so that we close cleanly, with the side effect that it will leave
some TIME_WAIT connections behind (hence why it should not be generalized
to all checks). It's worth noting that in issue #187 it's mentioned that
this patch doesn't seem to be sufficient for Postfix, however based only
on local network activity this looks OK, so maybe this will need to be
improved later.

Given that the patch above was backported to 2.0 and 1.9, this one should
as well.

5 years agoBUG/MINOR: mux-h2: always reset rcvd_s when switching to a new frame
Willy Tarreau [Tue, 6 Aug 2019 13:49:51 +0000 (15:49 +0200)] 
BUG/MINOR: mux-h2: always reset rcvd_s when switching to a new frame

In Patrick's trace it was visible that after a stream had been missed,
the next stream would receive a WINDOW_UPDATE with the first one's
credit added to its own. This makes sense because in case of error
h2c->rcvd_s is not reset. Given that this counter is per frame, better
reset it when starting to parse a new frame, it's easier and safer.

This must be backported as far as 1.8.

5 years agoBUG/MINOR: mux-h2: always send stream window update before connection's
Willy Tarreau [Tue, 6 Aug 2019 13:39:32 +0000 (15:39 +0200)] 
BUG/MINOR: mux-h2: always send stream window update before connection's

In h2_process_mux() if we have some room and an attempt to send a window
update for the connection was pending, it's done first. But it's not done
for the stream, which will have for effect of postponing this attempt
till next pass into h2_process_demux(), at the risk of seeing the send
buffer full again. Let's always try to send both pending frames as soon
as possible.

This should be backported as far as 1.8.

5 years agoBUG/MEDIUM: mux-h2: do not recheck a frame type after a state transition
Willy Tarreau [Tue, 6 Aug 2019 13:21:45 +0000 (15:21 +0200)] 
BUG/MEDIUM: mux-h2: do not recheck a frame type after a state transition

Patrick Hemmer reported a rare case where the H2 mux emits spurious
RST_STREAM(STREAM_CLOSED) that are triggered by the send path and do
not even appear to be associated with a previous incoming frame, while
the send path never emits such a thing.

The problem is particularly complex (hence its rarity). What happens is
that when data are uploaded (POST) we must refill the sending stream's
window by sending a WINDOW_UPDATE message (and we must refill the
connection's too). But in a highly bidirectional traffic, it is possible
that the mux's buffer will be full and that there is no more room to build
this WINDOW_UPDATE frame. In this case the demux parser switches to the
H2_CS_FRAME_A state, noting that an "acknowledgement" is needed for the
current frame, and it doesn't change the current stream nor frame type.
But the stream's state was possibly updated (typically OPEN->HREM when
a DATA frame carried the ES flag).

Later the data can leave the buffer, wake up h2_io_cb(), which calls
h2_send() to send pending data, itself calling h2_process_mux() which
detects that there are unacked data in the connection's window so it
emits a WINDOW_UPDATE for the connection and resets the counter. so it
emits a WINDOW_UPDATE for the connection and resets the counter. Then
h2_process() calls h2_process_demux() which continues the processing
based on the current frame type and the current state H2_CS_FRAME_A.
Unfortunately the protocol compliance checks matching the frame type
against the current state are still present. These tests are designed
for new frames only, not for those in progress, but they are not
limited by frame types. Thus the current DATA frame is checked again
against the current stream state that is now HREM, and fails the test
with a STREAM_CLOSED error.

The quick and backportable solution consists in adding the test for
this ACK and bypass all these checks that were already validated prior
to the state transition. A better long-term solution would consist in
having a new state between H and P indicating the frame is new and
needs to be checked ("N" for new?) and apply the protocol tests only
in this state. In addition everywhere we decide to send a window
update, we should send a stream WU first if there are unacked data
for the current stream. Last, rcvd_s should always be reset when
transitioning to FRAME_H (and a BUGON for this in dev would help).

The bug will be way harder to trigger on 2.0 than on 1.8/1.9 because
we have a ring buffer for the connection so the buffer full situations
are extremely rare.

This fix must be backpored to all versions having H2 (as far as 1.8).

Special thanks to Patrick for providing exploitable traces.

5 years agoBUG/MINOR: mux-h2: do not send REFUSED_STREAM on aborted uploads
Willy Tarreau [Tue, 6 Aug 2019 08:30:58 +0000 (10:30 +0200)] 
BUG/MINOR: mux-h2: do not send REFUSED_STREAM on aborted uploads

If the server decides to close early, we don't want to send a
REFUSED_STREAM error but a CANCEL, so that the client doesn't want
to retry. The test in h2_do_shutw() was wrong for this as it would
handle the HLOC case like the case where nothing had been sent for
this stream, which is wrong. Now h2_do_shutw() does nothing in this
case and lets h2_do_shutr() decide.

Note that this partially undoes f983d00a1 ("BUG/MINOR: mux-h2: make
the do_shut{r,w} functions more robust against retries").

This must be backported to 2.0. The patch above was not backported to
1.9 for being too risky there, but if it eventually gets to it, this
one will be needed as well.

5 years agoBUG/MINOR: mux-h2: use CANCEL, not STREAM_CLOSED in h2c_frt_handle_data()
Willy Tarreau [Tue, 6 Aug 2019 08:11:02 +0000 (10:11 +0200)] 
BUG/MINOR: mux-h2: use CANCEL, not STREAM_CLOSED in h2c_frt_handle_data()

There is a test on the existence of the conn_stream when receiving data,
to be sure to have somewhere to deliver it. Right now it responds with
STREAM_CLOSED, which is not correct since from an H2 point of view the
stream is not closed and a peer could be upset to see this. After some
analysis, it is important to keep this test to be sure not to fill the
rxbuf then stall the connection. Another option could be to modiffy
h2_frt_transfer_data() to silently discard any contents but the CANCEL
error code is designed exactly for this and to save the peer from
continuing to stream data that will be discarded, so better switch to
using this.

This must be backported as far as 1.8.

5 years agoBUG/MINOR: mux-h2: don't refrain from sending an RST_STREAM after another one
Willy Tarreau [Tue, 6 Aug 2019 08:01:40 +0000 (10:01 +0200)] 
BUG/MINOR: mux-h2: don't refrain from sending an RST_STREAM after another one

The test in h2s_send_rst_stream() is excessive, it refrains from sending an
RST_STREAM if *the last frame* was an RST_STREAM, regardless of the stream
ID. In a context where both clients and servers abort a lot, it could happen
that one RST_STREAM is dropped from responses from time to time, causing
delays to the client.

This must be backported to 2.0, 1.9 and 1.8.

5 years agoBUILD: travis-ci: enable daily Coverity scan
Ilya Shipitsin [Fri, 2 Aug 2019 17:59:03 +0000 (22:59 +0500)] 
BUILD: travis-ci: enable daily Coverity scan

Coverity scan takes some time, also we are limited to 3 scans per day.
So, running it once a day seem to be good idea.

5 years agoBUG/MEDIUM: pollers: Clear the poll_send bits as well.
Olivier Houchard [Mon, 5 Aug 2019 21:54:37 +0000 (23:54 +0200)] 
BUG/MEDIUM: pollers: Clear the poll_send bits as well.

In _update_fd(), if we're about to remove the FD from the poller, remove
both the receive and the send bits, instead of removing the receive bits
twice.

5 years agoBUG/MEDIUM: fd: Always reset the polled_mask bits in fd_dodelete().
Olivier Houchard [Mon, 5 Aug 2019 16:51:52 +0000 (18:51 +0200)] 
BUG/MEDIUM: fd: Always reset the polled_mask bits in fd_dodelete().

In fd_dodelete(), always reset the polled_mask bits, instead on only doing
it if we're closing the file descriptor. We call the poller clo() method
anyway, and failing to do so means that if fd_remove() is used while the
fd is polled, the poller won't attempt to poll on a fd with the same value
as the old one.
This leads to fd being stuck in the SSL code while using the async engine.

This should be backported to 2.0, 1.9 and 1.8.

5 years agoDOC: Add 'Question.md' issue template, discouraging asking questions
Tim Duesterhus [Fri, 2 Aug 2019 10:21:54 +0000 (12:21 +0200)] 
DOC: Add 'Question.md' issue template, discouraging asking questions

This commit adds a new `Question.md` issue template that aims to prevent
users asking questions within the issue tracker, because the other two
templates do not fit "question", thus leading to people not seeing that
the issue tracker is not for questions.

This template inspired by TypeScript's Question template at:
https://github.com/microsoft/TypeScript/blob/master/.github/ISSUE_TEMPLATE/Question.md

5 years agoBUG/MEDIUM: proxy: Make sure to destroy the stream on upgrade from TCP to H2
Olivier Houchard [Wed, 31 Jul 2019 16:05:26 +0000 (18:05 +0200)] 
BUG/MEDIUM: proxy: Make sure to destroy the stream on upgrade from TCP to H2

In stream_set_backend(), if we have a TCP stream, and we want to upgrade it
to H2 instead of attempting ot reuse the stream, just destroy the
conn_stream, make sure we don't log anything about the stream, and pretend
we failed setting the backend, so that the stream will get destroyed.
New streams will then be created by the mux, as if the connection just
happened.
This fixes a crash when upgrading from TCP to H2, as the H2 mux totally
ignored the conn_stream provided by the upgrade, as reported in github
issue #196.

This should be backported to 2.0.

5 years agoBUG/MEDIUM: mux-h2: split the stream's and connection's window sizes
Willy Tarreau [Fri, 2 Aug 2019 05:52:08 +0000 (07:52 +0200)] 
BUG/MEDIUM: mux-h2: split the stream's and connection's window sizes

The SETTINGS frame parser updates all streams' window for each
INITIAL_WINDOW_SIZE setting received on the connection (like h2spec
does in test 6.5.3), which can start to be expensive if repeated when
there are many streams (up to 100 by default). A quick test shows that
it's possible to parse only 35000 settings per second on a 3 GHz core
for 100 streams, which is rather small.

Given that window sizes are relative and may be negative, there's no
point in pre-initializing them for each stream and update them from
the settings. Instead, let's make them relative to the connection's
initial window size so that any change immediately affects all streams.
The only thing that remains needed is to wake up the streams that were
unblocked by the update, which is now done once at the end of
h2_process_demux() instead of once per setting. This now results in
5.7 million settings being processed per second, which is way better.

In order to keep the change small, the h2s' mws field was renamed to
"sws" for "stream window size", and an h2s_mws() function was added
to add it to the connection's initial window setting and determine the
window size to use when muxing. The h2c_update_all_ws() function was
renamed to h2c_unblock_sfctl() since it's now only used to unblock
previously blocked streams.

This needs to be backported to all versions till 1.8.

5 years agoBUG/MEDIUM: mux-h2: unbreak receipt of large DATA frames
Willy Tarreau [Fri, 2 Aug 2019 05:48:47 +0000 (07:48 +0200)] 
BUG/MEDIUM: mux-h2: unbreak receipt of large DATA frames

Recent optimization in commit 4d7a88482 ("MEDIUM: mux-h2: don't try to
read more than needed") broke the receipt of large DATA frames because
it would unconditionally subscribe if there was some room left, thus
preventing any new rx from being done since subscription may only be
done once the end was reached, as indicated by ret == 0.

However, fixing this uncovered that in HTX mode previous versions might
occasionally be affected as well, when an available frame is the same
size as the maximum data that may fit into an HTX buffer, we may end
up reading that whole frame and still subscribe since it's still allowed
to receive, thus causing issues to read the next frame.

This patch will only work for 2.1-dev but a minor adaptation will be
needed for earlier versions (down to 1.9, where subscribe() was added).

5 years agoBUG/MINOR: stream-int: also update analysers timeouts on activity
Willy Tarreau [Thu, 1 Aug 2019 16:51:38 +0000 (18:51 +0200)] 
BUG/MINOR: stream-int: also update analysers timeouts on activity

Between 1.6 and 1.7, some parts of the stream forwarding process were
moved into lower layers and the stream-interface had to keep the
stream's task up to date regarding the timeouts. The analyser timeouts
were not updated there as it was believed this was not needed during
forwarding, but actually there is a case for this which is "option
contstats" which periodically triggers the analyser timeout, and this
change broke the option in case of sustained traffic (if there is some
I/O activity during the same millisecond as the timeout expires, then
the update will be missed).

This patch simply brings back the analyser expiration updates from
process_stream() to stream_int_notify().

It may be backported as far as 1.7, taking care to adjust the fields
names if needed.

5 years agoBUG/MEDIUM: ssl: open the right path for multi-cert bundle
William Lallemand [Thu, 1 Aug 2019 12:43:20 +0000 (14:43 +0200)] 
BUG/MEDIUM: ssl: open the right path for multi-cert bundle

Multi-cert bundle was not working anymore because we tried to open the
wrong path.

5 years agoBUG/MINOR: stream-int: make sure to always release empty buffers after sending
Willy Tarreau [Thu, 1 Aug 2019 12:17:02 +0000 (14:17 +0200)] 
BUG/MINOR: stream-int: make sure to always release empty buffers after sending

There are some situations, after sending a request or response, upon I/O
completion, or applet execution, where we end up with an empty buffer that
was not released. This results in excessive memory usage (back to 1.5) and
a lower CPU cache efficiency since buffers are not recycled as fast. This
has changed since the places where we send have changed with the new
layering, but not all cases susceptible of leaving an empty buffer were
properly spotted. Doing so reduces the memory pressure on buffers by about
2/3 in high traffic tests.

This should be backported to 2.0 and maybe 1.9.

5 years agoBUG/MAJOR: http/sample: use a static buffer for raw -> htx conversion
Richard Russo [Wed, 31 Jul 2019 18:45:56 +0000 (11:45 -0700)] 
BUG/MAJOR: http/sample: use a static buffer for raw -> htx conversion

Multiple calls to smp_fetch_fhdr use the header context to keep track of
header parsing position; however, when using header sampling on a raw
connection, the raw buffer is converted into an HTX structure each time, and
this was done in the trash areas; so the block reference would be invalid on
subsequent calls.

This patch must be backported to 2.0 and 1.9.

5 years agoBUG/MEDIUM: lb-chash: Ensure the tree integrity when server weight is increased
Christopher Faulet [Thu, 1 Aug 2019 08:09:29 +0000 (10:09 +0200)] 
BUG/MEDIUM: lb-chash: Ensure the tree integrity when server weight is increased

When the server weight is increased in consistant hash, extra nodes have to be
allocated. So a realloc() is performed on the nodes array of the server. the
previous commit 962ea7732 ("BUG/MEDIUM: lb-chash: Remove all server's entries
before realloc() to re-insert them after") have fixed the size used during the
realloc() to avoid segfaults. But another bug remains. After the realloc(), the
memory area allocated for the nodes array may change, invalidating all node
addresses in the chash tree.

So, to fix the bug, we must remove all server's entries from the chash tree
before the realloc to insert all of them after, old nodes and new ones. The
insert will be automatically handled by the loop at the end of the function
chash_queue_dequeue_srv().

Note that if the call to realloc() failed, no new entries will be created for
the server, so the effective server weight will be unchanged.

This issue was reported on Github (#189).

This patch must be backported to all versions since the 1.6.

5 years agoBUG/MINOR: ssl: fix ressource leaks on error
Emmanuel Hocdet [Wed, 31 Jul 2019 16:30:33 +0000 (18:30 +0200)] 
BUG/MINOR: ssl: fix ressource leaks on error

Commit 36b84637 "MEDIUM: ssl: split the loading of the certificates"
introduce leaks on fd/memory in case of error.

5 years agoBUG/MEDIUM: ssl: don't free the ckch in multi-cert bundle
William Lallemand [Thu, 1 Aug 2019 08:59:34 +0000 (10:59 +0200)] 
BUG/MEDIUM: ssl: don't free the ckch in multi-cert bundle

When using a ckch we should never try to free its content, because it
won't be usable  after and can result in a NULL derefence during
parsing.

The content was previously freed because the ckch wasn't stored in a
tree to be used later, now that we use it multiple time, we need to keep
the data.

5 years agoBUILD: ssl: BoringSSL add EVP_PKEY_base_id
Emmanuel Hocdet [Thu, 1 Aug 2019 08:56:45 +0000 (10:56 +0200)] 
BUILD: ssl: BoringSSL add EVP_PKEY_base_id

Remove EVP_PKEY_base_id compatibility, it is now included in BoringSSL.

5 years agoREGTESTS: checks: make 4be_1srv_health_checks more reliable
Willy Tarreau [Thu, 1 Aug 2019 07:51:29 +0000 (09:51 +0200)] 
REGTESTS: checks: make 4be_1srv_health_checks more reliable

This test occasionally fails on the Travis CI tests because the
"in progress" bit is sometimes still set (or set again) in the show
servers state output and is not expected in all regexes (some do
already cover it), like in this one :

   https://travis-ci.com/haproxy/haproxy/jobs/221324920

Let's extend the remaining ones to accept this as well. Other tests
do not seem affected as they only expect sequences of digits there.

5 years agoMINOR: wdt: also consider that waiting in the thread dumper is normal
Willy Tarreau [Wed, 31 Jul 2019 17:20:39 +0000 (19:20 +0200)] 
MINOR: wdt: also consider that waiting in the thread dumper is normal

It happens that upon looping threads the watchdog fires, starts a dump,
and other threads expire their budget while waiting for the other threads
to get dumped and trigger a watchdog event again, adding some confusion
to the traces. With this patch the situation becomes clearer as we export
the list of threads being dumped so that the watchdog can check it before
deciding to trigger. This way such threads in queue for being dumped are
not attempted to be reported in turn.

This should be backported to 2.0 as it helps understand stack traces.

5 years agoBUG/MINOR: debug: fix a small race in the thread dumping code
Willy Tarreau [Wed, 31 Jul 2019 17:15:45 +0000 (19:15 +0200)] 
BUG/MINOR: debug: fix a small race in the thread dumping code

If a thread dump is requested from a signal handler, it may interrupt
a thread already waiting for a dump to complete, and may see the
threads_to_dump variable go to zero while others are waiting, steal
the lock and prevent other threads from ever completing. This tends
to happen when dumping many threads upon a watchdog timeout, to threads
waiting for their turn.

Instead now we proceed in two steps :
  1) the last dumped thread sets all bits again
  2) all threads only wait for their own bit to appear, then clear it
     and quit

This way there's no risk that a bit performs a double flip in the same
loop and threads cannot get stuck here anymore.

This should be backported to 2.0 as it clarifies stack traces.

5 years agoBUG/MEDIUM: ssl: does not try to free a DH in a ckch
William Lallemand [Wed, 31 Jul 2019 16:31:34 +0000 (18:31 +0200)] 
BUG/MEDIUM: ssl: does not try to free a DH in a ckch

ssl_sock_load_dh_params() should not free the DH * of a ckch, or the
ckch won't be usable during the next call.

5 years agoBUG/BUILD: ssl: fix build with openssl < 1.0.2
William Lallemand [Wed, 31 Jul 2019 14:50:08 +0000 (16:50 +0200)] 
BUG/BUILD: ssl: fix build with openssl < 1.0.2

Recent changes use struct cert_key_and_chain to load all certificates in
frontends, this structure was previously used only to load multi-cert
bundle, which is supported only on >= 1.0.2.

5 years agoMEDIUM: mux-h2: don't try to read more than needed
Willy Tarreau [Wed, 31 Jul 2019 14:00:48 +0000 (16:00 +0200)] 
MEDIUM: mux-h2: don't try to read more than needed

The h2_recv() loop was historically built around a loop to deal with
the callback model but this is not needed anymore, as it the upper
layer wants more data, it will simply try to read again. Right now
50% of the recvfrom() calls made over H2 return EAGAIN. With this
change it doesn't happen anymore. Note that the code simply consists
in breaking the loop, and reporting real data receipt instead of
always returning 1.

A test was made not to subscribe if we actually read data but it
doesn't change anything since we might be subscribed very early
already.

5 years agoMEDIUM: pollers: Remember the state for read and write for each threads.
Olivier Houchard [Thu, 25 Jul 2019 14:00:18 +0000 (14:00 +0000)] 
MEDIUM: pollers: Remember the state for read and write for each threads.

In the poller code, instead of just remembering if we're currently polling
a fd or not, remember if we're polling it for writing and/or for reading, that
way, we can avoid to modify the polling if it's already polled as needed.

5 years agoMAJOR: fd: Get rid of the fd cache.
Olivier Houchard [Wed, 24 Jul 2019 16:07:06 +0000 (18:07 +0200)] 
MAJOR: fd: Get rid of the fd cache.

Now that the architecture was changed so that attempts to receive/send data
always come from the upper layers, instead of them only trying to do so when
the lower layer let them know they could try, we can finally get rid of the
fd cache. We don't really need it anymore, and removing it gives us a small
performance boost.

5 years agoMINOR: ssl: clean ret variable in ssl_sock_load_ckchn
Emmanuel Hocdet [Tue, 30 Jul 2019 15:17:03 +0000 (17:17 +0200)] 
MINOR: ssl: clean ret variable in ssl_sock_load_ckchn

In ssl_sock_load_ckchn, ret variable is now in a half dead usage.
Remove it to clean compilation warnings.

5 years agoCLEANUP: ssl: ssl_sock_load_crt_file_into_ckch
Emmanuel Hocdet [Tue, 4 Dec 2018 16:37:47 +0000 (17:37 +0100)] 
CLEANUP: ssl: ssl_sock_load_crt_file_into_ckch

Fix comments for this function and remove free before alloc call: ckch
call is correctly balanced  (alloc/free).

5 years agoMINOR: ssl: do not look at DHparam with OPENSSL_NO_DH
Emmanuel Hocdet [Tue, 30 Jul 2019 15:04:01 +0000 (17:04 +0200)] 
MINOR: ssl: do not look at DHparam with OPENSSL_NO_DH

OPENSSL_NO_DH can be defined to avoid obsolete and heavy DH processing.
With OPENSSL_NO_DH, parse the entire PEM file to look at DHparam is wast
of time.

5 years agoMINOR: ssl: check private key consistency in loading
Emmanuel Hocdet [Tue, 30 Jul 2019 12:21:25 +0000 (14:21 +0200)] 
MINOR: ssl: check private key consistency in loading

Load a PEM certificate and use it in CTX are now decorrelated.
Checking the certificate and private key consistency can be done
earlier: in loading phase instead CTX set phase.

5 years agoMINOR: ssl: add extra chain compatibility
Emmanuel Hocdet [Mon, 3 Dec 2018 17:07:44 +0000 (18:07 +0100)] 
MINOR: ssl: add extra chain compatibility

cert_key_and_chain handling is now outside openssl 1.0.2 #if: the
code must be libssl compatible. SSL_CTX_add1_chain_cert and
SSL_CTX_set1_chain requires openssl >= 1.0.2, replace it by legacy
SSL_CTX_add_extra_chain_cert when SSL_CTX_set1_chain is not provided.

5 years agoMINOR: ssl: use STACK_OF for chain certs
Emmanuel Hocdet [Fri, 30 Nov 2018 15:00:21 +0000 (16:00 +0100)] 
MINOR: ssl: use STACK_OF for chain certs

Used native cert chain manipulation with STACK_OF from ssl lib.

5 years agoBUG/MAJOR: queue/threads: avoid an AB/BA locking issue in process_srv_queue()
Willy Tarreau [Tue, 30 Jul 2019 09:59:34 +0000 (11:59 +0200)] 
BUG/MAJOR: queue/threads: avoid an AB/BA locking issue in process_srv_queue()

A problem involving server slowstart was reported by @max2k1 in issue #197.
The problem is that pendconn_grab_from_px() takes the proxy lock while
already under the server's lock while process_srv_queue() first takes the
proxy's lock then the server's lock.

While the latter seems more natural, it is fundamentally incompatible with
mayn other operations performed on servers, namely state change propagation,
where the proxy is only known after the server and cannot be locked around
the servers. Howwever reversing the lock in process_srv_queue() is trivial
and only the few functions related to dynamic cookies need to be adjusted
for this so that the proxy's lock is taken for each server operation. This
is possible because the proxy's server list is built once at boot time and
remains stable. So this is what this patch does.

The comments in the proxy and server structs were updated to mention this
rule that the server's lock may not be taken under the proxy's lock but
may enclose it.

Another approach could consist in using a second lock for the proxy's queue
which would be different from the regular proxy's lock, but given that the
operations above are rare and operate on small servers list, there is no
reason for overdesigning a solution.

This fix was successfully tested with 10000 servers in a backend where
adjusting the dyncookies in loops over the CLI didn't have a measurable
impact on the traffic.

The only workaround without the fix is to disable any occurrence of
"slowstart" on server lines, or to disable threads using "nbthread 1".

This must be backported as far as 1.8.

5 years agoMEDIUM: ssl: load DH param in struct cert_key_and_chain
William Lallemand [Tue, 23 Jul 2019 14:06:08 +0000 (16:06 +0200)] 
MEDIUM: ssl: load DH param in struct cert_key_and_chain

Load the DH param at the same time as the certificate, we don't need to
open the file once more and read it again. We store it in the ckch_node.

There is a minor change comparing to the previous way of loading the DH
param in a bundle. With a bundle, the DH param in a certificate file was
never loaded, it only used the global DH or the default DH, now it's
able to use the DH param from a certificate file.

5 years agoMEDIUM: ssl: lookup and store in a ckch_node tree
William Lallemand [Tue, 23 Jul 2019 13:00:54 +0000 (15:00 +0200)] 
MEDIUM: ssl: lookup and store in a ckch_node tree

Don't read a certificate file again if it was already stored in the
ckchn tree. It allows HAProxy to start more quickly if the same
certificate is used at different places in the configuration.

HAProxy lookup in the ssl_sock_load_cert() function, doing it at this
level allows to skip the reading of the certificate in the filesystem.

If the certificate is not found in the tree, we insert the ckch_node in
the tree once the certificate is read on the filesystem, the filename or
the bundle name is used as the key.

5 years agoMEDIUM: ssl: split the loading of the certificates
William Lallemand [Thu, 18 Jul 2019 17:28:17 +0000 (19:28 +0200)] 
MEDIUM: ssl: split the loading of the certificates

Split the functions which open the certificates.

Instead of opening directly the certificates and inserting them directly
into a SSL_CTX, we use a struct cert_key_and_chain to store them in
memory and then we associate a SSL_CTX to the certificate stored in that
structure.

Introduce the struct ckch_node for the multi-cert bundles so we can
store multiple cert_key_and_chain in the same structure.

The functions ssl_sock_load_multi_cert() and ssl_sock_load_cert_file()
were modified so they don't open the certicates anymore on the
filesystem. (they still open the sctl and ocsp though).  These functions
were renamed ssl_sock_load_ckchn() and ssl_sock_load_multi_ckchn().

The new function ckchn_load_cert_file() is in charge of loading the
files in the cert_key_and_chain. (TODO: load ocsp and sctl from there
too).

The ultimate goal is to be able to load a certificate from a certificate
tree without doing any filesystem access, so we don't try to open it
again if it was already loaded, and we share its configuration.

5 years agoMEDIUM: ssl: use cert_key_and_chain struct in ssl_sock_load_cert_file()
William Lallemand [Wed, 15 May 2019 14:08:56 +0000 (16:08 +0200)] 
MEDIUM: ssl: use cert_key_and_chain struct in ssl_sock_load_cert_file()

This structure was only used in the case of the multi-cert bundle.

Using these primitives everywhere when we load the file are a first step
in the deduplication of the code.

5 years agoMINOR: ssl: merge ssl_sock_load_cert_file() and ssl_sock_load_cert_chain_file()
William Lallemand [Wed, 15 May 2019 13:33:54 +0000 (15:33 +0200)] 
MINOR: ssl: merge ssl_sock_load_cert_file() and ssl_sock_load_cert_chain_file()

This commit merges the function ssl_sock_load_cert_file() and
ssl_sock_load_cert_chain_file().

The goal is to refactor the SSL code and use the cert_key_and_chain
struct to load everything.

5 years agoBUG/MINOR: htx: Fix free space addresses calculation during a block expansion
Christopher Faulet [Mon, 29 Jul 2019 08:50:28 +0000 (10:50 +0200)] 
BUG/MINOR: htx: Fix free space addresses calculation during a block expansion

When the payload of a block is shrinked or enlarged, addresses of the free
spaces must be updated. There are many possible cases. One of them is
buggy. When there is only one block in the HTX message and its payload is just
before the tail room and it needs to be moved in the head room to be enlarged,
addresses are not correctly updated. This bug may be hit by the compression
filter.

This patch must be backported to 2.0.

5 years agoBUG/MINOR: hlua: Only execute functions of HTTP class if the txn is HTTP ready
Christopher Faulet [Fri, 26 Jul 2019 14:31:34 +0000 (16:31 +0200)] 
BUG/MINOR: hlua: Only execute functions of HTTP class if the txn is HTTP ready

The flag HLUA_TXN_HTTP_RDY was added in the previous commit to know when a
function is called for a channel with a valid HTTP message or not. Of course it
also depends on the calling direction. In this commit, we allow the execution of
functions of the HTTP class only if this flag is set.

Nobody seems to use them from an unsupported context (for instance, trying to
set an HTTP header from a tcp-request rule). But it remains a bug leading to
undefined behaviors or crashes.

This patch may be backported to all versions since the 1.6. It depends on the
commits "MINOR: hlua: Add a flag on the lua txn to know in which context it can
be used" and "MINOR: hlua: Don't set request analyzers on response channel for
lua actions".

5 years agoMINOR: hlua: Add a flag on the lua txn to know in which context it can be used
Christopher Faulet [Fri, 26 Jul 2019 13:09:53 +0000 (15:09 +0200)] 
MINOR: hlua: Add a flag on the lua txn to know in which context it can be used

When a lua action or a lua sample fetch is called, a lua transaction is
created. It is an entry in the stack containing the class TXN. Thanks to it, we
can know the direction (request or response) of the call. But, for some
functions, it is also necessary to know if the buffer is "HTTP ready" for the
given direction. "HTTP ready" means there is a valid HTTP message in the
channel's buffer. So, when a lua action or a lua sample fetch is called, the
flag HLUA_TXN_HTTP_RDY is set if it is appropriate.

5 years agoMINOR: hlua: Don't set request analyzers on response channel for lua actions
Christopher Faulet [Fri, 26 Jul 2019 12:54:52 +0000 (14:54 +0200)] 
MINOR: hlua: Don't set request analyzers on response channel for lua actions

Setting some requests analyzers on the response channel was an old trick to be
sure to re-evaluate the request's analyers after the response's ones have been
called. It is no more necessary. In fact, this trick was removed in the version
1.8 and backported up to the version 1.6.

This patch must be backported to all versions since 1.6 to ease the backports of
fixes on the lua code.

5 years agoBUG/MEDIUM: hlua: Check the calling direction in lua functions of the HTTP class
Christopher Faulet [Fri, 26 Jul 2019 14:17:01 +0000 (16:17 +0200)] 
BUG/MEDIUM: hlua: Check the calling direction in lua functions of the HTTP class

It is invalid to manipulate responses from http-request rules or to manipulate
requests from http-response rules. When http-request rules are evaluated, the
connection to server is not yet established, so there is no response at all. And
when http-response rules are evaluated, the request has already been sent to the
server.

Now, the calling direction is checked. So functions "txn.http:req_*" can now
only be called from http-request rules and the functions "txn.http:res_*" can
only be called from http-response rules.

This issue was reported on Github (#190).

This patch must be backported to all versions since the 1.6.

5 years agoBUG/MINOR: hlua/htx: Reset channels analyzers when txn:done() is called
Christopher Faulet [Fri, 26 Jul 2019 14:40:24 +0000 (16:40 +0200)] 
BUG/MINOR: hlua/htx: Reset channels analyzers when txn:done() is called

For HTX streams, when txn:done() is called, the work is delegated to the
function http_reply_and_close(). But it is not enough. The channel's analyzers
must also be reset. Otherwise, some analyzers may still be called while
processing should be aborted.

For instance, if the function is called from an http-request rules on the
frontend, request analyzers on the backend side are still called. So we may try
to add an header to the request, while this one was already reset.

This patch must be backported to 2.0 and 1.9.

5 years agoREGTESTS: checks: exclude freebsd target for tcp-check_multiple_ports.vtc
Jérôme Magnin [Tue, 23 Jul 2019 21:23:16 +0000 (23:23 +0200)] 
REGTESTS: checks: exclude freebsd target for tcp-check_multiple_ports.vtc

This patch excludes freebsd, osx and generic targets for this vtc.

Basic tcp checks performed by haproxy on a linux system leverage the
TCP_QUICKACK option which implies that the connection is never
established from the perspective of the backend server. On other systems
a regular tcp 3 way handshake is performed immediately followed by a
reset, which from the perspective of the server is an aborted connection.

When we run this regtest on FreeBSD (or anything other than linux) there
is a race condition in the server_thread() function of the vtc_server.c
file. If we receive the reset when we are in accept() then fd is -1 and
vtest calls vtc_fatal, failing the test.

Other checks specific reg-tests were excluded on FreeBSD, osx and
generic for the same reason, but were at the time documented as being
disabled because they used TCP_DEFER_ACCEPT. These commits are
15685c791 ("REGTEST: Exclude freebsd target for some reg tests") and
03c6ab0cb ("REGTEST: exclude osx and generic targets for
40be_2srv_odd_health_checks")

5 years agoMEDIUM: h1: Don't wake the H1 tasklet if we got the whole request.
Olivier Houchard [Fri, 26 Jul 2019 13:12:38 +0000 (15:12 +0200)] 
MEDIUM: h1: Don't wake the H1 tasklet if we got the whole request.

In h1_rcv_buf(), don't wake the H1 tasklet to attempt to receive more data
if we got the whole request. It will lead to a recv and maybe to a subscribe
while it may not be needed.
If the connection is keep alive, the tasklet will be woken up later by
h1_detach(), so that we'll be able to get the next request, or an end of
connection.

5 years agoMEDIUM: h1: Don't try to subscribe if we managed to read data.
Olivier Houchard [Fri, 26 Jul 2019 13:11:11 +0000 (15:11 +0200)] 
MEDIUM: h1: Don't try to subscribe if we managed to read data.

In h1_recv(), don't subscribe if we managed to receive data. We may not have
to, if we received a complete request, and a new receive will be attempted
later, as the tasklet is woken up either by h1_rcv_buf() or by h1_detach.

5 years agoDOC: improve the wording in CONTRIBUTING about how to document a bug fix
Willy Tarreau [Fri, 26 Jul 2019 13:21:54 +0000 (15:21 +0200)] 
DOC: improve the wording in CONTRIBUTING about how to document a bug fix

Insufficiently described bug fixes are still too frequent. It's a real
pain to create each new maintenance release, as 3/4 of the time is spent
trying to guess what problem a patch fixes, which is already important
in order to decide whether to pick the fix or not, but is even more
capital in order to write understandable release notes.

Christopher rightfully demands that a patch tagged "BUG" MUST ABSOLUTELY
describe the problem and why this problem is a bug. Describing the fix
is one thing but if the bug is unknown, why would there be a fix ? How
can a stable maintainer be convinced to take a fix if its author didn't
care about checking whether it was a real bug ? This patch tries to
explain a bit better what really needs to appear in the commit message
and how to describe a bug.

To be backported to all relevant stable versions.

5 years agoBUG/MINOR: log: make sure writev() is not interrupted on a file output
Willy Tarreau [Fri, 26 Jul 2019 13:10:39 +0000 (15:10 +0200)] 
BUG/MINOR: log: make sure writev() is not interrupted on a file output

Since 1.9 we support sending logs to various non-blocking outputs like
stdou/stderr or flies, by using writev() which guarantees that it only
returns after having written everything or nothing. However the syscall
may be interrupted while doing so, and this is visible when writing to
a tty during debug sessions, as some logs occasionally appear interleaved
if an xterm or SSH connection is not very fast. Performance here is not a
critical concern, log correctness is. Let's simply take the logger's lock
around the writev() call to prevent multiple senders from stepping onto
each other's toes.

This may be backported to 2.0 and 1.9.

5 years agoBUG/MEDIUM: streams: Don't switch the SI to SI_ST_DIS if we have data to send.
Olivier Houchard [Fri, 26 Jul 2019 12:54:34 +0000 (14:54 +0200)] 
BUG/MEDIUM: streams: Don't switch the SI to SI_ST_DIS if we have data to send.

In sess_established(), don't immediately switch the backend stream_interface
to SI_ST_DIS if we only got a SHUTR. We may still have something to send,
ie if the request is a POST, and we should be switched to SI_ST8DIS later
when the shutw will happen.

This should be backported to 2.0 and 1.9.

5 years agoBUG/MEDIUM: lb-chash: Fix the realloc() when the number of nodes is increased
Christopher Faulet [Fri, 26 Jul 2019 11:52:13 +0000 (13:52 +0200)] 
BUG/MEDIUM: lb-chash: Fix the realloc() when the number of nodes is increased

When the number of nodes is increased because the server weight is changed, the
nodes array must be realloc. But its new size is not correctly set. Only the
total number of nodes is used to set the new size. But it must also depends on
the size of a node. It must be the total nomber of nodes times the size of a
node.

This issue was reported on Github (#189).

This patch must be backported to all versions since the 1.6.

5 years agoBUILD: threads: add the definition of PROTO_LOCK
Willy Tarreau [Thu, 25 Jul 2019 05:53:56 +0000 (07:53 +0200)] 
BUILD: threads: add the definition of PROTO_LOCK

This one was added by commit daacf3664 ("BUG/MEDIUM: protocols: add a
global lock for the init/deinit stuff") but I forgot to add it to the
include file, breaking DEBUG_THREAD.

5 years agoMEDIUM: mux-h1: Add the support of headers adjustment for bogus HTTP/1 apps
Christopher Faulet [Mon, 22 Jul 2019 14:18:24 +0000 (16:18 +0200)] 
MEDIUM: mux-h1: Add the support of headers adjustment for bogus HTTP/1 apps

There is no standard case for HTTP header names because, as stated in the
RFC7230, they are case-insensitive. So applications must handle them in a
case-insensitive manner. But some bogus applications erroneously rely on the
case used by most browsers. This problem becomes critical with HTTP/2
because all header names must be exchanged in lowercase. And HAProxy uses the
same convention. All header names are sent in lowercase to clients and servers,
regardless of the HTTP version.

This design choice is linked to the HTX implementation. So, for previous
versions (2.0 and 1.9), a workaround is to disable the HTX mode to fall
back to the legacy HTTP mode.

Since the legacy HTTP mode was removed, some users reported interoperability
issues because their application was not able anymore to handle HTTP/1 message
received from HAProxy. So, we've decided to add a way to change the case of some
headers before sending them. It is now possible to define a "mapping" between a
lowercase header name and a version supported by the bogus application. To do
so, you must use the global directives "h1-case-adjust" and
"h1-case-adjust-file". Then options "h1-case-adjust-bogus-client" and
"h1-case-adjust-bogus-server" may be used in proxy sections to enable the
conversion. See the configuration manual for more info.

Of course, our advice is to urgently upgrade these applications for
interoperability concerns and because they may be vulnerable to various types of
content smuggling attacks. But, if your are really forced to use an unmaintained
bogus application, you may use these directive, at your own risks.

If it is relevant, this feature may be backported to 2.0.

5 years agoBUG/MINOR: proxy: always lock stop_proxy()
Willy Tarreau [Wed, 24 Jul 2019 15:42:44 +0000 (17:42 +0200)] 
BUG/MINOR: proxy: always lock stop_proxy()

There is one unprotected call to stop_proxy() from the manage_proxy()
task, so there is a single caller by definition, but there is also
another such call from the CLI's "shutdown frontend" parser. This
one does it under the proxy's lock but the first one doesn't use it.
Thus it is theorically possible to corrupt the list of listeners in a
proxy by issuing "shutdown frontend" and SIGUSR1 exactly at the same
time. While it sounds particularly contrived or stupid, it could
possibly happen with automated tools that would send actions via
various channels. This could cause the process to loop forever or
to crash and thus stop faster than expected.

This might be backported as far as 1.8.

5 years agoBUG/MEDIUM: protocols: add a global lock for the init/deinit stuff
Willy Tarreau [Wed, 24 Jul 2019 14:45:02 +0000 (16:45 +0200)] 
BUG/MEDIUM: protocols: add a global lock for the init/deinit stuff

Dragan Dosen found that the listeners lock is not sufficient to protect
the listeners list when proxies are stopping because the listeners are
also unlinked from the protocol list, and under certain situations like
bombing with soft-stop signals or shutting down many frontends in parallel
from multiple CLI connections, it could be possible to provoke multiple
instances of delete_listener() to be called in parallel for different
listeners, thus corrupting the protocol lists.

Such operations are pretty rare, they are performed once per proxy upon
startup and once per proxy on shut down. Thus there is no point trying
to optimize anything and we can use a global lock to protect the protocol
lists during these manipulations.

This fix (or a variant) will have to be backported as far as 1.8.

5 years agoBUG/CRITICAL: http_ana: Fix parsing of malformed cookies which start by a delimiter
Olivier Houchard [Mon, 22 Jul 2019 15:43:46 +0000 (17:43 +0200)] 
BUG/CRITICAL: http_ana: Fix parsing of malformed cookies which start by a delimiter

When client-side or server-side cookies are parsed, HAProxy enters in an
infinite loop if a Cookie/Set-Cookie header value starts by a delimiter (a colon
or a semicolon). Depending on the operating system, the service may become
degraded, unresponsive, or may trigger haproxy's watchdog causing a service stop
or automatic restart.

To fix this bug, in the loop parsing the attributes, we must be sure to always
skip delimiters once the first attribute-value pair was parsed, empty or
not. The credit for the fix goes to Olivier.

CVE-2019-14241 was assigned to this bug. This patch fixes the Github issue #181.

This patch must be backported to 2.0 and 1.9. However, the patch will have to be
adapted.

5 years agoBUG/MINOR: http_htx: Support empty errorfiles
Christopher Faulet [Mon, 22 Jul 2019 14:49:30 +0000 (16:49 +0200)] 
BUG/MINOR: http_htx: Support empty errorfiles

Empty error files may be used to disable the sending of any message for specific
error codes. A common use-case is to use the file "/dev/null". This way the
default error message is overridden and no message is returned to the client. It
was supported in the legacy HTTP mode, but not in HTX. Because of a bug, such
messages triggered an error.

This patch must be backported to 2.0 and 1.9. However, the patch will have to be
adapted.

5 years agoBUG/MINOR: http_ana: Be sure to have an allocated buffer to generate an error
Christopher Faulet [Mon, 22 Jul 2019 14:41:43 +0000 (16:41 +0200)] 
BUG/MINOR: http_ana: Be sure to have an allocated buffer to generate an error

In http_reply_and_close() and http_server_error(), we must be sure to have an
allocated buffer (buf.size > 0) to consider it as a valid HTX message. For now,
there is no way to hit this bug. But a fix to support "empty" error messages in
HTX is pending. Such empty messages, after parsing, will be converted into
unallocated buffer (buf.size == 0).

This patch must be backported to 2.0 and 1.9. owever, the patch will have to be
adapted.