- Fix lock race condition in dns cache dname synthesis.

git-svn-id: file:///svn/unbound/trunk@4495 be551aaa-1e26-0410-a405-d3ace91eadb9

diff --git a/doc/Changelog b/doc/Changelog
index 6e284156e6b31b27cd0490e67e38463b8fcd476e..11aec5b35a804186c4eeb0af9e33fea5610f7f20 100644 (file)
--- 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 b725d8a34e93ad48d88ece9c3454d6bbf0e73403..af1bd1bd66ae4e1aaf5c5de314017571d2f81b28 100644 (file)
--- 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 1998be13e9e753d4179b06e7c75d39350b3d7602..d0138d3ef7967bcf3b1fbbb01ea9997cb38e38f8 100644 (file)
--- 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