MEDIUM: servers: make the server deletion code run under full thread isolation

In 2.4, runtime server deletion was brought by commit e558043e1 ("MINOR:
server: implement delete server cli command"). A comment remained in the
code about a theoretical race between the thread_isolate() call and another
thread being in the process of allocating memory before accessing the
server via a reference that was grabbed before the memory allocation,
since the thread_harmless_now()/thread_harmless_end() pair around mmap()
may have the effect of allowing cli_parse_delete_server() to proceed.

Now that the full thread isolation is available, let's update the code
to rely on this. Now it is guaranteed that competing threads will either
be in the poller or queued in front of thread_isolate_full().

This may be backported to 2.4 if any report of breakage suggests the bug
really exists, in which case the two following patches will also be
needed:
  MINOR: threads: make thread_release() not wait for other ones to complete
  MEDIUM: threads: add a stronger thread_isolate_full() call

diff --git a/src/server.c b/src/server.c
index 7bef5bf7f0789b35570ed1538af4bd0f8dd3ec2c..d5fceb9a790929f23ac75135ad32b58b34a0097b 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -4631,19 +4631,12 @@ static int cli_parse_delete_server(char **args, char *payload, struct appctx *ap
                return cli_err(appctx, "Require 'backend/server'.");
 
        /* The proxy servers list is currently not protected by a lock so this
-        * requires thread isolation.
+        * requires thread isolation. In addition, any place referencing the
+        * server about to be deleted would be unsafe after our operation, so
+        * we must be certain to be alone so that no other thread has even
+        * started to grab a temporary reference to this server.
         */
-
-       /* WARNING there is maybe a potential violation of the thread isolation
-        * mechanism by the pool allocator. The allocator marks the thread as
-        * harmless before the allocation, but a processing outside of it could
-        * relies on a particular server triggered at the same time by a
-        * 'delete server'. Currently, it is unknown if such case is present in
-        * the current code. If it happens to be, the thread isolation
-        * mechanism should be improved, maybe with a differentiation between
-        * read and read+write safe sections.
-        */
-       thread_isolate();
+       thread_isolate_full();
 
        get_backend_server(be_name, sv_name, &be, &srv);
        if (!be) {