From cb28d35bd214c551c9e7f119a9d2f463f5626ad3 Mon Sep 17 00:00:00 2001
From: Wouter Wijngaards <wouter@nlnetlabs.nl>
Date: Fri, 2 Feb 2018 10:33:19 +0000
Subject: [PATCH] - Fix lock race condition in dns cache dname synthesis.

git-svn-id: file:///svn/unbound/trunk@4495 be551aaa-1e26-0410-a405-d3ace91eadb9
---
 doc/Changelog                                |  1 +
 services/cache/dns.c                         | 48 +++++++++++++-------
 testdata/03-testbound.tdir/03-testbound.test |  2 +-
 3 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/doc/Changelog b/doc/Changelog
index 6e284156e..11aec5b35 100644
--- a/doc/Changelog
+++ b/doc/Changelog
@@ -1,6 +1,7 @@
 2 February 2018: Wouter
 	- Fix unfreed locks in log and arc4random at exit of unbound.
 	- unit test with valgrind
+	- Fix lock race condition in dns cache dname synthesis.
 
 1 February 2018: Wouter
 	- fix unaligned structure making a false positive in checklock
diff --git a/services/cache/dns.c b/services/cache/dns.c
index b725d8a34..af1bd1bd6 100644
--- a/services/cache/dns.c
+++ b/services/cache/dns.c
@@ -568,7 +568,7 @@ rrset_msg(struct ub_packed_rrset_key* rrset, struct regional* region,
 /** synthesize DNAME+CNAME response from cached DNAME item */
 static struct dns_msg*
 synth_dname_msg(struct ub_packed_rrset_key* rrset, struct regional* region, 
-	time_t now, struct query_info* q, struct module_env* env)
+	time_t now, struct query_info* q, enum sec_status* sec_status)
 {
 	struct dns_msg* msg;
 	struct ub_packed_rrset_key* ck;
@@ -580,19 +580,9 @@ synth_dname_msg(struct ub_packed_rrset_key* rrset, struct regional* region,
 		return NULL;
 	/* only allow validated (with DNSSEC) DNAMEs used from cache 
 	 * for insecure DNAMEs, query again. */
-	if(d->security != sec_status_secure) {
-		/* but if we have a CNAME cached with this name, then we
-		 * have previously already allowed this name to pass.
-		 * the next cache lookup is going to fetch that CNAME itself,
-		 * but it is better to have the (unsigned)DNAME + CNAME in
-		 * that case */
-		struct ub_packed_rrset_key* cname_rrset = rrset_cache_lookup(
-			env->rrset_cache, q->qname, q->qname_len,
-			LDNS_RR_TYPE_CNAME, q->qclass, 0, now, 0);
-		if(!cname_rrset)
-			return NULL;
-		lock_rw_unlock(&cname_rrset->entry.lock);
-	}
+	*sec_status = d->security;
+	/* return sec status, so the status of the CNAME can be checked
+	 * by the calling routine. */
 	msg = gen_dns_msg(region, q, 2); /* DNAME + CNAME RRset */
 	if(!msg)
 		return NULL;
@@ -759,13 +749,37 @@ dns_cache_lookup(struct module_env* env,
 		(rrset=find_closest_of_type(env, qname, qnamelen, qclass, now,
 		LDNS_RR_TYPE_DNAME, 1))) {
 		/* synthesize a DNAME+CNAME message based on this */
+		enum sec_status sec_status = sec_status_unchecked;
 		struct dns_msg* msg = synth_dname_msg(rrset, region, now, &k,
-			env);
+			&sec_status);
 		if(msg) {
+			struct ub_packed_rrset_key* cname_rrset;
+			lock_rw_unlock(&rrset->entry.lock);
+			/* now, after unlocking the DNAME rrset lock,
+			 * check the sec_status, and see if we need to look
+			 * up the CNAME record associated before it can
+			 * be used */
+			/* normally, only secure DNAMEs allowed from cache*/
+			if(sec_status == sec_status_secure)
+				return msg;
+			/* but if we have a CNAME cached with this name, then we
+			 * have previously already allowed this name to pass.
+			 * the next cache lookup is going to fetch that CNAME itself,
+			 * but it is better to have the (unsigned)DNAME + CNAME in
+			 * that case */
+			cname_rrset = rrset_cache_lookup(
+				env->rrset_cache, qname, qnamelen,
+				LDNS_RR_TYPE_CNAME, qclass, 0, now, 0);
+			if(cname_rrset) {
+				/* CNAME already synthesized by
+				 * synth_dname_msg routine, so we can
+				 * straight up return the msg */
+				lock_rw_unlock(&cname_rrset->entry.lock);
+				return msg;
+			}
+		} else {
 			lock_rw_unlock(&rrset->entry.lock);
-			return msg;
 		}
-		lock_rw_unlock(&rrset->entry.lock);
 	}
 
 	/* see if we have CNAME for this domain,
diff --git a/testdata/03-testbound.tdir/03-testbound.test b/testdata/03-testbound.tdir/03-testbound.test
index 1998be13e..d0138d3ef 100644
--- a/testdata/03-testbound.tdir/03-testbound.test
+++ b/testdata/03-testbound.tdir/03-testbound.test
@@ -107,7 +107,7 @@ for input in $PRE/testdata/*.rpl $PRE/testdata/*.crpl; do
 		if grep "All heap blocks were freed -- no leaks are possible" tmpout >/dev/null 2>&1; then
 			:  # clean
 		else
-			cat tmpout
+			grep "^==" tmpout
 			echo "Memory leaked in $cleaninput"
 			grep "in use at exit" tmpout
 			exitval=1
-- 
2.47.3