Joe Williams [Wed, 10 Aug 2016 14:06:44 +0000 (07:06 -0700)]
MINOR: tcp: add further tcp info fetchers
Adding on to Thierry's work (http://git.haproxy.org/?p=haproxy.git;h=6310bef5)
I have added a few more fetchers for counters based on the tcp_info struct
maintained by the kernel :
Two fields were not added because they're version-dependant :
fc_rcv_rtt, fc_total_retrans
The fields name depend on the operating system. FreeBSD and NetBSD prefix
all the field names with "__" so we have to rely on a few #ifdef for
portability.
Willy Tarreau [Wed, 10 Aug 2016 19:09:24 +0000 (21:09 +0200)]
BUILD: tcp: define SOL_TCP when only IPPROTO_TCP exists
FreeBSD prefers to use IPPROTO_TCP over SOL_TCP, just like it does
with their *_IP counterparts. It's worth noting that there are a few
inconsistencies between SOL_TCP and IPPROTO_TCP in the code, eg on
TCP_QUICKACK. The two values are the same but it's worth applying
what implementations recommend.
No backport is needed, this was uncovered by the recent tcp_info stuff.
Willy Tarreau [Wed, 10 Aug 2016 17:29:09 +0000 (19:29 +0200)]
BUILD: checks: remove the last strcat and eliminate a warning on OpenBSD
OpenBSD emits warnings on usages of strcpy, strcat and sprintf (and
probably a few others). Here we have a single such warning in all the code
reintroduced by commit 0ba0e4a ("MEDIUM: Support sending email alerts") in
1.6-dev1. Let's get rid of it, the open-coding of strcat is as small as its
usage and the the result is even more efficient.
Willy Tarreau [Wed, 10 Aug 2016 16:57:38 +0000 (18:57 +0200)]
BUILD: connection: fix build breakage on openbsd due to missing in_systm.h
Recent commit 93b227d ("MINOR: listener: add the "accept-netscaler-cip"
option to the "bind" keyword") introduced an include of netinet/ip.h
which requires in_systm.h on OpenBSD. No backport is needed.
Willy Tarreau [Wed, 10 Aug 2016 16:42:17 +0000 (18:42 +0200)]
BUILD: tcp: do not include netinet/ip.h for IP_TTL
On OpenBSD, netinet/ip.h fails unless in_systm.h is included. This
include was added by the silent-drop feature introduced with commit 2d392c2 ("MEDIUM: tcp: add new tcp action "silent-drop"") in 1.6-dev6,
but we don't need it, IP_TTL is defined in netinet/in.h, so let's drop
this useless include.
Willy Tarreau [Tue, 9 Aug 2016 14:46:18 +0000 (16:46 +0200)]
MINOR: tcp: add dst_is_local and src_is_local
It is sometimes needed in application server environments to easily tell
if a source is local to the machine or a remote one, without necessarily
knowing all the local addresses (dhcp, vrrp, etc). Similarly in transparent
proxy configurations it is sometimes desired to tell the difference between
local and remote destination addresses.
This patch adds two new sample fetch functions for this :
dst_is_local : boolean
Returns true if the destination address of the incoming connection is local
to the system, or false if the address doesn't exist on the system, meaning
that it was intercepted in transparent mode. It can be useful to apply
certain rules by default to forwarded traffic and other rules to the traffic
targetting the real address of the machine. For example the stats page could
be delivered only on this address, or SSH access could be locally redirected.
Please note that the check involves a few system calls, so it's better to do
it only once per connection.
src_is_local : boolean
Returns true if the source address of the incoming connection is local to the
system, or false if the address doesn't exist on the system, meaning that it
comes from a remote machine. Note that UNIX addresses are considered local.
It can be useful to apply certain access restrictions based on where the
client comes from (eg: require auth or https for remote machines). Please
note that the check involves a few system calls, so it's better to do it only
once per connection.
Willy Tarreau [Tue, 9 Aug 2016 12:29:38 +0000 (14:29 +0200)]
MINOR: sample: use smp_make_rw() in upper/lower converters
There's no point in always duplicating the sample, just ensure it's
writable, as was done prior to the smp_dup() change. This should be
backported to 1.6 to avoid a performance regression caused by this
change (about 30% more time for upper/lower due to the copy).
Willy Tarreau [Tue, 9 Aug 2016 10:08:41 +0000 (12:08 +0200)]
BUG/MEDIUM: stick-table: properly convert binary samples to keys
The binary sample to stick-table key conversion is wrong. It doesn't
check that the binary sample is writable before padding it. After a
quick audit, it doesn't look like any existing sample fetch function
can trigger this bug. The correct fix consists in calling smp_make_rw()
prior to padding the sample.
Willy Tarreau [Tue, 9 Aug 2016 09:59:12 +0000 (11:59 +0200)]
BUG/MEDIUM: stick-tables: do not fail on string keys with no allocated size
When a stick-table key is derived from a string-based sample, it checks
if it's properly zero-terminated otherwise tries to do so. But the test
doesn't work for two reasons :
- the reported allocated size may be zero while the sample is maked as
not CONST (eg: certain sample fetch functions like smp_fetch_base()
do this), so smp_dup() prior to the recent changes will fail on this.
- the string might have been converted from a binary sample, where the
trailing zero is not appended. If the sample was writable, smp_dup()
would not modify it either and we would fail again here. This may
happen with req.payload or req.body_param for example.
The correct solution consists in calling smp_make_safe() to ensure the
sample is usable as a valid string.
Willy Tarreau [Tue, 9 Aug 2016 09:55:21 +0000 (11:55 +0200)]
BUG/MAJOR: server: the "sni" directive could randomly cause trouble
The "sni" server directive does some bad stuff on many occasions because
it works on a sample of type string and limits len to size-1 by hand. The
problem is that size used to be zero on many occasions before the recent
changes to smp_dup() and that it effectively results in setting len to -1
and writing the zero byte *before* the string (and not terminating the
string).
This patch makes use of the recently introduced smp_make_safe() to address
this issue.
Willy Tarreau [Tue, 9 Aug 2016 09:49:20 +0000 (11:49 +0200)]
MINOR: sample: provide smp_is_rw() and smp_make_rw()
At some places, smp_dup() is inappropriately called to ensure a modification
is possible while in fact we only need to ensure the sample may be modified
in place. Let's provide smp_is_rw() to check for this capability and
smp_make_rw() to perform the smp_dup() when it is not the case.
Note that smp_is_rw() will also try to add the trailing zero on strings when
needed if possible, to avoid a useless duplication.
Willy Tarreau [Tue, 9 Aug 2016 09:37:54 +0000 (11:37 +0200)]
MINOR: sample: implement smp_is_safe() and smp_make_safe()
These functions ensure that the designated sample is "safe for use",
which means that its size is known, its length is correct regarding its
size, and that strings are properly zero-terminated.
smp_is_safe() only checks (and optionally sets the trailing zero when
needed and possible). smp_make_safe() will call smp_dup() after
smp_is_safe() fails.
Willy Tarreau [Mon, 8 Aug 2016 17:21:09 +0000 (19:21 +0200)]
BUG/MEDIUM: samples: make smp_dup() always duplicate the sample
Vedran Furac reported a strange problem where the "base" sample fetch
would not always work for tracking purposes.
In fact, it happens that commit bc8c404 ("MAJOR: stick-tables: use sample
types in place of dedicated types") merged in 1.6 exposed a fundamental
bug related to the way samples use chunks as strings. The problem is that
chunks convey a base pointer, a length and an optional size, which may be
zero when unknown or when the chunk is allocated from a read-only location.
The sole purpose of this size is to know whether or not the chunk may be
appended new data. This size cause some semantics issue in the sample,
which has its own SMP_F_CONST flag to indicate read-only contents.
The problem was emphasized by the commit above because it made use of new
calls to smp_dup() to convert a sample to a table key. And since smp_dup()
would only check the SMP_F_CONST flag, it would happily return read-write
samples indicating size=0.
So some tests were added upon smp_dup() return to ensure that the actual
length is smaller than size, but this in fact made things even worse. For
example, the "sni" server directive does some bad stuff on many occasions
because it limits len to size-1 and effectively sets it to -1 and writes
the zero byte before the beginning of the string!
It is therefore obvious that smp_dup() needs to be modified to take this
nature of the chunks into account. It's not enough but is needed. The core
of the problem comes from the fact that smp_dup() is called for 5 distinct
needs which are not always fulfilled :
1) duplicate a sample to keep a copy of it during some operations
2) ensure that the sample is rewritable for a converter like upper()
3) ensure that the sample is terminated with a \0
4) set a correct size on the sample
5) grow the sample in case it was extracted from a partial chunk
Case 1 is not used for now, so we can ignore it. Case 2 indicates the wish
to modify the sample, so its R/O status must be removed if any, but there's
no implied requirement that the chunk becomes larger. Case 3 is used when
the sample has to be made compatible with libc's str* functions. There's no
need to make it R/W nor to duplicate it if it is already correct. Case 4
can happen when the sample's size is required (eg: before performing some
changes that must fit in the buffer). Case 5 is more or less similar but
will happen when the sample by be grown but we want to ensure we're not
bound by the current small size.
So the proposal is to have different functions for various operations. One
will ensure a sample is safe for use with str* functions. Another one will
ensure it may be rewritten in place. And smp_dup() will have to perform an
inconditional duplication to guarantee at least #5 above, and implicitly
all other ones.
This patch only modifies smp_dup() to make the duplication inconditional. It
is enough to fix both the "base" sample fetch and the "sni" server directive,
and all use cases in general though not always optimally. More patches will
follow to address them more optimally and even better than the current
situation (eg: avoid a dup just to add a \0 when possible).
The bug comes from an ambiguous design, so its roots are old. 1.6 is affected
and a backport is needed. In 1.5, the function already existed but was only
used by two converters modifying the data in place, so the bug has no effect
there.
Willy Tarreau [Mon, 8 Aug 2016 14:41:01 +0000 (16:41 +0200)]
BUG/MAJOR: compression: initialize avail_in/next_in even during flush
For quite some time, a few users have been experiencing random crashes
when compressing with zlib, from versions 1.2.3 to 1.2.8 included.
Upon thourough investigation in zlib's deflate.c, it appeared obvious
that avail_in and next_in are used during the flush operation and need
to be initialized, while admittedly it's not obvious in the documentation.
By simply forcing both values to -1 it's possible to immediately reproduce
the exact crash that these users have been experiencing :
(gdb) bt
#0 0x00007fa73ce10c00 in __memcpy_sse2 () from /lib64/libc.so.6
#1 0x00007fa73e0c5d49 in ?? () from /lib64/libz.so.1
#2 0x00007fa73e0c68e0 in ?? () from /lib64/libz.so.1
#3 0x00007fa73e0c73c7 in deflate () from /lib64/libz.so.1
#4 0x00000000004dca1c in deflate_flush_or_finish (comp_ctx=0x7b6580, out=0x7fa73e5bd010, flag=2) at src/compression.c:808
#5 0x00000000004dcb60 in deflate_flush (comp_ctx=0x7b6580, out=0x7fa73e5bd010) at src/compression.c:835
#6 0x00000000004dbc50 in http_compression_buffer_end (s=0x7c0050, in=0x7c00a8, out=0x78adf0 <tmpbuf.24662>, end=0) at src/compression.c:249
#7 0x000000000048bb5f in http_response_forward_body (s=0x7c0050, res=0x7c00a0, an_bit=1048576) at src/proto_http.c:7173
#8 0x00000000004cce54 in process_stream (t=0x7bffd8) at src/stream.c:1939
#9 0x0000000000427ddf in process_runnable_tasks () at src/task.c:238
#10 0x0000000000419892 in run_poll_loop () at src/haproxy.c:1573
#11 0x000000000041a4a5 in main (argc=4, argv=0x7fffcda38348) at src/haproxy.c:1933
Note that for all reports deflate_flush_or_finish() was always involved.
The crash is very hard to reproduce when using regular traffic because it
requires that the combination of avail_in and next_in are inadequate so
that the memcpy() call reads out of bounds. But this can very likely
happen when the input buffer points to an area reused by another stream
when the flush has been interrupted due to a full output buffer. This
also explains why this report is recent, as dynamic buffer allocation
was introduced in 1.6.
Anyway it's not acceptable to call a function with a randomly set input
buffer. The deflate() function explicitly checks for the case where both
avail_in and next_in are null and doesn't use it in this case during a
flush, so this is the best solution.
Special thanks to Sasha Litvak, James Hartshorn and Paul Bauer for
reporting very useful stack traces which were critical to finding the
root cause of this bug.
This fix must be backported into 1.6 and 1.5, though 1.5 is less likely to
trigger this case given that it keeps its own buffers allocated all along
the session's life.
Willy Tarreau [Sun, 7 Aug 2016 06:12:35 +0000 (08:12 +0200)]
BUG/MEDIUM: stream-int: completely detach connection on connect error
Tim Butler reported a troubling issue affecting all versions since 1.5.
When a connection error occurs and a retry is performed on the same server,
the server connection first goes into the turn-around state (SI_ST_TAR) for
one second. During this time, the client may speak and try to push some
data. The tests in place confirm that the stream interface is in a state
<= SI_ST_EST and that a connection exists, so all ingredients are present
to try to perform a send() to forward data. The send() cannot be performed
since the connection's control layer is marked as not ready, but the
polling flags are changed, and due to the remaining error flag present
on the connection, the polling on the FD is disabled in both directions.
But if this FD was reassigned to another connection in the mean time, it
is this FD which is disabled, and it causes a timeout on another connection.
A configuration allowing to reproduce the issue looks like this :
listen test
bind :8003
server s1 127.0.0.1:8001 # this one should be closed
listen victim
bind :8002
server s1 127.0.0.1:8000 # this one should respond slowly (~50ms)
Two parallel injections should be run with short time-outs (100ms). After
some time, some dead connections will appear in listener "victim" due to
their I/Os being disabled by some of the failed transfers on "test"
instance. These ones will only be flushed on time out. A dead connection
looks like this :
BUG/MEDIUM: lua: somme HTTP manipulation functions are called without valid requests
Somme HTTP manipulation functions are executed without valid and parsed
requests or responses. This causes a segmentation fault when the executed
code tries to change an empty buffer.
MINOR: tcp: Return TCP statistics like RTT and RTT variance
This patch adds 4 new sample fetches which returns the RTT of the
established connexion and the RTT variance. The established connection
can be between the client and HAProxy, and between HAProxy and the
server. This is very useful for statistics. A great use case is the
estimation of the TCP connection time of the client. Note that the
RTT of the server side is not so interesting because we already have
the connect() time.
BUG/MEDIUM: log: use function "escape_string" instead of "escape_chunk"
In function lf_text_len(), we used escape_chunk() to escape special
characters. There could be a problem if len is greater than the real src
string length (zero-terminated), eg. when calling lf_text_len() from
lf_text().
Similar to "escape_chunk", this function tries to prefix all characters
tagged in the <map> with the <escape> character. The specified <string>
contains the input to be escaped.
BUG/MINOR: peers: don't count track-sc multiple times on errors
Ruoshan Huang found that the call to session_inc_http_err_ctr() in the
recent http-response patch was not a good idea because it also increments
counters that are already tracked (eg: http-request track-sc or previous
http-response track-sc). Better open-code the update, it's simple.
This enables tracking of sticky counters from current response. The only
difference from "http-request track-sc" is the <key> sample expression
can only make use of samples in response (eg. res.*, status etc.) and
samples below Layer 6.
BUG/MEDIUM: lua: the function txn_done() from action wrapper can crash
If an action wrapper stops the processing of the transaction
with a txn_done() function, the return code of the action is
"continue". So the continue can implies the processing of other
like adding headers. However, the HTTP content is flushed and
a segfault occurs.
This patchs add a flag indicating that the Lua code want to
stop the processing, ths flags is forwarded to the haproxy core,
and other actions are ignored.
BUG/MEDIUM: lua: the function txn_done() from sample fetches can crash
The function txn_done() ends a transaction. It does not make
sense to call this function from a lua sample-fetch wrapper,
because the role of a sample-fetch is not to terminate a
transaction.
This patch modify the role of the fucntion txn_done() if it
is called from a sample-fetch wrapper, now it just ends the
execution of the Lua code like the done() function.
BUG/MINOR: Fix endiness issue in DNS header creation code
Alexander Lebedev reported that the response bit is set on SPARC when
DNS queries are sent. This has been tracked to the endianess issue, so
this patch makes the code portable.
BUG/MEDIUM: dns: fix alignment issues in the DNS response parser
Alexander Lebedev reported that the DNS parser crashes in 1.6 with a bus
error on Sparc when it receives a response. This is obviously caused by
some alignment issues. The issue can also be reproduced on ARMv5 when
setting /proc/cpu/alignment to 4 (which helps debugging).
Two places cause this crash in turn, the first one is when the IP address
from the packet is compared to the current one, and the second place is
when the address is assigned because an unaligned address is passed to
update_server_addr().
This patch modifies these places to properly use memcpy() and memcmp()
to manipulate the unaligned data.
Nenad Merdanovic found another set of places specific to 1.7 in functions
in_net_ipv4() and in_net_ipv6(), which are used to compare networks. 1.6
has the functions but does not use them. There we perform a temporary copy
to a local variable to fix the problem. The type of the function's argument
is wrong since it's not necessarily aligned, so we change it for a const
void * instead.
This fix must be backported to 1.6. Note that in 1.6 the code is slightly
different, there's no rec[] array, the pointer is used directly from the
buffer.
BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()
Roberto Guimaraes reported that Valgrind complains about a leak
in ssl_get_dh_1024().
This is caused caused by an oversight in ssl_sock_load_dh_params(),
where local_dh_1024 is always replaced by a new DH object even if
it already holds one. This patch simply checks whether local_dh_1024
is NULL before calling ssl_get_dh_1024().
David Carlier [Mon, 4 Jul 2016 21:51:33 +0000 (22:51 +0100)]
CLEANUP: connection: using internal struct to hold source and dest port.
Originally, tcphdr's source and dest from Linux were used to get the
source and port which led to a build issue on BSD oses.
To avoid side problems related to network then we just use an internal
struct as we need only those two fields.
Certain very simple confs randomly segfault upon startup with openssl 1.0.2
with this patch, which seems to indicate a use after free. Better drop it
and let valgrind complain about the potential leak.
Also it's worth noting that the man page for SSL_CTX_set_tmp_dh() makes no
mention about whether or not the element should be freed, and the example
provided does not use it either.
This fix should be backported to 1.6 and 1.5 where the patch was just
included.
CLEANUP: fixed some usages of realloc leading to memory leak
Changed all the cases where the pointer passed to realloc is overwritten
by the pointer returned by realloc. The new function my_realloc2 has
been used except in function register_name. If register_name fails to
add a new variable because of an "out of memory" error, all the existing
variables remain valid. If we had used my_realloc2, the array of variables
would have been freed.
MINOR: new function my_realloc2 = realloc + free upon failure
When realloc fails to allocate memory, the original pointer is not
freed. Sometime people override the original pointer with the pointer
returned by realloc which is NULL in case of failure. This results
in a memory leak because the memory pointed by the original pointer
cannot be freed.
BUG/MINOR: Rework slightly commit 9962f8fc to clean code and avoid mistakes
In commit 9962f8fc (BUG/MEDIUM: http: unbreak uri/header/url_param hashing), we
take care to update 'msg->sov' value when the parser changes to state
HTTP_MSG_DONE. This works when no filter is used. But, if a filter is used and
if it loops on 'http_end' callback, the following block is evaluated two times
consecutively:
Today, in practice, because this happens when all data are parsed and forwarded,
the second test always fails (after the first update, msg->sov is always lower
or equal to 0). But it is useless and error prone. So to avoid misunderstanding
the code has been slightly changed. Now, in all cases, we try to update msg->sov
only once per iteration.
Vedran Furac reported that "balance uri" doesn't work anymore in recent
1.7-dev versions. Dragan Dosen found that the first faulty commit was dbe34eb ("MEDIUM: filters/http: Move body parsing of HTTP messages in
dedicated functions"), merged in 1.7-dev2.
After this patch, the hashing is performed on uninitialized data,
indicating that the buffer is not correctly rewound. In fact, all forms
of content-based hashing are broken since the commit above. Upon code
inspection, it appears that the new functions http_msg_forward_chunked_body()
and http_msg_forward_body() forget to rewind the buffer in the success
case, when the parser changes to state HTTP_MSG_DONE. The rewinding code
was reinserted in both functions and the fix was confirmed by two test,
with and without chunking.
Willy Tarreau [Sun, 26 Jun 2016 17:37:59 +0000 (19:37 +0200)]
DOC: add missing doc for http-request deny [deny_status <status>]
The feature was introduced in 1.6-dev2 by commit 108b1dd ("MEDIUM:
http: configurable http result codes for http-request deny") but the
doc was missing. Thanks to Cyril for noticing.
Willy Tarreau [Fri, 24 Jun 2016 16:30:22 +0000 (18:30 +0200)]
BUG/BUILD: don't automatically run "make" on "make install"
Kay Fuchs reported that the recent changes to automatically rebuild files
on config option changes caused "make install" to rebuild the whole code
with the wrong options. That's caused by the fact that the "install-bin"
target depends on the "haproxy" target, which detects the lack of options
and causes a rebuild with different ones.
This patch makes a simple change, it removes this automatic dependency
which was already wrong since it could cause some files to be built with
different options prior to these changes, and instead emits an error
message indicating that "make" should be run prior to "make install".
The patches were backported into 1.6 so this fix must go there as well.
Willy Tarreau [Wed, 22 Jun 2016 12:51:40 +0000 (14:51 +0200)]
BUG/MINOR: srv-state: fix incorrect output of state file
Eric Webster reported that the state file wouldn't reload in 1.6.5
while it used to work in 1.6.4. The issue is that headers are now
missing from the output when a specific backend is dumped since
commit 4c1544d ("BUG/MEDIUM: stats: show servers state may show an
empty or incomplete result"). This patch fixes this by introducing
a dump state.
BUG/MINOR: filters: Fix HTTP parsing when a filter loops on data forwarding
A filter can choose to loop on data forwarding. When this loop occurs in
HTTP_MSG_ENDING state, http_foward_data callbacks are called twice because of a
goto on the wrong label.
A filter can also choose to loop at the end of a HTTP message, in http_end
callback. Here the goto is good but the label is not at the right place. We must
be sure to upate msg->sov value.
BUG/MEDIUM: filters: Fix data filtering when data are modified
Filters can alter data during the parsing, i.e when http_data or tcp_data
callbacks are called. For now, the update must be done by hand. So we must
handle changes in the channel buffers, especially on the number of input bytes
pending (buf->i).
In addition, a filter can choose to switch channel buffers to do its
updates. So, during data filtering, we must always use the right buffer and not
use variable to reference them.
Without this patch, filters cannot safely alter data during the data parsing.
Willy Tarreau [Tue, 21 Jun 2016 15:34:14 +0000 (17:34 +0200)]
CLEANUP: external-check: don't block/unblock SIGCHLD when manipulating the list
There's no point in blocking/unblocking sigchld when removing entries
from the list since the code is called asynchronously.
Similarly the blocking/unblocking could be removed from the connect_proc_chk()
function but it happens that at high signal rates, fork() takes twice as much
time to execute as it is regularly interrupted by a signal, so in the end this
signal blocking is beneficial there for performance reasons.
Willy Tarreau [Tue, 21 Jun 2016 15:29:46 +0000 (17:29 +0200)]
BUG/MINOR: external-checks: do not unblock undesired signals
The external checks code makes use of block_sigchld() and unblock_sigchld()
to ensure nobody modifies the signals list while they're being manipulated.
It happens that these functions clear the list of blocked signals, so they
can possibly have a side effect if other signals are blocked. For now no
other signal is blocked but it may very well change in the future so rather
correctly use SIG_BLOCK/SIG_UNBLOCK instead of touching unrelated signals.
This fix should be backported to 1.6 for correctness.
Willy Tarreau [Tue, 21 Jun 2016 14:27:34 +0000 (16:27 +0200)]
BUG/MAJOR: external-checks: use asynchronous signal delivery
There are random segfaults occuring when using external checks. The
reason is that when receiving a SIGCHLD, a call to task_wakeup() is
performed. There are two situations where this causes trouble :
- the scheduler is in process_running_tasks(), since task_wakeup()
sets rq_next to NULL, when the former dereferences it to get the
next pointer, the program crashes ;
- when another task_wakeup() is being called and during eb_next()
in process_running_tasks(), because the structure of the run queue
tree changes while it is being processed.
The solution against this is to use asynchronous signal processing
thanks to the internal signal API. The signal handler is registered,
and upon delivery, the signal is added to the queue and processed
out of any other processing.
This code was stressed at 2500 forks/s and their respective signals
for quite some time and the segfault is now gone.
Willy Tarreau [Tue, 21 Jun 2016 13:32:29 +0000 (15:32 +0200)]
BUG/MEDIUM: external-checks: close all FDs right after the fork()
Lukas Erlacher reported an interesting problem : since we don't close
FDs after the fork() upon external checks, any script executed that
writes data on stdout/stderr will possibly send its data to wrong
places, very likely an existing connection.
After some analysis, the problem is even wider. It's not enough to
just close stdin/stdout/stderr, as all sockets are passed to the
sub-process, and as long as they're not closed, they are usable for
whatever mistake can be done. Furthermore with epoll, such FDs will
continue to be reported after a close() as the underlying file is
not closed yet.
CLOEXEC would be an acceptable workaround except that 1) it adds an
extra syscall on the fast path, and 2) we have no control over FDs
created by external libs (eg: openssl using /dev/crypto, libc using
/dev/random, lua using anything else), so in the end we still need
to close them all.
On some BSD systems there's a closefrom() syscall which could be
very useful for this.
Based on an insightful idea from Simon Horman, we don't close 0/1/2
when we're in verbose mode since they're properly connected to
stdin/stdout/stderr and can become quite useful during debugging
sessions to detect some script output errors or execve() failures.
Willy Tarreau [Tue, 21 Jun 2016 09:51:59 +0000 (11:51 +0200)]
BUG/MINOR: init: ensure that FD limit is raised to the max allowed
When the requested amount of FDs cannot be allocated, setrlimit() fails.
That's bad because if the limit is set to 1024 and we need 10000, we
stay on 1024 while we could possibly raise it to 4096 thanks to rlim_max.
This patch takes care of trying to assign rlim_cur to rlim_max on failure
so that we get as much as possible if we can't get all we need. The case
is particularly visible when starting haproxy as a non-privileged user
and a large maxconn is specified in the configuration.
Another point of doing this is that it is the only way to allow us to
close inherited FDs upon fork(), ie those between rlim_cur and rlim_max.
Willy Tarreau [Tue, 21 Jun 2016 09:48:18 +0000 (11:48 +0200)]
BUG/MINOR: init: always ensure that global.rlimit_nofile matches actual limits
global.rlimit_nofile contains the mxa number of file descriptors that
can be allocated, except if the user is not allowed to reach this limit,
where it still contains the initially requested value. It is important
that this value always matches what is really configured so that it is
properly reported in the stats and that we can use it later to close
all FDs without wasting time closing impossible FDs.
This configures the client-facing connection to receive a NetScaler
Client IP insertion protocol header before any byte is read from the
socket. This is equivalent to having the "accept-netscaler-cip" keyword
on the "bind" line, except that using the TCP rule allows the PROXY
protocol to be accepted only for certain IP address ranges using an ACL.
This is convenient when multiple layers of load balancers are passed
through by traffic coming from public hosts.
MINOR: listener: add the "accept-netscaler-cip" option to the "bind" keyword
When NetScaler application switch is used as L3+ switch, informations
regarding the original IP and TCP headers are lost as a new TCP
connection is created between the NetScaler and the backend server.
NetScaler provides a feature to insert in the TCP data the original data
that can then be consumed by the backend server.
Specifications and documentations from NetScaler:
https://support.citrix.com/article/CTX205670
https://www.citrix.com/blogs/2016/04/25/how-to-enable-client-ip-in-tcpip-option-of-netscaler/
When CIP is enabled on the NetScaler, then a TCP packet is inserted just after
the TCP handshake. This is composed as:
- CIP magic number : 4 bytes
Both sender and receiver have to agree on a magic number so that
they both handle the incoming data as a NetScaler Client IP insertion
packet.
- Header length : 4 bytes
Defines the length on the remaining data.
- IP header : >= 20 bytes if IPv4, 40 bytes if IPv6
Contains the header of the last IP packet sent by the client during TCP
handshake.
- TCP header : >= 20 bytes
Contains the header of the last TCP packet sent by the client during TCP
handshake.
Willy Tarreau [Mon, 20 Jun 2016 21:01:57 +0000 (23:01 +0200)]
BUILD: ssl: fix typo causing a build failure in the multicert patch
I just noticed that SSL wouldn't build anymore since this afternoon's patch :
src/ssl_sock.c: In function 'ssl_sock_load_multi_cert':
src/ssl_sock.c:1982:26: warning: left-hand operand of comma expression has no effect [-Wunused-value]
for (i = 0; i < fcount, i++)
^
src/ssl_sock.c:1982:31: error: expected ';' before ')' token
for (i = 0; i < fcount, i++)
^
Makefile:791: recipe for target 'src/ssl_sock.o' failed
make: *** [src/ssl_sock.o] Error 1
Pavlos Parissis [Tue, 14 Jun 2016 11:28:20 +0000 (13:28 +0200)]
MINOR: systemd: Use variable for config and pidfile paths
Users can set the location of haproxy.cfg and pidfile files by providing
a systemd overwrite file /etc/systemd/system/haproxy.service.d/overwrite.conf
with the following content:
http-response set-log-level can't parse the log level correctly,
as the level argument ptr is one byte ahead when passed to get_log_level
---
src/proto_http.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Dragan Dosen [Thu, 16 Jun 2016 09:08:08 +0000 (11:08 +0200)]
BUG/MINOR: http: url32+src should use the big endian version of url32
This is similar to the commit 5ad6e1dc ("BUG/MINOR: http: base32+src
should use the big endian version of base32"). Now we convert url32 to big
endian when building the binary block.
MEDIUM: dumpstats: make stats_tlskeys_list() yield-aware during tls-keys dump
The previous dump algorithm was not trying to yield when the buffer is
full, it's not a problem with the TLS_TICKETS_NO which is 3 by default
but it can become one if the buffer size is lowered and if the
TLS_TICKETS_NO is increased.
The index of the latest ticket dumped is now stored to ensure we can
resume the dump after a yield.
BUG/MEDIUM: dumpstats: undefined behavior in stats_tlskeys_list()
The function stats_tlskeys_list() can meet an undefined behavior when
called with appctx->st2 == STAT_ST_LIST, indeed the ref pointer is used
uninitialized.
However this function was using NULL in appctx->ctx.tlskeys.ref as a
flag to dump every tickets from every references. A real flag
appctx->ctx.tlskeys.dump_all is now used for this behavior.
This patch delete the 'ref' variable and use appctx->ctx.tlskeys.ref
directly.
BUG/MINOR: ssl: fix potential memory leak in ssl_sock_load_dh_params()
Valgrind reports that the memory allocated in ssl_get_dh_1024() was leaking. Upon further inspection of openssl code, it seems that SSL_CTX_set_tmp_dh makes a copy of the data, so calling DH_free afterwards makes sense.
If we use the action "http-request add-header" with a Lua sample-fetch or
converter, and the Lua function calls one of the Lua log function, the
header name is corrupted, it contains an extract of the last loggued data.
This is due to an overwrite of the trash buffer, because his scope is not
respected in the "add-header" function. The scope of the trash buffer must
be limited to the function using it. The build_logline() function can
execute a lot of other function which can use the trash buffer.
This patch fix the usage of the trash buffer. It limits the scope of this
global buffer to the local function, we build first the header value using
build_logline, and after we store the header name.
BUG/MINOR: http: add-header: header name copied twice
The header name is copied two time in the buffer. The first copy is a printf-like
function writing the name and the http separators in the buffer, and the second
form is a memcopy. This seems to be inherited from some changes. This patch
removes the printf like, format.
This patch must be backported in 1.6 and 1.5 versions
Thierry Fournier [Fri, 27 May 2016 14:35:01 +0000 (16:35 +0200)]
BUG/MEDIUM: lua: converters doesn't work
The number of arguments pushed in the stack are false, so we try to execute a
function out of the stack. This function is always a nil pointer, so the
following message is displayed.
Lua converter 'testconv': runtime error: attempt to call a nil value.
Willy Tarreau [Mon, 30 May 2016 13:16:10 +0000 (15:16 +0200)]
BUILD/MEDIUM: force a full rebuild if some build options change
We now instrument the makefile to keep a copy of previous build options.
The goal is to ensure that we'll rebuild everything when build options
change. The options that are watched are TARGET, VERBOSE_CFLAGS, and
BUILD_OPTIONS. These ones are copied into a file ".build_opts" and
compared to the new ones upon each build. This file is referenced in
the DEP variable which all .o files depend on, and it depends on the
code which updates it only upon changes. This ensures that a new file
is regenerated and detected upon change and that everything is rebuilt.
Willy Tarreau [Mon, 30 May 2016 11:39:32 +0000 (13:39 +0200)]
BUILD/MEDIUM: rebuild everything when an include file is changed
Some users tend to get caught by incorrect builds when they try patches
that modify some include file after they forget to run "make clean".
While we can't blame users who are not developers, forcing developers
to rely on a painful autodepend is not nice either and will cause them
to test their changes less often. Here we propose a reasonable tradeoff.
This patch introduces a new "INCLUDES" variable which enumerates all
the ".h" files and sets them as a build dependency for all ".o" files.
This list is then copied into a "DEP" variable which can safely be
overridden if desired. This way by default all .c files are rebuilt if
any include file changes. This is the safe method for all users. And
developers can simply add "DEP=" to their quick build scripts to keep
the old fast and efficient behaviour.
BUG/MEDIUM: sticktables: segfault in some configuration error cases
When a stick table is tracked, and another one is used later on the
configuration, a segfault occurs.
The function "smp_create_src_stkctr" can return a NULL value, and
its value is not tested, so one other function try to dereference
a NULL pointer. This patch just add a verification of the NULL
pointer.
The problem is reproduced with this configuration:
listen www
mode http
bind :12345
tcp-request content track-sc0 src table IPv4
http-request allow if { sc0_inc_gpc0(IPv6) gt 0 }
server dummy 127.0.0.1:80
backend IPv4
stick-table type ip size 10 expire 60s store gpc0
backend IPv6
stick-table type ipv6 size 10 expire 60s store gpc0
Thank to kabefuna@gmail.com for the bug report.
This patch must be backported in the 1.6 and 1.5 version.
MEDIUM: tcp: add 'set-src' to 'tcp-request connection'
The 'set-src' action was not available for tcp actions The action code
has been converted into a function in proto_tcp.c to be used for both
'http-request' and 'tcp-request connection' actions.
Both http and tcp keywords are registered in proto_tcp.c
Cyril Bonté [Thu, 26 May 2016 22:06:45 +0000 (00:06 +0200)]
BUG/MEDIUM: stats: show servers state may show an servers from another backend
Olivier Doucet reported that "show servers state" was producing an invalid
output with some configurations where nbproc > 1.
Indeed, commit 76a99784f4 fixed some issues but unfortunately introduced a
regression when a backend bound to the same process as the stats socket and a
previous backend is bound to another one.
For example :
global
daemon
nbproc 2
stats socket /var/run/haproxy-1.sock process 1
stats socket /var/run/haproxy-2.sock process 2
listen proc1
bind 127.0.0.1:9001
bind-process 1
server WRONG 127.0.0.1:80
listen proc2
bind 127.0.0.1:9002
bind-process 2
server RIGHT 127.0.0.1:80
Requesting "show servers state" on /var/run/haproxy-2.sock was producing a line
like :
3 proc2 1 WRONG 127.0.0.1 2 0 1 1 4 1 0 2 0 0 0 0
whereas the line below was awaited :
3 proc2 1 RIGHT 127.0.0.1 2 0 1 1 5 1 0 2 0 0 0 0
This was caused by the initialization of the server loop too early, before the
bind_proc filtering whereas it should be done after.
This fix should be backported to 1.6, where the regression has unfortunately
been backported.
Willy Tarreau [Thu, 26 May 2016 15:55:28 +0000 (17:55 +0200)]
BUG/MEDIUM: config: fix multiple declaration of section parsers
Ben Cabot reported that after commit 5e4261b ("CLEANUP: config:
detect double registration of a config section") recently introduced
in 1.7-dev, it's not possible anymore to load multiple configuration
files. Bryan Talbot provided a simple reproducer to exhibit the issue.
It turns out that function readcfgfile() registers new parsers for
section keywords for each new file. In addition to being useless, this
has the negative effect of wasting memory and slowing down the config
parser as the number of configuration files increases.
This fix only needs to be backported if/where the commit above is
backported.
Willy Tarreau [Fri, 20 May 2016 04:29:59 +0000 (06:29 +0200)]
BUILD: fix build on Solaris 11
htonll()/ntohll() already exist on Solaris 11 with a different declaration,
causing a build error as reported by Jonathan Fisher. They used to exist on
OSX with a #define which allowed us to detect them. It was a bad idea to give
these functions a name subject to conflicts like this. Simply rename them
my_htonll()/my_ntohll() to definitely get rid of the conflict.
Willy Tarreau [Wed, 25 May 2016 15:16:38 +0000 (17:16 +0200)]
MINOR: stick-table: change all stick-table converters' inputs to SMP_T_ANY
The stick-table converters used to take a string on input because it
was the only type that could be casted to from any other type. This is
inefficient and possibly inaccurate sometimes. For example in order to
look up an IP address, it must first be converted to a string then
converted back to an IP address.
We've had SMP_T_ANY introduced long ago in 1.6, but unfortunately it
was not propagated to these converters, so let's do it now.
It's important to note that a few direct type conversions which already
would not make any sense are not possible (for example, converting a
boolean to an IP address or an HTTP method to an integer). While this
would have caused the lookup to be performed on the wrong key, now the
lookup will fail and the converter will return no data. While there
should not be any case where this happens, it's probably best to avoid
backporting this change before a longer observation period.
Willy Tarreau [Wed, 25 May 2016 15:07:56 +0000 (17:07 +0200)]
BUG/MEDIUM: stick-tables: fix breakage in table converters
Baptiste reported that the table_conn_rate() converter would always
return zero in 1.6.5. In fact, commit bc8c404 ("MAJOR: stick-tables:
use sample types in place of dedicated types") broke all stick-table
converters because smp_to_stkey() now returns a pointer to the sample
instead of holding a copy of the key, and the converters used to
reinitialize the sample prior to performing the lookup. Only
"in_table()" continued to work.
The construct is still fragile, so some comments were added to a few
function to clarify their impacts. It's also worth noting that there
is no point anymore in forcing these converters to take a string on
input, but that will be changed in another commit.
The bug was introduced in 1.6-dev4, this fix must be backported to 1.6.
Willy Tarreau [Wed, 25 May 2016 14:23:59 +0000 (16:23 +0200)]
BUG/MAJOR: http: fix breakage of "reqdeny" causing random crashes
Commit 108b1dd ("MEDIUM: http: configurable http result codes for
http-request deny") introduced in 1.6-dev2 was incomplete. It introduced
a new field "rule_deny_status" into struct http_txn, which is filled only
by actions "http-request deny" and "http-request tarpit". It's then used
in the deny code path to emit the proper error message, but is used
uninitialized when the deny comes from a "reqdeny" rule, causing random
behaviours ranging from returning a 200, an empty response, or crashing
the process. Often upon startup only 200 was returned but after the fields
are used the crash happens. This can be sped up using -dM.
There's no need at all for storing this status in the http_txn struct
anyway since it's used immediately after being set. Let's store it in
a temporary variable instead which is passed as an argument to function
http_req_get_intercept_rule().
As an extra benefit, removing it from struct http_txn reduced the size
of this struct by 8 bytes.
This fix must be backported to 1.6 where the bug was detected. Special
thanks to Falco Schmutz for his detailed report including an exploitable
core and a reproducer.
Vincent Bernat [Wed, 18 May 2016 14:17:44 +0000 (16:17 +0200)]
BUG/MAJOR: fix listening IP address storage for frontends
When compiled with GCC 6, the IP address specified for a frontend was
ignored and HAProxy was listening on all addresses instead. This is
caused by an incomplete copy of a "struct sockaddr_storage".
With the GNU Libc, "struct sockaddr_storage" is defined as this:
struct sockaddr_storage
{
sa_family_t ss_family;
unsigned long int __ss_align;
char __ss_padding[(128 - (2 * sizeof (unsigned long int)))];
};
Doing an aggregate copy (ss1 = ss2) is different than using memcpy():
only members of the aggregate have to be copied. Notably, padding can be
or not be copied. In GCC 6, some optimizations use this fact and if a
"struct sockaddr_storage" contains a "struct sockaddr_in", the port and
the address are part of the padding (between sa_family and __ss_align)
and can be not copied over.
Therefore, we replace any aggregate copy by a memcpy(). There is another
place using the same pattern. We also fix a function receiving a "struct
sockaddr_storage" by copy instead of by reference. Since it only needs a
read-only copy, the function is converted to request a reference.
Willy Tarreau [Mon, 16 May 2016 15:01:12 +0000 (17:01 +0200)]
SCRIPTS: make git-show-backports capable of limiting its history
When comparing very different branches, it can take a very long time
to scan all commits from the very old common ancestor (eg: haproxy
1.4 to 1.7). Now it is possible to specify a range of commits instead
of a specific branch, and the analysis will be limited to this range
for all commits. The user is responsible for ensuring that the range
covers all possible backports from base to ref, otherwise some of them
may be reported missing while they are not.
Willy Tarreau [Mon, 16 May 2016 14:39:38 +0000 (16:39 +0200)]
SCRIPTS: teach git-show-backports how to report upstream commits
When git-show-backports is called with -u, instead of reporting the
commit IDs of the original branch on the left, it will display the
upstream commit IDs when such IDs are known, and will also display
them in the suggested "git cherry-pick" command line. This is useful
when the new branch is more recent than the one being checked and/or
it is desired to get rid of intermediary changes.
Maxime de Roucy [Wed, 18 May 2016 21:13:38 +0000 (23:13 +0200)]
BUG/MEDIUM: init: don't use environment locale
This patch removes setlocale from the main function. It was introduced
by commit 379d9c7 ("MEDIUM: init: allow directory as argument of -f")
in 1.7-dev a few commits ago after a discussion on the mailing list.
Some regex may have different behaviours depending on the
locale. Some LUA scripts may change their behaviour too
(http://lua-users.org/wiki/LuaLocales).
Without this patch (haproxy is using setlocale) :
$ cat locale.cfg
defaults
mode http
frontend test
bind :9000
mode http
use_backend testbk if { hdr_reg(X-Test) ^\w+$ }
MEDIUM: filters: Add pre and post analyzer callbacks
'channel_analyze' callback has been removed. Now, there are 2 callbacks to
surround calls to analyzers:
* channel_pre_analyze: Called BEFORE all filterable analyzers. it can be
called many times for the same analyzer, once at each loop until the
analyzer finishes its processing. This callback is resumable, it returns a
negative value if an error occurs, 0 if it needs to wait, any other value
otherwise.
* channel_post_analyze: Called AFTER all filterable analyzers. Here, AFTER
means when an analyzer finishes its processing. This callback is NOT
resumable, it returns a negative value if an error occurs, any other value
otherwise.
Pre and post analyzer callbacks are not automatically called. 'pre_analyzers'
and 'post_analyzers' bit fields in the filter structure must be set to the right
value using AN_* flags (see include/types/channel.h).
The flag AN_RES_ALL has been added (AN_REQ_ALL already exists) to ease the life
of filter developers. AN_REQ_ALL and AN_RES_ALL include all filterable
analyzers.
projects / thirdparty / haproxy.git / log
