Fix datarace between unlocking fctx lock and shuttingdown fctx

There was a data race where new fetch response could be added to the
fetch context after we unlock the fetch context and before we shut it
down.  This could cause assertion failure when fctx__done() was called
with ISC_R_SUCCESS because there was originally no fetch response, but
new fetch response without associated dataset was added before we had a
chance to shutdown the fetch context.  This manifested in the
validated() callback, where cache_rrset() now returns ISC_R_SUCCESS
instead of DNS_R_UNCHANGED when cache was not changed.  However the data
race was wrong on a general level.

When the fctx__done() is called with ISC_R_SUCCESS as result is expects
the fctx->lock to be already acquired to prevent these data races.

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index ef848ef3840269a3507c3acbda7e73b0fd06884d..fe0c36fd081559d611ca068fd2d448c2d4331eee 100644 (file)
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -1664,7 +1664,9 @@ fctx__done(fetchctx_t *fctx, isc_result_t result, const char *func,
        UNUSED(func);
 #endif
 
-       LOCK(&fctx->lock);
+       if (result != ISC_R_SUCCESS) {
+               LOCK(&fctx->lock);
+       }
        /* We need to do this under the lock for intra-thread synchronization */
        if (fctx->state == fetchstate_done) {
                UNLOCK(&fctx->lock);
@@ -2899,11 +2901,8 @@ fctx_finddone(void *arg) {
                        }
                }
        }
-
        UNLOCK(&fctx->lock);
 
-       dns_adb_destroyfind(&find);
-
        if (want_done) {
                FCTXTRACE("fetch failed in finddone(); return "
                          "ISC_R_FAILURE");
@@ -2913,6 +2912,8 @@ fctx_finddone(void *arg) {
                fctx_try(fctx, true);
        }
 
+       dns_adb_destroyfind(&find);
+
        fetchctx_detach(&fctx);
 }
 
@@ -4108,7 +4109,6 @@ resume_qmin(void *arg) {
        dns_rdataset_t sigrdataset;
        dns_db_t *db = NULL;
        dns_dbnode_t *node = NULL;
-       bool fixup_result = false;
 
        REQUIRE(VALID_FCTX(fctx));
 
@@ -4265,14 +4265,16 @@ resume_qmin(void *arg) {
                                        dns_db_attachnode(node, &resp->node);
                                }
                                dns_name_copy(fname, resp->foundname);
+                               clone_results(fctx);
                                if (result == DNS_R_CNAME &&
                                    dns_rdataset_isassociated(&rdataset) &&
                                    fctx->type == dns_rdatatype_cname)
                                {
-                                       fixup_result = true;
+                                       fctx_success_unref(fctx);
+                                       result = ISC_R_SUCCESS;
+                               } else {
+                                       UNLOCK(&fctx->lock);
                                }
-                               clone_results(fctx);
-                               UNLOCK(&fctx->lock);
                                goto cleanup;
                        }
                        UNLOCK(&fctx->lock);
@@ -4370,7 +4372,7 @@ cleanup:
        }
        if (result != ISC_R_SUCCESS) {
                /* An error occurred, tear down whole fctx */
-               fctx_failure_unref(fctx, fixup_result ? ISC_R_SUCCESS : result);
+               fctx_failure_unref(fctx, result);
        }
        fetchctx_detach(&fctx);
 }
@@ -5603,9 +5605,19 @@ answer_response:
        done = true;
 
 cleanup:
-       UNLOCK(&fctx->lock);
-cleanup_unlocked:
+       if (!done || result != ISC_R_SUCCESS) {
+               UNLOCK(&fctx->lock);
+       }
+
+       if (done) {
+               if (result == ISC_R_SUCCESS) {
+                       fctx_success_unref(fctx);
+               } else {
+                       fctx_failure_unref(fctx, result);
+               }
+       }
 
+cleanup_unlocked:
        if (node != NULL) {
                dns_db_detachnode(&node);
        }
@@ -5614,12 +5626,6 @@ cleanup_unlocked:
                dns_validator_send(nextval);
        }
 
-       if (done && result == ISC_R_SUCCESS) {
-               fctx_success_unref(fctx);
-       } else if (done) {
-               fctx_failure_unref(fctx, result);
-       }
-
        /*
         * val->name points to name on a message on one of the
         * queries on the fetch context so the name has to be
@@ -5628,7 +5634,6 @@ cleanup_unlocked:
        dns_validator_shutdown(val);
        dns_validator_detach(&val);
        fetchctx_detach(&fctx);
-       INSIST(node == NULL);
 }
 
 static void
@@ -9204,15 +9209,18 @@ rctx_done(respctx_t *rctx, isc_result_t result) {
                rctx->next_server = false;
                rctx->resend = false;
        }
-       UNLOCK(&fctx->lock);
 
        if (rctx->next_server) {
+               UNLOCK(&fctx->lock);
                rctx_nextserver(rctx, message, addrinfo, result);
        } else if (rctx->resend) {
+               UNLOCK(&fctx->lock);
                rctx_resend(rctx, addrinfo);
        } else if (result == DNS_R_CHASEDSSERVERS) {
+               UNLOCK(&fctx->lock);
                rctx_chaseds(rctx, message, addrinfo, result);
        } else if (result == ISC_R_SUCCESS && !HAVE_ANSWER(fctx)) {
+               UNLOCK(&fctx->lock);
                /*
                 * All has gone well so far, but we are waiting for the DNSSEC
                 * validator to validate the answer.
@@ -9220,10 +9228,11 @@ rctx_done(respctx_t *rctx, isc_result_t result) {
                FCTXTRACE("wait for validator");
                fctx_cancelqueries(fctx, true, false);
        } else if (result == ISC_R_SUCCESS) {
-               /* We are done. */
+               /* Ended successfully. */
                fctx_success_detach(&rctx->fctx);
        } else {
-               /* Done with an error. */
+               UNLOCK(&fctx->lock);
+               /* Ended with failure. */
                fctx_failure_detach(&rctx->fctx, result);
        }