Willy Tarreau [Thu, 21 Feb 2019 21:12:47 +0000 (22:12 +0100)]
MINOR: fd: implement an optimised my_closefrom() function
The idea is that poll() can set the POLLNVAL flag for each invalid
FD in a pollfd list. Thus this function makes use of poll() when
compiled in, and builds lists of up to 1024 FDs at once, checks the
output and only closes those which do not have this flag set. Tests
show that this is about twice as fast as blindly calling close() for
each closed fd.
Willy Tarreau [Thu, 21 Feb 2019 21:19:17 +0000 (22:19 +0100)]
MINOR: fd: add a new my_closefrom() function to close all FDs
This is a naive implementation of closefrom() which closes all FDs
starting from the one passed in argument. closefrom() is not provided
on all operating systems, and other versions will follow.
Olivier Houchard [Mon, 18 Feb 2019 15:41:17 +0000 (16:41 +0100)]
BUG/MEDIUM: servers: Add a per-thread counter of idle connections.
Add a per-thread counter of idling connections, and use it to determine
how many connections we should kill after the timeout, instead of using
the global counter, or we're likely to just kill most of the connections.
Willy Tarreau [Thu, 21 Feb 2019 17:16:35 +0000 (18:16 +0100)]
BUG/MEDIUM: mux-h2/htx: send an empty DATA frame on empty HTX trailers
When chunked-encoding is used in HTX mode, a trailers HTX block is always
made due to the way trailers are currently implemented (verbatim copy of
the H1 representation). Because of this it's not possible to know when
processing data that we've reached the end of the stream, and it's up
to the function encoding the trailers (h2s_htx_make_trailers) to put the
end of stream. But when there are no trailers and only an empty HTX block,
this one cannot produce a HEADERS frame, thus it cannot send the END_STREAM
flag either, leaving the other end with an incomplete message, waiting for
either more data or some trailers. This is particularly visible with POST
requests where the server continues to wait.
What this patch does is transform the HEADERS frame into an empty DATA
frame when meeting an empty trailers block. It is possible to do this
because we've not sent any trailers so the other end is still waiting
for DATA frames. The check is made after attempting to encode the list
of headers, so as to minimize the specific code paths.
Thanks to Dragan Dosen for reporting the issue with a reproducer.
Willy Tarreau [Thu, 21 Feb 2019 12:24:36 +0000 (13:24 +0100)]
MINOR: mux-h2: make the H2 MAX_FRAME_SIZE setting configurable
This creates a new tunable "tune.h2.max-frame-size" to adjust the
advertised max frame size. When not set it still defaults to the buffer
size. It is convenient to advertise sizes lower than the buffer size,
for example when using very large buffers.
Willy Tarreau [Thu, 21 Feb 2019 14:06:30 +0000 (15:06 +0100)]
BUG/MEDIUM: htx: count the amount of copied data towards the final count
Currently htx_xfer_blks() respects the <count> limit for each block
instead of for the sum of the transfered blocks. This causes it to
return slightly more than requested when both headers and data are
present in the source buffer, which happens early in the transfer
when the reserve is still active. Thus with large enough headers,
the reserve will not be respected.
Note that this function is only called from h2_rcv_buf() thus this only
affects data entering over H2 (H2 requests or H2 responses).
For now, this can be only done when a content-length is specified. In fact, it
is the same value than h2s->body_len, the remaining body length according to
content-length. Setting this field allows the fast forwarding at the channel
layer, improving significantly data transfer for big objects.
BUG/MEDIUM: h2/htx: Correctly handle interim responses when HTX is enabled
1xx responses does not work in HTTP2 when the HTX is enabled. First of all, when
a response is parsed, only one HEADERS frame is expected. So when an interim
response is received, the flag H2_SF_HEADERS_RCVD is set and the next HEADERS
frame (for another interim repsonse or the final one) is parsed as a trailers
one. Then when the response is sent, because an EOM block is found at the end of
the interim HTX response, the ES flag is added on the frame, closing too early
the stream. Here, it is a design problem of the HTX. Iterim responses are
considered as full messages, leading to some ambiguities when HTX messages are
processed. This will not be fixed now, but we need to keep it in mind for future
improvements.
To fix the parsing bug, the flag H2_MSGF_RSP_1XX is added when the response
headers are decoded. When this flag is set, an EOM block is added into the HTX
message, despite the fact that there is no ES flag on the frame. And we don't
set the flag H2_SF_HEADERS_RCVD on the corresponding H2S. So the next HEADERS
frame will not be parsed as a trailers one.
To fix the sending bug, the ES flag is not set on the frame when an interim
response is processed and the flag H2_SF_HEADERS_SENT is not set on the
corresponding H2S.
BUG/MINOR: proto-htx: Consider a XFER_LEN message as chunked by default
An HTX message with a known body length is now considered by default as
chunked. It means the header "content-length" must be found to consider it as a
non-chunked message. Before, it was the reverse, the message was considered with
a content length by default. But it is a bug for HTTP/2 messages. There is no
chunked transfer encoding in HTTP/2 but internally messages without content
length are considered as chunked. It eases HTTP/1 <-> HTTP/2 conversions.
BUG/MINOR: mux-h1: Add "transfer-encoding" header on outgoing requests if needed
As for outgoing response, if an HTTP/1.1 or above request is sent to a server
with neither the headers "content-length" nor "transfer-encoding", it is
considered as a chunked request and the header "transfer-encoding: chunked" is
automatically added. Of course, it is only true for requests with a
body. Concretely, it only happens for incoming HTTP/2 requests sent to an
HTTP/1.1 server.
MINOR: h2/htx: Set the flag HTX_SL_F_BODYLESS for messages without body
This information is usefull to know if a body is expected or not, regardless the
presence or not of the header "Content-Length" and its value. Once the ES flag
is set on the header frame or when the content length is 0, we can safely add
the flag HTX_SL_F_BODYLESS on the HTX start-line.
Among other things, it will help the mux-h1 to know if it should add TE header
or not. It will also help the HTTP compression filter.
This patch must be backported to 1.9 because a bug fix depends on it.
BUG/MEDIUM: mux-h2/htx: Always set CS flags before exiting h2_rcv_buf()
It is especially important when some data are blocked in the RX buf and the
channel buffer is already full. In such case, instead of exiting the function
directly, we need to set right flags on the conn_stream. CS_FL_RCV_MORE and
CS_FL_WANT_ROOM must be set, otherwise, the stream-interface will subscribe to
receive events, thinking it is not blocked.
This bug leads to connection freeze when everything was received with some data
blocked in the RX buf and a channel full.
Dragan Dosen [Thu, 14 Feb 2019 11:30:53 +0000 (12:30 +0100)]
BUG/MEDIUM: http_fetch: fix "req.body_len" and "req.body_size" fetch methods in HTX mode
When in HTX mode, in functions smp_fetch_body_len() and
smp_fetch_body_size() we were subtracting the size of each header block
from the total size htx->data to calculate the size of body, and that
could result in wrong calculated value.
To avoid this, we now loop on blocks to sum up the size of only those
that are of type HTX_BLK_DATA.
BUG/MEDIUM: proto_htx: Fix data size update if end of the cookie is removed
When client-side or server-side cookies are parsed, if the end of the cookie
line is removed, the HTX message must be updated. The length of the HTX block is
decreased and the data size of the HTX message is modified accordingly. The
update of the HTX block was ok but the update of the HTX message was wrong,
leading to undefined behaviours during the data forwarding. One of possible
effect was a freeze of the connection and no data forward.
Initialize ->srv peer field for all the peers, the local peer included.
Indeed, a haproxy process needs to connect to the local peer of a remote
process. Furthermore, when a "peer" or "server" line is parsed by parse_server()
the address must be copied to ->addr field of the peer object only if this address
has been also parsed by parse_server(). This is not the case if this address belongs
to the local peer and is provided on a "server" line.
After having parsed the "peer" or "server" lines of a peer
sections, the ->srv part of all the peer must be initialized for SSL, if
enabled. Same thing for the binding part.
Willy Tarreau [Tue, 12 Feb 2019 11:02:27 +0000 (12:02 +0100)]
BUILD/MINOR: htx: fix some potential null-deref warnings with http_find_stline
http_find_stline() carefully verifies that it finds a start line otherwise
returns NULL when not found. But a few calling functions ignore this NULL
in return and dereference this pointer without checking. Let's add the
test where needed in the callers. If it turns out that over the long term
a start line is mandatory, then the test will be removed and the faulty
function will have to be simplified.
Willy Tarreau [Tue, 12 Feb 2019 10:59:35 +0000 (11:59 +0100)]
BUILD/MINOR: peers: remove an impossible null test in intencode()
intencode() tests for the nullity of the target pointer passed in
argument, but the code calling intencode() never does so and happily
dereferences it. gcc at -O3 detects this as a potential null deref.
Let's remove this incorrect and misleading test. If this pointer was
null, the code would already crash in the calling functions.
Willy Tarreau [Tue, 12 Feb 2019 10:26:29 +0000 (11:26 +0100)]
BUILD/MINOR: tools: fix build warning in the date conversion functions
Some gcc versions emit potential null deref warnings at -O3 in
date2str_log(), gmt2str_log() and localdate2str_log() after utoa_pad()
because this function may return NULL if its size argument is too small
for the integer value. And it's true that we can't guarantee that the
input number is always valid.
Willy Tarreau [Sun, 10 Feb 2019 17:49:37 +0000 (18:49 +0100)]
BUG/MAJOR: stream: avoid double free on unique_id
Commit 32211a1 ("BUG/MEDIUM: stream: Don't forget to free
s->unique_id in stream_free().") addressed a memory leak but in
exchange may cause double-free due to the fact that after freeing
s->unique_id it doesn't null it and then calls http_end_txn()
which frees it again. Thus the process quickly crashes at runtime.
This fix must be backported to all stable branches where the
aforementioned patch was backported.
Ben51Degrees [Tue, 5 Feb 2019 13:24:00 +0000 (13:24 +0000)]
MEDIUM: 51d: Enabled multi threaded operation in the 51Degrees module.
The existing threading flag in the 51Degrees API
(FIFTYONEDEGREES_NO_THREADING) has now been mapped to the HAProxy
threading flag (USE_THREAD), and the 51Degrees module code has been made
thread safe.
In Pattern, the cache is now locked with a spin lock from hathreads.h
using a new lable 'OTHER_LOCK'. The workset pool is now created with the
same size as the number of threads to avoid any time waiting on a
worket.
In Hash Trie, the global device offsets structure is only used in single
threaded operation. Multi threaded operation creates a new offsets
structure in each thread.
Ben51Degrees [Tue, 5 Feb 2019 13:23:06 +0000 (13:23 +0000)]
BUG: 51d: In Hash Trie, multi header matching was affected by the header names stored globaly.
Some logic around mutli header matching in Hash Trie has been improved
where only the name of the most important header was stored in the
global heade_names structure. Now all headers are stored, so are used in
the mutli header matching correctly.
Willy Tarreau [Fri, 8 Feb 2019 14:35:38 +0000 (15:35 +0100)]
BUG/MINOR: mux-h1: verify the request's version before dropping connection: keep-alive
The mux h1 properly avoid to set "connection: keep-alive" when the response
is in HTTP/1.1 but it forgot to check the request's version. Thus when the
client requests using HTTP/1.0 and connection: keep-alive (like ab does),
the response is in 1.1 with no keep-alive and ab waits for the close without
checking for the content-length. Response headers actually depend on the
recipient, thus on both request and response's version.
CONTRIB: contrib/prometheus-exporter: Add a Prometheus exporter for HAProxy
It has been developped as a service applet. Internally, it is called
"promex". To build HAProxy with the promex service, you should use the Makefile
variable "EXTRA_OBJS". To be used, it must be enabled in the configuration with
an "http-request" rule and the corresponding HTTP proxy must enable the HTX
support. For instance:
frontend test
mode http
...
option http-use-htx
http-request use-service prometheus-exporter if { path /metrics }
...
See contrib/prometheus-exporter/README for details.
Willy Tarreau [Fri, 8 Feb 2019 09:22:31 +0000 (10:22 +0100)]
BUG/MEDIUM: peers: check that p->srv actually exists before using p->srv->use_ssl
Commit 1055e687a ("MINOR: peers: Make outgoing connection to SSL/TLS
peers work.") introduced an "srv" field in the peers, which points to
the equivalent server to hold SSL settings. This one is not set when
the peer is local so we must always test it before testing p->srv->use_ssl
otherwise haproxy dies during reloads.
BUG/MINOR: config: Reinforce validity check when a process number is parsed
Now, in the function parse_process_number(), when a process number or a set of
processes is parsed, an error is triggered if an invalid character is found. It
means following syntaxes are not forbidden and will emit an alert during the
HAProxy startup:
BUG/MAJOR: spoe: Don't try to get agent config during SPOP healthcheck
During SPOP healthchecks, a dummy appctx is used to create the HAPROXY-HELLO
frame and then to parse the AGENT-HELLO frame. No agent are attached to it. So
it is important to not rely on an agent during these stages. When HAPROXY-HELLO
frame is created, there is no problem, all accesses to an agent are
guarded. This is not true during the parsing of the AGENT-HELLO frame. Thus, it
is possible to crash HAProxy with a SPOA declaring the async or the pipelining
capability during a healthcheck.
Willy Tarreau [Thu, 7 Feb 2019 09:39:36 +0000 (10:39 +0100)]
MINOR: config: make MAX_PROCS configurable at build time
For some embedded systems, it's pointless to have 32- or even 64- large
arrays of processes when it's known that much fewer processes will be
used in the worst case. Let's introduce this MAX_PROCS define which
contains the highest number of processes allowed to run at once. It
still defaults to LONGBITS but may be lowered.
Willy Tarreau [Thu, 7 Feb 2019 13:59:29 +0000 (14:59 +0100)]
BUG/MEDIUM: server: initialize the orphaned conns lists and tasks at the end
This also depends on the nbthread count, so it must only be performed after
parsing the whole config file. As a side effect, this removes some code
duplication between servers and server-templates.
Willy Tarreau [Thu, 7 Feb 2019 13:46:29 +0000 (14:46 +0100)]
BUG/MEDIUM: server: initialize the idle conns list after parsing the config
The idle conns lists are sized according to the number of threads. As such
they cannot be initialized during the parsing since nbthread can be set
later, as revealed by this simple config which randomly crashes when used.
Let's do this at the end instead.
listen proxy
bind :4445
mode http
timeout client 10s
timeout server 10s
timeout connect 10s
http-reuse always
server s1 127.0.0.1:8000
Willy Tarreau [Thu, 7 Feb 2019 12:40:33 +0000 (13:40 +0100)]
BUG/MEDIUM: spoe: initialization depending on nbthread must be done last
The agent used to be configured depending on global.nbthread while nbthread
may be set after the agent is parsed. Let's move this part to the spoe_check()
function to make sure nbthread is always correct and arrays are appropriately
sized.
Willy Tarreau [Thu, 7 Feb 2019 13:48:24 +0000 (14:48 +0100)]
BUG/MINOR: lua: initialize the correct idle conn lists for the SSL sockets
Commit 40a007cf2 ("MEDIUM: threads/server: Make connection list
(priv/idle/safe) thread-safe") made a copy-paste error when initializing
the Lua sockets, as the TCP one was initialized twice. Fortunately it has
no impact because the pointers are set to NULL after a memset(0) and are
not changed in between.
Willy Tarreau [Thu, 7 Feb 2019 13:22:52 +0000 (14:22 +0100)]
BUG/MINOR: spoe: do not assume agent->rt is valid on exit
As reported by Christopher, we may call spoe_release_agent() when leaving
after an allocation failure or a config parse error. We must not assume
agent->rt is valid there as the allocation could have failed.
Since TLS ciphers are not well understand, it is very common pratice to
copy and paste parameters from documentation and use them as-is. Since RC4
should not be used anymore, it is wiser to link users to up to date
documnetation from Mozilla to avoid unsafe configuration in the wild.
Clarify the location of man pages for OpenSSL when missing.
Willy Tarreau [Wed, 6 Feb 2019 09:25:07 +0000 (10:25 +0100)]
BUG/MINOR: config: make sure to count the error on incorrect track-sc/stick rules
When commit 151e1ca98 ("BUG/MAJOR: config: verify that targets of track-sc
and stick rules are present") added a check for some process inconsistencies
between rules and their stick tables, some errors resulted in a "return 0"
statement, which is taken as "no error" in some cases. Let's fix this.
This must be backported to all versions using the above commit.
BUG/MAJOR: htx/backend: Make all tests on HTTP messages compatible with HTX
A piece of code about the HTX was lost this summer, after the "big merge"
(htx/http2/connection layer refactoring). I forgot to keep HTX changes in the
functions connect_server() and assign_server(). So, this patch fixes "uri",
"url_param" and "hdr" LB algorithms when the HTX is enabled. It also fixes
evaluation of the "sni" expression on server lines.
Willy Tarreau [Tue, 5 Feb 2019 12:37:19 +0000 (13:37 +0100)]
BUG/MAJOR: spoe: verify that backends used by SPOE cover all their callers' processes
When a filter is installed on a proxy and references spoe, we must be
absolutely certain that the whole chain is valid on a given process
when running in multi-process mode. The problem here is that if a proxy
1 runs on process 1, referencing an SPOE agent itself based on a backend
running on process 2, this last one will be completely deinited on
process 1, and will thus cause random crashes when it gets messages
from this proess.
This patch makes sure that the whole chain is valid on all of the caller's
processes.
This fix must be backported to all spoe-enabled maintained versions. It
may potentially disrupt configurations which already randomly crash.
There hardly is any intermediary solution though, such configurations
need to be fixed.
Willy Tarreau [Tue, 5 Feb 2019 10:38:38 +0000 (11:38 +0100)]
BUG/MAJOR: config: verify that targets of track-sc and stick rules are present
Stick and track-sc rules may optionally designate a table in a different
proxy. In this case, a number of verifications are made such as validating
that this proxy actually exists. However, in multi-process mode, the target
table might indeed exist but not be bound to the set of processes the rules
will execute on. This will definitely result in a random behaviour especially
if these tables do require peer synchronization, because some tasks will be
started to try to synchronize form uninitialized areas.
The typical issue looks like this :
peers my-peers
peer foo ...
listen proxy
bind-process 1
stick on src table ip
...
backend ip
bind-process 2
stick-table type ip size 1k peers my-peers
While it appears obvious that the example above will not work, there are
less obvious situations, such as having bind-process in a defaults section
and having a larger set of processes for the referencing proxy than the
referenced one.
The present patch adds checks for such situations by verifying that all
processes from the referencing proxy are present on the other one in all
track-sc* and stick-* rules, and in sample fetch / converters referencing
another table so that sc_inc_gpc0() and similar are safe as well.
This fix must be backported to all maintained versions. It may potentially
disrupt configurations which already randomly crash. There hardly is any
intermediary solution though, such configurations need to be fixed.
Willy Tarreau [Mon, 4 Feb 2019 09:26:53 +0000 (10:26 +0100)]
BUG/MINOR: task: close a tiny race in the inter-thread wakeup
__task_wakeup() takes care of a small race that exists between threads,
but it uses a store barrier that is not sufficient since apparently the
state read after clearing the leaf_p pointer sometimes is incorrect. This
results in missed wakeups between threads competing at a high rate. Let's
use a full barrier instead to serialize the operations.
This may be backported to 1.9 though it's extremely unlikely that this
bug will ever manifest itself there.
Willy Tarreau [Mon, 4 Feb 2019 10:48:03 +0000 (11:48 +0100)]
BUG/MINOR: compression: properly report compression stats in HTX mode
When HTX support was added to HTTP compression, a set of counters was missed,
namely comp_in and comp_byp, resulting in no stats being available for compression.
Willy Tarreau [Sat, 2 Feb 2019 16:39:53 +0000 (17:39 +0100)]
MINOR: config: simplify bind_proc processing using proc_mask()
At a number of places we used to have null tests on bind_proc for
listeners and proxies. Let's simplify all these tests by always
having the proper bits reported via proc_mask().
Willy Tarreau [Sat, 2 Feb 2019 16:22:19 +0000 (17:22 +0100)]
MINOR: global: add proc_mask() and thread_mask()
These two functions return either all_{proc,threads}_mask, or the argument.
This is used to default to all_proc_mask or all_threads_mask when not set
on bind_conf or proxies.
Willy Tarreau [Sat, 2 Feb 2019 19:17:31 +0000 (20:17 +0100)]
MINOR: tools: improve the popcount() operation
We'll call popcount() more often so better use a parallel method
than an iterative one. One optimal design is proposed at the site
below. It requires a fast multiplication though, but even without
it will still be faster than the iterative one, and all relevant
64 bit platforms do have a multiply unit.
Willy Tarreau [Sun, 3 Feb 2019 09:28:24 +0000 (10:28 +0100)]
OPTIM: listener: optimize cache-line packing for struct listener
Some unused fields were placed early and some important ones were on
the second cache line. Let's move the proto_list and name closer to
the end of the structure to bring accept() and default_target() into
the first cache line.
Willy Tarreau [Sat, 2 Feb 2019 16:46:24 +0000 (17:46 +0100)]
BUG/MINOR: config: fix bind line thread mask validation
When no nbproc is specified, a computation leads to reading bind_thread[-1]
before checking if the thread mask is valid for a bind conf. It may either
report a false warning and compute a wrong mask, or miss some incorrect
configs.
Willy Tarreau [Sat, 2 Feb 2019 12:18:01 +0000 (13:18 +0100)]
BUG/MINOR: threads: fix the process range of thread masks
Commit 421f02e ("MINOR: threads: add a MAX_THREADS define instead of
LONGBITS") used a MAX_THREADS macros to fix threads limits. However,
one change was wrong as it affected the upper bound of the process
loop when setting threads masks. No backport is needed.
BUG/MEDIUM: stream: Don't forget to free s->unique_id in stream_free().
In stream_free(), free s->unique_id. We may still have one, because it's
allocated in log.c::strm_log() no matter what, even if it's a TCP connection
and thus it won't get free'd by http_end_txn().
Failure to do so leads to a memory leak.
This should probably be backported to all maintained branches.
Willy Tarreau [Fri, 1 Feb 2019 15:13:59 +0000 (16:13 +0100)]
BUG/MEDIUM: mux-h2: always set :authority on request output
PiBa-NL reported that some servers don't fall back to the Host header when
:authority is absent. After studying all the combinations of Host and
:authority, it appears that we always have to send the latter, hence we
never need the former. In case of CONNECT method, the authority is retrieved
from the URI part, otherwise it's extracted from the Host field.
The tricky part is that we have to scan all headers for the Host header
before dumping other headers. This is due to the fact that we must emit
pseudo headers before other ones. One improvement could possibly be made
later in the request parser to search and emit the Host header immediately
if authority was not found. This would cost nothing on the vast marjority
of requests and make the lookup faster on output since Host would appear
first.
Willy Tarreau [Fri, 1 Feb 2019 15:38:48 +0000 (16:38 +0100)]
BUG/MINOR: backend: check srv_conn before dereferencing it
Commit 3c4e19f42 ("BUG/MEDIUM: backend: always release the previous
connection into its own target srv_list") introduced a valid warning
about a null-deref risk since we didn't check conn_new()'s return value.
This patch must be backported to 1.9 with the patch above.
BUG/MINOR: tune.fail-alloc: Don't forget to initialize ret.
In mem_should_fail(), if we don't want to fail the allocation, either
because mem_fail_rate is 0, or because we're still initializing, don't
forget to initialize ret, or we may return a non-zero value, and fail
an allocation we didn't want to fail.
This should only affect users that compile with DEBUG_FAIL_ALLOC.
Willy Tarreau [Fri, 1 Feb 2019 14:06:09 +0000 (15:06 +0100)]
BUG/MEDIUM: htx: check the HTX compatibility in dynamic use-backend rules
I would have sworn it was done, probably we lost it during the refactoring.
If a frontend is in HTX and the backend not (and conersely), this is
normally detected at config parsing time unless the rule is dynamic. In
this case we must abort with an error 500. The logs will report "RR"
(resource issue while processing request) with the frontend and the
backend assigned, so that it's possible to figure what was attempted.
Willy Tarreau [Fri, 1 Feb 2019 10:54:23 +0000 (11:54 +0100)]
BUG/MEDIUM: backend: always release the previous connection into its own target srv_list
There was a bug reported in issue #19 regarding the fact that haproxy
could mis-route requests to the wrong server. It turns out that when
switching to another server, the old connection was put back into the
srv_list corresponding to the stream's target instead of this connection's
target. Thus if this connection was later picked, it was pointing to the
wrong server.
The patch fixes this and also clarifies the assignment to srv_conn->target
so that it's clear we don't change it when picking it from the srv_list.
Olivier Houchard [Tue, 29 Jan 2019 14:20:16 +0000 (15:20 +0100)]
MINOR: debug: Add an option that causes random allocation failures.
When compiling with DEBUG_FAIL_ALLOC, add a new option, tune.fail-alloc,
that gives the percentage of chances an allocation fails.
This is useful to check that allocation failures are always handled
gracefully.
Willy Tarreau [Thu, 31 Jan 2019 09:42:05 +0000 (10:42 +0100)]
BUG/MEDIUM: mux-h2: properly consider the peer's advertised max-concurrent-streams
Till now we used to only rely on tune.h2.max-concurrent-streams but if
a peer advertises a lower limit this can cause streams to be reset or
even the conection to be killed. Let's respect the peer's value for
outgoing streams.
This patch should be backported to 1.9, though it depends on the following
ones :
BUG/MINOR: server: fix logic flaw in idle connection list management
MINOR: mux-h2: max-concurrent-streams should be unsigned
MINOR: mux-h2: make sure to only check concurrency limit on the frontend
MINOR: mux-h2: learn and store the peer's advertised MAX_CONCURRENT_STREAMS setting
Willy Tarreau [Thu, 31 Jan 2019 09:34:07 +0000 (10:34 +0100)]
MINOR: mux-h2: learn and store the peer's advertised MAX_CONCURRENT_STREAMS setting
We used not to take it into account because we only used the configured
parameter everywhere. This patch makes sure we can actually learn the
value advertised by the peer. We still enforce our own limit on top of
it however, to make sure we can actually limit resources or stream
concurrency in case of suboptimal server settings.
Willy Tarreau [Thu, 31 Jan 2019 09:31:51 +0000 (10:31 +0100)]
MINOR: mux-h2: make sure to only check concurrency limit on the frontend
h2_has_too_many_cs() was renamed to h2_frt_has_too_many_cs() to make it
clear it's only used to throttle the frontend connection, and the call
places were adjusted to only call this code on a front connection. In
practice it was already the case since the H2_CF_DEM_TOOMANY flag is
only set there. But now the ambiguity is removed.
Willy Tarreau [Sat, 26 Jan 2019 11:19:01 +0000 (12:19 +0100)]
BUG/MINOR: server: fix logic flaw in idle connection list management
With variable connection limits, it's not possible to accurately determine
whether the mux is still in use by comparing usage and max to be equal due
to the fact that one determines the capacity and the other one takes care
of the context. This can cause some connections to be dropped before they
reach their stream ID limit.
It seems it could also cause some connections to be terminated with
streams still alive if the limit was reduced to match the newly computed
avail_streams() value, though this cannot yet happen with existing muxes.
Instead let's switch to usage reports and simply check whether connections
are both unused and available before adding them to the idle list.
Willy Tarreau [Thu, 31 Jan 2019 18:12:48 +0000 (19:12 +0100)]
BUG/MEDIUM: mux-h2: do not close the connection on aborted streams
We used to rely on a hint that a shutw() or shutr() without data is an
indication that the upper layer had performed a tcp-request content reject
and really wanted to kill the connection, but sadly there is another
situation where this happens, which is failed keep-alive request to a
server. In this case the upper layer stream silently closes to let the
client retry. In our case this had the side effect of killing all the
connection.
Instead of relying on such hints, let's address the problem differently
and rely on information passed by the upper layers about the intent to
kill the connection. During shutr/shutw, this is detected because the
flag CS_FL_KILL_CONN is set on the connstream. Then only in this case
we send a GOAWAY(ENHANCE_YOUR_CALM), otherwise we only send the reset.
This makes sure that failed backend requests only fail frontend requests
and not the whole connections anymore.
This fix relies on the two previous patches adding SI_FL_KILL_CONN and
CS_FL_KILL_CONN as well as the fix for the connection close, and it must
be backported to 1.9 and 1.8, though the code in 1.8 could slightly differ
(cs is always valid) :
BUG/MEDIUM: mux-h2: wait for the mux buffer to be empty before closing the connection
MINOR: stream-int: add a new flag to mention that we want the connection to be killed
MINOR: connstream: have a new flag CS_FL_KILL_CONN to kill a connection
Willy Tarreau [Thu, 31 Jan 2019 18:02:43 +0000 (19:02 +0100)]
MINOR: stream-int: add a new flag to mention that we want the connection to be killed
The new flag SI_FL_KILL_CONN is now set by the rare actions which
deliberately want the whole connection (and not just the stream) to be
killed. This is only used for "tcp-request content reject",
"tcp-response content reject", "tcp-response content close" and
"http-request reject". The purpose is to desambiguate the close from
a regular shutdown. This will be used by the next patches.
Willy Tarreau [Thu, 31 Jan 2019 17:48:20 +0000 (18:48 +0100)]
BUG/MEDIUM: mux-h2: wait for the mux buffer to be empty before closing the connection
When finishing to respond on a stream, a shutw() is called (resulting
in either an end of stream or RST), then h2_detach() is called, and may
decide to kill the connection is a number of conditions are satisfied.
Actually one of these conditions is that a GOAWAY frame was already sent
or attempted to be sent. This one is wrong, because it can happen in at
least these two situations :
- a shutw() sends a GOAWAY to obey tcp-request content reject
- a graceful shutdown is pending
In both cases, the connection will be aborted with the mux buffer holding
some data. In case of a strong abort the client will not see the GOAWAY or
RST and might want to try again, which is counter-productive. In case of
the graceful shutdown, it could result in truncated data. It looks like a
valid candidate for the issue reported here :
Willy Tarreau [Thu, 31 Jan 2019 17:58:06 +0000 (18:58 +0100)]
BUG/MINOR: stream: don't close the front connection when facing a backend error
In 1.5-dev13, a bug was introduced by commit e3224e870 ("BUG/MINOR:
session: ensure that we don't retry connection if some data were sent").
If a connection error is reported after some data were sent (and lost),
we used to accidently mark the front connection as being in error instead
of only the back one because the two direction flags were applied to the
same channel. This case is extremely rare with raw connections but can
happen a bit more often with multiplexed streams. This will result in
the error not being correctly reported to the client.
This patch can be backported to all supported versions.
Olivier Houchard [Thu, 31 Jan 2019 18:31:19 +0000 (19:31 +0100)]
BUG/MEDIUM: connections: Don't forget to remove CO_FL_SESS_IDLE.
If we're adding a connection to the server orphan idle list, don't forget
to remove the CO_FL_SESS_IDLE flag, or we will assume later it's still
attached to a session.
BUG/MEDIUM: mux-h1: Don't add "transfer-encoding" if message-body is forbidden
When a HTTP/1.1 or above response is emitted to a client, if the flag
H1_MF_XFER_LEN is set whereas H1_MF_CLEN and H1_MF_CHNK are not, the header
"transfer-encoding" is added. It is a way to make HTX chunking consistent with
H2. But we must exclude all cases where the message-body is explicitly forbidden
by the RFC:
* for all 1XX, 204 and 304 responses
* for any responses to HEAD requests
* for 200 responses to CONNECT requests
For these 3 cases, the flag H1_MF_XFER_LEN is set but H1_MF_CLEN and H1_MF_CHNK
not. And the header "transfer-encoding" must not be added.
See issue #27 on github for details about the bug.
This bug was introduced by 355b203 commit which prevented the peer
addresses to be parsed for the local peer of a "peers" section.
When adding "parse_addr" boolean parameter to parse_server(), this commit
missed the case where the syntax with "peer" keyword should still be
supported in addition to the new syntax with "server"+"bind" keyword.
Willy Tarreau [Thu, 31 Jan 2019 06:23:00 +0000 (07:23 +0100)]
MINOR: mux-h2: consistently rely on the htx variable to detect the mode
In h2_frt_transfer_data(), we support both HTX and legacy modes. The
HTX mode is detected from the proxy option and sets a valid pointer
into the htx variable. Better rely on this variable in all the function
rather than testing the option again. This way the code is clearer and
even the compiler knows this pointer is valid when it's dereferenced.
This should be backported to 1.9 if the b_is_null() patch is backported.
Willy Tarreau [Thu, 31 Jan 2019 06:31:18 +0000 (07:31 +0100)]
MINOR: htx: never check for null htx pointer in htx_is_{,not_}empty()
The previous patch clarifies the fact that the htx pointer is never null
along all the code. This test for a null will never match, didn't catch
the pointer 1 before the fix for b_is_null(), but it confuses the compiler
letting it think that any dereferences made to this pointer after this
test could actually mean we're dereferencing a null. Let's now drop this
test. This saves us from having to add impossible tests everywhere to
avoid the warning.
This should be backported to 1.9 if the b_is_null() patch is backported.
Willy Tarreau [Thu, 31 Jan 2019 06:21:42 +0000 (07:21 +0100)]
DOC: htx: make it clear that htxbuf() and htx_from_buf() always return valid pointers
Update the comments above htxbuf() and htx_from_buf() to make it clear
that they always return valid htx pointers so that callers know they do
not have to test them. This is only true after the fix on b_is_null()
which was the only known corner case.
This should be backported to 1.9 if the b_is_null() patch is backported.
Olivier Houchard [Tue, 29 Jan 2019 18:10:02 +0000 (19:10 +0100)]
BUG/MEDIUM: buffer: Make sure b_is_null handles buffers waiting for allocation.
In b_is_null(), make sure we return 1 if the buffer is waiting for its
allocation, as users assume there's memory allocated if b_is_null() returns
0.
The indirect impact of not having this was that htxbuf() would not match
b_is_null() for a buffer waiting for an allocation, and would thus return
the value 1 for the htx pointer, causing various crashes under low memory
condition.
Note that this patch makes gcc versions 6 and above report two null-deref
warnings in proto_htx.c since htx_is_empty() continues to check for a null
pointer without knowing that this is protected by the test on b_is_null().
This is addressed by the following patches.
Tim Duesterhus [Wed, 30 Jan 2019 22:46:04 +0000 (23:46 +0100)]
DOC: compression: Update the reasons for disabled compression
- Update the list of status codes to include 201 - 203.
- Remove the fact about the temporary workaround for chunked responses
(this is verified using reg-test compression/h00000.vtc).
- Add malformed ETags
This commit should be backported together with b229f018eedef4d18571ce6da23d8e153249a836
the changes should be correct until 1.7 at the very least, possibly older.
Willy Tarreau [Wed, 30 Jan 2019 10:44:07 +0000 (11:44 +0100)]
BUG/MINOR: mux-h2: make sure request trailers on aborted streams don't break the connection
We used to respond a connection error in case we received a trailers
frame on a closed stream, but it's a problem to do this if the error
was caused by a reset because the sender has not yet received it and
is just a victim of the timing. Thus we must not close the connection
in this case.
This patch may be backported to 1.9 but then it requires the following
previous ones :
MINOR: h2: add a generic frame checker
MEDIUM: mux-h2: check the frame validity before considering the stream state
CLEANUP: mux-h2: remove stream ID and frame length checks from the frame parsers
Willy Tarreau [Wed, 30 Jan 2019 14:39:55 +0000 (15:39 +0100)]
CLEANUP: mux-h2: remove stream ID and frame length checks from the frame parsers
It's not convenient to have such structural checks mixed with the ones
related to the stream state. Let's remove all these basic tests that are
already covered once for all when reading the frame header.
Willy Tarreau [Wed, 30 Jan 2019 14:11:03 +0000 (15:11 +0100)]
MEDIUM: mux-h2: check the frame validity before considering the stream state
There are some uneasy situation where it's difficult to validate a frame's
format without being in an appropriate state. This patch makes sure that
each frame passes through h2_frame_check() before being checked in the
context of the stream's state. This makes sure we can always return a GOAWAY
for protocol violations even if we can't process the frame.
Willy Tarreau [Wed, 30 Jan 2019 14:09:21 +0000 (15:09 +0100)]
MINOR: h2: add a generic frame checker
The new function h2_frame_check() checks the protocol limits for the
received frame (length, ID, direction) and returns a verdict made of
a connection error code. The purpose is to be able to validate any
frame regardless of the state and the ability to call the frame handler,
and to emit a GOAWAY early in this case.
Willy Tarreau [Wed, 30 Jan 2019 15:55:48 +0000 (16:55 +0100)]
BUG/MINOR: mux-h2: make sure response HEADERS are not received in other states than OPEN and HLOC
RFC7540#5.1 states that these are the only states allowing any frame
type. For response HEADERS frames, we cannot accept that they are
delivered on idle streams of course, so we're left with these two
states only. It is important to test this so that we can remove the
generic CLOSE_STREAM test for such frames in the main loop.
This must be backported to 1.9 (1.8 doesn't have response HEADERS).
Willy Tarreau [Wed, 30 Jan 2019 15:58:30 +0000 (16:58 +0100)]
BUG/MEDIUM: mux-h2: do not abort HEADERS frame before decoding them
If a response HEADERS frame arrives on a closed connection (due to a
client abort sending an RST_STREAM), it's currently immediately rejected
with an RST_STREAM, like any other frame. This is incorrect, as HEADERS
frames must first be decoded to keep the HPACK decoder synchronized,
possibly breaking subsequent responses.
This patch excludes HEADERS/CONTINUATION/PUSH_PROMISE frames from the
central closed state test and leaves to the respective frame parsers
the responsibility to decode the frame then send RST_STREAM.
This fix must be backported to 1.9. 1.8 is not directly impacted since
it doesn't have response HEADERS nor trailers thus cannot recover from
such situations anyway.
Willy Tarreau [Wed, 30 Jan 2019 18:20:09 +0000 (19:20 +0100)]
BUG/MEDIUM: mux-h2: make sure never to send GOAWAY on too old streams
The H2 spec requires to send GOAWAY when the client sends a frame after
it has already closed using END_STREAM. Here the corresponding case was
the fallback of a series of tests on the stream state, but it unfortunately
also catches old closed streams which we don't know anymore. Thus any late
packet after we've sent an RST_STREAM will trigger this GOAWAY and break
other streams on the connection.
This can happen when launching two tabs in a browser targetting the same
slow page through an H2-to-H2 proxy, and pressing Escape to stop one of
them. The other one gets an error when the page finally responds (and it
generally retries), and the logs in the middle indicate SD-- flags since
the late response was cancelled.
This patch takes care to only send GOAWAY on streams we still know.
Willy Tarreau [Wed, 30 Jan 2019 18:28:32 +0000 (19:28 +0100)]
BUG/MEDIUM: mux-h2: fix two half-closed to closed transitions
When receiving a HEADERS or DATA frame with END_STREAM set, we would
inconditionally switch to half-closed(remote). This is wrong because we
could already have been in half-closed(local) and need to switch to closed.
This happens in the following situations :
- receipt of the end of a client upload after we've already responded
(e.g. redirects to POST requests)
- receipt of a response on the backend side after we've already finished
sending the request (most common case).
This may possibly have caused some streams to stay longer than needed
at the end of a transfer, though this is not apparent in tests.
Willy Tarreau [Wed, 30 Jan 2019 15:11:20 +0000 (16:11 +0100)]
BUG/MEDIUM: mux-h2: wake up flow-controlled streams on initial window update
When a settings frame updates the initial window, all affected streams's
window is updated as well. However the streams are not put back into the
send list if they were already blocked on flow control. The effect is that
such a stream will only be woken up by a WINDOW_UPDATE message but not by
a SETTINGS changing the initial window size. This can be verified with
h2spec's test http2/6.9.2/1 which occasionally fails without this patch.
It is unclear whether this situation is really met in field, but the
fix is trivial, it consists in adding each unblocked streams to the
wait list as is done for the window updates.
This fix must be backported to 1.9. For 1.8 the patch needs quite
a few adaptations. It's better to copy-paste the code block from
h2c_handle_window_update() adding the stream to the send_list when
its mws is > 0.
Willy Tarreau [Wed, 30 Jan 2019 14:42:44 +0000 (15:42 +0100)]
CLEANUP: mux-h2: remove misleading leftover test on h2s' nullity
The WINDOW_UPDATE and DATA frame handlers used to still have a check on
h2s to return either h2s_error() or h2c_error(). This is a leftover from
the early code. The h2s cannot be null there anymore as it has already
been dereferenced before reaching these locations.
Tim Duesterhus [Tue, 29 Jan 2019 15:38:56 +0000 (16:38 +0100)]
BUG/MEDIUM: compression: Rewrite strong ETags
RFC 7232 section 2.3.3 states:
> Note: Content codings are a property of the representation data,
> so a strong entity-tag for a content-encoded representation has to
> be distinct from the entity tag of an unencoded representation to
> prevent potential conflicts during cache updates and range
> requests. In contrast, transfer codings (Section 4 of [RFC7230])
> apply only during message transfer and do not result in distinct
> entity-tags.
Thus a strong ETag must be changed when compressing. Usually this is done
by converting it into a weak ETag, which represents a semantically, but not
byte-by-byte identical response. A conversion to a weak ETag still allows
If-None-Match to work.
This should be backported to 1.9 and might be backported to every supported
branch with compression.
Olivier Houchard [Tue, 29 Jan 2019 18:11:16 +0000 (19:11 +0100)]
BUG/MEDIUM: servers: Close the connection if we failed to install the mux.
If we failed to install the mux, just close the connection, or
conn_fd_handler() will be called for the FD, and crash as soon as it attempts
to access the mux' wake method.