MINOR: proxy: add findserver_unique_id() and findserver_unique_name()

Adding alternative findserver() functions to be able to perform an
unique match based on name or puid and by leveraging revision id (rid)
to make sure the function won't match with a new server reusing the
same name or puid of the "potentially deleted" server we were initially
looking for.

For example, if you were in the position of finding a server based on
a given name provided to you by a different context:

Since dynamic servers were implemented, between the time the name was
picked and the time you will perform the findserver() call some dynamic
server deletion/additions could've been performed in the mean time.

In such cases, findserver() could return a new server that re-uses the
name of a previously deleted server. Depending on your needs, it could
be perfectly fine, but there are some cases where you want to lookup
the original server that was provided to you (if it still exists).

MINOR: event_hdl: pause/resume for subscriptions

While working on event handling from lua, the need for a pause/resume
function to temporarily disable a subscription was raised.

We solve this by introducing the EHDL_SUB_F_PAUSED flag for
subscriptions.

The flag is set via _pause() and cleared via _resume(), and it is
checked prior to notifying the subscription in publish function.

Pause and Resume functions are also available for via lookups for
identified subscriptions.

If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.

MINOR: event_hdl: add event_hdl_async_equeue_size() function

Use event_hdl_async_equeue_size() in advanced async task handler to
get the near real-time event queue size.

By near real-time, you should understand that the queue size is not
updated during element insertion/removal, but shortly before insertion
and shortly after removal, so the size should reflect the approximate
queue size at a given time but should definitely not be used as a
unique source of truth.

If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.

MINOR: event_hdl: add event_hdl_async_equeue_isempty() function

Add event_hdl_async_equeue_isempty() to check is the event queue is
empty from an advanced async task handler.

If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.

MINOR: event_hdl: normal tasks support for advanced async mode

advanced async mode (EVENT_HDL_ASYNC_TASK) provided full support for
custom tasklets registration.

Due to the similarities between tasks and tasklets, it may be useful
to use the advanced mode with an existing task (not a tasklet).
While the API did not explicitly disallow this usage, things would
get bad if we try to wakeup a task using tasklet_wakeup() for notifying
the task about new events.

To make the API support both custom tasks and tasklets, we use the
TASK_IS_TASKLET() macro to call the proper waking function depending
on the task's type:

  - For tasklets: we use tasklet_wakeup()
  - For tasks: we use task_wakeup()

If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.

BUG/MEDIUM: event_hdl: fix async data refcount issue

In _event_hdl_publish(), when publishing an event to async handler(s),
async_data is allocated only once and then relies on a refcount
logic to reuse the same data block for multiple async event handlers.
(this allows to save significant amount of memory)

Because the refcount is first set to 0, there is a small race where
the consumers could consume async data (async data refcount reaching 0)
before publishing is actually over.
The consequence is that async data may be freed by one of the consumers
while we still rely on it within _event_hdl_publish().

This was discovered by chance when stress-testing the API with multiple
async handlers registered to the same event: some of the handlers were
notified about a new event for which the event data was already freed,
resulting in invalid reads and/or segfaults.

To fix this, we first set the refcount to 1, assuming that the
publish function relies on async_data until the publish is over.
At the end of the publish, the reference to the async data is dropped.

This way, async_data is either freed by _event_hdl_publish() itself
or by one of the consumers, depending on who is the last one relying
on it.

If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.

BUG/MEDIUM: event_hdl: clean soft-stop handling

soft-stop was not explicitly handled in event_hdl API.

Because of this, event_hdl was causing some leaks on deinit paths.
Moreover, a task responsible for handling events could require some
additional cleanups (ie: advanced async task), and as the task was not
protected against abort when soft-stopping, such cleanup could not be
performed unless the task itself implements the required protections,
which is not optimal.

Consider this new approach:
 'jobs' global variable is incremented whenever an async subscription is
 created to prevent the related task from being aborted before the task
 acknowledges the final END event.

 Once the END event is acknowledged and freed by the task, the 'jobs'
 variable is decremented, and the deinit process may continue (including
 the abortion of remaining tasks not guarded by the 'jobs' variable).

To do this, a new global mt_list is required: known_event_hdl_sub_list
This list tracks the known (initialized) subscription lists within the
process.

sub_lists are automatically added to the "known" list when calling
event_hdl_sub_list_init(), and are removed from the list with
event_hdl_sub_list_destroy().

This allows us to implement a global thread-safe event_hdl deinit()
function that is automatically called on soft-stop thanks to signal(0).
When event_hdl deinit() is initiated, we simply iterate against the known
subscription lists to destroy them.

event_hdl_subscribe_ptr() was slightly modified to make sure that a sub_list
may not accept new subscriptions once it is destroyed (removed from the
known list)
This can occur between the time the soft-stop is initiated (signal(0)) and
haproxy actually enters in the deinit() function (once tasks are either
finished or aborted and other threads already joined).

It is safe to destroy() the subscription list multiple times as long
as the pointer is still valid (ie: first on soft-stop when handling
the '0' signal, then from regular deinit() path): the function does
nothing if the subscription list is already removed.

We partially reverted "BUG/MINOR: event_hdl: make event_hdl_subscribe thread-safe"
since we can use parent mt_list locking instead of a dedicated lock to make
the check gainst duplicate subscription ID.
(insert_lock is not useful anymore)

The check in itself is not changed, only the locking method.

sizeof(event_hdl_sub_list) slightly increases: from 24 bits to 32bits due
to the additional mt_list struct within it.

With that said, having thread-safe list to store known subscription lists
is a good thing: it could help to implement additional management
logic for subcription lists and could be useful to add some stats or
debugging tools in the future.

If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.

MINOR: event_hdl: global sublist management clarification

event_hdl_sub_list_init() and event_hdl_sub_list_destroy() don't expect
to be called with a NULL argument (to use global subscription list
implicitly), simply because the global subscription list init and
destroy is internally managed.

Adding BUG_ON() to detect such invalid usages, and updating some comments
to prevent confusion around these functions.

If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.

BUG/MINOR: event_hdl: make event_hdl_subscribe thread-safe

List insertion in event_hdl_subscribe() was not thread-safe when dealing
with unique identifiers. Indeed, in this case the list insertion is
conditional (we check for a duplicate, then we insert). And while we're
using mt lists for this, the whole operation is not atomic: there is a
race between the check and the insertion.
This could lead to the same ID being registered multiple times with
concurrent calls to event_hdl_subscribe() on the same ID.

To fix this, we add 'insert_lock' dedicated lock in the subscription
list struct. The lock's cost is nearly 0 since it is only used when
registering identified subscriptions and the lock window is very short:
we only guard the duplicate check and the list insertion to make the
conditional insertion "atomic" within a given subscription list.
This is the only place where we need the lock: as soon as the item is
properly inserted we're out of trouble because all other operations on
the list are already thread-safe thanks to mt lists.

A new lock hint is introduced: LOCK_EHDL which is dedicated to event_hdl

The patch may seem quite large since we had to rework the logic around
the subscribe function and switch from simple mt_list to a dedicated
struct wrapping both the mt_list and the insert_lock for the
event_hdl_sub_list type.
(sizeof(event_hdl_sub_list) is now 24 instead of 16)

However, all the changes are internal: we don't break the API.

If 68e692da0 ("MINOR: event_hdl: add event handler base api")
is being backported, then this commit should be backported with it.

BUG/MINOR: event_hdl: fix rid storage type

rid is stored as a uint32_t within struct server, but it was stored as
a signed int within the server event data struct.

Switching from signed int to uint32_t in event_hdl_cb_data_server struct
to make sure it won't overflow.

If 129ecf441 ("MINOR: server/event_hdl: add support for SERVER_ADD and SERVER_DEL events")
is being backported, then this commit should be backported with it.

DOC: lua: silence "Unexpected indentation" Sphinx warnings

When building html documentation from doc/lua-api/index.rst, sphinx
complains about some unexpected indentations:

  "doc/lua-api/index.rst:3221: WARNING: Unexpected indentation"

Silencing them without altering the original output format.

DOC: lua: silence "literal block ends without a blank line" Sphinx warnings

When building html documentation from doc/lua-api/index.rst, sphinx
complains about some literal blocks ending without a blank line:

  "doc/lua-api/index.rst:534: WARNING: Literal block ends without a blank line; unexpected unindent."

Adding the missing blank lines to make sphinx happy

MINOR: hlua: support for optional arguments to core.register_task()

core.register_task(function) may now take up to 4 additional arguments
that will be passed as-is to the task function.
This could be convenient to spawn sub-tasks from existing functions
supporting core.register_task() without the need to use global
variables to pass some context to the newly created task function.

The new prototype is:

  core.register_task(function[, arg1[, arg2[, ...[, arg4]]]])

Implementation remains backward-compatible with existing scripts.

MINOR: hlua_fcn: add server->get_rid() method

Server revision ID was recently added to haproxy with 61e3894
("MINOR: server: add srv->rid (revision id) value")

Let's add it to the hlua server class.

BUG/MEDIUM: hlua: prevent deadlocks with main lua lock

Main lua lock is used at various places in the code.

Most of the time it is used from unprotected lua environments,
in which case the locking is mandatory.
But there are some cases where the lock is attempted from protected
lua environments, meaning that lock is already owned by the current
thread. Thus new locking attempt should be skipped to prevent any
deadlocks from occuring.

To address this, "already_safe" lock hint was implemented in
hlua_ctx_init() function with commit bf90ce1
("BUG/MEDIUM: lua: dead lock when Lua tasks are trigerred")

But this approach is not very safe, for 2 reasons:

First reason is that there are still some code paths that could lead
to deadlocks.
For instance, in register_task(), hlua_ctx_init() is called with
already_safe set to 1 to prevent deadlock from occuring.

But in case of task init failure, hlua_ctx_destroy() will be called
from the same environment (protected environment), and hlua_ctx_destroy()
does not offer the already_safe lock hint.. resulting in a deadlock.

Second reason is that already_safe hint is used to completely skip
SET_LJMP macros (which manipulates the lock internally), resulting
in some logics in the function being unprotected from lua aborts in
case of unexpected errors when manipulating the lua stack (the lock
does not protect against longjmps)

Instead of leaving the locking responsibility to the caller, which is
quite error prone since we must find out ourselves if we are or not in
a protected environment (and is not robust against code re-use),
we move the deadlock protection logic directly in hlua_lock() function.

Thanks to a thread-local lock hint, we can easily guess if the current
thread already owns the main lua lock, in which case the locking attempt
is skipped. The thread-local lock hint is implemented as a counter so
that the lock is properly dropped when the counter reaches 0.
(to match actual lock() and unlock() calls)

This commit depends on "MINOR: hlua: simplify lua locking"
It may be backported to every stable versions.
[prior to 2.5 lua filter API did not exist, filter-related parts
should be skipped]

MINOR: hlua: simplify lua locking

The check on lua state==0 to know whether locking is required or not can
be performed in a locking wrapper to simplify things a bit and prevent
implementation errors.

Locking from hlua context should now be performed via hlua_lock(L) and
unlocking via hlua_unlock(L)

CLEANUP: hlua: use hlua_unref() instead of luaL_unref()

Replacing some luaL_unref(, LUA_REGISTRYINDEX) calls with hlua_unref()
which is simpler to use and more explicit.

CLEANUP: hlua: use hlua_pushref() instead of lua_rawgeti()

Using hlua_pushref() everywhere temporary lua objects are involved.
(ie: hlua_checkfunction(), hlua_checktable...)
Those references are expected to be cleared using hlua_unref() when
they are no longer used.

CLEANUP: hlua: use hlua_ref() instead of luaL_ref()

Using hlua_ref() everywhere temporary lua objects are involved.
Those references are expected to be cleared using hlua_unref()
when they are no longer used.

BUG/MINOR: hlua: prevent function and table reference leaks on errors

Several error paths were leaking function or table references.
(Obtained through hlua_checkfunction() and hlua_checktable() functions)

Now we properly release the references thanks to hlua_unref() in
such cases.

This commit depends on "MINOR: hlua: add simple hlua reference handling API"

This could be backported in every stable versions although it is not
mandatory as such leaks only occur on rare error/warn paths.
[prior to 2.5 lua filter API did not exist, the hlua_register_filter()
part should be skipped]

BUG/MINOR: hlua: fix reference leak in hlua_post_init_state()

hlua init function references were not released during
hlua_post_init_state().

Hopefully, this function is only used during startup so the resulting
leak is not a big deal.
Since each init lua function runs precisely once, it is safe to release
the ref as soon as the function is restored on the stack.

This could be backported to every stable versions.
Please note that this commit depends on "MINOR: hlua: add simple hlua reference handling API"

BUG/MINOR: hlua: fix reference leak in core.register_task()

In core.register_task(): we take a reference to the function passed as
argument in order to push it in the new coroutine substack.
However, once pushed in the substack: the reference is not useful
anymore and should be cleared.
Currently, this is not the case in hlua_register_task().

Explicitly dropping the reference once the function is pushed to the
coroutine's stack to prevent any reference leak (which could contribute
to resource shortage)

This may be backported to every stable versions.
Please note that this commit depends on "MINOR: hlua: add simple hlua reference handling API"

MINOR: hlua: fix return type for hlua_checkfunction() and hlua_checktable()

hlua_checktable() and hlua_checkfunction() both return the raw
value of luaL_ref() function call.
As luaL_ref() returns a signed int, both functions should return a signed
int as well to prevent any misuse of the returned reference value.

MINOR: hlua: add simple hlua reference handling API

We're doing this in an attempt to simplify temporary lua objects
references handling.

Adding the new hlua_unref() function to release lua object references
created using luaL_ref(, LUA_REGISTRYINDEX)
(ie: hlua_checkfunction() and hlua_checktable())

Failure to release unused object reference prevents the reference index
from being re-used and prevents the referred ressource from being garbage
collected.

Adding hlua_pushref(L, ref) to replace
lua_rawgeti(L, LUA_REGISTRYINDEX, ref)

Adding hlua_ref(L) to replace luaL_ref(L, LUA_REGISTRYINDEX)

CLEANUP: hlua: fix conflicting comment in hlua_ctx_destroy()

The comment for the hlua_ctx_destroy() function states that the "lua"
struct is not freed.

This is not true anymore since 2c8b54e7 ("MEDIUM: lua: remove Lua struct
from session, and allocate it with memory pools")

Updating the function comment to properly report the actual behavior.

This could be backported in every stable versions with 2c8b54e7
("MEDIUM: lua: remove Lua struct from session, and allocate it with memory pools")

MEDIUM: hlua_fcn/api: remove some old server and proxy attributes

Since ("MINOR: hlua_fcn: alternative to old proxy and server attributes"):
 - s->name(), s->puid() are superseded by s->get_name() and s->get_puid()
 - px->name(), px->uuid() are superseded by px->get_name() and
   px->get_uuid()

And considering this is now the proper way to retrieve proxy name/uuid
and server name/puid from lua:

We're now removing such legacy attributes, but for retro-compatibility
purposes we will be emulating them and warning the user for some time
before completely dropping their support.

To do this, we first remove old legacy code.
Then we move server and proxy methods out of the metatable to allow
direct elements access without systematically involving the "__index"
metamethod.

This allows us to involve the "__index" metamethod only when the requested
key is missing from the table.

Then we define relevant hlua_proxy_index and hlua_server_index functions
that will be used as the "__index" metamethod to respectively handle
"name, uuid" (proxy) or "name, puid" (server) keys, in which case we
warn the user about the need to use the new getter function instead the
legacy attribute (to prepare for the potential upcoming removal), and we
call the getter function to return the value as if the getter function
was directly called from the script.

Note: Using the legacy variables instead of the getter functions results
in a slight overhead due to the "__index" metamethod indirection, thus
it is recommended to switch to the getter functions right away.

With this commit we're also adding a deprecation notice about legacy
attributes.

MEDIUM: hlua_fcn: dynamic server iteration and indexing

This patch proposes to enumerate servers using internal HAProxy list.
Also, remove the flag SRV_F_NON_PURGEABLE which makes the server non
purgeable each time Lua uses the server.

Removing reg-tests/cli_delete_server_lua.vtc since this test is no
longer relevant (we don't set the SRV_F_NON_PURGEABLE flag anymore)
and we already have a more generic test:
  reg-tests/server/cli_delete_server.vtc

Co-authored-by: Aurelien DARRAGON <adarragon@haproxy.com>

MINOR: hlua_fcn: alternative to old proxy and server attributes

This patch adds new lua methods:
- "Proxy.get_uuid()"
- "Proxy.get_name()"
- "Server.get_puid()"
- "Server.get_name()"

These methods will be equivalent to their old analog Proxy.{uuid,name}
and Server.{puid,name} attributes, but this will be the new preferred
way to fetch such infos as it duplicates memory only when necessary and
thus reduce the overall lua Server/Proxy objects memory footprint.

Legacy attributes (now superseded by the explicit getters) are expected
to be removed some day.

Co-authored-by: Aurelien DARRAGON <adarragon@haproxy.com>

MEDIUM: hlua: Dynamic list of frontend/backend in Lua

When HAproxy is loaded with a lot of frontends/backends (tested with 300k),
it is slow to start and it uses a lot of memory just for indexing backends
in the lua tables.

This patch uses the internal frontend/backend index of HAProxy in place of
lua table.

HAProxy startup is now quicker as each frontend/backend object is created
on demand and not at init.
This has to come with some cost: the execution of Lua will be a little bit
slower.

MINOR: hlua: Fix two functions that return nothing useful

Two lua init function seems to return something useful, but it
is not the case. The function "hlua_concat_init" seems to return
a failure status, but the function never fails. The function
"hlua_fcn_reg_core_fcn" seems to return a number of elements in
the stack, but it is not the case.

BUG/MINOR: hlua: enforce proper running context for register_x functions

register_{init, converters, fetches, action, service, cli, filter} are
meant to run exclusively from body context according to the
documentation (unlike register_task which is designed to work from both
init and runtime contexts)

A quick code inspection confirms that only register_task implements
the required precautions to make it safe out of init context.

Trying to use those register_* functions from a runtime lua task will
lead to a program crash since they all assume that they are running from
the main lua context and with no concurrent runs:

    core.register_task(function()
      core.register_init(function()
      end)
    end)

When loaded from the config, the above example would segfault.

To prevent this undefined behavior, we now report an explicit error if
the user tries to use such functions outside of init/body context.

This should be backported in every stable versions.
[prior to 2.5 lua filter API did not exist, the hlua_register_filter()
part should be skipped]

MINOR: hlua: properly handle hlua_process_task HLUA_E_ETMOUT

In hlua_process_task: when HLUA_E_ETMOUT was returned by
hlua_ctx_resume(), meaning that the lua task reached
tune.lua.task-timeout (default: none),
we logged "Lua task: unknown error." before stopping the task.

Now we properly handle HLUA_E_ETMOUT to report a meaningful error
message.

BUG/MINOR: hlua: hook yield does not behave as expected

In function hlua_hook, a yieldk is performed when function is yieldable.

But the following code in that function seems to assume that the yield
never returns, which is not the case!

Moreover, Lua documentation says that in this situation the yieldk call
must immediately be followed by a return.

This patch adds a return statement after the yieldk call.
It also adds some comments and removes a needless lua_sethook call.

It could be backported to all stable versions, but it is not mandatory,
because even if it is undefined behavior this bug doesn't seem to
negatively affect lua 5.3/5.4 stacks.

MINOR: server: correctly free servers on deinit()

srv_drop() function is reponsible for freeing the server when the
refcount reaches 0.
There is one exception: when global.mode has the MODE_STOPPING flag set,
srv_drop() will ignore the refcount and free the server on first
invocation.

This logic has been implemented with 13f2e2ce ("BUG/MINOR: server: do
not use refcount in free_server in stopping mode") and back then doing
so was not a problem since dynamic server API was just implemented and
srv_take() and srv_drop() were not widely used.

Now that dynamic server API is starting to get more popular we cannot
afford to keep the current logic: some modules or lua scripts may hold
references to existing server and also do their cleanup in deinit phases

In this kind of situation, it would be easy to trigger double-frees
since every call to srv_drop() on a specific server will try to free it.

To fix this, we take a different approach and try to fix the issue at
the source: we now properly drop server references involved with
checks/agent_checks in deinit_srv_check() and deinit_srv_agent_check().

While this could theorically be backported up to 2.6, it is not very
relevant for now since srv_drop() usage in older versions is very
limited and we're only starting to face the issue in mid 2.8
developments. (ie: lua core updates)

MINOR: server: always call ssl->destroy_srv when available

In srv_drop(), we only call the ssl->destroy_srv() method on
specific conditions.

But this has two downsides:

First, destroy_srv() is reponsible for freeing data that may have been
allocated in prepare_srv(), but not exclusively: it also frees
ssl-related parameters allocated when parsing a server entry, such as
ca-file for instance.

So this is quite error-prone, we could easily miss a condition where
some data needs to be deallocated using destroy_srv() even if
prepare_srv() was not used (since prepare_srv() is also conditional),
thus resulting in memory leaks.

Moreover, depending on srv->proxy to guard the check is probably not
a good idea here, since srv_drop() could be called in late de-init paths
in which related proxy could be freed already. srv_drop() should only
take care of freeing local server data without external logic.

Thankfully, destroy_srv() function performs the necessary checks to
ensure that a systematic call to the function won't result in invalid
reads or double frees.

No backport needed.

BUG/MINOR: log: free log forward proxies on deinit()

Proxies belonging to the cfg_log_forward proxy list are not cleaned up
in haproxy deinit() function.
We add the missing cleanup directly in the main deinit() function since
no other specific function may be used for this.

This could be backported up to 2.4

BUG/MINOR: sink: free forward_px on deinit()

When a ring section is configured, a new sink is created and forward_px
proxy may be allocated and assigned to the sink.
Such sink-related proxies are added to the sink_proxies_list and thus
don't belong to the main proxy list which is cleaned up in
haproxy deinit() function.

We don't have to manually clean up sink_proxies_list in the main deinit()
func:
sink API already provides the sink_deinit() function so we just add the
missing free_proxy(sink->forward_px) there.

This could be backported up to 2.4.
[in 2.4, commit b0281a49 ("MINOR: proxy: check if p is NULL in free_proxy()")
must be backported first]

BUG/MINOR: stats: properly handle server stats dumping resumption

In stats_dump_proxy_to_buffer() function, special care was taken when
dealing with servers dump.
Indeed, stats_dump_proxy_to_buffer() can be interrupted and resumed if
buffer space is not big enough to complete dump.
Thus, a reference is taken on the server being dumped in the hope that
the server will still be valid when the function resumes.
(to prevent the server from being freed in the meantime)

While this is now true thanks to:
-  "BUG/MINOR: server/del: fix legacy srv->next pointer consistency"

We still have an issue: when resuming, saved server reference is not
dropped.
This prevents the server from being freed when we no longer use it.
Moreover, as the saved server might now be deleted
(SRV_F_DELETED flag set), the current deleted server may still be dumped
in the stats and while this is not a bug, this could be misleading for
the user.

Let's add a px_st variable to detect if the stats_dump_proxy_to_buffer()
is being resumed at the STAT_PX_ST_SV stage: perform some housekeeping
to skip deleted servers and properly drop the reference on the saved
server.

This commit depends on:
 - "MINOR: server: add SRV_F_DELETED flag"
 - "BUG/MINOR: server/del: fix legacy srv->next pointer consistency"

This should be backported up to 2.6

BUG/MINOR: server/del: fix srv->next pointer consistency

We recently discovered a bug which affects dynamic server deletion:

When a server is deleted, it is removed from the "visible" server list.
But as we've seen in previous commit
("MINOR: server: add SRV_F_DELETED flag"), it can still be accessed by
someone who keeps a reference on it (waiting for the final srv_drop()).
Throughout this transient state, server ptr is still valid (may be
dereferenced) and the flag SRV_F_DELETED is set.

However, as the server is not part of server list anymore, we have
an issue: srv->next pointer won't be updated anymore as the only place
where we perform such update is in cli_parse_delete_server() by
iterating over the "visible" server list.

Because of this, we cannot guarantee that a server with the
SRV_F_DELETED flag has a valid 'next' ptr: 'next' could be pointing
to a fully removed (already freed) server.

This problem can be easily demonstrated with server dumping in
the stats:

server list dumping is performed in stats_dump_proxy_to_buffer()
The function can be interrupted and resumed later by design.
ie: output buffer is full: partial dump and finish the dump after
the flush

This is implemented by calling srv_take() on the server being dumped,
and only releasing it when we're done with it using srv_drop().
(drop can be delayed after function resume if buffer is full)

While the function design seems OK, it works with the assumption that
srv->next will still be valid after the function resumes, which is
not true. (especially if multiple servers are being removed in between
the 2 dumping attempts)

In practice, this did not cause any crash yet (at least this was not
reported so far), because server dumping is so fast that it is very
unlikely that multiple server deletions make their way between 2
dumping attempts in most setups. But still, this is a problem that we
need to address because some upcoming work might depend on this
assumption as well and for the moment it is not safe at all.

========================================================================

Here is a quick reproducer:

With this patch, we're creating a large deletion window of 3s as soon
as we reach a server named "t2" while iterating over the list.

This will give us plenty of time to perform multiple deletions before
the function is resumed.

    |  diff --git a/src/stats.c b/src/stats.c
    |  index 84a4f9b6e..15e49b4cd 100644
    |  --- a/src/stats.c
    |  +++ b/src/stats.c
    |  @@ -3189,11 +3189,24 @@ int stats_dump_proxy_to_buffer(struct stconn *sc, struct htx *htx,
    |                    * Temporarily increment its refcount to prevent its
    |                    * anticipated cleaning. Call free_server to release it.
    |                    */
    |  +                struct server *orig = ctx->obj2;
    |                   for (; ctx->obj2 != NULL;
    |                          ctx->obj2 = srv_drop(sv)) {
    |
    |                           sv = ctx->obj2;
    |  +                        printf("sv = %s\n", sv->id);
    |                           srv_take(sv);
    |  +                        if (!strcmp("t2", sv->id) && orig == px->srv) {
    |  +                                printf("deletion window: 3s\n");
    |  +                                thread_idle_now();
    |  +                                thread_harmless_now();
    |  +                                sleep(3);
    |  +                                thread_harmless_end();
    |  +
    |  +                                thread_idle_end();
    |  +
    |  +                                goto full; /* simulate full buffer */
    |  +                        }
    |
    |                           if (htx) {
    |                                   if (htx_almost_full(htx))
    |  @@ -4353,6 +4366,7 @@ static void http_stats_io_handler(struct appctx *appctx)
    |           struct channel *res = sc_ic(sc);
    |           struct htx *req_htx, *res_htx;
    |
    |  +        printf("http dump\n");
    |           /* only proxy stats are available via http */
    |           ctx->domain = STATS_DOMAIN_PROXY;
    |

Ok, we're ready, now we start haproxy with the following conf:

       global
          stats socket /tmp/ha.sock  mode 660  level admin  expose-fd listeners thread 1-1
          nbthread 2

       frontend stats
           mode http
           bind *:8081 thread 2-2
           stats enable
           stats uri /

       backend farm
               server t1 127.0.0.1:1899  disabled
               server t2 127.0.0.1:18999 disabled
               server t3 127.0.0.1:18998 disabled
               server t4 127.0.0.1:18997 disabled

And finally, we execute the following script:

       curl localhost:8081/stats&
       sleep .2
       echo "del server farm/t2" | nc -U /tmp/ha.sock
       echo "del server farm/t3" | nc -U /tmp/ha.sock

This should be enough to reveal the issue, I easily manage to
consistently crash haproxy with the following reproducer:

    http dump
    sv = t1
    http dump
    sv = t1
    sv = t2
    deletion window = 3s
    [NOTICE]   (2940566) : Server deleted.
    [NOTICE]   (2940566) : Server deleted.
    http dump
    sv = t2
    sv = �����U
    [1]    2940566 segmentation fault (core dumped)  ./haproxy -f ttt.conf

========================================================================

To fix this, we add prev_deleted mt_list in server struct.
For a given "visible" server, this list will contain the pending
"deleted" servers references that point to it using their 'next' ptr.

This way, whenever this "visible" server is going to be deleted via
cli_parse_delete_server() it will check for servers in its
'prev_deleted' list and update their 'next' pointer so that they no
longer point to it, and then it will push them in its
'next->prev_deleted' list to transfer the update responsibility to the
next 'visible' server (if next != NULL).

Then, following the same logic, the server about to be removed in
cli_parse_delete_server() will push itself as well into its
'next->prev_deleted' list (if next != NULL) so that it may still use its
'next' ptr for the time it is in transient removal state.

In srv_drop(), right before the server is finally freed, we make sure
to remove it from the 'next->prev_deleted' list so that 'next' won't
try to perform the pointers update for this server anymore.
This has to be done atomically to prevent 'next' srv from accessing a
purged server.

As a result:
  for a valid server, either deleted or not, 'next' ptr will always
  point to a non deleted (ie: visible) server.

With the proposed fix, and several removal combinations (including
unordered cli_parse_delete_server() and srv_drop() calls), I cannot
reproduce the crash anymore.

Example tricky removal sequence that is now properly handled:

sv list: t1,t2,t3,t4,t5,t6

ops:
   take(t2)
   del(t4)
   del(t3)
   del(t5)
   drop(t3)
   drop(t4)
   drop(t5)
   drop(t2)

MINOR: server: add SRV_F_DELETED flag

Set the SRV_F_DELETED flag when server is removed from the cli.

When removing a server from the cli (in cli_parse_delete_server()),
we update the "visible" server list so that the removed server is no
longer part of the list.

However, despite the server being removed from "visible" server list,
one could still access the server data from a valid ptr (ie: srv_take())

Deleted flag helps detecting when a server is in transient removal
state: that is, removed from the list, thus not visible but not yet
purged from memory.

MINOR: stconn/applet: Add BUG_ON_HOT() to be sure SE_FL_EOS is never set alone

SE_FL_EOS flag must never be set on the SE descriptor without SE_FL_EOI or
SE_FL_ERROR. When a mux or an applet report an end of stream, it must be
able to state if it is the end of input too or if it is an error.

Because all this part was recently refactored, especially the applet part,
it is a bit sensitive. Thus a BUG_ON_HOT() is used and not a BUG_ON().

MINOR: tree-wide: Simplifiy some tests on SHUT flags by accessing SCs directly

At many places, we simplify the tests on SHUT flags to remove calls to
chn_prod() or chn_cons() function because the corresponding SC is available.

MEDIUM: tree-wide: Move flags about shut from the channel to the SC

The purpose of this patch is only a one-to-one replacement, as far as
possible.

CF_SHUTR(_NOW) and CF_SHUTW(_NOW) flags are now carried by the
stream-connecter. CF_ prefix is replaced by SC_FL_ one. Of course, it is not
so simple because at many places, we were testing if a channel was shut for
reads and writes in same time. To do the same, shut for reads must be tested
on one side on the SC and shut for writes on the other side on the opposite
SC. A special care was taken with process_stream(). flags of SCs must be
saved to be able to detect changes, just like for the channels.

MINOR: stconn/channel: Move CF_EOI into the SC and rename it

The channel flag CF_EOI is renamed to SC_FL_EOI and moved into the
stream-connector.

MEDIUM: http_client: Use the sedesc to report and detect end of processing

Just like for other applets, we now use the SE descriptor instead of the
channel to report error and end-of-stream. Here, the applet is a bit
refactored to handle SE descriptor EOS, EOI and ERROR flags

MEDIUM: promex: Use the sedesc to report and detect end of processing

Just like for other applets, we now use the SE descriptor instead of the
channel to report error and end-of-stream.

MEDIUM: stats: Use the sedesc to report and detect end of processing

Just like for other applets, we now use the SE descriptor instead of the
channel to report error and end-of-stream.

MEDIUM: sink: Use the sedesc to report and detect end of processing

Just like for other applets, we now use the SE descriptor instead of the
channel to report error and end-of-stream.

MINOR: sink: Remove the tests on the opposite SC state to process messages

The state of the opposite SC is already tested to wait the connection is
established before sending messages. So, there is no reason to test it again
before looping on the ring buffer.

MEDIUM: peers: Use the sedesc to report and detect end of processing

Just like for other applets, we now use the SE descriptor instead of the
channel to report error and end-of-stream. We must just be sure to consume
request data when we are waiting the applet to be released.

MEDIUM: log: Use the sedesc to report and detect end of processing

Just like for other applets, we now use the SE descriptor instead of the
channel to report error and end-of-stream.

Here, the refactoring only reports errors by setting SE_FL_ERROR flag.

MEDIUM: hlua/applet: Use the sedesc to report and detect end of processing

There are 3 kinds of applet in lua: The co-sockets, the TCP services and the
HTTP services. The three are refactored to use the SE descriptor instead of
the channel to report error and end-of-stream.

MEDIUM: spoe: Use the sedesc to report and detect end of processing

Just like for other applets, we now use the SE descriptor instead of the
channel to report error and end-of-stream. We must just be sure to consume
request data when we are waiting the applet to be released.

This patch is bit different than others because messages handling is
dispatched in several functions. But idea if the same.

MEDIUM: dns: Use the sedesc to report and detect end of processing

It is now the dns turn to be refactored to use the SE descriptor instead of
the channel to report error and end-of-stream. We must just be sure to
consume request data when we are waiting the applet to be released.

MINOR: dns: Remove the test on the opposite SC state to send requests

The state of the opposite SC is already tested to wait the connection is
established before sending requests. So, there is no reason to test it again
before looping on the ring buffer.

MEDIUM: cli: Use the sedesc to report and detect end of processing

It is the same kind of change than for the cache applet. Idea is to use the SE
desc instead of the channel or the SC to report end-of-input, end-of-stream and
errors.

Truncated commands are now reported on error. Other changes are the same than
for the cache applet. We now set SE_FL_EOS flag instead of calling cf_shutr()
and calls to cf_shutw are removed.

MEDIUM: cache: Use the sedesc to report and detect end of processing

We now try, as far as possible, to rely on the SE descriptor to detect end
of processing. Idea is to no longer rely on the channel or the SC to do so.

First, we now set SE_FL_EOS instead of calling and cf_shutr() to report the
end of the stream. It happens when the response is fully sent (SE_FL_EOI is
already set in this case) or when an error is reported. In this last case,
SE_FL_ERROR is also set.

Thanks to this change, it is now possible to detect the applet must only
consume the request waiting for the upper layer releases it. So, if
SE_FL_EOS or SE_FL_ERROR are set, it means the reponse was fully
handled. And if SE_FL_SHR or SE_FL_SHW are set, it means the applet was
released by upper layer and is waiting to be freed.

MINOR: stconn/applet: Handle EOS in the applet .wake callback function

Just like for end of input, the end of stream reported by the endpoint
(SE_FL_EOS flag) is now handled in sc_applet_process(). The idea is to have
applets acting as muxes by reporting events through the SE descriptor, as
far as possible.

MINOR: applet: No longer set EOI on the SC

Thanks to the previous patch, it is now possible for applets to not set the
CF_EOI flag on the channels. On this point, the applets get closer to the
muxes.

MINOR: stconn/applet: Handle EOI in the applet .wake callback function

The end of input reported by the endpoint (SE_FL_EOI flag), is now handled
in sc_applet_process(). This function is always called after an applet was
called. So, the applets can now only report EOI on the SE descriptor and
have no reason to update the channel too.

MINOR: stconn: Always ack EOS at the end of sc_conn_recv()

EOS is now acknowledge at the end of sc_conn_recv(), even if an error was
encountered. There is no reason to not do so, especially because, if it not
performed here, it will be ack in sc_conn_process().

Note, it is still performed in sc_conn_process() because this function is
also the .wake callback function and can be directly called from the lower
layer.

MINOR: mux-h1: Report an error to the SE descriptor on truncated message

On truncated message, a parsing error is still reported. But an error on the
SE descriptor is also reported. This will avoid any bugs in future. We are
know sure the SC is able to detect the error, independently on the HTTP
analyzers.

CLEANUP: mux-h1/mux-pt: Remove useless test on SE_FL_SHR/SE_FL_SHW flags

It is already performed by the called, sc_conn_shutr() and
sc_conn_shutw(). So there is no reason to still test these flags in the PT
and H1 muxes.

BUG/MINOR: mux-h1: Properly report EOI/ERROR on read0 in h1_rcv_pipe()

In h1_rcv_pipe(), only the end of stream was reported when a read0 was
detected. However, it is also important to report the end of input or an
error, depending on the message state. This patch does not fix any real
issue for now, but some others, specific to the 2.8, rely on it.

No backport needed.

MINOR: mux-pt: Report end-of-input with the end-of-stream after a read

In the PT multiplexer, the end of stream is also the end of input. Thus
we must report EOI to the stream-endpoint descriptor when the EOS is
reported. For now, it is a bit useless but it will be important to
disginguish an shutdown to an error to an abort.

To be sure to not report an EOI on an error, the errors are now handled
first.

MINOR: stconn/channel: Move CF_EXPECT_MORE into the SC and rename it

The channel flag CF_EXPECT_MORE is renamed to SC_FL_SND_EXP_MORE and moved
into the stream-connector.

MINOR: stconn/channel: Move CF_NEVER_WAIT into the SC and rename it

The channel flag CF_NEVER_WAIT is renamed to SC_FL_SND_NEVERWAIT and moved
into the stream-connector.

MINOR: stconn/channel: Move CF_SEND_DONTWAIT into the SC and rename it

The channel flag CF_SEND_DONTWAIT is renamed to SC_FL_SND_ASAP and moved
into the stream-connector.

MINOR: stconn/channel: Move CF_READ_DONTWAIT into the SC and rename it

The channel flag CF_READ_DONTWAIT is renamed to SC_FL_RCV_ONCE and moved
into the stream-connector.

MINOR: stconn: Remove unecessary test on SE_FL_EOS before receiving data

In sc_conn_recv(), if the EOS is reported by the endpoint, it will always be
acknowledged by the SC and a read0 will be performed on the input
channel. Thus there is no reason to still test at the begining of the
function because there is already a test on CF_SHUTR.

BUG/MEDIUM: dns: Properly handle error when a response consumed

When a response is consumed, result for co_getblk() is never checked. It
seems ok because amount of output data is always checked first. But There is
an issue when we try to get the first 2 bytes to read the message length. If
there is only one byte followed by a shutdown, the applet ignore the
shutdown and loop till the timeout to get more data.

So to avoid any issue and improve shutdown detection, the co_getblk() return
value is always tested. In addition, if there is not enough data, the applet
explicitly ask for more data by calling applet_need_more_data().

This patch relies on the previous one:

   * BUG/MEDIUM: channel: Improve reports for shut in co_getblk()

Both should be backported as far as 2.4. On 2.5 and 2.4,
applet_need_more_data() must be replaced by si_rx_endp_more().

BUG/MEDIUM: channel: Improve reports for shut in co_getblk()

When co_getblk() is called with a length and an offset to 0, shutdown is
never reported. It may be an issue when the function is called to retrieve
all available output data, while there is no output data at all. And it
seems pretty annoying to handle this case in the caller.

Thus, now, in co_getblk(), -1 is returned when the channel is empty and a
shutdown was received.

There is no real reason to backport this patch alone. However, another fix
will rely on it.

CLEANUP: stconn: Remove remaining debug messages

It is now possible to enable traces for applets. Thus we can remove annoying
debug messages (DPRINTF) to track calls to applets.

MEDIUM: applet/trace: Register a new trace source with its events

Traces are now supported for applets. The first argument is always the
appctx. This will help to debug applets.

MINOR: applet: Uninline appctx_free()

This functin is uninlined and move in src/applet.c. It is mandatory to add
traces for applets.

BUG/MINOR: stream: Fix test on channels flags to set clientfin/serverfin touts

There is a bug in a way the channels flags are checked to set clientfin or
serverfin timeout. Indeed, to set the clientfin timeout, the request channel
must be shut for reads (CF_SHUTR) or the response channel must be shut for
writes (CF_SHUTW). As the opposite, the serverfin timeout must be set when
the request channel is shut for writes (CF_SHUTW) or the response channel is
shut for reads (CF_SHUTR).

It is a 2.8-dev specific issue. No backport needed.

BUG/MEDIUM: stconn: Add a missing return statement in sc_app_shutr()

In the commut b08c5259e ("MINOR: stconn: Always report READ/WRITE event on
shutr/shutw"), a return statement was erroneously removed from
sc_app_shutr(). As a consequence, CF_SHUTR flags was never set. Fortunately,
it is the default .shutr callback function. Thus when a connection or an
applet is attached to the SC, another callback is used to performe a
shutdown for reads.

It is a 28-dev specific issue. No backport needed.

BUG/MINOR: tcpcheck: Be able to expect an empty response

It is not possible to successfully match an empty response. However using
regex, it should be possible to reject response with any content. For
instance:

   tcp-check expect !rstring ".+"

It may seem a be strange to do that, but it is possible, it is a valid
config. So it must work. Thanks to this patch, it is now really supported.

This patch may be backported as far as 2.2. But only if someone ask for it.

BUG/MINOR: quic: Possible wrong PTO computing

As timestamps based on now_ms values are used to compute the probing timeout,
they may wrap. So, use ticks API to compared them.

Must be backported to 2.7 and 2.6.

BUILD: quic: 32bits compilation issue in cli_io_handler_dump_quic()

Replaced a %zu printf format by %llu for an uint64_t.

Must be backported to 2.7.

BUG/MINOR: quic: Wrong idle timer expiration (during 20s)

This this commit, this is ->idle_expire of quic_conn struct which must
be taken into an account to display the idel timer task expiration value:

     MEDIUM: quic: Ack delay implementation

Furthermore, this value was always zero until now_ms has wrapped (20 s after
the start time) due to this commit:
     MEDIUM: clock: force internal time to wrap early after boot

Do not rely on the value of now_ms compared to ->idle_expire to display the
difference but use ticks_remain() to compute it.

Must be backported to 2.7 where "show quic" has already been backported.

BUG/MINOR: quic: Unexpected connection closures upon idle timer task execution

This bug arrived with this commit:

      MEDIUM: quic: Ack delay implementation

It is possible that the idle timer task was already in the run queue when its
->expire field was updated calling qc_idle_timer_do_rearm(). To prevent this
task from running in this condition, one must check its ->expire field value
with this condition to run the task if its timer has really expired:

!tick_is_expired(t->expire, now_ms)

Furthermore, as this task may be directly woken up with a call to task_wakeup()
all, for instance by qc_kill_conn() to kill the connection, one must check this
task has really been woken up when it was in the wait queue and not by a direct
call to task_wakeup() thanks to this test:

(state & TASK_WOKEN_ANY) == TASK_WOKEN_TIMER

Again, when this condition is not fulfilled, the task must be run.

Must be backported where the commit mentionned above was backported.

MINOR: quic: Add trace to debug idle timer task issues

Add TRACE_PROTO() call where this is relevant to debug issues about
qc_idle_timer_task() issues.

Must be backported to 2.6 and 2.7.

DOC: config: strict-sni allows to start without certificate

The strict-sni keyword allows to start without certificate on a bind
line.

Must be backported as far as 2.2.

MINOR: http-act: emit a warning when a header field name contains forbidden chars

As found in issue #2089, it's easy to mistakenly paste a colon in a
header name, or other chars (e.g. spaces) when quotes are in use, and
this causes all sort of trouble in field because such chars are rejected
by the peer.

Better try to detect these upfront. That's what we're doing here during
the parsing of the add-header/set-header/early-hint actions, where a
warning is emitted if a non-token character is found in a header name.
A special case is made for the colon at the beginning so that it remains
possible to place any future pseudo-headers that may appear. E.g:

  [WARNING]  (14388) : config : parsing [badchar.cfg:23] : header name 'X-Content-Type-Options:' contains forbidden character ':'.

This should be backported to 2.7, and ideally it should be turned to an
error in future versions.

BUG/MINOR: quic: Remove useless BUG_ON() in newreno and cubic algo implementation

As now_ms may be zero, these BUG_ON() could be triggered when its value has wrapped.
These call to BUG_ON() may be removed because the values they was supposed to
check are safely used by the ticks API.

Must be backported to 2.6 and 2.7.

BUG/MINOR: ssl: Undefined reference when building with OPENSSL_NO_DEPRECATED

If OPENSSL_NO_DEPRECATED is set, we get a 'error: ‘RSA_PKCS1_PADDING’
undeclared' when building jwt.c. The symbol is not deprecated, we are
just missing an include.

This was raised in GitHub issue #2098.
It does not need to be backported.

BUG/MAJOR: quic: Congestion algorithms states shared between the connection

This very old bug is there since the first implementation of newreno congestion
algorithm implementation. This was a very bad idea to put a state variable
into quic_cc_algo struct which only defines the congestion control algorithm used
by a QUIC listener, typically its type and its callbacks.
This bug could lead to crashes since BUG_ON() calls have been added to each algorithm
implementation. This was revealed by interop test, but not very often as there was
not very often several connections run at the time during these tests.

Hopefully this was also reported by Tristan in GH #2095.

Move the congestion algorithm state to the correct structures which are private
to a connection (see cubic and nr structs).

Must be backported to 2.7 and 2.6.

MINOR: quic: Add missing traces in cubic algorithm implementation

May be useful to debug.

Must be backported to 2.7 and 2.6.

BUG/MINOR: quic: Cubic congestion control window may wrap

Add a check to prevent the cubic congestion control from wrapping (very low risk)
in slow start callback.

Must be backported to 2.6 and 2.7.

BUG/MINOR: quic: Remaining useless statements in cubic slow start callback

When entering a recovery period, the algo state is set by quic_enter_recovery().
And that's it!. These two lines should have been removed with this commit:

     BUG/MINOR: quic: Wrong use of now_ms timestamps (cubic algo)

Take the opportunity of this patch to add a missing TRACE_LEAVE() call in
quic_cc_cubic_ca_cb().

Must be backported to 2.7 and 2.6.

CI: exclude doc/{design-thoughts,internals} from spell check

as those directories do contain many documents written in French,
codespell is catching a lot of false positives scanning them.

CLEANUP: assorted typo fixes in the code and comments

This is 35th iteration of typo fixes

CI: run smoke tests on config syntax to check memory related issues

config syntax check seems add a value on testing code path not
covered by VTest, also checks are very fast

MINOR: quic: Add a fake congestion control algorithm named "nocc"

This algorithm does nothing except initializing the congestion control window
to a fixed value. Very smart!

Modify the QUIC congestion control configuration parser to support this new
algorithm. The congestion control algorithm must be set as follows:

     quic-cc-algo nocc-<cc window size(KB))

For instance if "nocc-15" is provided as quic-cc-algo keyword value, this
will set a fixed window of 15KB.

MINOR: cli: support filtering on FD types in "show fd"

Depending on what we're debugging, some FDs can represent pollution in
the "show fd" output. Here we add a set of filters allowing to pick (or
exclude) any combination of listener, frontend conn, backend conn, pipes,
etc. "show fd l" will only list listening connections for example.

BUG/MINOR: quic: Wrong rtt variance computing

In ->srtt quic_loss struct this is 8*srtt which is stored so that not to have to multiply/devide
it to compute the RTT variance (at least). This is where there was a bug in quic_loss_srtt_update():
each time ->srtt must be used, it must be devided by 8 or right shifted by 3.
This bug had a very bad impact for network with non negligeable packet loss.

Must be backported to 2.6 and 2.7.

MEDIUM: quic: Ack delay implementation

Reuse the idle timeout task to delay the acknowledgments. The time of the
idle timer expiration is for now on stored in ->idle_expire. The one to
trigger the acknowledgements is stored in ->ack_expire.
Add QUIC_FL_CONN_ACK_TIMER_FIRED new connection flag to mark a connection
as having its acknowledgement timer been triggered.
Modify qc_may_build_pkt() to prevent the sending of "ack only" packets and
allows the connection to send packet when the ack timer has fired.
It is possible that acks are sent before the ack timer has triggered. In
this case it is cancelled only if ACK frames are really sent.
The idle timer expiration must be set again when the ack timer has been
triggered or when it is cancelled.

Must be backported to 2.7.

MINOR: quic: Traces adjustments at proto level.

Dump variables displayed by TRACE_ENTER() or TRACE_LEAVE() by calls to TRACE_PROTO().
No more variables are displayed by the two former macros. For now on, these information
are accessible from proto level.
Add new calls to TRACE_PROTO() at important locations in relation whith QUIC transport
protocol.
When relevant, try to prefix such traces with TX or RX keyword to identify the
concerned subpart (transmission or reception) of the protocol.

Must be backported to 2.7.

MINOR: quic: Adjustments for generic control congestion traces

Display the elapsed time since packets were sent in place of the timestamp which
do not bring easy to read information.

Must be backported to 2.7.