mirrors/haproxy
1
0
Fork 0
mirror of https://git.haproxy.org/git/haproxy.git/ synced 2025-09-21 13:51:26 +02:00
Code Issues Projects Releases Wiki Activity

MINOR: server: correctly free servers on deinit()

Browse Source 
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)
This commit is contained in:
Aurelien DARRAGON 2023-03-09 11:56:14 +01:00 committed by Christopher Faulet
parent b5ee8bebfc
commit 32483ecaac
2 changed files with 11 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
src/check.c
View File

@ -1786,8 +1786,11 @@ int init_srv_agent_check(struct server *srv)
static void deinit_srv_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); 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->check.state &= ~CHK_ST_CONFIGURED & ~CHK_ST_ENABLED;
srv->do_check = 0; 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) 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); 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->agent.state &= ~CHK_ST_CONFIGURED & ~CHK_ST_ENABLED & ~CHK_ST_AGENT;
srv->do_agent = 0; srv->do_agent = 0;

8
src/server.c
View File

@ -2408,7 +2408,7 @@ void srv_take(struct server *srv)
/* Deallocate a server <srv> and its member. <srv> must be allocated. For /* Deallocate a server <srv> and its member. <srv> must be allocated. For
* dynamic servers, its refcount is decremented first. The free operations are * 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 * 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. * 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 /* For dynamic servers, decrement the reference counter. Only free the
* server when reaching zero. * server when reaching zero.
*/ */
if (likely(!(global.mode & MODE_STOPPING))) { if (HA_ATOMIC_SUB_FETCH(&srv->refcount, 1))
if (HA_ATOMIC_SUB_FETCH(&srv->refcount, 1)) goto end;
goto end;
}
/* make sure we are removed from our 'next->prev_deleted' list /* make sure we are removed from our 'next->prev_deleted' list
* This doesn't require full thread isolation as we're using mt lists * This doesn't require full thread isolation as we're using mt lists