[Asterisk-code-review] dns: Fix crash when invoking cancel in DNS recurring unit test. (asterisk[master])

[Joshua Colp]

Joshua Colp has uploaded a new change for review.

  https://gerrit.asterisk.org/772

Change subject: dns: Fix crash when invoking cancel in DNS recurring unit test.
......................................................................

dns: Fix crash when invoking cancel in DNS recurring unit test.

The recurring unit test expects the user data on a DNS query
created as a result of a recurring DNS query to be the recurring
structure itself. This is true, mostly. When invoking the user
provided callback this user data is changed to the user provided
data. This presents a race condition where the data may or may
not point to the recurring data.

This change simplifies the callback of the user provided callback
by creating a new query and populating it with the expected values.
This leaves the recurring DNS query alone and fixes the race
condition. This is more in line with how the API should be used
overall.

ASTERISK-25222 #close

Change-Id: I10fb6deec025dff097157e7ec17e6e4921778478
---
M main/dns_recurring.c
1 file changed, 15 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/72/772/1

diff --git a/main/dns_recurring.c b/main/dns_recurring.c
index 855273f..9925755 100644
--- a/main/dns_recurring.c
+++ b/main/dns_recurring.c
@@ -73,10 +73,21 @@
 static void dns_query_recurring_resolution_callback(const struct ast_dns_query *query)
 {
 	struct ast_dns_query_recurring *recurring = ast_dns_query_get_data(query);
+	struct ast_dns_query *callback_query;
 
-	/* Replace the user data so the actual callback sees what it provided */
-	((struct ast_dns_query*)query)->user_data = ao2_bump(recurring->user_data);
-	recurring->callback(query);
+	/* Create a separate query to invoke the user specific callback on as the
+	 * recurring query user data may get used externally (by the unit test)
+	 * and thus changing it is problematic
+	 */
+	callback_query = dns_query_alloc(query->name, query->rr_type, query->rr_class,
+		recurring->callback, recurring->user_data);
+	if (callback_query) {
+		/* The result is immutable at this point and can be safely provided */
+		callback_query->result = query->result;
+		callback_query->callback(callback_query);
+		callback_query->result = NULL;
+		ao2_ref(callback_query, -1);
+	}
 
 	ao2_lock(recurring);
 	/* So.. if something has not externally cancelled this we can reschedule based on the TTL */
@@ -87,7 +98,7 @@
 		if (ttl) {
 			recurring->timer = ast_sched_add(ast_dns_get_sched(), ttl * 1000, dns_query_recurring_scheduled_callback, ao2_bump(recurring));
 			if (recurring->timer < 0) {
-				/* It is impossible for this to be the last reference as this callback function holds a reference itself */
+				/* It is impossible for this to be the last reference as the query has a reference to it */
 				ao2_ref(recurring, -1);
 			}
 		}
@@ -95,9 +106,6 @@
 
 	ao2_replace(recurring->active, NULL);
 	ao2_unlock(recurring);
-
-	/* Since we stole the reference from the query we need to drop it ourselves */
-	ao2_ref(recurring, -1);
 }
 
 struct ast_dns_query_recurring *ast_dns_resolve_recurring(const char *name, int rr_type, int rr_class, ast_dns_resolve_callback callback, void *data)

-- 
To view, visit https://gerrit.asterisk.org/772
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I10fb6deec025dff097157e7ec17e6e4921778478
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp at digium.com>