MINOR: init: report in "haproxy -c" whether there were warnings or not
This helps quickly checking if the config produces any warning. For
this we reuse the "warned" bit field to add a new WARN_ANY bit that is
set by ha_warning(). The rest of the bit field was also cleaned from
unused bits.
BUG/MINOR: peers: Incomplete peers sections should be validated.
Before supporting "server" line in "peers" section, such sections without
any local peer were removed from the configuration to get it validated.
This patch fixes the issue where a "server" line without address and port which
is a remote peer without address and port makes the configuration parsing fail.
When encoutering such cases we now ignore such lines remove them from the
configuration.
Thank you to Jérôme Magnin for having reported this bug.
BUG/MINOR: connection: always send address-less LOCAL PROXY connections
Commit 7f26391bc5 ("BUG/MINOR: connection: make sure to correctly tag
local PROXY connections") revealed that some implementations do not
properly ignore addresses in LOCAL connections (at least Dovecot was
spotted). More context information in the thread below:
The patch above was using LOCAL on top of local addresses in order to
minimize the risk of breakage but revealed worse than a clean fix. So
let's partially revert it and send pure LOCAL connections instead now.
After a bit of observation, this patch should be progressively backported
to stable branches. However if it reveals new breakage, the backport of
the patch above will have to be reverted from stable branches while other
products work on fixing their code based on the master branch.
BUG/MINOR: ssl: ssl_conf always set to NULL on crt-list parsing
When reading a crt-list file, the SSL options betweeen square brackets
are parsed, however the calling function sets the ssl_conf ptr to NULL
leading to all options being ignored, and a memory leak.
This is a remaining of the previous code which was forgotten.
This bug was introduced by 97b0810 ("MINOR: ssl: split the line parsing
of the crt-list").
MINOR: ssl: ckch_store_new() alloc and init a ckch_store
Create a ckch_store_new() function which alloc and initialize a
ckch_store, allowing us to remove duplicated code and avoiding wrong
initialization in the future.
CLEANUP: ssl/cli: use the list of filters in the crtlist_entry
In 'commit ssl cert', instead of trying to regenerate a list of filters
from the SNIs, use the list provided by the crtlist_entry used to
generate the ckch_inst.
This list of filters doesn't need to be free'd anymore since they are
always reused from the crtlist_entry.
Use the refcount of the SSL_CTX' to free them instead of freeing them on
certains conditions. That way we can free the SSL_CTX everywhere its
pointer is used.
When deleting the previous SNI entries with 'set ssl cert', the old
SSL_CTX' were not free'd, which probably prevent the completion of the
free of the X509 in the old ckch_store, because of the refcounts in the
SSL library.
This bug was introduced by 150bfa8 ("MEDIUM: cli/ssl: handle the
creation of SSL_CTX in an IO handler").
BUG/MINOR: ssl: trailing slashes in directory names wrongly cached
The crtlist_load_cert_dir() caches the directory name without trailing
slashes when ssl_sock_load_cert_list_file() tries to lookup without
cleaning the trailing slashes.
This bug leads to creating the crtlist twice and prevents to remove
correctly a crtlist_entry since it exists in the serveral crtlists
created by accident.
Move the trailing slashes cleanup in ssl_sock_load_cert_list_file() to
fix the problem.
This bug was introduced by 6be66ec ("MINOR: ssl: directories are loaded
like crt-list")
MINOR: ssl/cli: 'del ssl cert' deletes a certificate
Delete a certificate store from HAProxy and free its memory. The
certificate must be unused and removed from any crt-list or directory.
The deletion doesn't work with a certificate referenced directly with
the "crt" directive in the configuration.
MINOR: ssl/cli: improve error for bundle in add/del ssl crt-list
Bundles are deprecated and can't be used with the crt-list command of
the CLI, improve the error output when trying to use them so the users
can disable them.
BUG/MINOR: ssl/cli: lock the ckch structures during crt-list delete
The cli_parse_del_crtlist() does unlock the ckch big lock, but it does
not lock it at the beginning of the function which is dangerous.
As a side effect it let the structures locked once it called the unlock.
This bug was introduced by 0a9b941 ("MINOR: ssl/cli: 'del ssl crt-list'
delete an entry")
CI: adopt openssl download script to download all versions
with recent change, OpenSSL download URL was changed in
incompatiable way. i.e. only the most recent openssl version
might be downloaded using previous script.
older versions are available under different URLs. as we need
several openssl versions, let us adopt script accordingly.
bug was caught after travis-ci cache was purged for some reason.
CI: travis-ci: enable DEBUG_STRICT=1 for CI builds
DEBUG_STRICT enables the BUG_ON() macro which validates some developers'
assertions in the code that are not enabled for production build but
may sometimes help catch certain rare bugs.
MINOR: ssl/cli: replace dump/show ssl crt-list by '-n' option
The dump and show ssl crt-list commands does the same thing, they dump
the content of a crt-list, but the 'show' displays an ID in the first
column. Delete the 'dump' command so it is replaced by the 'show' one.
The old 'show' command is replaced by an '-n' option to dump the ID.
And the ID which was a pointer is replaced by a line number and placed
after colons in the filename.
MINOR: ssl/cli: 'del ssl crt-list' delete an entry
Delete an entry in a crt-list, this is done by iterating over the
ckch_inst in the crtlist_entry. For each ckch_inst the bind_conf lock is
held during the deletion of the sni_ctx in the SNI trees. Everything
is free'd.
If there is several entries with the same certificate, a line number
must be provided to chose with entry delete.
BUG/MINOR: ssl/cli: initialize fcount int crtlist_entry
Initialize fcount to 0 when 'add ssl crt-list' does not contain any
filters. This bug can lead to trying to read some filters even if they
doesn't exist.
BUG/MINOR: protocol_buffer: Wrong maximum shifting.
This patch fixes a bad stop condition when decoding a protocol buffer variable integer
whose maximum lenghts are 10, shifting a uint64_t value by more than 63.
Willy Tarreau [Sun, 29 Mar 2020 06:53:31 +0000 (08:53 +0200)]
BUG/CRITICAL: hpack: never index a header into the headroom after wrapping
The HPACK header table is implemented as a wrapping list inside a contigous
area. Headers names and values are stored from right to left while indexes
are stored from left to right. When there's no more room to store a new one,
we wrap to the right again, or possibly defragment it if needed. The condition
do use the right part (called tailroom) or the left part (called headroom)
depends on the location of the last inserted header. After wrapping happens,
the code forces to stick to tailroom by pretending there's no more headroom,
so that the size fit test always fails. The problem is that nothing prevents
from storing a header with an empty name and empty value, resulting in a
total size of zero bytes, which satisfies the condition to use the headroom.
Doing this in a wrapped buffer results in changing the "front" header index
and causing miscalculations on the available size and the addresses of the
next headers. This may even allow to overwrite some parts of the index,
opening the possibility to perform arbitrary writes into a 32-bit relative
address space.
This patch fixes the issue by making sure the headroom is considered only
when the buffer does not wrap, instead of relying on the zero size. This
must be backported to all versions supporting H2, which is as far as 1.8.
Many thanks to Felix Wilhelm of Google Project Zero for responsibly
reporting this problem with a reproducer and a detailed analysis.
CVE-2020-11100 was assigned to this issue.
MINOR: ssl/cli: support filters and options in add ssl crt-list
Add the support for filters and SSL options in the CLI command
"add ssl crt-list".
The feature was implemented by applying the same parser as the crt-list
file to the payload.
The new options are passed to the command as a payload with the same
format that is suppported by the crt-list file itself, so you can easily
copy a line from a file and push it via the CLI.
MINOR: ssl: split the line parsing of the crt-list
In order to reuse the crt-list line parsing in "add ssl crt-list",
the line parsing was extracted from crtlist_parse_file() to a new
function crtlist_parse_line().
This function fills a crtlist_entry structure with a bind_ssl_conf and
the list of filters.
BUG/MEDIUM: dns: Consider the fact that dns answers are case-insensitive
We can't expect the DNS answer to always match the case we used for the
request, so we can't just use memcmp() to compare the DNS answer with what
we are expected.
Instead, introduce dns_hostname_cmp(), which compares each string in a
case-insensitive way.
This should fix github issue #566.
This should be backported to 2.1, 2.0, 1.9 and 1.8.
REGTEST: make the unique-id test depend on version 2.0
Regtest unique-id.vtc was added by commit 5fcec84c58 ("REGTEST: Add
unique-id reg-test") but it relies on the "uuid" sample fetch which
is only available in version 2.0 and above. Let's reflect that in
the REQUIRE_VERSION tag.
Daniel Corbett [Sat, 28 Mar 2020 16:35:50 +0000 (12:35 -0400)]
BUG/MINOR: stats: Fix color of draining servers on stats page
This patch fixes #53 where it was noticed that when an active
server is set to DRAIN it no longer has the color blue reflected
within the stats page. This patch addresses that and adds the
color back to drain. It's to be noted that backup servers are
configured to have an orange color when they are draining.
Willy Tarreau [Tue, 31 Mar 2020 14:36:20 +0000 (16:36 +0200)]
REGTEST: make the PROXY TLV validation depend on version 2.2
Regtest proxy_protocol_tlv_validation was added by commit 488ee7fb6e
("BUG/MAJOR: proxy_protocol: Properly validate TLV lengths") but it
relies on a trick involving http-after-response to append a header
after a 400-badreq response, which is not possible in earlier versions,
so make it depend on 2.2.
The new 'add ssl crt-list' command allows to add a new crt-list entry in
a crt-list (or a directory since they are handled the same way).
The principle is basicaly the same as the "commit ssl cert" command with
the exception that it iterates on every bind_conf that uses the crt-list
and generates a ckch instance (ckch_inst) for each of them.
The IO handler yield every 10 bind_confs so HAProxy does not get stuck in
a too much time consuming generation if it needs to generate too much
SSL_CTX'.
This command does not handle the SNI filters and the SSL configuration
yet.
Example:
$ echo "new ssl cert foo.net.pem" | socat /tmp/sock1 -
New empty certificate store 'foo.net.pem'!
$ echo -e -n "set ssl cert foo.net.pem <<\n$(cat foo.net.pem)\n\n" | socat /tmp/sock1 -
Transaction created for certificate foo.net.pem!
MINOR: ssl/cli: update pointer to store in 'commit ssl cert'
The crtlist_entry structure use a pointer to the store as key.
That's a problem with the dynamic update of a certificate over the CLI,
because it allocates a new ckch_store. So updating the pointers is
needed. To achieve that, a linked list of the crtlist_entry was added in
the ckch_store, so it's easy to iterate on this list to update the
pointers. Another solution would have been to rework the system so we
don't allocate a new ckch_store, but it requires a rework of the ckch
code.
MINOR: ssl: add a list of crtlist_entry in ckch_store
When updating a ckch_store we may want to update its pointer in the
crtlist_entry which use it. To do this, we need the list of the entries
using the store.
BUG/MINOR: ssl: ckch_inst wrongly inserted in crtlist_entry
The instances were wrongly inserted in the crtlist entries, all
instances of a crt-list were inserted in the last crt-list entry.
Which was kind of handy to free all instances upon error.
Now that it's done correctly, the error path was changed, it must
iterate on the entries and find the ckch_insts which were generated for
this bind_conf. To avoid wasting time, it stops the iteration once it
found the first unsuccessful generation.
MINOR: ssl: add a list of bind_conf in struct crtlist
In order to be able to add new certificate in a crt-list, we need the
list of bind_conf that uses this crt-list so we can create a ckch_inst
for each of them.
Olivier Houchard [Sun, 29 Mar 2020 22:23:57 +0000 (00:23 +0200)]
MEDIUM: connections: Revamp the way idle connections are killed
The original algorithm always killed half the idle connections. This doesn't
take into account the way the load can change. Instead, we now kill half
of the exceeding connections (exceeding connection being the number of
used + idle connections past the last maximum used connections reached).
That way if we reach a peak, we will kill much less, and it'll slowly go back
down when there's less usage.
Olivier Houchard [Wed, 25 Mar 2020 18:41:03 +0000 (19:41 +0100)]
MINOR: servers: Add a counter for the number of currently used connections.
Add a counter to know the current number of used connections, as well as the
max, this will be used later to refine the algorithm used to kill idle
connections, based on current usage.
Jerome Magnin [Sun, 29 Mar 2020 07:37:12 +0000 (09:37 +0200)]
MEDIUM: stream: support use-server rules with dynamic names
With server-template was introduced the possibility to scale the
number of servers in a backend without needing a configuration change
and associated reload. On the other hand it became impractical to
write use-server rules for these servers as they would only accept
existing server labels as argument. This patch allows the use of
log-format notation to describe targets of a use-server rules, such
as in the example below:
listen test
bind *:1234
use-server %[hdr(srv)] if { hdr(srv) -m found }
use-server s1 if { path / }
server s1 127.0.0.1:18080
server s2 127.0.0.1:18081
If a use-server rule is applied because it was conditionned by an
ACL returning true, but the target of the use-server rule cannot be
resolved, no other use-server rule is evaluated and we fall back to
load balancing.
This feature was requested on the ML, and bumped with issue #563.
Jerome Magnin [Fri, 27 Mar 2020 21:08:40 +0000 (22:08 +0100)]
MINOR: listener: add so_name sample fetch
Add a sample fetch for the name of a bind. This can be useful to
take decisions when PROXY protocol is used and we can't rely on dst,
such as the sample config below.
defaults
mode http
listen bar
bind 127.0.0.1:1111
server s1 127.0.1.1:1234 send-proxy
listen foo
bind 127.0.1.1:1234 name foo accept-proxy
http-request return status 200 hdr dst %[dst] if { dst 127.0.1.1 }
Emmanuel Hocdet [Fri, 28 Feb 2020 15:00:34 +0000 (16:00 +0100)]
MINOR: ssl: skip self issued CA in cert chain for ssl_ctx
First: self issued CA, aka root CA, is the enchor for chain validation,
no need to send it, client must have it. HAProxy can skip it in ssl_ctx.
Second: the main motivation to skip root CA in ssl_ctx is to be able to
provide it in the chain without drawback. Use case is to provide issuer
for ocsp without the need for .issuer and be able to share it in
issuers-chain-path. This concerns all certificates without intermediate
certificates. It's useless for BoringSSL, .issuer is ignored because ocsp
bits doesn't need it.
Baptiste Assmann [Wed, 19 Feb 2020 00:08:51 +0000 (01:08 +0100)]
BUG/MEDIUM: dns: improper parsing of aditional records
13a9232ebc63fdf357ffcf4fa7a1a5e77a1eac2b introduced parsing of
Additionnal DNS response section to pick up IP address when available.
That said, this introduced a side effect for other query types (A and
AAAA) leading to consider those responses invalid when parsing the
Additional section.
This patch avoids this situation by ensuring the Additional section is
parsed only for SRV queries.
Olivier Houchard [Wed, 25 Mar 2020 11:24:11 +0000 (12:24 +0100)]
BUG/MEDIUM: mux_h1: Process a new request if we already received it.
In h1_detach(), if our input buffer isn't empty, don't just subscribe(), we
may hold a new request, and there's nothing left to read. Instead, call
h1_process() directly, so that a new stream is created.
Failure to do so means if we received the new request to early, the
connecetion will just hang, as it happens when using svn.
BUG/MINOR: peers: Use after free of "peers" section.
When a "peers" section has not any local peer, it is removed of the list
of "peers" sections by check_config_validity(). But a stick-table which
refers to a "peers" section stores a pointer to this peers section.
These pointer must be reset to NULL value for each stick-table refering to
such a "peers" section to prevent stktable_init() to start the peers frontend
attached to the peers section dereferencing the invalid pointer.
Furthemore this patch stops the peers frontend as this is done for other
configurations invalidated by check_config_validity().
Thank you to Olivier D for having reported this issue with such a
simple configuration file which made haproxy crash when started with
-c option for configuration file validation.
defaults
mode http
peers mypeers
peer toto 127.0.0.1:1024
backend test
stick-table type ip size 10k expire 1h store http_req_rate(1h) peers mypeers
BUG/MINOR: peers: init bind_proc to 1 if it wasn't initialized
Tim reported that in master-worker mode, if a stick-table is declared
but not used in the configuration, its associated peers listener won't
bind.
This problem is due the fact that the master-worker and the daemon mode,
depend on the bind_proc field of the peers proxy to disable the listener.
Unfortunately the bind_proc is left to 0 if no stick-table were used in
the configuration, stopping the listener on all processes.
This fixes sets the bind_proc to the first process if it wasn't
initialized.
Should fix bug #558. Should be backported as far as 1.8.
Emmanuel Hocdet [Fri, 28 Feb 2020 15:00:34 +0000 (16:00 +0100)]
MINOR: ssl: rework add cert chain to CTX to be libssl independent
SSL_CTX_set1_chain is used for openssl >= 1.0.2 and a loop with
SSL_CTX_add_extra_chain_cert for openssl < 1.0.2.
SSL_CTX_add_extra_chain_cert exist with openssl >= 1.0.2 and can be
used for all openssl version (is new name is SSL_CTX_add0_chain_cert).
This patch use SSL_CTX_add_extra_chain_cert to remove any #ifdef for
compatibilty. In addition sk_X509_num()/sk_X509_value() replace
sk_X509_shift() to extract CA from chain, as it is used in others part
of the code.
Emmanuel Hocdet [Mon, 23 Mar 2020 09:31:47 +0000 (10:31 +0100)]
BUG/MINOR: ssl: memory leak when find_chain is NULL
This bug was introduced by 85888573 "BUG/MEDIUM: ssl: chain must be
initialized with sk_X509_new_null()". No need to set find_chain with
sk_X509_new_null(), use find_chain conditionally to fix issue #516.
Willy Tarreau [Mon, 23 Mar 2020 08:43:45 +0000 (09:43 +0100)]
[RELEASE] Released version 2.2-dev5
Released version 2.2-dev5 with the following main changes :
- CLEANUP: ssl: is_default is a bit in ckch_inst
- BUG/MINOR: ssl/cli: sni_ctx' mustn't always be used as filters
- DOC: ssl: clarify security implications of TLS tickets
- CLEANUP: remove support for Linux i686 vsyscalls
- CLEANUP: drop support for USE_MY_ACCEPT4
- CLEANUP: remove support for USE_MY_EPOLL
- CLEANUP: remove support for USE_MY_SPLICE
- CLEANUP: remove the now unused common/syscall.h
- BUILD: make dladdr1 depend on glibc version and not __USE_GNU
- BUILD: wdt: only test for SI_TKILL when compiled with thread support
- BUILD: Makefile: the compiler-specific flags should all be in SPEC_CFLAGS
- CLEANUP: ssl: separate the directory loading in a new function
- BUG/MINOR: buffers: MT_LIST_DEL_SAFE() expects the temporary pointer.
- BUG/MEDIUM: mt_lists: Make sure we set the deleted element to NULL;
- MINOR: init: move the maxsock calculation code to compute_ideal_maxsock()
- MEDIUM: init: always try to push the FD limit when maxconn is set from -m
- BUG/MAJOR: list: fix invalid element address calculation
- BUILD: stream-int: fix a few includes dependencies
- MINOR: mt_lists: Appease gcc.
- MINOR: lists: Implement function to convert list => mt_list and mt_list => list
- MINOR: servers: Kill priv_conns.
- MINOR: lists: fix indentation.
- BUG/MEDIUM: random: align the state on 2*64 bits for ARM64
- BUG/MEDIUM: connections: Don't assume the connection has a valid session.
- BUG/MEDIUM: pools: Always update free_list in pool_gc().
- BUG/MINOR: haproxy: always initialize sleeping_thread_mask
- BUG/MINOR: listener/mq: do not dispatch connections to remote threads when stopping
- BUG/MINOR: haproxy/threads: try to make all threads leave together
- Revert "BUILD: travis-ci: enable s390x builds"
- BUILD: travis-ci: enable regular s390x builds
- DOC: proxy_protocol: Reserve TLV type 0x05 as PP2_TYPE_UNIQUE_ID
- MINOR: proxy_protocol: Ingest PP2_TYPE_UNIQUE_ID on incoming connections
- MEDIUM: proxy_protocol: Support sending unique IDs using PPv2
- CLEANUP: connection: Add blank line after declarations in PP handling
- CLEANUP: assorted typo fixes in the code and comments
- CI: add spellcheck github action
- DOC: correct typo in alert message about rspirep
- CI: travis: switch linux builds to clang-9
- MINOR: debug: add a new DISGUISE() macro to pass a value as identity
- MINOR: debug: consume the write() result in BUG_ON() to silence a warning
- MINOR: use DISGUISE() everywhere we deliberately want to ignore a result
- BUILD: pools: silence build warnings with DEBUG_MEMORY_POOLS and DEBUG_UAF
- CLEANUP: connection: Stop directly setting an ist's .ptr
- CI: travis: revert to clang-7 for BoringSSL tests
- BUILD: on ARM, must be linked to libatomic.
- BUILD: makefile: fix regex syntax in ARM platform detection
- BUG/MEDIUM: peers: resync ended with RESYNC_PARTIAL in wrong cases.
- REORG: ssl: move ssl_sock_load_cert()
- MINOR: ssl: pass ckch_inst to ssl_sock_load_ckchs()
- MEDIUM: ssl: allow crt-list caching
- MINOR: ssl: directories are loaded like crt-list
- BUG/MINOR: ssl: can't open directories anymore
- BUG/MEDIUM: spoe: dup agent's engine_id string from trash.area
- MINOR: fd: Use a separate lock for logs instead of abusing the fd lock.
- MINOR: mux_pt: Don't try to remove the connection from the idle list.
- MINOR: ssl/cli: show/dump ssl crt-list
- BUG/MINOR: ssl/cli: free the trash chunk in dump_crtlist
- MEDIUM: fd: Introduce a running mask, and use it instead of the spinlock.
- BUG/MINOR: ssl: memory leak in crtlist_parse_file()
- MINOR: tasks: Provide the tasklet to the callback.
- BUG/MINOR: ssl: memleak of struct crtlist_entry
- BUG/MINOR: pattern: Do not pass len = 0 to calloc()
- BUILD: makefile: fix expression again to detect ARM platform
- CI: travis: re-enable ASAN on clang
- CI: travis: proper group output redirection together with travis_wait
- DOC: assorted typo fixes in the documentation
- MINOR: wdt: Move the definitions of WDTSIG and DEBUGSIG into types/signal.h.
- BUG/MEDIUM: wdt: Don't ignore WDTSIG and DEBUGSIG in __signal_process_queue().
- MINOR: memory: Change the flush_lock to a spinlock, and don't get it in alloc.
- MINOR: ssl/cli: 'new ssl cert' command
- MINOR: ssl/cli: show certificate status in 'show ssl cert'
- MEDIUM: sessions: Don't be responsible for connections anymore.
- MEDIUM: servers: Split the connections into idle, safe, and available.
- MINOR: fd: Implement fd_takeover().
- MINOR: connections: Add a new mux method, "takeover".
- MINOR: connections: Make the "list" element a struct mt_list instead of list.
- MINOR: connections: Add a flag to know if we're in the safe or idle list.
- MEDIUM: connections: Attempt to get idle connections from other threads.
- MEDIUM: mux_h1: Implement the takeover() method.
- MEDIUM: mux_h2: Implement the takeover() method.
- MEDIUM: mux_fcgi: Implement the takeover() method.
- MEDIUM: connections: Kill connections even if we are reusing one.
- BUG/MEDIUM: connections: Don't forget to decrement idle connection counters.
- BUG/MINOR: ssl: Do not free garbage pointers on memory allocation failure
- BUG/MINOR: ssl: Correctly add the 1 for the sentinel to the number of elements
- BUG/MINOR: ssl: crtlist_dup_filters() must return NULL with fcount == 0
- BUG/MEDIUM: build: Fix compilation by spelling decl correctly.
- BUILD/MEDIUM: fd: Declare fd_mig_lock as extern.
- CI: run travis-ci builds on push only, skip pull requests
- CI: temporarily disable unstable travis arm64 builds
- BUG/MINOR: ssl/cli: free BIO upon error in 'show ssl cert'
- BUG/MINOR: connections: Make sure we free the connection on failure.
- BUG/MINOR: ssl/cli: fix a potential NULL dereference
- BUG/MEDIUM: h1: Make sure we subscribe before going into idle list.
- BUG/MINOR: connections: Set idle_time before adding to idle list.
- MINOR: muxes: Note that we can't usee a connection when added to the srv idle.
- REGTEST: increase timeouts on the seamless-reload test
- BUG/MINOR: haproxy/threads: close a possible race in soft-stop detection
- CLEANUP: haproxy/threads: don't check global_tasks_mask twice
In run_thread_poll_loop() we test both for (global_tasks_mask & tid_bit)
and thread_has_tasks(), but the former is useless since this test is
already part of the latter.
Willy Tarreau [Mon, 23 Mar 2020 08:27:28 +0000 (09:27 +0100)]
BUG/MINOR: haproxy/threads: close a possible race in soft-stop detection
Commit 4b3f27b ("BUG/MINOR: haproxy/threads: try to make all threads
leave together") improved the soft-stop synchronization but it left a
small race open because it looks at tasks_run_queue, which can drop
to zero then back to one while another thread picks the task from the
run queue to insert it into the tasklet_list. The risk is very low but
not null. In addition the condition didn't consider the possible presence
of signals in the queue.
This patch moves the stopping detection just after the "wake" calculation
which already takes care of the various queues' sizes and signals. It
avoids needlessly duplicating these tests.
The bug was discovered during a code review but will probably never be
observed. This fix may be backported to 2.1 and 2.0 along with the commit
above.
Willy Tarreau [Mon, 23 Mar 2020 08:11:51 +0000 (09:11 +0100)]
REGTEST: increase timeouts on the seamless-reload test
The abns_socket in seamless-reload regtest regularly fails in Travis-CI
on smaller machines only (typically the ppc64le and sometimes s390x).
The error always reports an incomplete HTTP header as seen from the
client. And this can occasionally be reproduced on the minicloud ppc64le
image when setting a huge file descriptors limit (1 million).
What happens in fact is the following: depending on the binding order,
some connections from the client might reach the TCP listener on the
old instance and be forwarded to the ABNS listener of the second
instance just being prepared to start up. But due to the huge number
of FDs, setting them up takes slightly more time and the 20ms server
timeout may expire before the new instance finishes its startup. This
can result in an occasional 504, except that since the client timeout
is the same as the server timeout, both sides are closed at the same
time and the client doesn't receive the 504.
In addition a second problem plugs onto this: by default http-reuse is
enabled. Some requests being forwarded to the older instance will be
sent over an already established connection. But the CPU used by the
starting process using many FDs will be taken away from the older
process, whose abns listener will not see a request for more than 20ms,
and will decide to kill the idle client connection. At the same moment
the TCP proxy forwards a request over this closing connection, it
detects the close and silently closes the other side to let the
client retry, which is detected by the vtest client as another case
of empty header. This is easier to reproduce in VMs with few CPUs
(2 or less) and some noisy neighbors such as a few spinning loops in
background.
Let's just increase this tests' timeout to avoid this. While a few
ms are close to the scheduler's granularity, this test is never
supposed to trigger the timeouts so it's safe to go higher without
impacts on the test execution time. At one second the problem seems
impossible to reproduce on the minicloud VMs.
Olivier Houchard [Sun, 22 Mar 2020 22:25:51 +0000 (23:25 +0100)]
MINOR: muxes: Note that we can't usee a connection when added to the srv idle.
In the various muxes, add a comment documenting that once
srv_add_to_idle_list() got called, any thread may pick that conenction up,
so it is unsafe to access the mux context/the connection, the only thing we
can do is returning.
Olivier Houchard [Sun, 22 Mar 2020 18:59:52 +0000 (19:59 +0100)]
BUG/MINOR: connections: Set idle_time before adding to idle list.
In srv_add_to_idle_list(), make sure we set the idle_time before we add
the connection to an idle list, not after, otherwise another thread may
grab it, set the idle_time to 0, only to have the original thread set it
back to now_ms.
This may have an impact, as in conn_free() we check idle_time to decide
if we should decrement the idle connection counters for the server.
Olivier Houchard [Sun, 22 Mar 2020 18:56:03 +0000 (19:56 +0100)]
BUG/MEDIUM: h1: Make sure we subscribe before going into idle list.
In h1_detach(), make sure we subscribe before we call
srv_add_to_idle_list(), not after. As soon as srv_add_to_idle_list() is
called, and it is put in an idle list, another thread can take it, and
we're no longer allowed to subscribe.
This fixes a race condition when another thread grabs a connection as soon
as it is put, the original owner would subscribe, and thus the new thread
would fail to do so, and to activate polling.
Olivier Houchard [Fri, 20 Mar 2020 13:26:32 +0000 (14:26 +0100)]
BUG/MINOR: connections: Make sure we free the connection on failure.
In connect_server(), make sure we properly free a newly created connection
if we somehow fail, and it has not yet been attached to a conn_stream, or
it would lead to a memory leak.
This should appease coverity for backend.c, as reported in inssue #556.
[wt: arm64 shows timeouts during packages downloads and causes all
builds to be reported as failures; building for arm64 on real hardware
is still done on a regular basis and works fine however]
Tim Duesterhus [Thu, 19 Mar 2020 15:12:09 +0000 (16:12 +0100)]
BUG/MINOR: ssl: Do not free garbage pointers on memory allocation failure
In `ckch_inst_sni_ctx_to_sni_filters` use `calloc()` to allocate the filter
array. When the function fails to allocate memory for a single entry the
whole array will be `free()`d using free_sni_filters(). With the previous
`malloc()` the pointers for entries after the failing allocation could
possibly be a garbage value.
Olivier Houchard [Thu, 19 Mar 2020 22:52:28 +0000 (23:52 +0100)]
BUG/MEDIUM: connections: Don't forget to decrement idle connection counters.
In conn_backend_get(), when we manage to get an idle connection from the
current thread's pool, don't forget to decrement the idle connection
counters, or we may end up not reusing connections when we could, and/or
killing connections when we shouldn't.
Olivier Houchard [Mon, 16 Mar 2020 12:49:00 +0000 (13:49 +0100)]
MEDIUM: connections: Kill connections even if we are reusing one.
In connect_server(), if we notice we have more file descriptors opened than
we should, there's no reason not to close a connection just because we're
reusing one, so do it anyway.
projects / thirdparty / haproxy.git / log
