[Asterisk-code-review] bundled pjproject: Crashes while resolving DNS names. (asterisk[13])
Joshua Colp
asteriskteam at digium.com
Mon Oct 31 11:37:55 CDT 2016
Joshua Colp has submitted this change and it was merged. ( https://gerrit.asterisk.org/4228 )
Change subject: bundled pjproject: Crashes while resolving DNS names.
......................................................................
bundled pjproject: Crashes while resolving DNS names.
PJPROJECT 2.5.5 introduced a race condition with the -r5349 IPv6 DNS
patch.
The patches below fix the DNS lookup race condition crash caused by
attempting to send the same message twice for the single DNS lookup.
0006-r5471-svn-backport-Various-fixes-for-DNS-IPv6.patch
0006-r5473-svn-backport-Fix-pending-query.patch
The patch below removes a cached DNS response from the hash table when
another thread is referencing the old entry. The table still contained
the entry when it was destroyed which can result in inexplicable crashes.
0006-r5475-svn-backport-Remove-DNS-cache-entry.patch
ASTERISK-26344 #close
Reported by: Ian Gilmour
ASTERISK-26387 #close
Reported by: Harley Peters
Change-Id: I17fde80359e66f65a91341ceca58d914d0f61cc4
---
A third-party/pjproject/patches/0006-r5471-svn-backport-Various-fixes-for-DNS-IPv6.patch
A third-party/pjproject/patches/0006-r5473-svn-backport-Fix-pending-query.patch
A third-party/pjproject/patches/0006-r5475-svn-backport-Remove-DNS-cache-entry.patch
3 files changed, 232 insertions(+), 0 deletions(-)
Approvals:
George Joseph: Looks good to me, approved
Anonymous Coward #1000019: Verified
Joshua Colp: Looks good to me, but someone else must approve
diff --git a/third-party/pjproject/patches/0006-r5471-svn-backport-Various-fixes-for-DNS-IPv6.patch b/third-party/pjproject/patches/0006-r5471-svn-backport-Various-fixes-for-DNS-IPv6.patch
new file mode 100644
index 0000000..98c33e5
--- /dev/null
+++ b/third-party/pjproject/patches/0006-r5471-svn-backport-Various-fixes-for-DNS-IPv6.patch
@@ -0,0 +1,134 @@
+From 2ab7a9f67caf73be3f2215473f72882cfaef4972 Mon Sep 17 00:00:00 2001
+From: Richard Mudgett <rmudgett at digium.com>
+Date: Fri, 28 Oct 2016 12:11:30 -0500
+Subject: [PATCH 1/3] r5471 svn backport Various fixes for DNS IPv6
+
+Fixed #1974: Various fixes for DNS IPv6
+---
+ pjlib-util/src/pjlib-util/resolver.c | 11 +++++------
+ pjlib-util/src/pjlib-util/srv_resolver.c | 17 +++++++++++++++--
+ pjsip/src/pjsip/sip_resolve.c | 14 +++++++-------
+ 3 files changed, 27 insertions(+), 15 deletions(-)
+
+diff --git a/pjlib-util/src/pjlib-util/resolver.c b/pjlib-util/src/pjlib-util/resolver.c
+index e5e1bed..d24ef9d 100644
+--- a/pjlib-util/src/pjlib-util/resolver.c
++++ b/pjlib-util/src/pjlib-util/resolver.c
+@@ -835,7 +835,7 @@ PJ_DEF(pj_status_t) pj_dns_resolver_start_query( pj_dns_resolver *resolver,
+ pj_time_val now;
+ struct res_key key;
+ struct cached_res *cache;
+- pj_dns_async_query *q;
++ pj_dns_async_query *q, *p_q = NULL;
+ pj_uint32_t hval;
+ pj_status_t status = PJ_SUCCESS;
+
+@@ -849,9 +849,6 @@ PJ_DEF(pj_status_t) pj_dns_resolver_start_query( pj_dns_resolver *resolver,
+ /* Check type */
+ PJ_ASSERT_RETURN(type > 0 && type < 0xFFFF, PJ_EINVAL);
+
+- if (p_query)
+- *p_query = NULL;
+-
+ /* Build resource key for looking up hash tables */
+ init_res_key(&key, type, name);
+
+@@ -970,10 +967,12 @@ PJ_DEF(pj_status_t) pj_dns_resolver_start_query( pj_dns_resolver *resolver,
+ pj_hash_set_np(resolver->hquerybyres, &q->key, sizeof(q->key),
+ 0, q->hbufkey, q);
+
+- if (p_query)
+- *p_query = q;
++ p_q = q;
+
+ on_return:
++ if (p_query)
++ *p_query = p_q;
++
+ pj_mutex_unlock(resolver->mutex);
+ return status;
+ }
+diff --git a/pjlib-util/src/pjlib-util/srv_resolver.c b/pjlib-util/src/pjlib-util/srv_resolver.c
+index 02672aa..ff9c979 100644
+--- a/pjlib-util/src/pjlib-util/srv_resolver.c
++++ b/pjlib-util/src/pjlib-util/srv_resolver.c
+@@ -187,9 +187,12 @@ PJ_DEF(pj_status_t) pj_dns_srv_cancel_query(pj_dns_srv_async_query *query,
+ has_pending = PJ_TRUE;
+ }
+ if (srv->q_aaaa) {
+- pj_dns_resolver_cancel_query(srv->q_aaaa, PJ_FALSE);
++ /* Check if it is a dummy query. */
++ if (srv->q_aaaa != (pj_dns_async_query*)0x1) {
++ pj_dns_resolver_cancel_query(srv->q_aaaa, PJ_FALSE);
++ has_pending = PJ_TRUE;
++ }
+ srv->q_aaaa = NULL;
+- has_pending = PJ_TRUE;
+ }
+ }
+
+@@ -485,12 +488,22 @@ static pj_status_t resolve_hostnames(pj_dns_srv_async_query *query_job)
+ srv->common.type = PJ_DNS_TYPE_A;
+ srv->common_aaaa.type = PJ_DNS_TYPE_AAAA;
+ srv->parent = query_job;
++ srv->q_a = NULL;
++ srv->q_aaaa = NULL;
+
+ status = PJ_SUCCESS;
+
+ /* Start DNA A record query */
+ if ((query_job->option & PJ_DNS_SRV_RESOLVE_AAAA_ONLY) == 0)
+ {
++ if ((query_job->option & PJ_DNS_SRV_RESOLVE_AAAA) != 0) {
++ /* If there will be DNS AAAA query too, let's setup
++ * a dummy one here, otherwise app callback may be called
++ * immediately (before DNS AAAA query is sent) when
++ * DNS A record is available in the cache.
++ */
++ srv->q_aaaa = (pj_dns_async_query*)0x1;
++ }
+ status = pj_dns_resolver_start_query(query_job->resolver,
+ &srv->target_name,
+ PJ_DNS_TYPE_A, 0,
+diff --git a/pjsip/src/pjsip/sip_resolve.c b/pjsip/src/pjsip/sip_resolve.c
+index ed326ba..3f3654d 100644
+--- a/pjsip/src/pjsip/sip_resolve.c
++++ b/pjsip/src/pjsip/sip_resolve.c
+@@ -452,7 +452,7 @@ PJ_DEF(void) pjsip_resolve( pjsip_resolver_t *resolver,
+ }
+
+ /* Resolve DNS AAAA record if address family is not fixed to IPv4 */
+- if (af != pj_AF_INET()) {
++ if (af != pj_AF_INET() && status == PJ_SUCCESS) {
+ status = pj_dns_resolver_start_query(resolver->res,
+ &query->naptr[0].name,
+ PJ_DNS_TYPE_AAAA, 0,
+@@ -530,9 +530,9 @@ static void dns_a_callback(void *user_data,
+
+ ++srv->count;
+ }
+-
+- } else {
+-
++ }
++
++ if (status != PJ_SUCCESS) {
+ char errmsg[PJ_ERR_MSG_SIZE];
+
+ /* Log error */
+@@ -593,9 +593,9 @@ static void dns_aaaa_callback(void *user_data,
+
+ ++srv->count;
+ }
+-
+- } else {
+-
++ }
++
++ if (status != PJ_SUCCESS) {
+ char errmsg[PJ_ERR_MSG_SIZE];
+
+ /* Log error */
+--
+1.7.9.5
+
diff --git a/third-party/pjproject/patches/0006-r5473-svn-backport-Fix-pending-query.patch b/third-party/pjproject/patches/0006-r5473-svn-backport-Fix-pending-query.patch
new file mode 100644
index 0000000..4d11d57
--- /dev/null
+++ b/third-party/pjproject/patches/0006-r5473-svn-backport-Fix-pending-query.patch
@@ -0,0 +1,28 @@
+From 509d4339747f11cfbde3a0acc447ef5d521eea93 Mon Sep 17 00:00:00 2001
+From: Richard Mudgett <rmudgett at digium.com>
+Date: Fri, 28 Oct 2016 12:12:28 -0500
+Subject: [PATCH 2/3] r5473 svn backport Fix pending query
+
+Re #1974:
+If there is a pending query, set the return value to that query (instead of NULL)
+
+Thanks to Richard Mudgett for the patch.
+---
+ pjlib-util/src/pjlib-util/resolver.c | 1 +
+ 1 file changed, 1 insertion(+)
+
+diff --git a/pjlib-util/src/pjlib-util/resolver.c b/pjlib-util/src/pjlib-util/resolver.c
+index d24ef9d..fe687b7 100644
+--- a/pjlib-util/src/pjlib-util/resolver.c
++++ b/pjlib-util/src/pjlib-util/resolver.c
+@@ -940,6 +940,7 @@ PJ_DEF(pj_status_t) pj_dns_resolver_start_query( pj_dns_resolver *resolver,
+ /* Done. This child query will be notified once the "parent"
+ * query completes.
+ */
++ p_q = nq;
+ status = PJ_SUCCESS;
+ goto on_return;
+ }
+--
+1.7.9.5
+
diff --git a/third-party/pjproject/patches/0006-r5475-svn-backport-Remove-DNS-cache-entry.patch b/third-party/pjproject/patches/0006-r5475-svn-backport-Remove-DNS-cache-entry.patch
new file mode 100644
index 0000000..e378c30
--- /dev/null
+++ b/third-party/pjproject/patches/0006-r5475-svn-backport-Remove-DNS-cache-entry.patch
@@ -0,0 +1,70 @@
+From 46e1cfa18853a38b7fcdebad782710c5db676657 Mon Sep 17 00:00:00 2001
+From: Richard Mudgett <rmudgett at digium.com>
+Date: Fri, 28 Oct 2016 12:15:44 -0500
+Subject: [PATCH 3/3] r5475 svn backport Remove DNS cache entry
+
+Re #1974: Remove DNS cache entry from resolver's hash table when app callback has a reference.
+
+Thanks to Richard Mudgett for the patch.
+---
+ pjlib-util/src/pjlib-util/resolver.c | 29 +++++++++++++++--------------
+ 1 file changed, 15 insertions(+), 14 deletions(-)
+
+diff --git a/pjlib-util/src/pjlib-util/resolver.c b/pjlib-util/src/pjlib-util/resolver.c
+index fe687b7..52b7655 100644
+--- a/pjlib-util/src/pjlib-util/resolver.c
++++ b/pjlib-util/src/pjlib-util/resolver.c
+@@ -1444,10 +1444,12 @@ static void update_res_cache(pj_dns_resolver *resolver,
+ if (ttl > resolver->settings.cache_max_ttl)
+ ttl = resolver->settings.cache_max_ttl;
+
++ /* Get a cache response entry */
++ cache = (struct cached_res *) pj_hash_get(resolver->hrescache, key,
++ sizeof(*key), &hval);
++
+ /* If TTL is zero, clear the same entry in the hash table */
+ if (ttl == 0) {
+- cache = (struct cached_res *) pj_hash_get(resolver->hrescache, key,
+- sizeof(*key), &hval);
+ /* Remove the entry before releasing its pool (see ticket #1710) */
+ pj_hash_set(NULL, resolver->hrescache, key, sizeof(*key), hval, NULL);
+
+@@ -1457,24 +1459,23 @@ static void update_res_cache(pj_dns_resolver *resolver,
+ return;
+ }
+
+- /* Get a cache response entry */
+- cache = (struct cached_res *) pj_hash_get(resolver->hrescache, key,
+- sizeof(*key), &hval);
+ if (cache == NULL) {
+ cache = alloc_entry(resolver);
+- } else if (cache->ref_cnt > 1) {
+- /* When cache entry is being used by callback (to app), just decrement
+- * ref_cnt so it will be freed after the callback returns and allocate
+- * new entry.
+- */
+- cache->ref_cnt--;
+- cache = alloc_entry(resolver);
+ } else {
+ /* Remove the entry before resetting its pool (see ticket #1710) */
+ pj_hash_set(NULL, resolver->hrescache, key, sizeof(*key), hval, NULL);
+
+- /* Reset cache to avoid bloated cache pool */
+- reset_entry(&cache);
++ if (cache->ref_cnt > 1) {
++ /* When cache entry is being used by callback (to app),
++ * just decrement ref_cnt so it will be freed after
++ * the callback returns and allocate new entry.
++ */
++ cache->ref_cnt--;
++ cache = alloc_entry(resolver);
++ } else {
++ /* Reset cache to avoid bloated cache pool */
++ reset_entry(&cache);
++ }
+ }
+
+ /* Duplicate the packet.
+--
+1.7.9.5
+
--
To view, visit https://gerrit.asterisk.org/4228
To unsubscribe, visit https://gerrit.asterisk.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I17fde80359e66f65a91341ceca58d914d0f61cc4
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
More information about the asterisk-code-review
mailing list