3241. [bug] Address race conditions in the resolver code.

                        [RT #26889]

diff --git a/CHANGES b/CHANGES
index 230ec688f38457e5bbd17837c5f7bc69a02e641b..ef840b0f3e6524d2d7827e623e981716c7ce0f0e 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,3 +1,6 @@
+3241.  [bug]           Address race conditions in the resolver code.
+                       [RT #26889]
+
 3240.  [bug]           DNSKEY state change events could be missed. [RT #26874]
 
 3239.  [bug]           dns_dnssec_findmatchingkeys needs to use a consistent

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index 8b56aa21dc2916f7d87dc2dcc80edd56006fdbff..66fa4f861c840c2684662eef07953ea321bd5950 100644 (file)
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -15,7 +15,7 @@
  * PERFORMANCE OF THIS SOFTWARE.
  */
 
-/* $Id: resolver.c,v 1.445 2011/12/05 17:27:16 each Exp $ */
+/* $Id: resolver.c,v 1.446 2011/12/07 23:08:42 marka Exp $ */
 
 /*! \file */
 
@@ -453,7 +453,7 @@ static isc_result_t ncache_adderesult(dns_message_t *message,
                                      dns_rdataset_t *ardataset,
                                      isc_result_t *eresultp);
 static void validated(isc_task_t *task, isc_event_t *event);
-static void maybe_destroy(fetchctx_t *fctx, isc_boolean_t locked);
+static isc_boolean_t maybe_destroy(fetchctx_t *fctx, isc_boolean_t locked);
 static void add_bad(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo,
                    isc_result_t reason, badnstype_t badtype);
 
@@ -746,8 +746,11 @@ resquery_destroy(resquery_t **queryp) {
        INSIST(query->tcpsocket == NULL);
 
        query->fctx->nqueries--;
-       if (SHUTTINGDOWN(query->fctx))
-               maybe_destroy(query->fctx, ISC_FALSE);     /* Locks bucket. */
+       if (SHUTTINGDOWN(query->fctx)) {
+               dns_resolver_t *res = query->fctx->res;
+               if (maybe_destroy(query->fctx, ISC_FALSE))
+                       empty_bucket(res);
+       }
        query->magic = 0;
        isc_mem_put(query->mctx, query, sizeof(*query));
        *queryp = NULL;
@@ -2161,6 +2164,7 @@ fctx_finddone(isc_task_t *task, isc_event_t *event) {
        isc_boolean_t want_try = ISC_FALSE;
        isc_boolean_t want_done = ISC_FALSE;
        isc_boolean_t bucket_empty = ISC_FALSE;
+       isc_boolean_t destroy = ISC_FALSE;
        unsigned int bucketnum;
 
        find = event->ev_sender;
@@ -2172,6 +2176,9 @@ fctx_finddone(isc_task_t *task, isc_event_t *event) {
 
        FCTXTRACE("finddone");
 
+       bucketnum = fctx->bucketnum;
+       LOCK(&res->buckets[bucketnum].lock);
+
        INSIST(fctx->pending > 0);
        fctx->pending--;
 
@@ -2196,17 +2203,17 @@ fctx_finddone(isc_task_t *task, isc_event_t *event) {
                }
        } else if (SHUTTINGDOWN(fctx) && fctx->pending == 0 &&
                   fctx->nqueries == 0 && ISC_LIST_EMPTY(fctx->validators)) {
-               bucketnum = fctx->bucketnum;
-               LOCK(&res->buckets[bucketnum].lock);
                /*
                 * Note that we had to wait until we had the lock before
                 * looking at fctx->references.
                 */
                if (fctx->references == 0)
-                       bucket_empty = fctx_destroy(fctx);
-               UNLOCK(&res->buckets[bucketnum].lock);
+                       destroy = ISC_TRUE;
        }
+       UNLOCK(&res->buckets[bucketnum].lock);
 
+       if (destroy)
+               bucket_empty = fctx_destroy(fctx);
        isc_event_free(&event);
        dns_adb_destroyfind(&find);
 
@@ -3889,13 +3896,15 @@ clone_results(fetchctx_t *fctx) {
 
 /*
  * Destroy '*fctx' if it is ready to be destroyed (i.e., if it has
- * no references and is no longer waiting for any events).  If this
- * was the last fctx in the resolver, destroy the resolver.
+ * no references and is no longer waiting for any events).
  *
  * Requires:
  *      '*fctx' is shutting down.
+ *
+ * Returns:
+ *     true if the resolver is exiting and this is the last fctx in the bucket.
  */
-static void
+static isc_boolean_t
 maybe_destroy(fetchctx_t *fctx, isc_boolean_t locked) {
        unsigned int bucketnum;
        isc_boolean_t bucket_empty = ISC_FALSE;
@@ -3904,8 +3913,11 @@ maybe_destroy(fetchctx_t *fctx, isc_boolean_t locked) {
 
        REQUIRE(SHUTTINGDOWN(fctx));
 
+       bucketnum = fctx->bucketnum;
+       if (!locked)
+               LOCK(&res->buckets[bucketnum].lock);
        if (fctx->pending != 0 || fctx->nqueries != 0)
-               return;
+               goto unlock;
 
        for (validator = ISC_LIST_HEAD(fctx->validators);
             validator != NULL; validator = next_validator) {
@@ -3913,16 +3925,12 @@ maybe_destroy(fetchctx_t *fctx, isc_boolean_t locked) {
                dns_validator_cancel(validator);
        }
 
-       bucketnum = fctx->bucketnum;
-       if (!locked)
-               LOCK(&res->buckets[bucketnum].lock);
        if (fctx->references == 0 && ISC_LIST_EMPTY(fctx->validators))
                bucket_empty = fctx_destroy(fctx);
+ unlock:
        if (!locked)
                UNLOCK(&res->buckets[bucketnum].lock);
-
-       if (bucket_empty)
-               empty_bucket(res);
+       return (bucket_empty);
 }
 
 /*
@@ -3930,31 +3938,33 @@ maybe_destroy(fetchctx_t *fctx, isc_boolean_t locked) {
  */
 static void
 validated(isc_task_t *task, isc_event_t *event) {
-       isc_result_t result = ISC_R_SUCCESS;
-       isc_result_t eresult = ISC_R_SUCCESS;
-       isc_stdtime_t now;
-       fetchctx_t *fctx;
-       dns_validatorevent_t *vevent;
-       dns_fetchevent_t *hevent;
-       dns_rdataset_t *ardataset = NULL;
-       dns_rdataset_t *asigrdataset = NULL;
+       dns_adbaddrinfo_t *addrinfo;
        dns_dbnode_t *node = NULL;
-       isc_boolean_t negative;
-       isc_boolean_t chaining;
-       isc_boolean_t sentresponse;
-       isc_uint32_t ttl;
        dns_dbnode_t *nsnode = NULL;
+       dns_fetchevent_t *hevent;
        dns_name_t *name;
+       dns_rdataset_t *ardataset = NULL;
+       dns_rdataset_t *asigrdataset = NULL;
        dns_rdataset_t *rdataset;
        dns_rdataset_t *sigrdataset;
+       dns_resolver_t *res;
        dns_valarg_t *valarg;
-       dns_adbaddrinfo_t *addrinfo;
+       dns_validatorevent_t *vevent;
+       fetchctx_t *fctx;
+       isc_boolean_t chaining;
+       isc_boolean_t negative;
+       isc_boolean_t sentresponse;
+       isc_result_t eresult = ISC_R_SUCCESS;
+       isc_result_t result = ISC_R_SUCCESS;
+       isc_stdtime_t now;
+       isc_uint32_t ttl;
 
        UNUSED(task); /* for now */
 
        REQUIRE(event->ev_type == DNS_EVENT_VALIDATORDONE);
        valarg = event->ev_arg;
        fctx = valarg->fctx;
+       res = fctx->res;
        addrinfo = valarg->addrinfo;
        REQUIRE(VALID_FCTX(fctx));
        REQUIRE(!ISC_LIST_EMPTY(fctx->validators));
@@ -3964,7 +3974,7 @@ validated(isc_task_t *task, isc_event_t *event) {
 
        FCTXTRACE("received validation completion event");
 
-       LOCK(&fctx->res->buckets[fctx->bucketnum].lock);
+       LOCK(&res->buckets[fctx->bucketnum].lock);
 
        ISC_LIST_UNLINK(fctx->validators, vevent->validator, link);
        fctx->validator = NULL;
@@ -3974,7 +3984,7 @@ validated(isc_task_t *task, isc_event_t *event) {
         * destroy the fctx if necessary.
         */
        dns_validator_destroy(&vevent->validator);
-       isc_mem_put(fctx->res->buckets[fctx->bucketnum].mctx,
+       isc_mem_put(res->buckets[fctx->bucketnum].mctx,
                    valarg, sizeof(*valarg));
 
        negative = ISC_TF(vevent->rdataset == NULL);
@@ -3987,10 +3997,12 @@ validated(isc_task_t *task, isc_event_t *event) {
         * so, destroy the fctx.
         */
        if (SHUTTINGDOWN(fctx) && !sentresponse) {
-               isc_mutex_t *bucketlock =
-                       &fctx->res->buckets[fctx->bucketnum].lock;
-               maybe_destroy(fctx, ISC_TRUE);
-               UNLOCK(bucketlock);
+               isc_uint32_t bucketnum = fctx->bucketnum;
+               isc_boolean_t bucket_empty;
+               bucket_empty = maybe_destroy(fctx, ISC_TRUE);
+               UNLOCK(&res->buckets[bucketnum].lock);
+               if (bucket_empty)
+                       empty_bucket(res);
                goto cleanup_event;
        }
 
@@ -4039,7 +4051,7 @@ validated(isc_task_t *task, isc_event_t *event) {
 
        if (vevent->result != ISC_R_SUCCESS) {
                FCTXTRACE("validation failed");
-               inc_stats(fctx->res, dns_resstatscounter_valfail);
+               inc_stats(res, dns_resstatscounter_valfail);
                fctx->valfail++;
                fctx->vresult = vevent->result;
                if (fctx->vresult != DNS_R_BROKENCHAIN) {
@@ -4088,7 +4100,7 @@ validated(isc_task_t *task, isc_event_t *event) {
                result = fctx->vresult;
                add_bad(fctx, addrinfo, result, badns_validation);
                isc_event_free(&event);
-               UNLOCK(&fctx->res->buckets[fctx->bucketnum].lock);
+               UNLOCK(&res->buckets[fctx->bucketnum].lock);
                INSIST(fctx->validator == NULL);
                fctx->validator = ISC_LIST_HEAD(fctx->validators);
                if (fctx->validator != NULL)
@@ -4107,8 +4119,7 @@ validated(isc_task_t *task, isc_event_t *event) {
                             fctx->type == dns_rdatatype_dlv ||
                             fctx->type == dns_rdatatype_ds) &&
                             tresult == ISC_R_SUCCESS)
-                               dns_resolver_addbadcache(fctx->res,
-                                                        &fctx->name,
+                               dns_resolver_addbadcache(res, &fctx->name,
                                                         fctx->type, &expire);
                        fctx_done(fctx, result, __LINE__); /* Locks bucket. */
                } else
@@ -4121,7 +4132,7 @@ validated(isc_task_t *task, isc_event_t *event) {
                dns_rdatatype_t covers;
                FCTXTRACE("nonexistence validation OK");
 
-               inc_stats(fctx->res, dns_resstatscounter_valnegsuccess);
+               inc_stats(res, dns_resstatscounter_valnegsuccess);
 
                if (fctx->rmessage->rcode == dns_rcode_nxdomain)
                        covers = dns_rdatatype_any;
@@ -4138,10 +4149,9 @@ validated(isc_task_t *task, isc_event_t *event) {
                 * to zero to facilitate locating the containing zone of
                 * a arbitrary zone.
                 */
-               ttl = fctx->res->view->maxncachettl;
+               ttl = res->view->maxncachettl;
                if (fctx->type == dns_rdatatype_soa &&
-                   covers == dns_rdatatype_any &&
-                   fctx->res->zero_no_soa_ttl)
+                   covers == dns_rdatatype_any && res->zero_no_soa_ttl)
                        ttl = 0;
 
                result = ncache_adderesult(fctx->rmessage, fctx->cache, node,
@@ -4151,7 +4161,7 @@ validated(isc_task_t *task, isc_event_t *event) {
                        goto noanswer_response;
                goto answer_response;
        } else
-               inc_stats(fctx->res, dns_resstatscounter_valsuccess);
+               inc_stats(res, dns_resstatscounter_valsuccess);
 
        FCTXTRACE("validation OK");
 
@@ -4199,14 +4209,17 @@ validated(isc_task_t *task, isc_event_t *event) {
        }
 
        if (sentresponse) {
+               isc_boolean_t bucket_empty = ISC_FALSE;
                /*
                 * If we only deferred the destroy because we wanted to cache
                 * the data, destroy now.
                 */
                dns_db_detachnode(fctx->cache, &node);
-               UNLOCK(&fctx->res->buckets[fctx->bucketnum].lock);
                if (SHUTTINGDOWN(fctx))
-                       maybe_destroy(fctx, ISC_FALSE);    /* Locks bucket. */
+                       bucket_empty = maybe_destroy(fctx, ISC_TRUE);
+               UNLOCK(&res->buckets[fctx->bucketnum].lock);
+               if (bucket_empty)
+                       empty_bucket(res);
                goto cleanup_event;
        }
 
@@ -4221,7 +4234,7 @@ validated(isc_task_t *task, isc_event_t *event) {
                 * be validated.
                 */
                dns_db_detachnode(fctx->cache, &node);
-               UNLOCK(&fctx->res->buckets[fctx->bucketnum].lock);
+               UNLOCK(&res->buckets[fctx->bucketnum].lock);
                dns_validator_send(ISC_LIST_HEAD(fctx->validators));
                goto cleanup_event;
        }
@@ -4296,7 +4309,7 @@ validated(isc_task_t *task, isc_event_t *event) {
        if (node != NULL)
                dns_db_detachnode(fctx->cache, &node);
 
-       UNLOCK(&fctx->res->buckets[fctx->bucketnum].lock);
+       UNLOCK(&res->buckets[fctx->bucketnum].lock);
        fctx_done(fctx, result, __LINE__); /* Locks bucket. */
 
  cleanup_event: