]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
7 years agoMINOR: pools/threads: Implement lockless memory pools.
Olivier Houchard [Wed, 24 Jan 2018 17:38:31 +0000 (18:38 +0100)] 
MINOR: pools/threads: Implement lockless memory pools.

On CPUs that support a double-width compare-and-swap, implement lockless
pools.

7 years agoMINOR: threads: add test and set/reset operations
Willy Tarreau [Thu, 25 Jan 2018 16:43:58 +0000 (17:43 +0100)] 
MINOR: threads: add test and set/reset operations

This just adds a set of naive bts/btr operations based on OR/AND. Later
it could rely on pl_bts/btr to use arch-specific versions if needed.

7 years agoMINOR: threads: Introduce double-width CAS on x86_64 and arm.
Olivier Houchard [Thu, 21 Dec 2017 16:13:05 +0000 (17:13 +0100)] 
MINOR: threads: Introduce double-width CAS on x86_64 and arm.

Introduce double-width compare-and-swap on arches that support it, right now
x86_64, arm, and aarch64.
Also introduce functions to do memory barriers.

7 years agoMINOR: compiler: introduce offsetoff().
Olivier Houchard [Wed, 24 Jan 2018 17:17:06 +0000 (18:17 +0100)] 
MINOR: compiler: introduce offsetoff().

Add a offsetof() macro, if it is no there already.

7 years agoMINOR: early data: Never remove the CO_FL_EARLY_DATA flag.
Olivier Houchard [Wed, 29 Nov 2017 18:51:19 +0000 (19:51 +0100)] 
MINOR: early data: Never remove the CO_FL_EARLY_DATA flag.

It may be useful to keep the CO_FL_EARLY_DATA flag, so that we know early
data were used, so instead of doing this, only add the Early-data header,
and have the sample fetch ssl_fc_has_early return 1, if CO_FL_EARLY_DATA is
set, and if the handshake isn't done yet.

7 years agoMINOR: early data: Don't rely on CO_FL_EARLY_DATA to wake up streams.
Olivier Houchard [Mon, 27 Nov 2017 17:41:32 +0000 (18:41 +0100)] 
MINOR: early data: Don't rely on CO_FL_EARLY_DATA to wake up streams.

Instead of looking for CO_FL_EARLY_DATA to know if we have to try to wake
up a stream, because it is waiting for a SSL handshake, instead add a new
conn_stream flag, CS_FL_WAIT_FOR_HS. This way we don't have to rely on
CO_FL_EARLY_DATA, and we will only wake streams that are actually waiting.

7 years agoMINOR: init: make stdout unbuffered
Olivier Houchard [Sat, 3 Feb 2018 14:15:21 +0000 (15:15 +0100)] 
MINOR: init: make stdout unbuffered

printf is unusable for debugging without this, and printf() is not used
for anything else.

7 years agoMINOR: spoe: Add max-waiting-frames directive in spoe-agent configuration
Christopher Faulet [Thu, 25 Jan 2018 14:32:22 +0000 (15:32 +0100)] 
MINOR: spoe: Add max-waiting-frames directive in spoe-agent configuration

This is the maximum number of frames waiting for an acknowledgement on the same
connection. This value is only used when the pipelinied or asynchronus exchanges
between HAProxy and SPOA are enabled. By default, it is set to 20.

7 years agoMINOR: spoa_example: Count the number of frames processed by each worker
Christopher Faulet [Tue, 23 Jan 2018 13:46:51 +0000 (14:46 +0100)] 
MINOR: spoa_example: Count the number of frames processed by each worker

This is done for debug purpose. This way, it is easy to know if the load is
equally distributed between workers.

7 years agoMEDIUM: spoe: Use an ebtree to manage idle applets
Christopher Faulet [Wed, 24 Jan 2018 15:37:57 +0000 (16:37 +0100)] 
MEDIUM: spoe: Use an ebtree to manage idle applets

Instead of using a list of applets with idle ones in front, we now use an
ebtree. Aapplets in the tree are idle by definition. And the key is the applet's
weight. When a new frame is queued, the first idle applet (with the lowest
weight) is woken up and its weight is increased by one. And when an applet sends
a frame to a SPOA, its weight is decremented by one.

This is empirical, but it should avoid to overuse a very few number of applets
and increase the balancing between idle applets.

7 years agoMINOR: spoe: Count the number of frames waiting for an ack for each applet
Christopher Faulet [Wed, 24 Jan 2018 15:23:03 +0000 (16:23 +0100)] 
MINOR: spoe: Count the number of frames waiting for an ack for each applet

So it is easier to respect the max_fpa value. This is no more the maximum frames
processed by an applet at each loop but the maximum frames waiting for an ack
for a specific applet.

The function spoe_handle_processing_appctx has been rewritten accordingly.

7 years agoMINOR: spoe: Replace sending_rate by a frequency counter
Christopher Faulet [Wed, 24 Jan 2018 15:13:48 +0000 (16:13 +0100)] 
MINOR: spoe: Replace sending_rate by a frequency counter

sending_rate was a counter used to evaluate the SPOE capacity to process
frames. Because it was not really accurrate, it has been replaced by a frequency
counter representing the number of frames handled by the SPOE per second. We
just check this counter is higher than the number of streams waiting for a
reply. If not, a new applet is created.

7 years agoMINOR: spoe: Always link a SPOE context with the applet processing it
Christopher Faulet [Wed, 24 Jan 2018 14:59:32 +0000 (15:59 +0100)] 
MINOR: spoe: Always link a SPOE context with the applet processing it

This was already done for fragmented frames. Now, this is true for all
frames.

7 years agoMINOR: spoe: Remove check on min_applets number when a SPOE context is queued
Christopher Faulet [Wed, 24 Jan 2018 14:49:45 +0000 (15:49 +0100)] 
MINOR: spoe: Remove check on min_applets number when a SPOE context is queued

The calculation of a minimal number of active applets was really empirical and
finally useless. On heavy load, there are always many active applets (most of
time, more than the minimal required) and when the load is low, there is no
reason to keep unused applets opened.

Because of this change, the flag SPOE_APPCTX_FL_PERSIST is now unused. So it has
been removed.

7 years agoBUG/MEDIUM: spoe: Allow producer to read and to forward shutdown on request side
Christopher Faulet [Thu, 1 Feb 2018 07:45:45 +0000 (08:45 +0100)] 
BUG/MEDIUM: spoe: Allow producer to read and to forward shutdown on request side

This is mandatory to correctly set right timeout on the stream. Else the client
timeout is never set. So only SPOE processing timeout will be evaluated. If it
is not defined (ie infinity), the stream can be blocked for a while, waiting the
SPOA reply. Of course, this is not a good idea to let the SPOE processing
timeout undefined, but it can happen.

This patch must be backported in 1.8.

7 years agoBUG/MEDIUM: spoe: Always try to receive or send the frame to detect shutdowns
Christopher Faulet [Thu, 1 Feb 2018 07:45:22 +0000 (08:45 +0100)] 
BUG/MEDIUM: spoe: Always try to receive or send the frame to detect shutdowns

Before, we checked if the buffer was allocated or not to avoid sending or
receiving a frame. This was done to not call ci_putblk or co_getblk if there is
nothing to do. But the checks on the buffers are also done in these
functions. So this is not mandatory here. But in these functions, the channel
state is also checked, so an error is returned if it is closed. By skipping the
call, we also skip the checks on the channel state, delaying shutdowns
detection.

Now, we always try to send or receive a frame. So if the corresponding channel
is closed, we can immediatly handle the error.

This patch must be backported in 1.8

7 years agoMINOR: introduce proxy-v2-options for send-proxy-v2
Emmanuel Hocdet [Thu, 1 Feb 2018 14:20:32 +0000 (15:20 +0100)] 
MINOR: introduce proxy-v2-options for send-proxy-v2

Proxy protocol v2 can transport many optional informations. To avoid
send-proxy-v2-* explosion, this patch introduce proxy-v2-options parameter
and will allow to write: "send-proxy-v2 proxy-v2-options ssl,cert-cn".

7 years agoDOC: don't suggest using http-server-close
Lukas Tribus [Thu, 1 Feb 2018 22:58:59 +0000 (23:58 +0100)] 
DOC: don't suggest using http-server-close

Remove the old suggestion to use http-server-close mode, from the
beginnings of keep-alive mode in commit 16bfb021 "MINOR: config: add
option http-keep-alive").

We made http-keep-alive default in commit 70dffdaa "MAJOR: http:
switch to keep-alive mode by default".

7 years agoBUG/MINOR: epoll/threads: only call epoll_ctl(DEL) on polled FDs
Willy Tarreau [Wed, 31 Jan 2018 08:49:29 +0000 (09:49 +0100)] 
BUG/MINOR: epoll/threads: only call epoll_ctl(DEL) on polled FDs

Commit d9e7e36 ("BUG/MEDIUM: epoll/threads: use one epoll_fd per thread")
addressed an issue with the polling and required that cloned FDs are removed
from all polling threads on close. But in fact it does it for all bound
threads, some of which may not necessarily poll the FD. This is harmless,
but it may also make it harder later to deal with FD migration between
threads. Better use polled_mask which only reports threads still aware
of the FD instead of thread_mask.

This fix should be backported to 1.8.

7 years agoMINOR: stick-tables: Adds support for new "gpc1" and "gpc1_rate" counters.
Frédéric Lécaille [Mon, 29 Jan 2018 14:22:53 +0000 (15:22 +0100)] 
MINOR: stick-tables: Adds support for new "gpc1" and "gpc1_rate" counters.

Implement exactly the same code as this has been done for "gpc0" and "gpc0_rate"
counters.

7 years agoBUG/MINOR: threads: Update labels array because of changes in lock_label enum
Christopher Faulet [Tue, 30 Jan 2018 10:04:29 +0000 (11:04 +0100)] 
BUG/MINOR: threads: Update labels array because of changes in lock_label enum

Recent changes to the enum were not synchronized with the lock debugging
code. Now we use a switch/case instead of an array so that the compiler
throws a warning if there is any inconsistency.

To be backported to 1.8 (at least to add the START entry).

7 years agoMINOR: fd: pass the iocb and owner to fd_insert()
Willy Tarreau [Thu, 25 Jan 2018 06:22:13 +0000 (07:22 +0100)] 
MINOR: fd: pass the iocb and owner to fd_insert()

fd_insert() is currently called just after setting the owner and iocb,
but proceeding like this prevents the operation from being atomic and
requires a lock to protect the maxfd computation in another thread from
meeting an incompletely initialized FD and computing a wrong maxfd.
Fortunately for now all fdtab[].owner are set before calling fd_insert(),
and the first lock in fd_insert() enforces a memory barrier so the code
is safe.

This patch moves the initialization of the owner and iocb to fd_insert()
so that the function will be able to properly arrange its operations and
remain safe even when modified to become lockless. There's no other change
beyond the internal API.

7 years agoMEDIUM: poll: don't use the old FD state anymore
Willy Tarreau [Thu, 25 Jan 2018 16:11:33 +0000 (17:11 +0100)] 
MEDIUM: poll: don't use the old FD state anymore

The polling updates are now performed exactly like the epoll/kqueue
ones : only the new polled state is considered, and the previous one
is checked using polled_mask. The only specific stuff here is that
the fd state is shared between all threads, so an FD removal has to
be done only once.

7 years agoMEDIUM: select: don't use the old FD state anymore
Willy Tarreau [Thu, 25 Jan 2018 16:09:33 +0000 (17:09 +0100)] 
MEDIUM: select: don't use the old FD state anymore

The polling updates are now performed exactly like the epoll/kqueue
ones : only the new polled state is considered, and the previous one
is checked using polled_mask. The only specific stuff here is that
the fd state is shared between all threads, so an FD removal has to
be done only once.

7 years agoMEDIUM: fd: use atomic ops for hap_fd_{clr,set} and remove poll_lock
Willy Tarreau [Thu, 25 Jan 2018 15:59:09 +0000 (16:59 +0100)] 
MEDIUM: fd: use atomic ops for hap_fd_{clr,set} and remove poll_lock

Now that we can use atomic ops to set/clear an fd occurrence in an
fd_set, we don't need the poll_lock anymore. Let's remove it.

7 years agoMEDIUM: select: make use of hap_fd_* functions
Willy Tarreau [Thu, 25 Jan 2018 15:48:46 +0000 (16:48 +0100)] 
MEDIUM: select: make use of hap_fd_* functions

Given that FD_{CLR,SET} are not always guaranteed to be thread safe,
let's fall back to using the hap_fd_* functions as we used to till
1.5-dev18 and as poll() continues to use. This will make it easier
to remove the poll_lock.

7 years agoMINOR: fd: move the hap_fd_{clr,set,isset} functions to fd.h
Willy Tarreau [Thu, 25 Jan 2018 15:37:04 +0000 (16:37 +0100)] 
MINOR: fd: move the hap_fd_{clr,set,isset} functions to fd.h

These functions were created for poll() in 1.5-dev18 (commit 80da05a4) to
replace the previous FD_{CLR,SET,ISSET} that were shared with select()
because some libcs enforce a limit on FD_SET. But FD_SET doesn't seem
to be universally MT-safe, requiring locks in the select() code that
are not needed in the poll code. So let's move back to the initial
situation where we used to only use bit fields, since that has been in
use since day one without a problem, and let's use these hap_fd_*
functions instead of FD_*.

This patch only moves the functions to fd.h and revives hap_fd_isset()
that was recently removed to kill an "unused" warning.

7 years agoCLEANUP: fd: remove the unused "new" field
Willy Tarreau [Sat, 20 Jan 2018 22:59:40 +0000 (23:59 +0100)] 
CLEANUP: fd: remove the unused "new" field

This field has been unused since 1.6, it's only updated and never
tested. Let's remove it.

7 years agoMINOR: poll: more accurately compute the new maxfd in the loop
Willy Tarreau [Mon, 29 Jan 2018 14:56:24 +0000 (15:56 +0100)] 
MINOR: poll: more accurately compute the new maxfd in the loop

Last commit 173d995 ("MEDIUM: polling: start to move maxfd computation
to the pollers") moved the maxfd computation to the polling loop, but
it still adds an entry when removing an fd, forcing the next loop to
seek from further away than necessary. Let's only update the max when
actually adding an entry.

7 years agoCLEANUP: fd/threads: remove the now unused fdtab_lock
Willy Tarreau [Mon, 29 Jan 2018 14:24:37 +0000 (15:24 +0100)] 
CLEANUP: fd/threads: remove the now unused fdtab_lock

It was only used to protect maxfd computation and is not needed
anymore.

7 years agoMEDIUM: polling: start to move maxfd computation to the pollers
Willy Tarreau [Fri, 26 Jan 2018 20:48:23 +0000 (21:48 +0100)] 
MEDIUM: polling: start to move maxfd computation to the pollers

Since only select() and poll() still make use of maxfd, let's move
its computation right there in the pollers themselves, and only
during each fd update pass. The computation doesn't need a lock
anymore, only a few atomic ops. It will be accurate, be done much
less often and will not be required anymore in the FD's fast patch.

This provides a small performance increase of about 1% in connection
rate when using epoll since we get rid of this computation which was
performed under a lock.

7 years agoMINOR: fd: don't report maxfd in alert messages
Willy Tarreau [Mon, 29 Jan 2018 14:06:04 +0000 (15:06 +0100)] 
MINOR: fd: don't report maxfd in alert messages

The listeners and connectors may complain that process-wide or
system-wide FD limits have been reached and will in this case report
maxfd as the limit. This is wrong in fact since there's no reason for
the whole FD space to be contiguous when the total # of FD is reached.
A better approach would consist in reporting the accurate number of
opened FDs, but this is pointless as what matters here is to give a
hint about what might be wrong. So let's simply report the configured
maxsock, which will generally explain why the process' limits were
reached, which is the most common reason. This removes another
dependency on maxfd.

7 years agoMINOR: polling: make epoll and kqueue not depend on maxfd anymore
Willy Tarreau [Mon, 29 Jan 2018 13:58:02 +0000 (14:58 +0100)] 
MINOR: polling: make epoll and kqueue not depend on maxfd anymore

Maxfd is really only useful to poll() and select(), yet epoll and
kqueue reference it almost by mistake :
  - cloning of the initial FDs (maxsock should be used here)
  - max polled events, it's maxpollevents which should be used here.

Let's fix these places.

7 years agoBUG/MINOR: cli: use global.maxsock and not maxfd to list all FDs
Willy Tarreau [Mon, 29 Jan 2018 14:17:05 +0000 (15:17 +0100)] 
BUG/MINOR: cli: use global.maxsock and not maxfd to list all FDs

The "show fd" command on the CLI doesn't list the last FD in use since
it doesn't include maxfd. We don't need to use maxfd here anyway as
global.maxsock will do the job pretty well and removes this dependency.
This patch may be backported to 1.8.

7 years agoMINOR: config: Enable tracking of up to MAX_SESS_STKCTR stick counters.
Frédéric Lécaille [Mon, 29 Jan 2018 11:05:07 +0000 (12:05 +0100)] 
MINOR: config: Enable tracking of up to MAX_SESS_STKCTR stick counters.

This patch really adds support for up to MAX_SESS_STKCTR stick counters.

7 years agoMEDIUM: sample: Add IPv6 support to the ipmask converter
Tim Duesterhus [Thu, 25 Jan 2018 15:24:51 +0000 (16:24 +0100)] 
MEDIUM: sample: Add IPv6 support to the ipmask converter

Add an optional second parameter to the ipmask converter that specifies
the number of bits to mask off IPv6 addresses.

If the second parameter is not given IPv6 addresses fail to mask (resulting
in an empty string), preserving backwards compatibility: Previously
a sample like `src,ipmask(24)` failed to give a result for IPv6 addresses.

This feature can be tested like this:

  defaults
   log global
   mode http
   option httplog
   option dontlognull
   timeout connect 5000
   timeout client  50000
   timeout server  50000

  frontend fe
   bind :::8080 v4v6

   # Masked IPv4 for IPv4, empty for IPv6 (with and without this commit)
   http-response set-header Test %[src,ipmask(24)]
   # Correctly masked IP addresses for both IPv4 and IPv6
   http-response set-header Test2 %[src,ipmask(24,ffff:ffff:ffff:ffff::)]
   # Correctly masked IP addresses for both IPv4 and IPv6
   http-response set-header Test3 %[src,ipmask(24,64)]

   default_backend be

  backend be
   server s example.com:80

Tested-By: Jarno Huuskonen <jarno.huuskonen@uef.fi>
7 years agoMINOR: config: Add support for ARGT_MSK6
Tim Duesterhus [Thu, 25 Jan 2018 15:24:50 +0000 (16:24 +0100)] 
MINOR: config: Add support for ARGT_MSK6

This commit adds support for ARGT_MSK6 to make_arg_list().

7 years agoMINOR: standard: Add str2mask6 function
Tim Duesterhus [Thu, 25 Jan 2018 15:24:49 +0000 (16:24 +0100)] 
MINOR: standard: Add str2mask6 function

This new function mirrors the str2mask() function for IPv4 addresses.

This commit is in preparation to support ARGT_MSK6.

7 years agoCLEANUP: standard: Use len2mask4 in str2mask
Tim Duesterhus [Thu, 25 Jan 2018 15:24:48 +0000 (16:24 +0100)] 
CLEANUP: standard: Use len2mask4 in str2mask

The len2mask4 function was introduced in commit:
70473a5f8c56d8ec2e837b9b66443dc252b24da9
which is about six years later than the commit that introduced the
str2mask function:
2937c0dd20f2f3c0065b671bbfe3fafcd8862eaf

This is a clean up in preparation for a str2mask6 function which
will use len2mask6.

7 years agoCLEANUP: Fix typo in ARGT_MSK6 comment
Tim Duesterhus [Thu, 25 Jan 2018 15:24:47 +0000 (16:24 +0100)] 
CLEANUP: Fix typo in ARGT_MSK6 comment

The incorrect comment was introduced in commit:
2ac5718dbd4ec722ece228e9f613d2be74eee9da

v1.5-dev9 is the first tag containing this comment, the fix
should be backported to haproxy 1.5 and newer.

7 years agoBUG/MINOR: sample: Fix output type of c_ipv62ip
Tim Duesterhus [Thu, 25 Jan 2018 15:24:46 +0000 (16:24 +0100)] 
BUG/MINOR: sample: Fix output type of c_ipv62ip

c_ipv62ip failed to set the output type of the cast to SMP_T_IPV4
even for a successful conversion.

This bug exists as of commit cc4d1716a2e72516c2505a6459a9ddbbfb186da2
which is the first commit adding this function.

v1.6-dev4 is the first tag containing this commit, the fix should
be backported to haproxy 1.6 and newer.

7 years agoCLEANUP: sample: Fix outdated comment about sample casts functions
Tim Duesterhus [Thu, 25 Jan 2018 15:24:45 +0000 (16:24 +0100)] 
CLEANUP: sample: Fix outdated comment about sample casts functions

The cast functions modify their output type as of commit:
b805f71d1bb1487f01f78a6ffab26d44919e9944

v1.5-dev20 is the first tag containing this comment, the fix
should be backported to haproxy 1.5 and newer.

7 years agoCLEANUP: sample: Fix comment encoding of sample.c
Tim Duesterhus [Thu, 25 Jan 2018 15:24:44 +0000 (16:24 +0100)] 
CLEANUP: sample: Fix comment encoding of sample.c

The file contained an 'e' with an gravis accent and thus was
not US-ASCII, but ISO-8859-1.

Also correct the spelling in the incorrect comment.

The incorrect character was introduced in commit:
4d9a1d1a5c4720a169654ee47f9a4364261ffab4

v1.6-dev1 is the first tag containing this comment, the fix
should be backported to haproxy 1.6 and newer.

7 years agoBUILD: kqueue/threads: Add test on MAX_THREADS to avoid warnings when complied withou...
Christopher Faulet [Thu, 25 Jan 2018 15:40:35 +0000 (16:40 +0100)] 
BUILD: kqueue/threads: Add test on MAX_THREADS to avoid warnings when complied without threads

This is the same patch than the previous one ("BUILD: epoll/threads: Add test on
MAX_THREADS to avoid warnings when complied without threads ").

It should be backported in 1.8 with the commit 7a2364d4 ("BUG/MEDIUM:
kqueue/threads: use one kqueue_fd per thread").

7 years agoBUILD: epoll/threads: Add test on MAX_THREADS to avoid warnings when complied without...
Christopher Faulet [Thu, 25 Jan 2018 15:18:09 +0000 (16:18 +0100)] 
BUILD: epoll/threads: Add test on MAX_THREADS to avoid warnings when complied without threads

When HAProxy is complied without threads, gcc throws following warnings:

  src/ev_epoll.c:222:3: warning: array subscript is outside array bounds [-Warray-bounds]
  ...
  src/ev_epoll.c:199:11: warning: array subscript is outside array bounds [-Warray-bounds]
  ...

Of course, this is not a bug. In such case, tid is always equal to 0. But to
avoid the noise, a check on MAX_THREADS in "if (tid)" lines makes gcc happy.

This patch should be backported in 1.8 with the commit d9e7e36c ("BUG/MEDIUM:
epoll/threads: use one epoll_fd per thread").

7 years agoMINOR: threads: Use __decl_hathreads instead of #ifdef/#endif
Christopher Faulet [Thu, 25 Jan 2018 15:10:16 +0000 (16:10 +0100)] 
MINOR: threads: Use __decl_hathreads instead of #ifdef/#endif

A #ifdef/#endif on USE_THREAD was added in the commit 0048dd04 ("MINOR: threads:
Fix build when we're not compiling with threads.") to conditionally define the
start_lock variable, because HA_SPINLOCK_T is only defined when HAProxy is
compiled with threads.

If fact, to do that, we should use the macro __decl_hathreads instead.

If commit 0048dd04 is backported in 1.8, this one can also be backported.

7 years agoBUG/MINOR: kqueue/threads: Don't forget to close kqueue_fd[tid] on each thread
Christopher Faulet [Thu, 25 Jan 2018 15:32:18 +0000 (16:32 +0100)] 
BUG/MINOR: kqueue/threads: Don't forget to close kqueue_fd[tid] on each thread

in deinit_kqueue_per_thread, kqueue_fd[tid] must be closed, except for the main
thread (the first one, tid==0).

This patch must be backported in 1.8 with commit 7a2364d4.

7 years agoBUG/MEDIUM: checks: Don't try to release undefined conn_stream when a check is freed
Christopher Faulet [Thu, 25 Jan 2018 10:36:35 +0000 (11:36 +0100)] 
BUG/MEDIUM: checks: Don't try to release undefined conn_stream when a check is freed

When a healt-check is released, the attached conn_stream may be undefined. For
instance, this happens when 'no-check' option is used on a server line. So we
must check it is defined before trying to release it.

This patch must be backported in 1.8.

7 years agoBUG/MEDIUM: threads/server: Fix deadlock in srv_set_stopping/srv_set_admin_flag
Christopher Faulet [Wed, 24 Jan 2018 20:49:41 +0000 (21:49 +0100)] 
BUG/MEDIUM: threads/server: Fix deadlock in srv_set_stopping/srv_set_admin_flag

Because of a typo (HA_SPIN_LOCK instead of HA_SPIN_UNLOCK), there is a deadlock
in srv_set_stopping and srv_set_admin_flag when there is at least one trackers.

This patch must be backported in 1.8.

7 years agoBUG/MINOR: threads: always set an owner to the thread_sync pipe
Willy Tarreau [Thu, 25 Jan 2018 06:28:37 +0000 (07:28 +0100)] 
BUG/MINOR: threads: always set an owner to the thread_sync pipe

The owner of the fd used by the synchronization pipe was set to NULL,
making it ignored by maxfd computation. The risk would be that some
synchronization events get delayed between threads when using poll()
or select(). However this is only theorical since the pipe is created
before listeners are bound so normally its FD should be lower and
this should normally not happen. The only possible situation would
be if all listeners are bound to inherited FDs which are lower than
the pipe's.

This patch must be backported to 1.8.

7 years agoMINOR: threads: Fix build when we're not compiling with threads.
Olivier Houchard [Wed, 24 Jan 2018 14:41:04 +0000 (15:41 +0100)] 
MINOR: threads: Fix build when we're not compiling with threads.

Only declare the start_lock if threads are compiled in, otherwise
HA_SPINLOCK_T won't be defined.
This should be backported to 1.8 when/if
1605c7ae6154d8c2cfcf3b325872b1a7266c5bc2 is backported.

7 years agoBUG/MINOR: mworker: only write to pidfile if it exists
Willy Tarreau [Tue, 23 Jan 2018 18:20:19 +0000 (19:20 +0100)] 
BUG/MINOR: mworker: only write to pidfile if it exists

A missing test causes a write(-1, $PID) to appear in strace output when
in master-worker mode. This is totally harmless though.

This fix must be backported to 1.8.

7 years agoBUG/MEDIUM: threads/mworker: fix a race on startup
Willy Tarreau [Tue, 23 Jan 2018 18:01:49 +0000 (19:01 +0100)] 
BUG/MEDIUM: threads/mworker: fix a race on startup

Marc Fournier reported an interesting case when using threads with the
master-worker mode : sometimes, a listener would have its FD closed
during startup. Sometimes it could even be health checks seeing this.

What happens is that after the threads are created, and the pollers
enabled on each threads, the master-worker pipe is registered, and at
the same time a close() is performed on the write side of this pipe
since the children must not use it.

But since this is replicated in every thread, what happens is that the
first thread closes the pipe, thus releases the FD, and the next thread
starting a listener in parallel gets this FD reassigned. Then another
thread closes the FD again, which this time corresponds to the listener.
It can also happen with the health check sockets if they're started
early enough.

This patch splits the mworker_pipe_register() function in two, so that
the close() of the write side of the FD is performed very early after the
fork() and long before threads are created (we don't need to delay it
anyway). Only the pipe registration is done in the threaded code since
it is important that the pollers are properly allocated for this.
The mworker_pipe_register() function now takes care of registering the
pipe only once, and this is guaranteed by a new surrounding lock.

The call to protocol_enable_all() looks fragile in theory since it
scans the list of proxies and their listeners, though in practice
all threads scan the same list and take the same locks for each
listener so it's not possible that any of them escapes the process
and finishes before all listeners are started. And the operation is
idempotent.

This fix must be backported to 1.8. Thanks to Marc for providing very
detailed traces clearly showing the problem.

7 years agoBUG/MEDIUM: kqueue/threads: use one kqueue_fd per thread
Willy Tarreau [Fri, 19 Jan 2018 07:56:14 +0000 (08:56 +0100)] 
BUG/MEDIUM: kqueue/threads: use one kqueue_fd per thread

This is the same principle as the previous patch (BUG/MEDIUM:
epoll/threads: use one epoll_fd per thread) except that this time it's
for kqueue. We don't want all threads to wake up because of activity on
a single other thread that the other ones are not interested in.

Just like with previous patch, this one shows that the polling state
doesn't need to be changed here and that some simplifications are now
possible. This patch only implements the minimum required for a stable
backport.

This should be backported to 1.8.

7 years agoBUG/MEDIUM: epoll/threads: use one epoll_fd per thread
Willy Tarreau [Thu, 18 Jan 2018 18:16:02 +0000 (19:16 +0100)] 
BUG/MEDIUM: epoll/threads: use one epoll_fd per thread

There currently is a problem regarding epoll(). While select() and poll()
compute their polling state on the fly upon each call, epoll() keeps a
shared state between all threads via the epoll_fd. The problem is that
once an fd is registered on *any* thread, all other threads receive
events for that FD as well. It is clearly visible when binding a listener
to a single thread like in the configuration below where all 4 threads
will work, 3 of them simply spinning to skip the event :

    global
        nbthread 4

    frontend foo
        bind :1234 process 1/1

The worst case happens when some slow operations are in progress on a
busy thread, preventing it from processing its task and causing the
other ones to wake up not being able to do anything with this event.
Typically computing a large TLS key will delay processing of next
events on the same thread while others will still wake up.

All this simply shows that the poller must remain thread-specific, with
its own events and its own ability to sleep when it doesn't have anyhing
to do.

This patch does exactly this. For this, it proceeds like this :

   - have one epoll_fd per thread instead of one per process
   - initialize these epoll_fd when threads are created.
   - mark all known FDs as updated so that the next invocation of
     _do_poll() recomputes their polling status (including a possible
     removal of undesired polling from the original FD) ;
   - use each fd's polled_mask to maintain an accurate status of
     the current polling activity for this FD.
   - when scanning updates, only focus on events whose new polling
     status differs from the existing one
   - during updates, always verify the thread_mask to resist migration
   - on __fd_clo(), for cloned FDs (typically listeners inherited
     from the parent during a graceful shutdown), run epoll_ctl(DEL)
     on all epoll_fd. This is the reason why epoll_fd is stored in a
     shared array and not in a thread_local storage. Note: maybe this
     can be moved to an update instead.

Interestingly, this shows that we don't need the FD's old state anymore
and that we only use it to convert it to the new state based on stable
information. It appears clearly that the FD code can be further improved
by computing the final state directly when manipulating it.

With this change, the config above goes from 22000 cps at 380% CPU to
43000 cps at 100% CPU : not only the 3 unused threads are not activated,
but they do not disturb the activity anymore.

The output of "show activity" before and after the patch on a 4-thread
config where a first listener on thread 2 forwards over SSL to threads
3 & 4 shows this a much smaller amount of undesired events (thread 1
doesn't wake up anymore, poll_skip remains zero, fd_skip stays low) :

  // before: 400% CPU, 7700 cps, 13 seconds
  loops: 11380717 65879 5733468 5728129
  wake_cache: 0 63986 317547 314174
  wake_tasks: 0 0 0 0
  wake_applets: 0 0 0 0
  wake_signal: 0 0 0 0
  poll_exp: 0 63986 317547 314174
  poll_drop: 1 0 49981 48893
  poll_dead: 65514 0 31334 31934
  poll_skip: 46293690 34071 22867786 22858208
  fd_skip: 66068135 174157 33732685 33825727
  fd_lock: 0 2 2809 2905
  fd_del: 0 494361 80890 79464
  conn_dead: 0 0 0 0
  stream: 0 407747 50526 49474
  empty_rq: 11380718 1914 5683023 5678715
  long_rq: 0 0 0 0

  // after: 200% cpu, 9450 cps, 11 seconds
  loops: 17 66147 1001631 450968
  wake_cache: 0 66119 865139 321227
  wake_tasks: 0 0 0 0
  wake_applets: 0 0 0 0
  wake_signal: 0 0 0 0
  poll_exp: 0 66119 865139 321227
  poll_drop: 6 5 38279 60768
  poll_dead: 0 0 0 0
  poll_skip: 0 0 0 0
  fd_skip: 54 172661 4411407 2008198
  fd_lock: 0 0 10890 5394
  fd_del: 0 492829 58965 105091
  conn_dead: 0 0 0 0
  stream: 0 406223 38663 61338
  empty_rq: 18 40 962999 390549
  long_rq: 0 0 0 0

This patch presents a few risks but fixes a real problem with threads,
and as such it needs be backported to 1.8. It depends on previous patch
("MINOR: fd: add a bitmask to indicate that an FD is known by the poller").

Special thanks go to Samuel Reed for providing a large amount of useful
debugging information and for testing fixes.

7 years agoMINOR: fd: add a bitmask to indicate that an FD is known by the poller
Willy Tarreau [Wed, 17 Jan 2018 17:44:46 +0000 (18:44 +0100)] 
MINOR: fd: add a bitmask to indicate that an FD is known by the poller

Some pollers like epoll() need to know if the fd is already known or
not in order to compute the operation to perform (add, mod, del). For
now this is performed based on the difference between the previous FD
state and the new state but this will not be usable anymore once threads
become responsible for their own polling.

Here we come with a different approach : a bitmask is stored with the
fd to indicate which pollers already know it, and the pollers will be
able to simply perform the add/mod/del operations based on this bit
combined with the new state.

This patch only adds the bitmask declaration and initialization, it
is it not yet used. It will be needed by the next two fixes and will
need to be backported to 1.8.

7 years agoBUG/MEDIUM: fd: maintain a per-thread update mask
Willy Tarreau [Sat, 20 Jan 2018 22:53:50 +0000 (23:53 +0100)] 
BUG/MEDIUM: fd: maintain a per-thread update mask

Since the fd update tables are per-thread, we need to have a bit per
thread to indicate whether an update exists, otherwise this can lead
to lost update events every time multiple threads want to update the
same FD. In practice *for now*, it only happens at start time when
listeners are enabled and ask for polling after facing their first
EAGAIN. But since the pollers are still shared, a lost event is still
recovered by a neighbor thread. This will not reliably work anymore
with per-thread pollers, where it has been observed a few times on
startup that a single-threaded listener would not always accept
incoming connections upon startup.

It's worth noting that during this code review it appeared that the
"new" flag in the fdtab isn't used anymore.

This fix should be backported to 1.8.

7 years agoBUG/MEDIUM: threads/polling: Use fd_cache_mask instead of fd_cache_num
Christopher Faulet [Mon, 15 Jan 2018 11:16:34 +0000 (12:16 +0100)] 
BUG/MEDIUM: threads/polling: Use fd_cache_mask instead of fd_cache_num

fd_cache_num is the number of FDs in the FD cache. It is a global variable. So
it is underoptimized because we may be lead to consider there are waiting FDs
for the current thread in the FD cache while in fact all FDs are assigned to the
other threads. So, in such cases, the polling loop will be evaluated many more
times than necessary.

Instead, we now check if the thread id is set in the bitfield fd_cache_mask.

[wt: it's not exactly a bug, rather a design limitation of the thread
 which was not addressed in time for the 1.8 release. It can appear more
 often than we initially predicted, when more threads are running than
 the number of assigned CPU cores, or when certain threads spend
 milliseconds computing crypto keys while other threads spin on
 epoll_wait(0)=0]

This patch should be backported to 1.8.

7 years agoMINOR: threads/fd: Use a bitfield to know if there are FDs for a thread in the FD...
Christopher Faulet [Mon, 15 Jan 2018 10:57:03 +0000 (11:57 +0100)] 
MINOR: threads/fd: Use a bitfield to know if there are FDs for a thread in the FD cache

A bitfield has been added to know if there are some FDs processable by a
specific thread in the FD cache. When a FD is inserted in the FD cache, the bits
corresponding to its thread_mask are set. On each thread, the bitfield is
updated when the FD cache is processed. If there is no FD processed, the thread
is removed from the bitfield by unsetting its tid_bit.

Note that this bitfield is updated but not checked in
fd_process_cached_events. So, when this function is called, the FDs cache is
always processed.

[wt: should be backported to 1.8 as it will help fix a design limitation]

7 years agoMINOR: global: add some global activity counters to help debugging
Willy Tarreau [Sat, 20 Jan 2018 18:30:13 +0000 (19:30 +0100)] 
MINOR: global: add some global activity counters to help debugging

A number of counters have been added at special places helping better
understanding certain bug reports. These counters are maintained per
thread and are shown using "show activity" on the CLI. The "clear
counters" commands also reset these counters. The output is sent as a
single write(), which currently produces up to about 7 kB of data for
64 threads. If more counters are added, it may be necessary to write
into multiple buffers, or to reset the counters.

To backport to 1.8 to help collect more detailed bug reports.

7 years agoMINOR: threads: add a MAX_THREADS define instead of LONGBITS
Willy Tarreau [Sat, 20 Jan 2018 17:19:22 +0000 (18:19 +0100)] 
MINOR: threads: add a MAX_THREADS define instead of LONGBITS

This one allows not to inflate some structures when threads are
disabled. Now struct global is 1.4 kB instead of 33 kB.

Should be backported to 1.8 for ease of backporting of upcoming
patches.

7 years agoMINOR: global/threads: move cpu_map at the end of the global struct
Willy Tarreau [Sat, 20 Jan 2018 17:12:15 +0000 (18:12 +0100)] 
MINOR: global/threads: move cpu_map at the end of the global struct

The "thread" part is 32kB long, better move it at the end of the
structure since it's only used during initialization, to keep the
rest grouped together.

Should be backported to 1.8 to ease backporting of upcoming patches,
no functional impact.

7 years agoMINOR: servers: Don't report duplicate dyncookies for disabled servers.
Olivier Houchard [Wed, 17 Jan 2018 16:39:34 +0000 (17:39 +0100)] 
MINOR: servers: Don't report duplicate dyncookies for disabled servers.

Especially with server-templates, it can happen servers starts with a
placeholder IP, in the disabled state. In this case, we don't want to report
that the same cookie was generated for multiple servers. So defer the test
until the server is enabled.

This should be backported to 1.8.

7 years agoBUG/MEDIUM: peers: fix expire date wasn't updated if entry is modified remotely.
Emeric Brun [Mon, 22 Jan 2018 14:10:08 +0000 (15:10 +0100)] 
BUG/MEDIUM: peers: fix expire date wasn't updated if entry is modified remotely.

The stktable_touch_remote considers the expire field stored in the stksess
struct.
The expire field was updated on the a newly created stksess to store.

But if the stksess with a same key is still present the expire was not updated.

This patch postpones the update of the expire field of the stksess just before
processing the "touch".

These bug was introduced in commit:

MEDIUM: threads/stick-tables: handle multithreads on stick tables.

And the fix should be backported on 1.8.

7 years agoMINOR: sample: add date_us sample
Etienne Carriere [Wed, 17 Jan 2018 12:43:24 +0000 (13:43 +0100)] 
MINOR: sample: add date_us sample

Add date_us sample that returns the microsecond part of the timeval
structure representing the date of the structure. The "second" part of
the timeval can already be fetched by the "date" sample

7 years agoBUG/MINOR: poll: too large size allocation for FD events
Willy Tarreau [Wed, 17 Jan 2018 14:48:53 +0000 (15:48 +0100)] 
BUG/MINOR: poll: too large size allocation for FD events

Commit 80da05a ("MEDIUM: poll: do not use FD_* macros anymore") which
appeared in 1.5-dev18 and which was backported to 1.4.23 made explicit
use of arrays of FDs mapped to unsigned ints. The problem lies in the
allocated size for poll(), as the resulting size is in bits and not
bytes, resulting in poll() arrays being 8 times larger than necessary!

In practice poll() is not used on highly loaded systems, explaining why
nobody noticed. But it definetely has to be addressed.

This fix needs to be backported to all stable versions.

7 years agoCONTRIB: debug: fix a few flags definitions
Willy Tarreau [Mon, 15 Jan 2018 17:59:16 +0000 (18:59 +0100)] 
CONTRIB: debug: fix a few flags definitions

Commit f4cfcf9 ("MINOR: debug/flags: Add missing flags") added a number
of missing flags but a few of them were incorrect, hiding real values.
This can be backported to 1.8.

7 years agoDOC: clarify the scope of ssl_fc_is_resumed
Jérôme Magnin [Mon, 15 Jan 2018 13:01:17 +0000 (14:01 +0100)] 
DOC: clarify the scope of ssl_fc_is_resumed

Clarify that it's for incoming connections.

7 years agoMINOR: spoe: Don't queue a SPOE context if nothing is sent
Christopher Faulet [Fri, 12 Jan 2018 09:45:47 +0000 (10:45 +0100)] 
MINOR: spoe: Don't queue a SPOE context if nothing is sent

When some messages must be sent to an agent, the SPOE context of the stream is
queued to be handled by an SPOE applet. If there is no available applet, a new
one is created, thus opening a connection with the agent.

Since the support of ACLs on messages, some processing can now be discarded. So,
to avoid opening a connection for nothing, the SPOE context is now queued after
the messages encoding.

7 years agoMINOR: spoe: add register-var-names directive in spoe-agent configuration
Christopher Faulet [Fri, 22 Dec 2017 09:00:55 +0000 (10:00 +0100)] 
MINOR: spoe: add register-var-names directive in spoe-agent configuration

In addition to "option force-set-var", recently added, this directive can be
used to selectivelly register unknown variable names, without totally relaxing
their registration during the runtime, like "option force-set-var" does.

So there is no way for a malicious agent to exhaust memory by defining a too
high number of variable names. In other hand, you need to enumerate all
variable names. This could be painfull in some circumstances.

Remember, this directive is only usefull when the variable names are not
referenced anywhere in the HAProxy configuration or the SPOE one.

Thanks to Etienne Carrière for his help on this part.

7 years agoBUG/MEDIUM: stream: properly handle client aborts during redispatch
Willy Tarreau [Fri, 12 Jan 2018 09:42:12 +0000 (10:42 +0100)] 
BUG/MEDIUM: stream: properly handle client aborts during redispatch

James Mc Bride reported an interesting case affecting all versions since
at least 1.5 : if a client aborts a connection on an empty buffer at the
exact moment a server redispatch happens, the CF_SHUTW_NOW flag on the
channel is immediately turned into CF_SHUTW, which is not caught by
check_req_may_abort(), leading the redispatch to be performed anyway
with the channel marked as shut in both directions while the stream
interface correctly establishes. This situation makes no sense.
Ultimately the transfer times out and the server-side stream interface
remains in EST state while the client is in CLO state, and this case
doesn't correspond to anything we can handle in process_stream, leading
to poll() being woken up all the time without any progress being made.
And the session cannot even be killed from the CLI.

So we must ensure that check_req_may_abort() also considers the case
where the channel is already closed, which is what this patch does.
Thanks to James for providing detailed captures allowing to diagnose
the problem.

This fix must be backported to all maintained versions.

7 years agoBUILD/MINOR: ancient gcc versions atomic fix
David Carlier [Thu, 11 Jan 2018 14:20:43 +0000 (14:20 +0000)] 
BUILD/MINOR: ancient gcc versions atomic fix

Commit 1a69af6d3892fe1946bb8babb3044d2d26afd46e introduced code
for atomic prior to 4.7. Unfortunately clang uses as well those
constants which is misleading.

7 years agoMINOR: hathreads: add support for gcc < 4.7
Willy Tarreau [Thu, 4 Jan 2018 17:49:31 +0000 (18:49 +0100)] 
MINOR: hathreads: add support for gcc < 4.7

Till now the use of __atomic_* gcc builtins required gcc >= 4.7. Since
some supported and quite common operating systems like CentOS 6 still
come with older versions (4.4) and the mapping to the older builtins
is reasonably simple, let's implement it.

This code is only used for gcc < 4.7. It has been quickly tested on a
machine using gcc 4.4.4 and provided expected results.

This patch should be backported to 1.8.

7 years agoBUG/MEDIUM: mworker: execvp failure depending on argv[0]
William Lallemand [Tue, 9 Jan 2018 22:12:27 +0000 (23:12 +0100)] 
BUG/MEDIUM: mworker: execvp failure depending on argv[0]

The copy_argv() function lacks a check on '-' to remove the -x, -sf and
-st parameters.

When reloading a master process with a path starting by /st, /sf, or
/x..  the copy_argv() function skipped argv[0] leading to an execvp()
without the binary.

7 years agoMINOR: dns: Handle SRV record weight correctly.
Olivier Houchard [Mon, 8 Jan 2018 15:28:57 +0000 (16:28 +0100)] 
MINOR: dns: Handle SRV record weight correctly.

A SRV record weight can range from 0 to 65535, while haproxy weight goes
from 0 to 256, so we have to divide it by 256 before handing it to haproxy.
Also, a SRV record with a weight of 0 doesn't mean the server shouldn't be
used, so use a minimum weight of 1.

This should probably be backported to 1.8.

7 years agoBUG/MINOR: lua: Fix return value of Socket.settimeout
Tim Duesterhus [Sat, 6 Jan 2018 18:16:25 +0000 (19:16 +0100)] 
BUG/MINOR: lua: Fix return value of Socket.settimeout

The `socket.tcp.settimeout` method of Lua returns `1` in all cases,
while the `Socket.settimeout` method of haproxy returns `0` in all
cases. This breaks the `socket.http` module, because it validates
the return value of `settimeout`.

This bug was introduced in commit 7e7ac32dad1e15c19152d37aaf9ea6b3f00a7226
(which is the very first commit adding the Socket class to Lua). This
bugfix should be backported to every branch containing that commit:
- 1.6
- 1.7
- 1.8

A test case for this bug is as follows:

The 'Test' response header will contain an HTTP status code with the
patch applied and will be zero (nil) without the patch applied.

http.lua:
  http = require("socket.http")

  core.register_action("bug", { "http-req" }, function(txn)
   local b, c, h = http.request {
   url = "http://93.184.216.34",
   headers = {
   Host = "example.com"
   },
   create = core.tcp,
   redirect = false
   }

   txn:set_var("txn.foo", c)
  end)

haproxy.cfg:
  global
   lua-load /scratch/haproxy/http.lua

  frontend fe
   bind 127.0.0.1:8080
   http-request lua.bug
   http-response set-header Test %[var(txn.foo)]

   default_backend be

  backend be
   server s example.com:80

7 years agoBUG/MEDIUM: lua: Fix IPv6 with separate port support for Socket.connect
Tim Duesterhus [Sat, 6 Jan 2018 18:04:45 +0000 (19:04 +0100)] 
BUG/MEDIUM: lua: Fix IPv6 with separate port support for Socket.connect

The `socket.tcp.connect` method of Lua requires at least two parameters:
The host and the port. The `Socket.connect` method of haproxy requires
only one when a host with a combined port is provided. This stems from
the fact that `str2sa_range` is used internally in `hlua_socket_connect`.
This very fact unfortunately causes a diversion in the behaviour of
Lua's socket class and haproxy's for IPv6 addresses:

  sock:connect("::1", "80")

works fine with Lua, but fails with:

  connect: cannot parse destination address '::1'

in haproxy, because `str2sa_range` parses the trailing `:1` as the port.

This patch forcefully adds a `:` to the end of the address iff a port
number greater than `0` is given as the second parameter.

Technically this breaks backwards compatibility, because the docs state:

> The syntax "127.0.0.1:1234" is valid. in this case, the
> parameter *port* is ignored.

But: The connect() call can only succeed if the second parameter is left
out (which causes no breakage) or if the second parameter is an integer
or a numeric string.

It seems unlikely that someone would provide an address with a port number
and would also provide a second parameter containing a number other than
zero. Thus I feel this breakage is warranted to fix the mismatch between
haproxy's socket class and Lua's one.

This commit should be backported to haproxy 1.8 only, because of the
possible breakage of existing Lua scripts.

7 years agoDOC: lua: Fix typos in comments of hlua_socket_receive
Tim Duesterhus [Thu, 4 Jan 2018 18:32:14 +0000 (19:32 +0100)] 
DOC: lua: Fix typos in comments of hlua_socket_receive

7 years agoBUG/MINOR: lua: Fix default value for pattern in Socket.receive
Tim Duesterhus [Thu, 4 Jan 2018 18:32:13 +0000 (19:32 +0100)] 
BUG/MINOR: lua: Fix default value for pattern in Socket.receive

The default value of the pattern in `Socket.receive` is `*l` according
to the documentation and in the `socket.tcp.receive` method of Lua.

The default value of `wanted` in `int hlua_socket_receive(struct lua_State *)`
reflects this requirement, but the function fails to ensure this
nonetheless:

If no parameter is given the top of the Lua stack will have the index 1.
`lua_pushinteger(L, wanted);` then pushes the default value onto the stack
(with index 2).
The following `lua_replace(L, 2);` then pops the top index (2) and tries to
replace the index 2 with it.
I am not sure why exactly that happens (possibly, because one cannot replace
non-existent stack indicies), but this causes the stack index to be lost.

`hlua_socket_receive_yield` then tries to read the stack index 2, to
determine what to read and get the value `0`, instead of the correct
HLSR_READ_LINE, thus taking the wrong branch.

Fix this by ensuring that the top of the stack is not replaced by itself.

This bug was introduced in commit 7e7ac32dad1e15c19152d37aaf9ea6b3f00a7226
(which is the very first commit adding the Socket class to Lua). This
bugfix should be backported to every branch containing that commit:
- 1.6
- 1.7
- 1.8

A test case for this bug is as follows:

The 'Test' response header will contain an HTTP status line with the
patch applied and will be empty without the patch applied. Replacing
the `sock:receive()` with `sock:receive("*l")` will cause the status
line to appear with and without the patch

http.lua:
  core.register_action("bug", { "http-req" }, function(txn)
   local sock = core.tcp()
   sock:settimeout(60)
   sock:connect("127.0.0.1:80")
   sock:send("GET / HTTP/1.0\r\n\r\n")
   response = sock:receive()
   sock:close()
   txn:set_var("txn.foo", response)
  end)

haproxy.cfg (bits omitted for brevity):
  global
   lua-load /scratch/haproxy/http.lua

  frontend fe
   bind 127.0.0.1:8080
   http-request lua.bug
   http-response set-header Test %[var(txn.foo)]

   default_backend be

  backend be
   server s 127.0.0.1:80

7 years agoBUG/MEDIUM: ssl: cache doesn't release shctx blocks
William Lallemand [Wed, 3 Jan 2018 18:15:51 +0000 (19:15 +0100)] 
BUG/MEDIUM: ssl: cache doesn't release shctx blocks

Since the rework of the shctx with the hot list system, the ssl cache
was putting session inside the hot list, without removing them.
Once all block were used, they were all locked in the hot list, which
was forbiding to reuse them for new sessions.

Bug introduced by 4f45bb9 ("MEDIUM: shctx: separate ssl and shctx")

Thanks to Jeffrey J. Persch for reporting this bug.

Must be backported to 1.8.

7 years agoCLEANUP: rbtree: remove
Olivier Houchard [Thu, 4 Jan 2018 16:59:02 +0000 (17:59 +0100)] 
CLEANUP: rbtree: remove

Remove the rbtree implementation. It's not used, it's not even connected to
the build, and we probably have no use for it .

7 years agoBUILD: ssl: silence a warning when building without NPN nor ALPN support
Willy Tarreau [Thu, 4 Jan 2018 17:55:19 +0000 (18:55 +0100)] 
BUILD: ssl: silence a warning when building without NPN nor ALPN support

When building with a library not offering any of these, ssl_conf_cur
is not used.

Can be backported to 1.8.

7 years agoBUG/MEDIUM: h2: properly handle the END_STREAM flag on empty DATA frames
Willy Tarreau [Thu, 4 Jan 2018 13:41:00 +0000 (14:41 +0100)] 
BUG/MEDIUM: h2: properly handle the END_STREAM flag on empty DATA frames

Peter Lindegaard Hansen reported a problem affecting some POST requests
sent by MSIE on 1.8.3. Lukas found that we incorrectly dealt with the
END_STREAM flag on empty DATA frames.

What happens in fact is that while we correctly report that we've read a
zero-byte frame, since commit 8fc016d ("BUG/MEDIUM: h2: support uploading
partial DATA frames") backported into 1.8.2, we've been able to return
without updating the parser's state nor checking the frame flags in this
case.

The fix is trival, we just need not to return too early.

This fix must be backported to 1.8.

7 years agoMEDIUM: h2: prepare a graceful shutdown when the frontend is stopped
Willy Tarreau [Sat, 30 Dec 2017 17:08:13 +0000 (18:08 +0100)] 
MEDIUM: h2: prepare a graceful shutdown when the frontend is stopped

During a reload operation, instead of keeping the H2 connections opened
forever causing confusion during configuration changes, let's send a
graceful shutdown so that the client knows that it would better open a
new connection for future requests. We can't really catch the signal
from H2, but we can advertise this graceful shutdown upon the next I/O
event (eg: a WINDOW_UPDATE from the client or a new request). One of
the visible effect is that the old process quits much faster.

This patch should be backported to 1.8 since it is affected by this
problem.

7 years agoCONTRIB: hpack: add an hpack decoder
Willy Tarreau [Sat, 30 Dec 2017 12:18:25 +0000 (13:18 +0100)] 
CONTRIB: hpack: add an hpack decoder

This decoder takes a series of hex codes on stdin using one line
per HEADERS frame and shows the decoded headers.

7 years agoDEBUG: hpack: add more traces to the hpack decoder
Willy Tarreau [Sat, 30 Dec 2017 12:17:37 +0000 (13:17 +0100)] 
DEBUG: hpack: add more traces to the hpack decoder

These ones are only enabled when DEBUG_HPACK is defined so they have no
effect on the production code.

7 years agoDEBUG: hpack: make hpack_dht_dump() expose the output file
Willy Tarreau [Sat, 30 Dec 2017 16:08:46 +0000 (17:08 +0100)] 
DEBUG: hpack: make hpack_dht_dump() expose the output file

It's more convenient to be able to choose between stdout and stderr.

7 years agoMINOR: h2: add a function to report pseudo-header names
Willy Tarreau [Sat, 30 Dec 2017 13:39:09 +0000 (14:39 +0100)] 
MINOR: h2: add a function to report pseudo-header names

For debugging we need to be able to dump pseudo headers when we know
their name, let's put this there as we already have the other way
around.

7 years agoBUG/MAJOR: hpack: don't return direct references to the dynamic headers table
Willy Tarreau [Sat, 30 Dec 2017 15:56:28 +0000 (16:56 +0100)] 
BUG/MAJOR: hpack: don't return direct references to the dynamic headers table

Maximilian Böhm and Lucas Rolff both reported some random failed requests
with HTTP/2. Upon deep investigation on detailed traces provided by Lucas,
it turned out that some header names were occasionally corrupted and used
to point to random strings within the dynamic headers table.

The HPACK decoder must always return copies of header names that point
to the dynamic headers table. Otherwise, the insertion of a header after
the current one leading to a reorganization of the table will change the
data the pointer designates. Unfortunately, one such copy was missing for
indexed names, leading to random request failures due to invalid header
names.

Many thanks to Lucas who ran a large number of tests with full traces
helping to capture a reproduceable sequence exhibiting this issue.

This patch must be backported to 1.8.

7 years agoBUG/MEDIUM: http: don't automatically forward request close
Willy Tarreau [Fri, 29 Dec 2017 15:30:31 +0000 (16:30 +0100)] 
BUG/MEDIUM: http: don't automatically forward request close

Maximilian Böhm, and Lucas Rolff reported some frequent HTTP/2 POST
failures affecting version 1.8.2 that were not affecting 1.8.1. Lukas
Tribus determined that these ones appeared consecutive to commit a48c141
("BUG/MAJOR: connection: refine the situations where we don't send shutw()").

It turns out that the HTTP request forwarding engine lets a shutr from
the client be automatically forwarded to the server unless chunked
encoding is in use. It's a bit tricky to meet this condition as it only
happens if the shutr is not reported in the initial request. So if a
request is large enough or the body is delayed after the headers (eg:
Expect: 100-continue), the the function quits with channel_auto_close()
left enabled. The patch above was not really related in fact. It's just
that a previous bug was causing this shutw to be skipped at the lower
layers, and the two bugs used to cancel themselves.

In the HTTP request we should only pass the close in tunnel mode, as
other cases either need to keep the connection alive (eg: for reuse)
or will force-close it. Also the forced close will properly take care
of avoiding the painful time-wait, which is not possible with the early
close.

This patch must be backported to 1.8 as it directly impacts HTTP/2, and
may be backported to older version to save them from being abused by
clients causing TIME_WAITs between haproxy and the server.

Thanks to Lukas and Lucas for running many tests with captures allowing
the bug to be narrowed down.

7 years agoMINOR: don't close stdio anymore
William Lallemand [Thu, 28 Dec 2017 15:09:36 +0000 (16:09 +0100)] 
MINOR: don't close stdio anymore

Closing the standard IO FDs (0,1,2) can be troublesome, especially in
the case of the master-worker.

Instead of closing those FDs, they are now pointing to /dev/null which
prevents sending debugging messages to the wrong FDs.

This patch could be backported in 1.8.

7 years agoBUG/MEDIUM: mworker: don't close stdio several time
PiBa-NL [Mon, 25 Dec 2017 20:03:31 +0000 (21:03 +0100)] 
BUG/MEDIUM: mworker: don't close stdio several time

This patch makes sure that a frontend socket that gets created after
initialization won't be closed when the master gets re-executed.

When used in daemon mode, the master-worker is closing the FDs 0, 1, 2
after the fork of the children.

When the master was reloading, those FDs were assigned again during the
parsing of the configuration (probably for some listeners), and the
workers were closing them thinking it was the stdio.

This patch must be backported to 1.8.

7 years agoBUG/MEDIUM: h2: ensure we always know the stream before sending a reset
Willy Tarreau [Fri, 29 Dec 2017 10:34:40 +0000 (11:34 +0100)] 
BUG/MEDIUM: h2: ensure we always know the stream before sending a reset

The recent patch introducing the H2_CS_FRAME_E state to emit stream
resets was not totally correct in that in the rare case where there is
no room left to emit the reset, the next call to process it later could
use an uninitialized stream. This only affects responses to frames that
are sent on closed streams though.

This fix must be backported to 1.8.

7 years agoDOC/MINOR: configuration: typo, formatting fixes
Davor Ocelic [Mon, 25 Dec 2017 16:49:28 +0000 (17:49 +0100)] 
DOC/MINOR: configuration: typo, formatting fixes

- Add simple typo and formatting fixes
- Eliminate a couple > 80 column lines

Changes do not affect technical content and can be backported.

7 years agoBUG/MEDIUM: h2: improve handling of frames received on closed streams
Willy Tarreau [Wed, 27 Dec 2017 14:07:30 +0000 (15:07 +0100)] 
BUG/MEDIUM: h2: improve handling of frames received on closed streams

The h2spec utility found certain situations where we're returning an
RST_STREAM while a GOAWAY is expected. While we can't always reliably
decide which one to use (eg: after a stream has been closed for a long
time), in practice we often still have the stream available until it's
destroyed at the application level. This provides the flags we need to
verify the conditions that led to its closure, namely if RST was sent
or received, or if it was regularly closed using a double ES.

The first step consists in marking all closed streams as having already
sent an RST_STREAM frame. This will ensure that we can send an RST_STREAM
for a late transmission on a stream we have forgotten about instead of
risking to break the connection. The next steps consist in re-arranging
the H2_SS_CLOSED checks so that we can deliver a GOAWAY frame for the
few cases where an unexpected frame was received after a double ES.

By carefully taking care of these specificities, we can reduce by 4 the
number of remaining compliance issues.

Note: some tests start to become a bit long and to be repeated at various
places. Probably that adding a bitmask of allowed/forbidden frame types
per state and/or per situation could significantly help. It's likely
that some deeper tests in the frame handlers could also be removed now
as they can't be triggered anymore.

This fix should be backported to 1.8.

7 years agoBUG/MEDIUM: h2: properly handle and report some stream errors
Willy Tarreau [Wed, 27 Dec 2017 10:02:06 +0000 (11:02 +0100)] 
BUG/MEDIUM: h2: properly handle and report some stream errors

Some stream errors applied to half-closed and closed streams are not
properly reported, especially after the stream transistions to the
closed state. The reason is that the code checks for this "error"
stream state in order to send an RST frame. But if the stream was
just closed or was already closed, there's no way to validate this
condition, and the error is never reported to the peer.

In order to address this situation, we'll add a new FRAME_E demux state
which indicates that the previously parsed frame triggered a stream error
of type STREAM CLOSED that needs to be reported. Proceeding like this
will ensure that we don't lose that information even if we can't
immediately send the message. It also removes the confusion where FRAME_A
could be used either for ACKs or for RST.

The state transition has been added after every h2s_error() on the demux
path. It seems that we might need to have two distinct h2s_error()
functions, one for the mux and another one for the demux, though it
would provide little benefit. It also becomes more apparent that the
H2_SS_ERROR state is only used to detect the need to report an error
on the mux direction. Maybe this will have to be revisited later.

This simple change managed to eliminate 5 bugs reported by h2spec.

This fix must be backported to 1.8.

7 years agoBUG/MEDIUM: checks: properly set servers to stopping state on 404
Willy Tarreau [Sat, 23 Dec 2017 10:16:49 +0000 (11:16 +0100)] 
BUG/MEDIUM: checks: properly set servers to stopping state on 404

Paul Lockaby reported that since 1.8, disable-on-404 doesn't work
anymore in that the server stay up despite returning 404. Cyril spotted
that this was caused by a copy-paste error introduced by commit 5a13351
("BUG/MEDIUM: log: check result details truncated.") causing
set_server_running() to be called instead of set_server_stopping() in
this case.

It can be reproduced with the simple test config below :

  defaults
     mode http
     timeout connect 1s
     timeout client  10s
     timeout server  10s

  listen http
     bind :8888
     option httpchk GET /
     http-check disable-on-404
     server s1 127.0.0.1:9001 check
     server s2 127.0.0.1:9002 check
     http-response add-header x-served-by %s

  listen s1
     bind :9001
     server next 127.0.0.1:9002
     http-response set-status 404

  frontend s2
     bind :9002
     http-request redirect location /

S1 is supposed to be stopping and s2 up, which is not the case. After
calling the correct function, only S2 is used now.

This needs to be backported to 1.8.

7 years agoBUG/MAJOR: connection: refine the situations where we don't send shutw()
Willy Tarreau [Fri, 22 Dec 2017 17:46:33 +0000 (18:46 +0100)] 
BUG/MAJOR: connection: refine the situations where we don't send shutw()

Since commit f9ce57e ("MEDIUM: connection: make conn_sock_shutw() aware
of lingering"), we refrain from performing the shutw() on the socket if
there is no lingering risk. But there is a problem with this in tunnel
and in TCP modes where a client is explicitly allowed to send a shutw
to the server, eventhough it it risky.

Not doing it creates this situation reported by Ricardo Fraile and
diagnosed by Christopher : a typical HTTP client (eg: curl) connecting
via the config below to an HTTP server would receive its response,
immediately close while the server remains in keep-alive mode. The
shutr() received by haproxy from the client is "propagated" to the
server side but not acted upon because fdtab[fd].linger_risk is set,
so we expect that the next close will immediately complete this
operation.

  listen proxy-tcp
    bind 127.0.0.1:8888
    mode tcp
    timeout connect 5s
    timeout server  10s
    timeout client  10s
    server server1 127.0.0.1:8000

But since the whole stream will not end until the server closes in
turn, the server doesn't close and haproxy expires on server timeout.
This problem has already struck by waking up an older bug and was
partially fixed with commit 8059351 ("BUG/MEDIUM: http: don't disable
lingering on requests with tunnelled responses") though it was not
enough.

The problem is that linger_risk is not suited here. In fact we need to
know whether or not it is desired to close normally or silently, and
whether or not a shutr() has already been received on this connection.

This is the approach this patch takes, and it solves the problem for
the various difficult modes (tcp, http-server-close, pretend-keepalive).

This fix needs to be backported to 1.8. Many thanks to Ricardo for
providing very detailed traces and configurations.

7 years agoBUG/MEDIUM: cache: don't cache the response on no-cache="set-cookie"
Willy Tarreau [Fri, 22 Dec 2017 17:03:04 +0000 (18:03 +0100)] 
BUG/MEDIUM: cache: don't cache the response on no-cache="set-cookie"

If the server mentions no-cache="set-cookie" in the response headers,
we must guarantee that any set-cookie field will not be stored. We
cannot edit the stored response on the fly to trim the set-cookie
header so we can refrain from storing a response containing such a
header. In theory we could use TX_SCK_PRESENT for this but this one
is only set when the cookie is being watched by the configuration.
Since these responses are not very frequent and often accompanied
with a set-cookie header, let's simply refrain from caching whenever
such directive is present.

This needs to be backported to 1.8.

7 years agoBUG/MEDIUM: cache: respect the request cache-control header
Willy Tarreau [Fri, 22 Dec 2017 16:47:35 +0000 (17:47 +0100)] 
BUG/MEDIUM: cache: respect the request cache-control header

Till now if a client emitted a request featureing a cache-control header,
this one was not respected and a stale object could still be delievered.r
 This patch ensures that :
  - cache-control: no-cache disables retrieval from the cache but does
    not prevent the newly fetched object from being stored ;
  - cache-control: no-store can safely retrieve from the cache but prevents
    from storing any fetched object
  - cache-control: max-age/max-stale/min-fresh act like no-cache
  - pragma: no-cache acts like cache-control: no-cache.

This needs to be backported to 1.8.