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

[SVN commits to the Asterisk project]

Matt Jordan has submitted this change and it was merged.

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

Approvals:
  Anonymous Coward #1000019: Verified
  Matt Jordan: Looks good to me, approved; Verified



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: merged
Gerrit-Change-Id: I10fb6deec025dff097157e7ec17e6e4921778478
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>