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.
No backport it needed.
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.
This must be backported into 1.6.
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.
Kay Fuchs reported that the error message is misleading in response
captures because it suggests that "len" is accepted while it's not.
This needs to be backported to 1.6.
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.
It must be backported to 1.6.
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.
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.
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.
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.
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.
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.
This fix must be backported to 1.6.
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.
This patch may be backported to 1.6 and 1.5.
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 fix may be backported to 1.6 and 1.5.
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.
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/CTX205670https://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.
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
SNI filters used to be ignored with multicerts (eg: those providing
ECDSA and RSA at the same time). This patch makes them work like
other certs.
Note: most of the changes in this patch are due to an extra level of
indent, read it with "git show -b".
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:
[Service]
Environment=CONFIG=/etc/foobar/haproxy.cfg
In function smp_fetch_url32_src(), it's better to check the value of
cli_conn before we go any further.
This patch needs to be backported to 1.6 and 1.5.
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.
This patch needs to be backported to 1.6 and 1.5.
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.
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.
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.
Thanks Michael Ezzell for the repporting.
This patch must be backported in 1.6 version
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
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.
Thanks Michael Ezzell for the repporting.
This patch must be backported in the 1.6 version.
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.
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.
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.
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
The reference to the tls_keys_ref was not deleted from the
tlskeys_reference linked list.
When the SSL is malconfigured, it can lead to an access to freed memory
during a "show tls-keys" on the admin socked.
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.
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.
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.
This patch must be backported to 1.6.
DNS requests (using the internal resolver) are corrupted since commit
e2f8497716 ("BUG/MINOR: dns: fix DNS header definition").
Fix it by defining the struct in network byte order, while complying
with RFC 2535, section 6.1.
First reported by Eduard Vopicka on discourse.
This must be backported to 1.6 (1.6.5 is affected).
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.
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.
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.
