From e4aac0596c89955ced3398d7e2896d05bcc70fd5 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Wed, 7 Dec 2011 23:08:42 +0000 Subject: [PATCH] 3241. [bug] Address race conditions in the resolver code. [RT #26889] --- CHANGES | 3 ++ lib/dns/resolver.c | 113 +++++++++++++++++++++++++-------------------- 2 files changed, 66 insertions(+), 50 deletions(-) diff --git a/CHANGES b/CHANGES index 230ec688f3..ef840b0f3e 100644 --- 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 8b56aa21dc..66fa4f861c 100644 --- 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_adbaddrinfo_t *addrinfo; + dns_dbnode_t *node = NULL; + dns_dbnode_t *nsnode = NULL; dns_fetchevent_t *hevent; + dns_name_t *name; dns_rdataset_t *ardataset = NULL; dns_rdataset_t *asigrdataset = NULL; - 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_name_t *name; 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: