BUG/MINOR: server: dont depend on proxy for server cleanup in srv_drop()

In commit b5ee8bebfc ("MINOR: server: always call ssl->destroy_srv when
available"), we made it so srv_drop() doesn't depend on proxy to perform
server cleanup.

It turns out this is now mandatory, because during deinit, free_proxy()
can occur before the final srv_drop(). This is the case when using Lua
scripts for instance.

In 2a9436f96 ("MINOR: lbprm: Add method to deinit server and proxy") we
added a freeing check under srv_drop() that depends on the proxy.
Because of that UAF may occur during deinit when using a Lua script that
manipulate server objects.

To fix the issue, let's perform the lbprm server deinit logic under
free_proxy() directly, where the DEINIT server hooks are evaluated.

Also, to prevent similar bugs in the future, let's explicitly document
in srv_drop() that server cleanups should assume that the proxy may
already be freed.

No backport needed unless 2a9436f96 is.

diff --git a/src/proxy.c b/src/proxy.c
index 1b4b9367d136f826f3d0e232ec57f8b22b9b00df..326ff3eb67df150b54f679b2261597f0b0c5c58b 100644 (file)
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -371,6 +371,10 @@ void deinit_proxy(struct proxy *p)
        while (s) {
                list_for_each_entry(srvdf, &server_deinit_list, list)
                        srvdf->fct(s);
+
+               if (p->lbprm.server_deinit)
+                       p->lbprm.server_deinit(s);
+
                s = srv_drop(s);
        }/* end while(s) */
 

diff --git a/src/server.c b/src/server.c
index 6b984029a4144435fa2ca4ef5c749f5803882822..8ccb1bc42061ca1f3ccea92eac07ab8633c06315 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -3113,6 +3113,9 @@ void srv_free_params(struct server *srv)
  * dynamic servers, its refcount is decremented first. The free operations are
  * conducted only if the refcount is nul.
  *
+ * A general rule is to assume that proxy may already be freed, so cleanup checks
+ * must not depend on the proxy
+ *
  * 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.
  */
@@ -3156,9 +3159,6 @@ struct server *srv_drop(struct server *srv)
 
        EXTRA_COUNTERS_FREE(srv->extra_counters);
 
-       if (srv->proxy->lbprm.server_deinit)
-               srv->proxy->lbprm.server_deinit(srv);
-
        ha_free(&srv);
 
  end: