Fix fetch context use-after-free bugs

fctx_decreference() may call fctx_destroy(), which in turn may free the
fetch context by calling isc_mem_putanddetach().  This means that
whenever fctx_decreference() is called, the fetch context pointer should
be assumed to point to garbage after that call.  Meanwhile, the
following pattern is used in several places in lib/dns/resolver.c:

    LOCK(&res->buckets[fctx->bucketnum].lock);
    bucket_empty = fctx_decreference(fctx);
    UNLOCK(&res->buckets[fctx->bucketnum].lock);

Given that 'fctx' may be freed by the fctx_decreference() call, there is
no guarantee that the value of fctx->bucketnum will be the same before
and after the fctx_decreference() call.  This can cause all kinds of
locking issues as LOCK() calls no longer match up with their UNLOCK()
counterparts.

Fix by always using a helper variable to hold the bucket number when the
pattern above is used.

Note that fctx_try() still uses 'fctx' after calling fctx_decreference()
(it calls fctx_done()).  This is safe to do because the reference count
for 'fctx' is increased a few lines earlier and it also cannot be zero
right before that increase happens, so the fctx_decreference() call in
that particular location never invokes fctx_destroy().  Nevertheless,
use a helper variable for that call site as well, to retain consistency
and to prevent copy-pasted code from causing similar problems in the
future.

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index 15297024c0e80123593eec1dd26975bf5e6f3321..154248ec891d1ce8478632860bbeadfdb922654d 100644 (file)
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -4357,9 +4357,9 @@ fctx_try(fetchctx_t *fctx, bool retrying, bool badcache) {
                        options, 0, fctx->qc, task, resume_qmin, fctx,
                        &fctx->qminrrset, NULL, &fctx->qminfetch);
                if (result != ISC_R_SUCCESS) {
-                       LOCK(&fctx->res->buckets[fctx->bucketnum].lock);
+                       LOCK(&res->buckets[bucketnum].lock);
                        RUNTIME_CHECK(!fctx_decreference(fctx));
-                       UNLOCK(&fctx->res->buckets[fctx->bucketnum].lock);
+                       UNLOCK(&res->buckets[bucketnum].lock);
                        fctx_done(fctx, DNS_R_SERVFAIL, __LINE__);
                }
                return;
@@ -6138,9 +6138,9 @@ cleanup_event:
        dns_message_detach(&message);
        isc_event_free(&event);
 
-       LOCK(&res->buckets[fctx->bucketnum].lock);
+       LOCK(&res->buckets[bucketnum].lock);
        bucket_empty = fctx_decreference(fctx);
-       UNLOCK(&res->buckets[fctx->bucketnum].lock);
+       UNLOCK(&res->buckets[bucketnum].lock);
        if (bucket_empty) {
                empty_bucket(res);
        }
@@ -7528,6 +7528,7 @@ resume_dslookup(isc_task_t *task, isc_event_t *event) {
        dns_resolver_t *res;
        fetchctx_t *fctx;
        isc_result_t result;
+       uint32_t bucketnum;
        bool bucket_empty;
        dns_rdataset_t nameservers;
        dns_fixedname_t fixed;
@@ -7538,6 +7539,7 @@ resume_dslookup(isc_task_t *task, isc_event_t *event) {
        fctx = event->ev_arg;
        REQUIRE(VALID_FCTX(fctx));
        res = fctx->res;
+       bucketnum = fctx->bucketnum;
 
        UNUSED(task);
        FCTXTRACE("resume_dslookup");
@@ -7665,9 +7667,9 @@ cleanup:
        if (dns_rdataset_isassociated(&nameservers)) {
                dns_rdataset_disassociate(&nameservers);
        }
-       LOCK(&res->buckets[fctx->bucketnum].lock);
+       LOCK(&res->buckets[bucketnum].lock);
        bucket_empty = fctx_decreference(fctx);
-       UNLOCK(&res->buckets[fctx->bucketnum].lock);
+       UNLOCK(&res->buckets[bucketnum].lock);
        if (bucket_empty) {
                empty_bucket(res);
        }