BUG/MEDIUM: server: close a race around ready_srv when deleting a server

When a server is being disabled or deleted, in case it matches the
backend's ready_srv, this one is reset. However it's currently done in
a non-atomic way when the server goes down, and that could occasionally
reset the entry matching another server, but more importantly if in
parallel some requests are dequeued for that server, it may re-appear
there after having been removed, leading to a possible crash once it
is fully removed, as shown in issue #3177.

Let's make sure we reset the pointer when detaching the server from
the proxy, and use a CAS in both cases to only reset this server.

This fix needs to be backported to 3.2. There, srv_detach() is in
server.c instead of server.h. Thanks to Basha Mougamadou for the
detailed report and the useful backtraces.

diff --git a/include/haproxy/server.h b/include/haproxy/server.h
index e8e00a5e7a3f6c434ed6f39eadcf6d6f2ca6971d..ad5baef348247fa4c165f0763c8f36c2ed80f5cb 100644 (file)
--- a/include/haproxy/server.h
+++ b/include/haproxy/server.h
@@ -370,6 +370,8 @@ static inline void srv_detach(struct server *srv)
 
                prev->next = srv->next;
        }
+       /* reset the proxy's ready_srv if it was this one */
+       HA_ATOMIC_CAS(&px->ready_srv, &srv, NULL);
 }
 
 /* Returns a pointer to the first server matching id <id> in backend <bk>.

diff --git a/src/server.c b/src/server.c
index 2487e13eeaf1d2c9132dbbbb8fb3ea48a4b5b5b6..60d138c9574deb194c901b963532738ceb878c8d 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -7151,8 +7151,10 @@ static void srv_update_status(struct server *s, int type, int cause)
                 * If the server is no longer running, let's not pretend
                 * it can handle requests.
                 */
-               if (s->cur_state != SRV_ST_RUNNING && s->proxy->ready_srv == s)
-                       HA_ATOMIC_STORE(&s->proxy->ready_srv, NULL);
+               if (s->cur_state != SRV_ST_RUNNING) {
+                       struct server *srv = s;
+                       HA_ATOMIC_CAS(&s->proxy->ready_srv, &srv, NULL);
+               }
 
                s->last_change = ns_to_sec(now_ns);
                if (s->counters.shared.tg[tgid - 1])