]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
4 years agoBUG/MINOR: http_fetch: make hdr_ip() reject trailing characters
Willy Tarreau [Thu, 25 Mar 2021 13:12:29 +0000 (14:12 +0100)] 
BUG/MINOR: http_fetch: make hdr_ip() reject trailing characters

The hdr_ip() sample fetch function will try to extract IP addresses
from a header field. These IP addresses are parsed using url2ipv4()
and if it fails it will fall back to inet_pton(AF_INET6), otherwise
will fail.

There is a small problem there which is that if a field starts with
an IP address and is immediately followed by some garbage, the IP
address part is still returned. This is a problem with fields such
as x-forwarded-for because it prevents detection of accidental
corruption or bug along the chain. For example, the following string:

   x-forwarded-for: 1.2.3.4; 5.6.7.8

or this one:

   x-forwarded-for: 1.2.3.4O    ( the last one being the letter 'O')

would still return "1.2.3.4" despite the trailing characters. This is
bad because it will silently cover broken code running on intermediary
proxies and may even in some cases allow haproxy to pass improperly
formatted headers after they were apparently validated, for example,
if someone extracts the address from this field to place it into
another one.

This issue would only affect the IPv4 parser, because the IPv6 parser
already uses inet_pton() which fails at the first invalid character and
rejects trailing port numbers.

In strict compliance with RFC7239, let's make sure that if there are any
characters left in the string, the parsing fails and makes hdr_ip()
return nothing. However, a special case has to be handled to support
IPv4 addresses followed by a colon and a valid port number, because till
now the parser used to implicitly accept them and it appears that this
practice, though rare, does exist at least in Azure:
   https://docs.microsoft.com/en-us/azure/application-gateway/how-application-gateway-works

This issue has always been there so the fix may be backported to all
versions. It will need the following commit in order to work as expected:

    MINOR: tools: make url2ipv4 return the exact number of bytes parsed

Many thanks to https://twitter.com/melardev and the BitMEX Security Team
for their detailed report.

4 years agoMINOR: tools: make url2ipv4 return the exact number of bytes parsed
Willy Tarreau [Thu, 25 Mar 2021 10:34:40 +0000 (11:34 +0100)] 
MINOR: tools: make url2ipv4 return the exact number of bytes parsed

The function's return value is currently used as a boolean but we'll
need it to return the number of bytes parsed. Right now it returns
it minus one, unless the last char doesn't match what is permitted.
Let's update this to make it more usable.

4 years agoBUG/MEDIUM: thread: Fix a deadlock if an isolated thread is marked as harmless
Christopher Faulet [Thu, 25 Mar 2021 13:11:36 +0000 (14:11 +0100)] 
BUG/MEDIUM: thread: Fix a deadlock if an isolated thread is marked as harmless

If an isolated thread is marked as harmless, it will loop forever in
thread_harmless_till_end() waiting no threads are isolated anymore. It never
happens because the current thread is isolated. To fix the bug, we exclude
the current thread for the test. We now wait for all other threads to leave
the rendez-vous point.

This bug only seems to occurr if HAProxy is compiled with DEBUG_UAF, when
pool_gc() is called. pool_gc() isolates the current thread, while
pool_free_area() set the thread as harmless when munmap is called.

This patch must be backported as far as 2.0.

4 years agoBUG/MEDIUM: release lock on idle conn killing on reached pool high count
Amaury Denoyelle [Tue, 23 Mar 2021 09:44:43 +0000 (10:44 +0100)] 
BUG/MEDIUM: release lock on idle conn killing on reached pool high count

Release the lock before calling mux destroy in connect_server when
trying to kill an idle connection because the pool high count has been
reached.

The lock must be released because the mux destroy will call
srv_release_conn which also takes the lock to remove the connection from
the tree. As the connection was already deleted from the tree at this
stage, it is safe to release the lock, and the removal in
srv_release_conn will be a noop.

It does not need to be backported because it is only present in the
current release. It has been introduced by
    5c7086f6b06d546c5800486ed9e4bb8d8d471e09
    MEDIUM: connection: protect idle conn lists with locks

4 years agoBUG/MEDIUM: fd: Take the fd_mig_lock when closing if no DWCAS is available.
Olivier Houchard [Thu, 25 Mar 2021 00:38:54 +0000 (01:38 +0100)] 
BUG/MEDIUM: fd: Take the fd_mig_lock when closing if no DWCAS is available.

In fd_delete(), if we're running with no double-width cas, take the
fd_mig_lock before setting thread_mask to 0 to make sure that
another thread calling fd_set_running() won't miss the new value of
thread_mask and set its bit in running_mask after we checked it.

This should be backported to 2.2 as part of the series fixing fd_delete().

4 years agoCLEANUP: fd: slightly simplify up _fd_delete_orphan()
Willy Tarreau [Wed, 24 Mar 2021 14:34:25 +0000 (15:34 +0100)] 
CLEANUP: fd: slightly simplify up _fd_delete_orphan()

Let's release the port range earlier so that all zeroes are grouped
together and that the compiler can slightly simplify the code.

4 years agoCLEANUP: fd: remove unused fd_set_running_excl()
Willy Tarreau [Wed, 24 Mar 2021 10:34:09 +0000 (11:34 +0100)] 
CLEANUP: fd: remove unused fd_set_running_excl()

This one is no longer used and was the origin of the previously mentioned
deadlock.

4 years agoBUG/MEDIUM: fd: do not wait on FD removal in fd_delete()
Willy Tarreau [Wed, 24 Mar 2021 09:51:32 +0000 (10:51 +0100)] 
BUG/MEDIUM: fd: do not wait on FD removal in fd_delete()

Christopher discovered an issue mostly affecting 2.2 and to a less extent
2.3 and above, which is that it's possible to deadlock a soft-stop when
several threads are using a same listener:

          thread1                             thread2

      unbind_listener()                   fd_set_running()
          lock(listener)                  listener_accept()
          fd_delete()                          lock(listener)
             while (running_mask);  -----> deadlock
          unlock(listener)

This simple case disappeared from 2.3 due to the removal of some locked
operations at the end of listener_accept() on the regular path, but the
architectural problem is still here and caused by a lock inversion built
around the loop on running_mask in fd_clr_running_excl(), because there
are situations where the caller of fd_delete() may hold a lock that is
preventing other threads from dropping their bit in running_mask.

The real need here is to make sure the last user deletes the FD. We have
all we need to know the last one, it's the one calling fd_clr_running()
last, or entering fd_delete() last, both of which can be summed up as
the last one calling fd_clr_running() if fd_delete() calls fd_clr_running()
at the end. And we can prevent new threads from appearing in running_mask
by removing their bits in thread_mask.

So what this patch does is that it sets the running_mask for the thread
in fd_delete(), clears the thread_mask, thus marking the FD as orphaned,
then clears the running mask again, and completes the deletion if it was
the last one. If it was not, another thread will pass through fd_clr_running
and will complete the deletion of the FD.

The bug is easily reproducible in 2.2 under high connection rates during
soft close. When the old process stops its listener, occasionally two
threads will deadlock and the old process will then be killed by the
watchdog. It's strongly believed that similar situations do exist in 2.3
and 2.4 (e.g. if the removal attempt happens during resume_listener()
called from listener_accept()) but if so, they should be much harder to
trigger.

This should be backported to 2.2 as the issue appeared with the FD
migration. It requires previous patches "fd: make fd_clr_running() return
the remaining running mask" and "MINOR: fd: remove the unneeded running
bit from fd_insert()".

Notes for backport: in 2.2, the fd_dodelete() function requires an extra
argument "do_close" indicating whether we want to remove and close the FD
(fd_delete) or just delete it (fd_remove). While this information is not
conveyed along the chain, we know that late calls always imply do_close=1
become do_close=0 exclusively results from fd_remove() which is only used
by the config parser and the master, both of which are single-threaded,
hence are always the last ones in the running_mask. Thus it is safe to
assume that a postponed FD deletion always implies do_close=1.

Thanks to Olivier for his help in designing this optimal solution.

4 years agoMINOR: fd: remove the unneeded running bit from fd_insert()
Willy Tarreau [Wed, 24 Mar 2021 09:42:16 +0000 (10:42 +0100)] 
MINOR: fd: remove the unneeded running bit from fd_insert()

There's no point taking the running bit in fd_insert() since by
definition there will never be more than one thread inserting the FD,
and that fd_insert() may only be done after the fd was allocated by
the system, indicating the end of use by any other thread.

This will need to be backported to 2.2 to fix an issue.

4 years agoMINOR: fd: make fd_clr_running() return the remaining running mask
Willy Tarreau [Wed, 24 Mar 2021 09:27:56 +0000 (10:27 +0100)] 
MINOR: fd: make fd_clr_running() return the remaining running mask

We'll need to know that a thread is the last one to use an fd, so let's
make fd_clr_running() return the remaining bits after removal. Note that
in practice we're only interested in knowing if it's zero but the compiler
doesn't make use of the clags after the AND and emits a CMPXCHG anyway :-/

This will need to be backported to 2.2 to fix an issue.

4 years agoBUG/MEDIUM: lua: Always init the lua stack before referencing the context
Christopher Faulet [Wed, 24 Mar 2021 14:03:01 +0000 (15:03 +0100)] 
BUG/MEDIUM: lua: Always init the lua stack before referencing the context

When a lua context is allocated, its stack must be initialized to NULL
before attaching it to its owner (task, stream or applet).  Otherwise, if
the watchdog is fired before the stack is really created, that may lead to a
segfault because we try to dump the traceback of an uninitialized lua stack.

It is easy to trigger this bug if a lua script do a blocking call while
another thread try to initialize a new lua context. Because of the global
lua lock, the init is blocked before the stack creation. Of course, it only
happens if the script is executed in the shared global context.

This patch must be backported as far as 2.0.

4 years agoBUG/MEDIUM: debug/lua: Use internal hlua function to dump the lua traceback
Christopher Faulet [Wed, 24 Mar 2021 13:52:24 +0000 (14:52 +0100)] 
BUG/MEDIUM: debug/lua: Use internal hlua function to dump the lua traceback

The commit reverts following commits:
  * 83926a04 BUG/MEDIUM: debug/lua: Don't dump the lua stack if not dumpable
  * a61789a1 MEDIUM: lua: Use a per-thread counter to track some non-reentrant parts of lua

Instead of relying on a Lua function to print the lua traceback into the
debugger, we are now using our own internal function (hlua_traceback()).
This one does not allocate memory and use a chunk instead. This avoids any
issue with a possible deadlock in the memory allocator because the thread
processing was interrupted during a memory allocation.

This patch relies on the commit "BUG/MEDIUM: debug/lua: Use internal hlua
function to dump the lua traceback". Both must be backported wherever the
patches above are backported, thus as far as 2.0

4 years agoMINOR: lua: Slightly improve function dumping the lua traceback
Christopher Faulet [Wed, 24 Mar 2021 13:48:45 +0000 (14:48 +0100)] 
MINOR: lua: Slightly improve function dumping the lua traceback

The separator string is now configurable, passing it as parameter when the
function is called. In addition, the message have been slightly changed to
be a bit more readable.

4 years agoBUILD: ssl: guard ecdh functions with SSL_CTX_set_tmp_ecdh macro
Ilya Shipitsin [Sun, 21 Mar 2021 07:50:47 +0000 (12:50 +0500)] 
BUILD: ssl: guard ecdh functions with SSL_CTX_set_tmp_ecdh macro

let us use feature macro SSL_CTX_set_tmp_ecdh instead of comparing openssl
version

4 years agoCLEANUP: ssl: remove unused definitions
Ilya Shipitsin [Sat, 20 Mar 2021 17:30:59 +0000 (22:30 +0500)] 
CLEANUP: ssl: remove unused definitions

not need since  e7eb1fec2f2349359c752c8fbb82357b14c7e4cf

4 years agoBUG/MINOR: ssl: Prevent disk access when using "add ssl crt-list"
Remi Tricot-Le Breton [Tue, 23 Mar 2021 15:41:53 +0000 (16:41 +0100)] 
BUG/MINOR: ssl: Prevent disk access when using "add ssl crt-list"

If an unknown CA file was first mentioned in an "add ssl crt-list" CLI
command, it would result in a call to X509_STORE_load_locations which
performs a disk access which is forbidden during runtime. The same would
happen if a "ca-verify-file" or "crl-file" was specified. This was due
to the fact that the crt-list file parsing and the crt-list related CLI
commands parsing use the same functions.
The patch simply adds a new parameter to all the ssl_bind parsing
functions so that they know if the call is made during init or by the
CLI, and the ssl_store_load_locations function can then reject any new
cafile_entry creation coming from a CLI call.

It can be backported as far as 2.2.

4 years agoBUILD: tools: fix build error with new PA_O_DEFAULT_DGRAM
Willy Tarreau [Tue, 23 Mar 2021 17:36:37 +0000 (18:36 +0100)] 
BUILD: tools: fix build error with new PA_O_DEFAULT_DGRAM

Previous commit 69ba35146 ("MINOR: tools: introduce new option
PA_O_DEFAULT_DGRAM on str2sa_range.") managed to introduce a
parenthesis imbalance that broke the build. No backport is needed.

4 years agoMINOR: tools: introduce new option PA_O_DEFAULT_DGRAM on str2sa_range.
Emeric Brun [Mon, 22 Mar 2021 16:17:34 +0000 (17:17 +0100)] 
MINOR: tools: introduce new option PA_O_DEFAULT_DGRAM on str2sa_range.

str2sa_range function options PA_O_DGRAM and PA_O_STREAM are used to
define the supported address types but also to set the default type
if it is not explicit. If the used address support both STREAM and DGRAM,
the default was always set to STREAM.

This patch introduce a new option PA_O_DEFAULT_DGRAM to force the
default to DGRAM type if it is not explicit in the address field
and both STREAM and DGRAM are supported. If only DGRAM or only STREAM
is supported, it continues to be considered as the default.

4 years agoBUG/MEDIUM: freq_ctr/threads: use the global_now_ms variable
Willy Tarreau [Tue, 23 Mar 2021 07:58:22 +0000 (08:58 +0100)] 
BUG/MEDIUM: freq_ctr/threads: use the global_now_ms variable

In commit a1ecbca0a ("BUG/MINOR: freq_ctr/threads: make use of the last
updated global time"), for period-based counters, the millisecond part
of the global_now variable was used as the date for the new period. But
it's wrong, it only works with sub-second periods as it wraps every
second, and for other periods the counters never rotate anymore.

Let's make use of the newly introduced global_now_ms variable instead,
which contains the global monotonic time expressed in milliseconds.

This patch needs to be backported wherever the patch above is backported.
It depends on previous commit "MINOR: time: also provide a global,
monotonic global_now_ms timer".

4 years agoMINOR: time: also provide a global, monotonic global_now_ms timer
Willy Tarreau [Tue, 23 Mar 2021 07:45:42 +0000 (08:45 +0100)] 
MINOR: time: also provide a global, monotonic global_now_ms timer

The period-based freq counters need the global date in milliseconds,
so better calculate it and expose it rather than letting all call
places incorrectly retrieve it.

Here what we do is that we maintain a new globally monotonic timer,
global_now_ms, which ought to be very close to the global_now one,
but maintains the monotonic approach of now_ms between all threads
in that global_now_ms is always ahead of any now_ms.

This patch is made simple to ease backporting (it will be needed for
a subsequent fix), but it also opens the way to some simplifications
on the time handling: instead of computing the local time and trying
to force it to the global one, we should soon be able to proceed in
the opposite way, that is computing the new global time an making the
local one just the latest snapshot of it. This will bring the benefit
of making sure that the global time is always ahead of the local one.

4 years agoCLEANUP: quic: use pool_zalloc() instead of pool_alloc+memset
Willy Tarreau [Mon, 22 Mar 2021 20:13:05 +0000 (21:13 +0100)] 
CLEANUP: quic: use pool_zalloc() instead of pool_alloc+memset

Two places used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: tcpcheck: use pool_zalloc() instead of pool_alloc+memset
Willy Tarreau [Mon, 22 Mar 2021 20:11:45 +0000 (21:11 +0100)] 
CLEANUP: tcpcheck: use pool_zalloc() instead of pool_alloc+memset

Two places used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: ssl: use pool_zalloc() in ssl_init_keylog()
Willy Tarreau [Mon, 22 Mar 2021 20:10:12 +0000 (21:10 +0100)] 
CLEANUP: ssl: use pool_zalloc() in ssl_init_keylog()

This one used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: resolvers: use pool_zalloc() in resolv_link_resolution()
Willy Tarreau [Mon, 22 Mar 2021 20:08:50 +0000 (21:08 +0100)] 
CLEANUP: resolvers: use pool_zalloc() in resolv_link_resolution()

This one used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: mailers: use pool_zalloc() in enqueue_one_email_alert()
Willy Tarreau [Mon, 22 Mar 2021 20:07:52 +0000 (21:07 +0100)] 
CLEANUP: mailers: use pool_zalloc() in enqueue_one_email_alert()

This one used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: frontend: use pool_zalloc() in frontend_accept()
Willy Tarreau [Mon, 22 Mar 2021 20:06:21 +0000 (21:06 +0100)] 
CLEANUP: frontend: use pool_zalloc() in frontend_accept()

The capture buffers were allocated then zeroed, let's have the allocator
do it.

4 years agoCLEANUP: spoe: use pool_zalloc() instead of pool_alloc+memset
Willy Tarreau [Mon, 22 Mar 2021 20:04:50 +0000 (21:04 +0100)] 
CLEANUP: spoe: use pool_zalloc() instead of pool_alloc+memset

Two places used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: filters: use pool_zalloc() in flt_stream_add_filter()
Willy Tarreau [Mon, 22 Mar 2021 20:02:50 +0000 (21:02 +0100)] 
CLEANUP: filters: use pool_zalloc() in flt_stream_add_filter()

This one used to alloc then zero the area, let's have the allocator do it.

4 years agoCLEANUP: connection: use pool_zalloc() in conn_alloc_hash_node()
Willy Tarreau [Mon, 22 Mar 2021 20:01:05 +0000 (21:01 +0100)] 
CLEANUP: connection: use pool_zalloc() in conn_alloc_hash_node()

This one used to alloc then zero the area, let's have the allocator do it.

4 years agoMINOR: pools: add pool_zalloc() to return a zeroed area
Willy Tarreau [Mon, 22 Mar 2021 21:05:05 +0000 (22:05 +0100)] 
MINOR: pools: add pool_zalloc() to return a zeroed area

It's like pool_alloc() but the output is zeroed before being returned
and is never poisonned.

4 years agoMINOR: pools: make the pool allocator support a few flags
Willy Tarreau [Mon, 22 Mar 2021 19:54:15 +0000 (20:54 +0100)] 
MINOR: pools: make the pool allocator support a few flags

The pool_alloc_dirty() function was renamed to __pool_alloc() and now
takes a set of flags indicating whether poisonning is permitted or not
and whether zeroing the area is needed or not. The pool_alloc() function
is now just a wrapper calling __pool_alloc(pool, 0).

4 years agoCLEANUP: pools: remove the unused pool_get_first() function
Willy Tarreau [Mon, 22 Mar 2021 13:56:30 +0000 (14:56 +0100)] 
CLEANUP: pools: remove the unused pool_get_first() function

This one used to maintain a shortcut in the pools allocation path that
was only justified by b_alloc_fast() which was not used! Let's get rid
of it as well so that the allocator becomes a bit more straight forward.

4 years agoCLEANUP: dynbuf: remove the unused b_alloc_fast() function
Willy Tarreau [Mon, 22 Mar 2021 13:53:56 +0000 (14:53 +0100)] 
CLEANUP: dynbuf: remove the unused b_alloc_fast() function

It is never used anymore since 1.7 where it was used by b_alloc_margin()
then replaced by direct calls to the pools function, and it maintains a
dependency on the exposed pools functions. It's time to get rid of it,
as it's not even certain it still works.

4 years agoCLEANUP: dynbuf: remove b_alloc_margin()
Willy Tarreau [Mon, 22 Mar 2021 13:49:19 +0000 (14:49 +0100)] 
CLEANUP: dynbuf: remove b_alloc_margin()

It's not used anymore, let's completely remove it before anyone uses it
again by accident.

4 years agoMEDIUM: dynbuf: remove last usages of b_alloc_margin()
Willy Tarreau [Mon, 22 Mar 2021 13:44:31 +0000 (14:44 +0100)] 
MEDIUM: dynbuf: remove last usages of b_alloc_margin()

The function's purpose used to be to fail a buffer allocation if that
allocation wouldn't result in leaving some buffers available. Thus,
some allocations could succeed and others fail for the sole purpose of
trying to provide 2 buffers at once to process_stream(). But things
have changed a lot with 1.7 breaking the promise that process_stream()
would always succeed with only two buffers, and later the thread-local
pool caches that keep certain buffers available that are not accounted
for in the global pool so that local allocators cannot guess anything
from the number of currently available pools.

Let's just replace all last uses of b_alloc_margin() with b_alloc() once
for all.

4 years agoMINOR: channel: simplify the channel's buffer allocation
Willy Tarreau [Mon, 22 Mar 2021 13:41:38 +0000 (14:41 +0100)] 
MINOR: channel: simplify the channel's buffer allocation

The channel's buffer allocator, channel_alloc_buffer(), was still relying
on the principle of a margin for the request and not for the response.
But this margin stopped working around 1.7 with the introduction of the
content filters such as SPOE, and was completely anihilated with the
local pools that came with threads. Let's simplify this and just use
b_alloc().

4 years agoCLEANUP: l7-retries: do not test the buffer before calling b_alloc()
Willy Tarreau [Mon, 22 Mar 2021 15:17:37 +0000 (16:17 +0100)] 
CLEANUP: l7-retries: do not test the buffer before calling b_alloc()

The return value is enough now to know if the allocation succeeded or
failed.

4 years agoCLEANUP: compression: do not test for buffer before calling b_alloc()
Willy Tarreau [Mon, 22 Mar 2021 15:16:22 +0000 (16:16 +0100)] 
CLEANUP: compression: do not test for buffer before calling b_alloc()

Now we know the function is idempotent, we don't need to run the
preliminary test anymore.

4 years agoMINOR: dynbuf: make b_alloc() always check if the buffer is allocated
Willy Tarreau [Mon, 22 Mar 2021 15:10:22 +0000 (16:10 +0100)] 
MINOR: dynbuf: make b_alloc() always check if the buffer is allocated

Right now there is a discrepancy beteween b_alloc() and b_allow_margin():
the former forcefully overwrites the target pointer while the latter tests
it and returns it as-is if already allocated.

As a matter of fact, all callers of b_alloc() either preliminary test the
buffer, or assume it's already null.

Let's remove this pain and make the function test the buffer's allocation
before doing it again, and match call places' expectations.

4 years agoMINOR: opentracing: use pool_alloc(), not pool_alloc_dirty()
Willy Tarreau [Mon, 22 Mar 2021 14:10:51 +0000 (15:10 +0100)] 
MINOR: opentracing: use pool_alloc(), not pool_alloc_dirty()

pool_alloc_dirty() is the version below pool_alloc() that never performs
the memory poisonning. It should only be called directly for very large
unstructured areas for which enabling memory poisonning would not bring
anything but could significantly hurt performance (e.g. buffers). Using
this function here will not provide any benefit and will hurt the ability
to debug.

It would be desirable to backport this, although it does not cause any
user-visible bug, it just complicates debugging.

4 years agoMINOR: ssl: use pool_alloc(), not pool_alloc_dirty()
Willy Tarreau [Mon, 22 Mar 2021 14:09:41 +0000 (15:09 +0100)] 
MINOR: ssl: use pool_alloc(), not pool_alloc_dirty()

pool_alloc_dirty() is the version below pool_alloc() that never performs
the memory poisonning. It should only be called directly for very large
unstructured areas for which enabling memory poisonning would not bring
anything but could significantly hurt performance (e.g. buffers). Using
this function here will not provide any benefit and will hurt the ability
to debug.

It would be desirable to backport this, although it does not cause any
user-visible bug, it just complicates debugging.

4 years agoMINOR: cache: use pool_alloc(), not pool_alloc_dirty()
Willy Tarreau [Mon, 22 Mar 2021 14:00:49 +0000 (15:00 +0100)] 
MINOR: cache: use pool_alloc(), not pool_alloc_dirty()

pool_alloc_dirty() is the version below pool_alloc() that never performs
the memory poisonning. It should only be called directly for very large
unstructured areas for which enabling memory poisonning would not bring
anything but could significantly hurt performance (e.g. buffers). Using
this function here will not provide any benefit and will hurt the ability
to debug.

It would be desirable to backport this, although it does not cause any
user-visible bug, it just complicates debugging.

4 years agoMINOR: fcgi-app: use pool_alloc(), not pool_alloc_dirty()
Willy Tarreau [Mon, 22 Mar 2021 14:07:37 +0000 (15:07 +0100)] 
MINOR: fcgi-app: use pool_alloc(), not pool_alloc_dirty()

pool_alloc_dirty() is the version below pool_alloc() that never performs
the memory poisonning. It should only be called directly for very large
unstructured areas for which enabling memory poisonning would not bring
anything but could significantly hurt performance (e.g. buffers). Using
this function here will not provide any benefit and will hurt the ability
to debug.

It would be desirable to backport this, although it does not cause any
user-visible bug, it just complicates debugging.

4 years agoMINOR: spoe: use pool_alloc(), not pool_alloc_dirty()
Willy Tarreau [Mon, 22 Mar 2021 14:05:54 +0000 (15:05 +0100)] 
MINOR: spoe: use pool_alloc(), not pool_alloc_dirty()

pool_alloc_dirty() is the version below pool_alloc() that never performs
the memory poisonning. It should only be called directly for very large
unstructured areas for which enabling memory poisonning would not bring
anything but could significantly hurt performance (e.g. buffers). Using
this function here will not provide any real benefit, it only avoids the
area being poisonned before being zeroed. Ideally a pool_calloc() function
should be provided for this.

4 years agoMINOR: compression: use pool_alloc(), not pool_alloc_dirty()
Willy Tarreau [Mon, 22 Mar 2021 14:08:17 +0000 (15:08 +0100)] 
MINOR: compression: use pool_alloc(), not pool_alloc_dirty()

pool_alloc_dirty() is the version below pool_alloc() that never performs
the memory poisonning. It should only be called directly for very large
unstructured areas for which enabling memory poisonning would not bring
anything but could significantly hurt performance (e.g. buffers). Using
this function here will not provide any benefit and will hurt the ability
to debug.

It would be desirable to backport this, although it does not cause any
user-visible bug, it just complicates debugging.

4 years agoREGTESTS: wait for proper return of enable server in cli add server test
Amaury Denoyelle [Mon, 22 Mar 2021 10:44:12 +0000 (11:44 +0100)] 
REGTESTS: wait for proper return of enable server in cli add server test

Add an empty expect statement after the 'enable server' cli command.
This ensures that the command has been properly handled by haproxy and
its processing is over.

It should fix the unstable behavior of the test which causes reports of
503 even after the server has been enabled.

This should fix the github issue #1188.

4 years agoREGTESTS: remove unneeded experimental-mode in cli add server test
Amaury Denoyelle [Mon, 22 Mar 2021 10:43:03 +0000 (11:43 +0100)] 
REGTESTS: remove unneeded experimental-mode in cli add server test

The experimental-mode cli command is only required for the moment for
'add server'. Remove it for the 'enable server' command.

4 years agoCLEANUP: mark defproxy as const on parse tune.fail-alloc
Amaury Denoyelle [Mon, 22 Mar 2021 10:21:36 +0000 (11:21 +0100)] 
CLEANUP: mark defproxy as const on parse tune.fail-alloc

This fixes a gcc warning about a missing const on defproxy for
mem_parse_global_fail_alloc.

This is needed since the commit :

018251667e4c95478ce0026f4d700e0420f8ce24
CLEANUP: config: make the cfg_keyword parsers take a const for the
defproxy

4 years agoREGTESTS: revert workaround for a crash with recent libressl on http-reuse sni
Ilya Shipitsin [Fri, 19 Mar 2021 17:31:14 +0000 (22:31 +0500)] 
REGTESTS: revert workaround for a crash with recent libressl on http-reuse sni

LibreSSL-3.2.5 has fixed "use-after-free" in tls session resumption,
let us enable session resumption back

4 years agoCI: github actions: update LibreSSL to 3.2.5
Ilya Shipitsin [Fri, 19 Mar 2021 17:29:16 +0000 (22:29 +0500)] 
CI: github actions: update LibreSSL to 3.2.5

LibreSSL-3.2.5 is released, let us update to it

4 years agoCLEANUP: assorted typo fixes in the code and comments
Ilya Shipitsin [Fri, 19 Mar 2021 17:21:44 +0000 (22:21 +0500)] 
CLEANUP: assorted typo fixes in the code and comments

This is 21st iteration of typo fixes

4 years agoCI: codespell: whitelist "Dragan Dosen"
Ilya Shipitsin [Fri, 19 Mar 2021 17:15:12 +0000 (22:15 +0500)] 
CI: codespell: whitelist "Dragan Dosen"

let us tell codespell that "Dosen" is legitimate spelling

4 years agoMEDIUM: quic: Fix build.
Olivier Houchard [Fri, 19 Mar 2021 19:09:22 +0000 (20:09 +0100)] 
MEDIUM: quic: Fix build.

Put the ) at the right place.

This should fix github issue #1190.

4 years agoMEDIUM: quic: Fix build.
Olivier Houchard [Fri, 19 Mar 2021 18:48:53 +0000 (19:48 +0100)] 
MEDIUM: quic: Fix build.

Spell conn_xprt_start() correctly.

This should fix github issue #1189.

4 years ago[RELEASE] Released version 2.4-dev13 v2.4-dev13
Willy Tarreau [Fri, 19 Mar 2021 16:16:18 +0000 (17:16 +0100)] 
[RELEASE] Released version 2.4-dev13

Released version 2.4-dev13 with the following main changes :
    - BUG/MEDIUM: cli: fix "help" crashing since recent spelling fixes
    - BUG/MINOR: cfgparse: use the GLOBAL not LISTEN keywords list for spell checking
    - MINOR: tools: improve word fingerprinting by counting presence
    - MINOR: tools: do not sum squares of differences for word fingerprints
    - MINOR: cli: improve fuzzy matching to work on all remaining words at once
    - MINOR: cli: sort the suggestions by order of relevance
    - MINOR: cli: limit spelling suggestions to 5
    - MINOR: cfgparse/proxy: also support spelling fixes on options
    - BUG/MINOR: resolvers: Add missing case-insensitive comparisons of DNS hostnames
    - MINOR: time: export the global_now variable
    - BUG/MINOR: freq_ctr/threads: make use of the last updated global time
    - MINOR: freq_ctr/threads: relax when failing to update a sliding window value
    - MINOR/BUG: mworker/cli: do not use the unix_bind prefix for the master CLI socket
    - MINOR: mworker/cli: alert the user if we enabled a master CLI but not the master-worker mode
    - MINOR: cli: implement experimental-mode
    - REORG: server: add a free server function
    - MINOR: cfgparse: always alloc idle conns task
    - REORG: server: move keywords in srv_kws
    - MINOR: server: remove fastinter from mistyped kw list
    - REORG: server: split parse_server
    - REORG: server: move alert traces in parse_server
    - REORG: server: rename internal functions from parse_server
    - REORG: server: attach servers in parse_server
    - REORG: server: use flags for parse_server
    - MINOR: server: prepare parsing for dynamic servers
    - MINOR: stats: export function to allocate extra proxy counters
    - MEDIUM: server: implement 'add server' cli command
    - REGTESTS: implement test for 'add server' cli
    - MINOR: server: enable standard options for dynamic servers
    - MINOR: server: support keyword proto in 'add server' cli
    - BUG/MINOR: protocol: add missing support of dgram unix socket.
    - CLEANUP: Fix a typo in fix_is_valid description
    - MINOR: raw_sock: Add a close method.
    - MEDIUM: connections: Introduce a new XPRT method, start().
    - MEDIUM: connections: Implement a start() method for xprt_handshake.
    - MEDIUM: connections: Implement a start() method in ssl_sock.
    - MINOR: muxes: garbage collect the reset() method.
    - CLEANUP: tcp-rules: Fix a typo in error messages about expect-netscaler-cip
    - MEDIUM: lua: Use a per-thread counter to track some non-reentrant parts of lua
    - BUG/MEDIUM: debug/lua: Don't dump the lua stack if not dumpable

4 years agoBUG/MEDIUM: debug/lua: Don't dump the lua stack if not dumpable
Christopher Faulet [Fri, 19 Mar 2021 14:41:08 +0000 (15:41 +0100)] 
BUG/MEDIUM: debug/lua: Don't dump the lua stack if not dumpable

When we try to dump the stack of a lua context, if it is not dumpable,
nothing is performed and a message is emitted instead. This happens when a
lua execution was interrupted inside a non-reentrant part.

This patch depends on following commit :

 * MEDIUM: lua: Use a per-thread counter to track some non-reentrant parts of lua

Thanks to this patch, we avoid a possible deadllock if the lua is
interrupted by the watchdog in the lua memory allocator, because realloc()
is not async-signal-safe.

Both patches must be backported as far as 2.0.

4 years agoMEDIUM: lua: Use a per-thread counter to track some non-reentrant parts of lua
Christopher Faulet [Fri, 19 Mar 2021 14:16:28 +0000 (15:16 +0100)] 
MEDIUM: lua: Use a per-thread counter to track some non-reentrant parts of lua

Some parts of the Lua are non-reentrant. We must be sure to carefully track
these parts to not dump the lua stack when it is interrupted inside such
parts. For now, we only identified the custom lua allocator. If the thread
is interrupted during the memory allocation, we must not try to print the
lua stack wich also allocate memory. Indeed, realloc() is not
async-signal-safe.

In this patch we introduce a thread-local counter. It is incremented before
entering in a non-reentrant part and decremented when exiting. It is only
performed in hlua_alloc() for now.

4 years agoCLEANUP: tcp-rules: Fix a typo in error messages about expect-netscaler-cip
Christopher Faulet [Fri, 19 Mar 2021 07:53:26 +0000 (08:53 +0100)] 
CLEANUP: tcp-rules: Fix a typo in error messages about expect-netscaler-cip

It was misspelled (expect-netscaler-ip instead of expect-netscaler-cip). 2
commits are concerned :

 * db67b0ed7 MINOR: tcp-rules: suggest approaching action names on mismatch
 * 72d012fbd CLEANUP: tcp-rules: add missing actions in the tcp-request error message

The first one will not be backported, but the second one was backported as
far as 1.8. Thus this one may also be backported, but only the 2nd part
about the list of accepted keywords.

4 years agoMINOR: muxes: garbage collect the reset() method.
Olivier Houchard [Sat, 13 Mar 2021 23:39:49 +0000 (00:39 +0100)] 
MINOR: muxes: garbage collect the reset() method.

Now that connections aren't being reused when they failed, remove the
reset() method. It was unimplemented anywhere, except for H1 where it did
nothing, anyway.

4 years agoMEDIUM: connections: Implement a start() method in ssl_sock.
Olivier Houchard [Fri, 5 Mar 2021 22:47:00 +0000 (23:47 +0100)] 
MEDIUM: connections: Implement a start() method in ssl_sock.

Add a start() method to ssl_sock. It is responsible with initiating the
SSL handshake, currently by just scheduling the tasklet, instead of doing
it in the init() method, when all the XPRT may not have been initialized.

4 years agoMEDIUM: connections: Implement a start() method for xprt_handshake.
Olivier Houchard [Fri, 5 Mar 2021 22:42:41 +0000 (23:42 +0100)] 
MEDIUM: connections: Implement a start() method for xprt_handshake.

Add a start_method to xprt_handshake. It schedules the tasklet that does
the handshake. This used to be done in xprt_handshake_add_xprt(), but that's
a much better place.

4 years agoMEDIUM: connections: Introduce a new XPRT method, start().
Olivier Houchard [Fri, 5 Mar 2021 22:37:48 +0000 (23:37 +0100)] 
MEDIUM: connections: Introduce a new XPRT method, start().

Introduce a new XPRT method, start(). The init() method will now only
initialize whatever is needed for the XPRT to run, but any action the XPRT
has to do before being ready, such as handshakes, will be done in the new
start() method. That way, we will be sure the full stack of xprt will be
initialized before attempting to do anything.
The init() call is also moved to conn_prepare(). There's no longer any reason
to wait for the ctrl to be ready, any action will be deferred until start(),
anyway. This means conn_xprt_init() is no longer needed.

4 years agoMINOR: raw_sock: Add a close method.
Olivier Houchard [Sat, 13 Mar 2021 23:34:49 +0000 (00:34 +0100)] 
MINOR: raw_sock: Add a close method.

Add a close() method, that explicitely cancels any subscription on the
connection, in preparation for future evolutions.

4 years agoCLEANUP: Fix a typo in fix_is_valid description
Christopher Faulet [Thu, 18 Mar 2021 16:40:56 +0000 (17:40 +0100)] 
CLEANUP: Fix a typo in fix_is_valid description

MsgType tag was misspelled.

4 years agoBUG/MINOR: protocol: add missing support of dgram unix socket.
Emeric Brun [Thu, 18 Mar 2021 15:52:17 +0000 (16:52 +0100)] 
BUG/MINOR: protocol: add missing support of dgram unix socket.

The proto "uxdg" (UNIX DGRAM) was not declared, causing an error trying
to put a socket unix on "dgram-bind" into a log-forward section.

This patch introduces the missing "uxdg" protocol by adding proto_uxdg.c
which was fully created based on the code available for the other
protocols.

This patch should be backported to version 2.3 and above.

4 years agoMINOR: server: support keyword proto in 'add server' cli
Amaury Denoyelle [Fri, 12 Mar 2021 17:03:27 +0000 (18:03 +0100)] 
MINOR: server: support keyword proto in 'add server' cli

Allow to specify the mux proto for a dynamic server. It must be
compatible with the backend mode to be accepted. The reg-tests has been
extended for this error case.

4 years agoMINOR: server: enable standard options for dynamic servers
Amaury Denoyelle [Tue, 9 Mar 2021 16:36:23 +0000 (17:36 +0100)] 
MINOR: server: enable standard options for dynamic servers

Enable a subset of server options to be used as keywords on the CLI
command 'add server'. These options are safe and can be applied
flawlessly for a dynamic server.

4 years agoREGTESTS: implement test for 'add server' cli
Amaury Denoyelle [Fri, 12 Mar 2021 09:45:12 +0000 (10:45 +0100)] 
REGTESTS: implement test for 'add server' cli

Write a regtest for the cli command 'add server'. This test will execute
some invalid commands and validates the reported error. A client will
then try to connect to a dynamic server just created and activated.

4 years agoMEDIUM: server: implement 'add server' cli command
Amaury Denoyelle [Mon, 8 Mar 2021 16:13:32 +0000 (17:13 +0100)] 
MEDIUM: server: implement 'add server' cli command

Add a new cli command 'add server'. This command is used to create a new
server at runtime attached on an existing backend. The syntax is the
following one :

$ add server <be_name>/<sv_name> [<kws>...]

This command is only available through experimental mode for the moment.

Currently, no server keywords are supported. They will be activated
individually when deemed properly functional and safe.

Another limitation is put on the backend load-balancing algorithm. The
algorithm must use consistent hashing to guarantee a minimal
reallocation of existing connections on the new server insertion.

4 years agoMINOR: stats: export function to allocate extra proxy counters
Amaury Denoyelle [Thu, 18 Mar 2021 14:48:14 +0000 (15:48 +0100)] 
MINOR: stats: export function to allocate extra proxy counters

Remove static qualifier on stats_allocate_proxy_counters_internal. This
function will be used to allocate extra counters at runtime for dynamic
servers.

4 years agoMINOR: server: prepare parsing for dynamic servers
Amaury Denoyelle [Mon, 8 Mar 2021 16:08:01 +0000 (17:08 +0100)] 
MINOR: server: prepare parsing for dynamic servers

Prepare the server parsing API to support dynamic servers.
- define a new parsing flag to be used for dynamic servers
- each keyword contains a new field dynamic_ok to indicate if it can be
  used for a dynamic server. For now, no keyword are supported.
- do not copy settings from the default server for a new dynamic server.
- a dynamic server is created in a maintenance mode and requires an
  explicit 'enable server' command.
- a new server flag named SRV_F_DYNAMIC is created. This flag is set for
  all servers created at runtime. It might be useful later, for example
  to know if a server can be purged.

4 years agoREORG: server: use flags for parse_server
Amaury Denoyelle [Mon, 8 Mar 2021 15:36:46 +0000 (16:36 +0100)] 
REORG: server: use flags for parse_server

Modify the API of parse_server function. Use flags to describe the type
of the parsed server instead of discrete arguments. These flags can be
used to specify if a server/default-server/server-template is parsed.
Additional parameters are also specified (parsing of the address
required, resolve of a name must be done immediately).

It is now unneeded to use strcmp on args[0] in parse_server. Also, the
calls to parse_server are more explicit thanks to the flags.

4 years agoREORG: server: attach servers in parse_server
Amaury Denoyelle [Mon, 8 Mar 2021 15:35:54 +0000 (16:35 +0100)] 
REORG: server: attach servers in parse_server

Move server linked into proxy backend list outside of _srv_parse_init to
parse_server.

This is groundwork for dynamic servers support. There will be two
differences in case of a dynamic server :
- the server will be attached to the proxy list only at the very end of the
  operations when everything is ok
- the server will be directly attached to the end of the server proxy
  list

4 years agoREORG: server: rename internal functions from parse_server
Amaury Denoyelle [Wed, 17 Mar 2021 13:25:39 +0000 (14:25 +0100)] 
REORG: server: rename internal functions from parse_server

Use a standard convention for the functions used through parse_server.
Use the prefix _srv_parse and specify their private scope in a comment.

4 years agoREORG: server: move alert traces in parse_server
Amaury Denoyelle [Mon, 8 Mar 2021 10:20:52 +0000 (11:20 +0100)] 
REORG: server: move alert traces in parse_server

Move every ha_alert calls in parsing functions into parse_server.
Parsing functions now support a pointer-to-string argument which will be
allocated with an error message if needed via memprintf.

parse_server has then the responsibility to display errors with ha_alert.

This is groundwork for dynamic server. No traces should be printed on
stderr as a response to a cli command. cli_err will replace ha_alert in
this case.

4 years agoREORG: server: split parse_server
Amaury Denoyelle [Mon, 8 Mar 2021 09:29:33 +0000 (10:29 +0100)] 
REORG: server: split parse_server

The huge parse_server function is splitted into two smaller ones.
* _srv_parse_init allocates a new server instance and parses the address
  parameter
* _srv_parse_kw parse the current server keyword

This simplify a bit the parse_server function. Besides, it will be
useful for dynamic server creation.

4 years agoMINOR: server: remove fastinter from mistyped kw list
Amaury Denoyelle [Fri, 12 Mar 2021 15:04:00 +0000 (16:04 +0100)] 
MINOR: server: remove fastinter from mistyped kw list

This keyword is already present in server kw list from checks.c.

4 years agoREORG: server: move keywords in srv_kws
Amaury Denoyelle [Wed, 10 Mar 2021 15:36:02 +0000 (16:36 +0100)] 
REORG: server: move keywords in srv_kws

Move server-keyword hardcoded in parse_server into the srv_kws list of
server.c. Now every server keywords is checked through srv_find_kw. This
has the effect to reduce the size of parse_server. As a side-effect,
common kw list can be reduced.

This change has been made to be able to quickly discard these keywords
in case of a dynamic server.

4 years agoMINOR: cfgparse: always alloc idle conns task
Amaury Denoyelle [Mon, 8 Mar 2021 16:31:39 +0000 (17:31 +0100)] 
MINOR: cfgparse: always alloc idle conns task

The idle conn task is is a global task used to cleanup backend
connections marked for deletion. Previously, it was only only allocated
if at least one server in the configuration has idle connections.

This assumption won't be valid anymore when new servers can be created
at runtime with idle connections. Always allocate the global idle conn
task.

4 years agoREORG: server: add a free server function
Amaury Denoyelle [Tue, 16 Mar 2021 16:20:15 +0000 (17:20 +0100)] 
REORG: server: add a free server function

Create a new server function named free_server. It can be used to
deallocate a server and its member.

4 years agoMINOR: cli: implement experimental-mode
Amaury Denoyelle [Thu, 18 Mar 2021 14:32:53 +0000 (15:32 +0100)] 
MINOR: cli: implement experimental-mode

Experimental mode is similar to expert-mode. It can be used to access to
features still in development.

4 years agoMINOR: mworker/cli: alert the user if we enabled a master CLI but not the master...
Eric Salama [Tue, 16 Mar 2021 14:11:17 +0000 (15:11 +0100)] 
MINOR: mworker/cli: alert the user if we enabled a master CLI but not the master-worker mode

Declaring a master CLI socket without activating the master-worker mode
is likely a user error, so we issue a warning.

This patch can be backported as far as 1.8.

4 years agoMINOR/BUG: mworker/cli: do not use the unix_bind prefix for the master CLI socket
Eric Salama [Tue, 16 Mar 2021 14:12:17 +0000 (15:12 +0100)] 
MINOR/BUG: mworker/cli: do not use the unix_bind prefix for the master CLI socket

If the configuration file contains a 'unix-bind prefix' directive, and
if we use the -S option and specify a UNIX socket path, the path of the
socket will be prepended with the value of the unix-bind prefix.

For instance, if we have 'unix-bind prefix /tmp/sockets/' and we use
'-S /tmp/master-socket' on the command line, we will get this error:

Starting proxy MASTER:
cannot bind UNIX socket (No such file or directory) [/tmp/sockets/tmp/master-socket]

So this patch adds an exception, and will ignore the unix-bind prefix
for the master CLI socket.

This patch can be backported as far as 1.9.

4 years agoMINOR: freq_ctr/threads: relax when failing to update a sliding window value
Willy Tarreau [Wed, 17 Mar 2021 18:22:03 +0000 (19:22 +0100)] 
MINOR: freq_ctr/threads: relax when failing to update a sliding window value

The swrate_add* functions would sping fast on a failed CAS, better place
a cpu_relax() call there to reduce contention if any.

4 years agoBUG/MINOR: freq_ctr/threads: make use of the last updated global time
Willy Tarreau [Wed, 17 Mar 2021 18:10:23 +0000 (19:10 +0100)] 
BUG/MINOR: freq_ctr/threads: make use of the last updated global time

The freq counters were using the thread's own time as the start of the
current period. The problem is that in case of contention, it was
occasionally possible to perform non-monotonic updates on the edge of
the next second, because if the upfront thread updates a counter first,
it causes a rotation, then the second thread loses the race from its
older time, and tries again, and detects a different time again, but
in the past so it only updates the counter, then a third thread on the
new date would detect a change again, thus provoking a rotation again.

The effect was triple:
  - rare loss of stored values during certain transitions from one
    period to the next one, causing counters to report 0
  - half of the threads forced to go through the slow path every second
  - difficult convergence when using many threads where the CAS can fail
    a lot and we can observe N(N-1) attempts for N threads to complete

This patch fixes this issue in two ways:
  - first, it now makes use og the monotonic global_now value which also
    happens to be volatile and to carry the latest known time; this way
    time will never jump backwards anymore and only the first thread
    updates it on transition, the other ones do not need to.

  - second, re-read the time in the loop after each failure, because
    if the date changed in the counter, it means that one thread knows
    a more recent one and we need to update. In this case if it matches
    the new current second, the fast path is usable.

This patch relies on previous patch "MINOR: time: export the global_now
variable" and must be backported as far as 1.8.

4 years agoMINOR: time: export the global_now variable
Willy Tarreau [Wed, 17 Mar 2021 17:52:18 +0000 (18:52 +0100)] 
MINOR: time: export the global_now variable

This is the process-wide monotonic time that is used to update each
thread's own time. It may be required at a few places where a strictly
monotonic clock is required such as freq_ctr. It will be have to be
backported as a dependency of a forthcoming fix.

4 years agoBUG/MINOR: resolvers: Add missing case-insensitive comparisons of DNS hostnames
Christopher Faulet [Tue, 16 Mar 2021 10:21:04 +0000 (11:21 +0100)] 
BUG/MINOR: resolvers: Add missing case-insensitive comparisons of DNS hostnames

DNS hostname comparisons were fixed to be case-insensitive (see b17b88487
"BUG/MEDIUM: dns: Consider the fact that dns answers are
case-insensitive"). However 2 comparisons are still case-sensitive.

This patch must be backported as far as 1.8.

4 years agoMINOR: cfgparse/proxy: also support spelling fixes on options
Willy Tarreau [Mon, 15 Mar 2021 10:11:55 +0000 (11:11 +0100)] 
MINOR: cfgparse/proxy: also support spelling fixes on options

Some are not always easy to spot with "chk" vs "check" or hyphens at
some places and not at others. Now entering "option http-close" properly
suggests "httpclose" and "option tcp-chk" suggests "tcp-check". There's
no need to consider the proxy's capabilities, what matters is to figure
what related word the user tried to spell, and there are not that many
options anyway.

4 years agoMINOR: cli: limit spelling suggestions to 5
Willy Tarreau [Mon, 15 Mar 2021 09:38:04 +0000 (10:38 +0100)] 
MINOR: cli: limit spelling suggestions to 5

There's no need to suggest up to 10 entries for matching keywords,
most of the times 5 are plenty, and will be more readable.

4 years agoMINOR: cli: sort the suggestions by order of relevance
Willy Tarreau [Mon, 15 Mar 2021 09:35:04 +0000 (10:35 +0100)] 
MINOR: cli: sort the suggestions by order of relevance

Now the suggested keywords are sorted with the most relevant ones first
instead of scanning them all in registration order and only dumping the
proposed ones:

- "tra"
   trace <module> [cmd [args...]] : manage live tracing
   operator       : lower the level of the current CLI session to operator
   user           : lower the level of the current CLI session to user
   show trace [<module>] : show live tracing state

- "pool"
   show pools     : report information about the memory pools usage
   add acl        : add acl entry
   del map        : delete map entry
   user           : lower the level of the current CLI session to user
   del acl        : delete acl entry

- "sh ta"
   show stat      : report counters for each proxy and server [desc|json|no-maint|typed|up]*
   show tasks     : show running tasks
   set table [id] : update or create a table entry's data
   show table [id]: report table usage stats or dump this table's contents
   trace <module> [cmd [args...]] : manage live tracing

- "sh state"
   show stat      : report counters for each proxy and server [desc|json|no-maint|typed|up]*
   set table [id] : update or create a table entry's data
   show table [id]: report table usage stats or dump this table's contents
   show servers state [id]: dump volatile server information (for backend <id>)
   show sess [id] : report the list of current sessions or dump this session

4 years agoMINOR: cli: improve fuzzy matching to work on all remaining words at once
Willy Tarreau [Mon, 15 Mar 2021 09:00:29 +0000 (10:00 +0100)] 
MINOR: cli: improve fuzzy matching to work on all remaining words at once

Till now the fuzzy matching would only work on the same number of words,
but this doesn't account for commands like "show servers conn" which
involve 3 words and were not proposed when entering only "show conn".
Let's improve the situation by building the two fingerprints separately
for the correct keyword sequence and the entered one, then compare them.
This can result in slightly larger variations due to the different string
lengths but is easily compensated for. Thanks to this, we can now see
"show servers conn" when entering "show conn", and the following choices
are relevant to correct typos:

- "show foo"
   show sess [id] : report the list of current sessions or dump this session
   show info      : report information about the running process [desc|json|typed]*
   show env [var] : dump environment variables known to the process
   show fd [num] : dump list of file descriptors in use
   show pools     : report information about the memory pools usage

- "show stuff"
   show sess [id] : report the list of current sessions or dump this session
   show info      : report information about the running process [desc|json|typed]*
   show stat      : report counters for each proxy and server [desc|json|no-maint|typed|up]*
   show fd [num] : dump list of file descriptors in use
   show tasks     : show running tasks

- "show stafe"
   show sess [id] : report the list of current sessions or dump this session
   show stat      : report counters for each proxy and server [desc|json|no-maint|typed|up]*
   show fd [num] : dump list of file descriptors in use
   show table [id]: report table usage stats or dump this table's contents
   show tasks     : show running tasks

- "show state"
   show stat      : report counters for each proxy and server [desc|json|no-maint|typed|up]*
   show servers state [id]: dump volatile server information (for backend <id>)

It's still visible that the shorter ones continue to easily match, such
as "show sess" not having much in common with "show foo" but what matters
is that the best candidates are definitely relevant. Probably that listing
them in match order would further help.

4 years agoMINOR: tools: do not sum squares of differences for word fingerprints
Willy Tarreau [Mon, 15 Mar 2021 08:44:53 +0000 (09:44 +0100)] 
MINOR: tools: do not sum squares of differences for word fingerprints

While sums of squares usually give excellent results in fixed-sise
patterns, they don't work well to compare different sized ones such
as when some sub-words are missing, because a word such as "server"
contains "er" twice, which will rsult in an extra distance of at
least 4 for just this e->r transition compared to another one missing
it. This is one of the main reasons why "show conn" only proposes
"show info" on the CLI. Maybe an improved approach consisting in
using squares only for exact same lengths would work, but it would
still make it difficult to spot reversed characters.

4 years agoMINOR: tools: improve word fingerprinting by counting presence
Willy Tarreau [Mon, 15 Mar 2021 08:34:27 +0000 (09:34 +0100)] 
MINOR: tools: improve word fingerprinting by counting presence

The distance between two words can be high due to a sub-word being missing
and in this case it happens that other totally unrealted words are proposed
because their average score looks lower thanks to being shorter. Here we're
introducing the notion of presence of each character so that word sequences
that contain existing sub-words are favored against the shorter ones having
nothing in common. In addition we do not distinguish being/end from a
regular delimitor anymore. That made it harder to spot inverted words.

4 years agoBUG/MINOR: cfgparse: use the GLOBAL not LISTEN keywords list for spell checking
Willy Tarreau [Mon, 15 Mar 2021 08:12:41 +0000 (09:12 +0100)] 
BUG/MINOR: cfgparse: use the GLOBAL not LISTEN keywords list for spell checking

In commit a0e8eb8ca ("MINOR: cfgparse: suggest correct spelling for
unknown words in global section") we got the ability to locate a better
matching word in case of error. But it mistakenly used the CFG_LISTEN
class of words instead of CFG_GLOBAL, resulting in proposing unsuitable
matches in addition to the long hard-coded list. Now, "tune.dh-param"
correctly proposes "tune.ssl.default-dh-param".

No backport is needed.

4 years agoBUG/MEDIUM: cli: fix "help" crashing since recent spelling fixes
Willy Tarreau [Sat, 13 Mar 2021 11:25:43 +0000 (12:25 +0100)] 
BUG/MEDIUM: cli: fix "help" crashing since recent spelling fixes

I somehow managed to re-break the "help" command in b736458bf ("MEDIUM:
cli: apply spelling fixes for known commands before listing them")
after fixing it once. A null-deref happens when checking the args
early in the processing.

No backport is needed as this was introduced in 2.4-dev12.

4 years ago[RELEASE] Released version 2.4-dev12 v2.4-dev12
Willy Tarreau [Sat, 13 Mar 2021 10:48:28 +0000 (11:48 +0100)] 
[RELEASE] Released version 2.4-dev12

Released version 2.4-dev12 with the following main changes :
    - CLEANUP: connection: Use `VAR_ARRAY` in `struct tlv` definition
    - CLEANUP: connection: Remove useless test for NULL before calling `pool_free()`
    - CLEANUP: connection: Use istptr / istlen for proxy_unique_id
    - MINOR: connection: Use a `struct ist` to store proxy_authority
    - CLEANUP: connection: Consistently use `struct ist` to process all TLV types
    - BUILD: task: fix build at -O0 with threads disabled
    - BUILD: bug: refine HA_LINK_ERROR() to only be used on gcc and derivatives
    - CLEANUP: config: make the cfg_keyword parsers take a const for the defproxy
    - BUILD: connection: do not use VAR_ARRAY in struct tlv
    - BUG/MEDIUM: session: NULL dereference possible when accessing the listener
    - MINOR: build: force CC to set a return code when probing options
    - CLEANUP: stream: rename a few remaining occurrences of "stream *sess"
    - BUG/MEDIUM: resolvers: handle huge responses over tcp servers.
    - CLEANUP: config: also address the cfg_keyword API change in the compression code
    - BUG/MEDIUM: ssl: properly remove the TASK_HEAVY flag at end of handshake
    - BUG/MINOR: sample: Rename SenderComID/TargetComID to SenderCompID/TargetCompID
    - MINOR: task: give the scheduler a bit more flexibility in the runqueue size
    - OPTIM: task: automatically adjust the default runqueue-depth to the threads
    - BUG/MINOR: connection: Missing QUIC initialization
    - BUG/MEDIUM: stick-tables: fix ref counter in table entry using multiple http tracksc.
    - BUILD: atomic/arm64: force the register pairs to use in __ha_cas_dw()
    - BUG/MEDIUM: filters: Set CF_FL_ANALYZE on channels when filters are attached
    - BUG/MINOR: tcpcheck: Update .health threshold of agent inside an agent-check
    - BUG/MINOR: proxy/session: Be sure to have a listener to increment its counters
    - BUG/MINOR: tcpcheck: Fix double free on error path when parsing tcp/http-check
    - BUG/MINOR: server-state: properly handle the case where the base is not set
    - BUG/MINOR: server-state: use the argument, not the global state
    - CLEANUP: tcp-rules: add missing actions in the tcp-request error message
    - CLEANUP: vars: make the error message clearer on missing arguments for set-var
    - CLEANUP: http-rules: remove the unexpected comma before the list of action keywords
    - CLEANUP: actions: the keyword must always be const from the rule
    - MINOR: tools: add simple word fingerprinting to find similar-looking words
    - MINOR: cfgparse: add cfg_find_best_match() to suggest an existing word
    - MINOR: cfgparse: suggest correct spelling for unknown words in proxy sections
    - MINOR: cfgparse: suggest correct spelling for unknown words in global section
    - MINOR: cfgparse/server: try to fix spelling mistakes on server lines
    - MINOR: cfgparse/bind: suggest correct spelling for unknown bind keywords
    - MINOR: actions: add a function to suggest an action ressembling a given word
    - MINOR: http-rules: suggest approaching action names on mismatch
    - MINOR: tcp-rules: suggest approaching action names on mismatch
    - BUG/MINOR: cfgparse/server: increment the extra keyword counter one at a time
    - Revert "BUG/MINOR: resolvers: Only renew TTL for SRV records with an additional record"
    - BUG/MINOR: resolvers: Consider server to have no IP on DNS resolution error
    - BUG/MINOR: resolvers: Reset server address on DNS error only on status change
    - BUG/MINOR: resolvers: Unlink DNS resolution to set RMAINT on SRV resolution
    - BUG/MEDIUM: resolvers: Don't set an address-less server as UP
    - BUG/MEDIUM: resolvers: Fix the loop looking for an existing ADD item
    - MINOR: resolvers: new function find_srvrq_answer_record()
    - BUG/MINOR; resolvers: Ignore DNS resolution for expired SRV item
    - BUG/MEDIUM: resolvers: Trigger a DNS resolution if an ADD item is obsolete
    - MINOR: resolvers: Use a function to remove answers attached to a resolution
    - MINOR: resolvers: Purge answer items when a SRV resolution triggers an error
    - MINOR: resolvers: Add function to change the srv status based on SRV resolution
    - MINOR: resolvers: Directly call srvrq_update_srv_state() when possible
    - BUG/MEDIUM: resolvers: Don't release resolution from a requester callbacks
    - BUG/MEDIUM: resolvers: Skip DNS resolution at startup if SRV resolution is set
    - MINOR: resolvers: Use milliseconds for cached items in resolver responses
    - MINOR: resolvers: Don't try to match immediatly renewed ADD items
    - CLEANUP: resolvers: Use ha_free() in srvrq_resolution_error_cb()
    - CLEANUP: resolvers: Perform unsafe loop on requester list when possible
    - BUG/MINOR: cli: make sure "help", "prompt", "quit" are enabled at master level
    - CLEANUP: cli: fix misleading comment and better indent the access level flags
    - MINOR: cli: set the ACCESS_MASTER* bits on the master bind_conf
    - MINOR: cli: test the appctx level for master access instead of comparing pointers
    - MINOR: cli: print the error message in the parser function itself
    - MINOR: cli: filter the list of commands to the matching part
    - MEDIUM: cli: apply spelling fixes for known commands before listing them
    - MINOR: tools: add the ability to update a word fingerprint
    - MINOR: cli: apply the fuzzy matching on the whole command instead of words
    - CLEANUP: cli: rename MAX_STATS_ARGS to MAX_CLI_ARGS
    - CLEANUP: cli: rename the last few "stats_" to "cli_"
    - CLEANUP: task: make sure tasklet handlers always indicate their statuses
    - CLEANUP: assorted typo fixes in the code and comments

4 years agoCLEANUP: assorted typo fixes in the code and comments
Ilya Shipitsin [Fri, 12 Mar 2021 16:53:34 +0000 (21:53 +0500)] 
CLEANUP: assorted typo fixes in the code and comments

This is 20th iteration of typo fixes

4 years agoCLEANUP: task: make sure tasklet handlers always indicate their statuses
Willy Tarreau [Sat, 13 Mar 2021 10:30:19 +0000 (11:30 +0100)] 
CLEANUP: task: make sure tasklet handlers always indicate their statuses

When tasklets were derived from tasks, there was no immediate need for
the scheduler to know their status after execution, and in a spirit of
simplicity they just started to always return NULL. The problem is that
it simply prevents the scheduler from 1) accounting their execution time,
and 2) keeping track of their current execution status. Indeed, a remote
wake-up could very well end up manipulating a tasklet that's currently
being executed. And this is the reason why those handlers have to take
the idle lock before checking their context.

In 2.5 we'll take care of making tasklets and tasks work more similarly,
but trouble is to be expected if we continue to propagate the trend of
returning NULL everywhere, especially if some fixes relying on a stricter
model later need to be backported. For this reason this patch updates all
known tasklet handlers to make them return NULL only when the tasklet was
freed. It has no effect for now and isn't even guaranteed to always be
100% safe but it puts the code into the right direction for this.

4 years agoCLEANUP: cli: rename the last few "stats_" to "cli_"
Willy Tarreau [Sat, 13 Mar 2021 10:00:33 +0000 (11:00 +0100)] 
CLEANUP: cli: rename the last few "stats_" to "cli_"

There were still a very small list of functions, variables and fields
called "stats_" while they were really purely CLI-centric. There's the
frontend called "stats_fe" in the global section, which instantiates a
"cli_applet" called "<CLI>" so it was renamed "cli_fe".

The "alloc_stats_fe" function cas renamed to "cli_alloc_fe" which also
better matches the naming convention of all cli-specific functions.

Finally the "stats_permission_denied_msg" used to return an error on
the CLI was renamed "cli_permission_denied_msg".

Now there's no more "stats_something" that designates the CLI.

4 years agoCLEANUP: cli: rename MAX_STATS_ARGS to MAX_CLI_ARGS
Willy Tarreau [Sat, 13 Mar 2021 09:59:23 +0000 (10:59 +0100)] 
CLEANUP: cli: rename MAX_STATS_ARGS to MAX_CLI_ARGS

This is the number of args accepted on a command received on the CLI,
is has long been totally independent of stats and should not carry
this misleading "stats" name anymore.