BUG/MEDIUM: resolvers: Track api calls with a counter to free resolutions
The kill list introduced in commit f766ec6b5 ("MEDIUM: resolvers: use a kill
list to preserve the list consistency") contains a bug. The deatch_row must
be initialized before calling resolv_process_responses() function. However,
this function is called for the dns code. The death_row is not visible from
the outside. So, it is possible to add a resolution in an uninitialized
death_row, leading to a crash.
But, with the current implementation, it is not possible to handle the
death_row in resolv_process_responses() function because, internally, the
kill list may be freed via a call to resolv_unlink_resolution(). At the end,
we are unable to determine all call chains to guarantee a safe use of the
kill list. It is a shameful observation, but unfortunatly true.
So, to make the fix simple, we track all calls to the public resolvers
api. A counter is incremented when we enter in the resolver code and
decremented when we leave it. This way, we are able to track the recursions
to init and release the kill list only once, at the edge.
Following functions are incrementing/decrementing the recurse counter:
* resolv_trigger_resolution()
* resolv_srvrq_expire_task()
* resolv_link_resolution()
* resolv_unlink_resolution()
* resolv_detach_from_resolution_answer_items()
* resolv_process_responses()
* process_resolvers()
* resolvers_finalize_config()
* resolv_action_do_resolve()
This patch should fix the issue #1404. It must be backported everywhere the
above commit was backported.
(cherry picked from commit 9ed1a0601d6b5ccaf6d410fee4d5d4b49d36652a)
Signed-off-by: Willy Tarreau <w@1wt.eu>
diff --git a/src/resolvers.c b/src/resolvers.c
index 826ed8d..db20a07 100644
--- a/src/resolvers.c
+++ b/src/resolvers.c
@@ -53,6 +53,7 @@
struct list resolv_srvrq_list = LIST_HEAD_INIT(resolv_srvrq_list);
static THREAD_LOCAL struct list death_row; /* list of deferred resolutions to kill, local validity only */
+static THREAD_LOCAL unsigned int recurse = 0; /* counter to track calls to public functions */
static THREAD_LOCAL uint64_t resolv_query_id_seed = 0; /* random seed */
struct resolvers *curr_resolvers = NULL;
@@ -65,6 +66,8 @@
static struct task *process_resolvers(struct task *t, void *context, unsigned int state);
static void resolv_free_resolution(struct resolv_resolution *resolution);
static void _resolv_unlink_resolution(struct resolv_requester *requester);
+static void enter_resolver_code();
+static void leave_resolver_code();
enum {
DNS_STAT_ID,
@@ -453,12 +456,16 @@
res = req->resolution;
resolvers = res->resolvers;
+ enter_resolver_code();
+
/* The resolution must not be triggered yet. Use the cached response, if
* valid */
exp = tick_add(res->last_resolution, resolvers->hold.valid);
if (resolvers->t && (res->status != RSLV_STATUS_VALID ||
!tick_isset(res->last_resolution) || tick_is_expired(exp, now_ms)))
task_wakeup(resolvers->t, TASK_WOKEN_OTHER);
+
+ leave_resolver_code();
}
@@ -577,13 +584,16 @@
/* Reinitialize the list of aborted resolutions before calling certain
* functions relying on it. The list must be processed by calling
- * free_aborted_resolutions() after operations.
+ * leave_resolver_code() after operations.
*/
-static void init_aborted_resolutions()
+static void enter_resolver_code()
{
- LIST_INIT(&death_row);
+ if (!recurse)
+ LIST_INIT(&death_row);
+ recurse++;
}
+/* Add a resolution to the death_row. */
static void abort_resolution(struct resolv_resolution *res)
{
LIST_DEL_INIT(&res->list);
@@ -591,16 +601,20 @@
}
/* This releases any aborted resolution found in the death row. It is mandatory
- * to call init_aborted_resolutions() first before the function (or loop) that
+ * to call enter_resolver_code() first before the function (or loop) that
* needs to defer deletions. Note that some of them are in relation via internal
* objects and might cause the deletion of other ones from the same list, so we
* must absolutely not use a list_for_each_entry_safe() nor any such thing here,
* and solely rely on each call to remove the first remaining list element.
*/
-static void free_aborted_resolutions()
+static void leave_resolver_code()
{
struct resolv_resolution *res;
+ recurse--;
+ if (recurse)
+ return;
+
while (!LIST_ISEMPTY(&death_row)) {
res = LIST_NEXT(&death_row, struct resolv_resolution *, list);
resolv_free_resolution(res);
@@ -615,7 +629,7 @@
* obsolete.
*
* Must be called with the DNS lock held, and with the death_row already
- * initialized via init_aborted_resolutions().
+ * initialized via enter_resolver_code().
*/
static void resolv_srvrq_cleanup_srv(struct server *srv)
{
@@ -649,19 +663,18 @@
if (!tick_is_expired(t->expire, now_ms))
goto end;
+ enter_resolver_code();
HA_SPIN_LOCK(DNS_LOCK, &srv->srvrq->resolvers->lock);
- init_aborted_resolutions();
resolv_srvrq_cleanup_srv(srv);
- free_aborted_resolutions();
HA_SPIN_UNLOCK(DNS_LOCK, &srv->srvrq->resolvers->lock);
+ leave_resolver_code();
end:
return t;
}
/* Checks for any obsolete record, also identify any SRV request, and try to
- * find a corresponding server. Must be called with the death_row already
- * initialized via init_aborted_resolutions().
+ * find a corresponding server.
*/
static void resolv_check_response(struct resolv_resolution *res)
{
@@ -1930,8 +1943,7 @@
}
/* Links a requester (a server or a resolv_srvrq) with a resolution. It returns 0
- * on success, -1 otherwise. Must be called with the death_row already initialized
- * via init_aborted_resolutions().
+ * on success, -1 otherwise.
*/
int resolv_link_resolution(void *requester, int requester_type, int requester_locked)
{
@@ -1944,6 +1956,7 @@
char **hostname_dn;
int hostname_dn_len, query_type;
+ enter_resolver_code();
switch (requester_type) {
case OBJ_TYPE_SERVER:
srv = (struct server *)requester;
@@ -2014,25 +2027,24 @@
req->resolution = res;
LIST_APPEND(&res->requesters, &req->list);
+ leave_resolver_code();
return 0;
err:
if (res && LIST_ISEMPTY(&res->requesters))
resolv_free_resolution(res);
+ leave_resolver_code();
return -1;
}
-/* This function removes all server/srvrq references on answer items
- * if <safe> is set to 1, in case of srvrq, sub server resquesters unlink
- * is called using safe == 1 to make it usable into callbacks. Must be called
- * with the death_row already initialized via init_aborted_resolutions().
- */
+/* This function removes all server/srvrq references on answer items. */
void resolv_detach_from_resolution_answer_items(struct resolv_resolution *res, struct resolv_requester *req)
{
struct resolv_answer_item *item, *itemback;
struct server *srv, *srvback;
struct resolv_srvrq *srvrq;
+ enter_resolver_code();
if ((srv = objt_server(req->owner)) != NULL) {
LIST_DEL_INIT(&srv->ip_rec_item);
}
@@ -2046,12 +2058,11 @@
}
}
}
+ leave_resolver_code();
}
/* Removes a requester from a DNS resolution. It takes takes care of all the
* consequences. It also cleans up some parameters from the requester.
- * if <safe> is set to 1, the corresponding resolution is not released. Must be
- * called with the death_row already initialized via init_aborted_resolutions().
*/
static void _resolv_unlink_resolution(struct resolv_requester *requester)
{
@@ -2102,9 +2113,9 @@
/* The public version of the function above that deals with the death row. */
void resolv_unlink_resolution(struct resolv_requester *requester)
{
- init_aborted_resolutions();
+ enter_resolver_code();
_resolv_unlink_resolution(requester);
- free_aborted_resolutions();
+ leave_resolver_code();
}
/* Called when a network IO is generated on a name server socket for an incoming
@@ -2113,8 +2124,6 @@
* - ensure the DNS packet received is valid and call requester's callback
* - call requester's error callback if invalid response
* - check the dn_name in the packet against the one sent
- *
- * Must be called with the death_row already initialized via init_aborted_resolutions().
*/
static int resolv_process_responses(struct dns_nameserver *ns)
{
@@ -2132,6 +2141,7 @@
int keep_answer_items;
resolvers = ns->parent;
+ enter_resolver_code();
HA_SPIN_LOCK(DNS_LOCK, &resolvers->lock);
/* process all pending input messages */
@@ -2300,7 +2310,7 @@
}
resolv_update_resolvers_timeout(resolvers);
HA_SPIN_UNLOCK(DNS_LOCK, &resolvers->lock);
-
+ leave_resolver_code();
return buflen;
}
@@ -2314,6 +2324,7 @@
struct resolv_resolution *res, *resback;
int exp;
+ enter_resolver_code();
HA_SPIN_LOCK(DNS_LOCK, &resolvers->lock);
/* Handle all expired resolutions from the active list. Elements that
@@ -2321,7 +2332,6 @@
* ones will be handled normally.
*/
- init_aborted_resolutions();
res = LIST_NEXT(&resolvers->resolutions.curr, struct resolv_resolution *, list);
while (&res->list != &resolvers->resolutions.curr) {
resback = LIST_NEXT(&res->list, struct resolv_resolution *, list);
@@ -2404,12 +2414,11 @@
LIST_APPEND(&resolvers->resolutions.wait, &res->list);
}
}
-
- /* now we can purge all queued deletions */
- free_aborted_resolutions();
-
resolv_update_resolvers_timeout(resolvers);
HA_SPIN_UNLOCK(DNS_LOCK, &resolvers->lock);
+
+ /* now we can purge all queued deletions */
+ leave_resolver_code();
return t;
}
@@ -2422,7 +2431,7 @@
struct resolv_requester *req, *reqback;
struct resolv_srvrq *srvrq, *srvrqback;
- init_aborted_resolutions();
+ enter_resolver_code();
list_for_each_entry_safe(resolvers, resolversback, &sec_resolvers, list) {
list_for_each_entry_safe(ns, nsback, &resolvers->nameservers, list) {
free(ns->id);
@@ -2480,7 +2489,7 @@
free(srvrq);
}
- free_aborted_resolutions();
+ leave_resolver_code();
}
/* Finalizes the DNS configuration by allocating required resources and checking
@@ -2493,7 +2502,7 @@
struct proxy *px;
int err_code = 0;
- init_aborted_resolutions();
+ enter_resolver_code();
/* allocate pool of resolution per resolvers */
list_for_each_entry(resolvers, &sec_resolvers, list) {
@@ -2588,10 +2597,10 @@
if (err_code & (ERR_ALERT|ERR_ABORT))
goto err;
- free_aborted_resolutions();
+ leave_resolver_code();
return err_code;
err:
- free_aborted_resolutions();
+ leave_resolver_code();
resolvers_deinit();
return err_code;
@@ -2881,7 +2890,7 @@
resolvers = rule->arg.resolv.resolvers;
- init_aborted_resolutions();
+ enter_resolver_code();
/* we have a response to our DNS resolution */
use_cache:
@@ -2965,7 +2974,7 @@
ret = ACT_RET_YIELD;
end:
- free_aborted_resolutions();
+ leave_resolver_code();
if (locked)
HA_SPIN_UNLOCK(DNS_LOCK, &resolvers->lock);
return ret;