From: Aurelien DARRAGON <adarragon@haproxy.com>
Date: Thu, 9 Mar 2023 10:56:14 +0000 (+0100)
Subject: MINOR: server: correctly free servers on deinit()
X-Git-Tag: v2.8-dev7~61
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=32483ecaac4195f56910dd2dc3f18b1fe243b42e;p=thirdparty%2Fhaproxy.git

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)
---

diff --git a/src/check.c b/src/check.c
index 0f7d198597..f39f88dd94 100644
--- a/src/check.c
+++ b/src/check.c
@@ -1786,8 +1786,11 @@ int init_srv_agent_check(struct server *srv)
 
 static void deinit_srv_check(struct server *srv)
 {
-	if (srv->check.state & CHK_ST_CONFIGURED)
+	if (srv->check.state & CHK_ST_CONFIGURED) {
 		free_check(&srv->check);
+		/* it is safe to drop now since the main server reference is still held by the proxy */
+		srv_drop(srv);
+	}
 	srv->check.state &= ~CHK_ST_CONFIGURED & ~CHK_ST_ENABLED;
 	srv->do_check = 0;
 }
@@ -1795,8 +1798,11 @@ static void deinit_srv_check(struct server *srv)
 
 static void deinit_srv_agent_check(struct server *srv)
 {
-	if (srv->agent.state & CHK_ST_CONFIGURED)
+	if (srv->agent.state & CHK_ST_CONFIGURED) {
 		free_check(&srv->agent);
+		/* it is safe to drop now since the main server reference is still held by the proxy */
+		srv_drop(srv);
+	}
 
 	srv->agent.state &= ~CHK_ST_CONFIGURED & ~CHK_ST_ENABLED & ~CHK_ST_AGENT;
 	srv->do_agent = 0;
diff --git a/src/server.c b/src/server.c
index 9c12288126..f18fc5b2b1 100644
--- a/src/server.c
+++ b/src/server.c
@@ -2408,7 +2408,7 @@ void srv_take(struct server *srv)
 
 /* Deallocate a server <srv> and its member. <srv> must be allocated. For
  * dynamic servers, its refcount is decremented first. The free operations are
- * conducted only if the refcount is nul, unless the process is stopping.
+ * conducted only if the refcount is nul.
  *
  * As a convenience, <srv.next> is returned if srv is not NULL. It may be useful
  * when calling srv_drop on the list of servers.
@@ -2425,10 +2425,8 @@ struct server *srv_drop(struct server *srv)
 	/* For dynamic servers, decrement the reference counter. Only free the
 	 * server when reaching zero.
 	 */
-	if (likely(!(global.mode & MODE_STOPPING))) {
-		if (HA_ATOMIC_SUB_FETCH(&srv->refcount, 1))
-			goto end;
-	}
+	if (HA_ATOMIC_SUB_FETCH(&srv->refcount, 1))
+		goto end;
 
 	/* make sure we are removed from our 'next->prev_deleted' list
 	 * This doesn't require full thread isolation as we're using mt lists