Properly handling dns_message_t shared references

This commit fix the problems that arose when moving the dns_message_t
object from fetchctx_t to the query structure.

Since the lifetime of query objects are different than that of a fetchctx
and the dns_message_t object held by the query may be being used by some
external module, e.g. validator, even after the query may have been destroyed,
propery handling of the references to the message were added in this commit to
avoid accessing an already destroyed object.

Specifically, in rctx_done(), a reference to the message is attached at
the beginning of the function and detached at the end, since a possible call
to fctx_cancelquery() would release the dns_message_t object, and in the next
lines of code a call to rctx_nextserver() or rctx_chaseds() would require
a valid pointer to the same object.

In valcreate() a new reference is attached to the message object, this
ensures that if the corresponding query object is destroyed before the
validator attempts to access it, no invalid pointer access occurs.

In validated() we have to attach a new reference to the message, since
we destroy the validator object at the beginning of the function,
and we need access to the message in the next lines of the same function.

rctx_nextserver() and rctx_chaseds() functions were adapted to receive
a new parameter of dns_message_t* type, this was so they could receive a
valid reference to a dns_message_t since using the response context respctx_t
to access the message through rctx->query->rmessage could lead to an already
released reference due to the query being canceled.

diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c
index 973c7d3bb3cc657678278a7799ab7c2a6b3d770e..a5f95e7f1640f29a0619327841fa05d3eb7206dc 100644 (file)
--- a/lib/dns/resolver.c
+++ b/lib/dns/resolver.c
@@ -819,8 +819,8 @@ static isc_result_t
 rctx_answer_none(respctx_t *rctx);
 
 static void
-rctx_nextserver(respctx_t *rctx, dns_adbaddrinfo_t *addrinfo,
-               isc_result_t result);
+rctx_nextserver(respctx_t *rctx, dns_message_t *message,
+               dns_adbaddrinfo_t *addrinfo, isc_result_t result);
 
 static void
 rctx_resend(respctx_t *rctx, dns_adbaddrinfo_t *addrinfo);
@@ -829,7 +829,8 @@ static void
 rctx_next(respctx_t *rctx);
 
 static void
-rctx_chaseds(respctx_t *rctx, dns_adbaddrinfo_t *addrinfo, isc_result_t result);
+rctx_chaseds(respctx_t *rctx, dns_message_t *message,
+            dns_adbaddrinfo_t *addrinfo, isc_result_t result);
 
 static void
 rctx_done(respctx_t *rctx, isc_result_t result);
@@ -894,7 +895,8 @@ valcreate(fetchctx_t *fctx, dns_message_t *message, dns_adbaddrinfo_t *addrinfo,
 
        valarg->fctx = fctx;
        valarg->addrinfo = addrinfo;
-       valarg->message = message;
+       valarg->message = NULL;
+       dns_message_attach(message, &valarg->message);
 
        if (!ISC_LIST_EMPTY(fctx->validators)) {
                valoptions |= DNS_VALIDATOR_DEFER;
@@ -913,6 +915,7 @@ valcreate(fetchctx_t *fctx, dns_message_t *message, dns_adbaddrinfo_t *addrinfo,
                }
                ISC_LIST_APPEND(fctx->validators, validator, link);
        } else {
+               dns_message_detach(&valarg->message);
                isc_mem_put(fctx->mctx, valarg, sizeof(*valarg));
        }
        return (result);
@@ -5538,14 +5541,14 @@ validated(isc_task_t *task, isc_event_t *event) {
        uint32_t bucketnum;
        dns_fixedname_t fwild;
        dns_name_t *wild = NULL;
-       dns_message_t *message;
+       dns_message_t *message = NULL;
 
        UNUSED(task); /* for now */
 
        REQUIRE(event->ev_type == DNS_EVENT_VALIDATORDONE);
        valarg = event->ev_arg;
-       message = valarg->message;
        fctx = valarg->fctx;
+       dns_message_attach(valarg->message, &message);
 
        REQUIRE(VALID_FCTX(fctx));
        res = fctx->res;
@@ -5574,6 +5577,7 @@ validated(isc_task_t *task, isc_event_t *event) {
                                wild);
        }
        dns_validator_destroy(&vevent->validator);
+       dns_message_detach(&valarg->message);
        isc_mem_put(fctx->mctx, valarg, sizeof(*valarg));
 
        negative = (vevent->rdataset == NULL);
@@ -5719,6 +5723,7 @@ validated(isc_task_t *task, isc_event_t *event) {
                        fctx_try(fctx, true, true); /* Locks bucket. */
                }
 
+               dns_message_detach(&message);
                return;
        }
 
@@ -9510,8 +9515,8 @@ again:
  * useful to try another one.
  */
 static void
-rctx_nextserver(respctx_t *rctx, dns_adbaddrinfo_t *addrinfo,
-               isc_result_t result) {
+rctx_nextserver(respctx_t *rctx, dns_message_t *message,
+               dns_adbaddrinfo_t *addrinfo, isc_result_t result) {
        fetchctx_t *fctx = rctx->fctx;
 
        if (result == DNS_R_FORMERR) {
@@ -9522,8 +9527,8 @@ rctx_nextserver(respctx_t *rctx, dns_adbaddrinfo_t *addrinfo,
                 * Add this server to the list of bad servers for
                 * this fctx.
                 */
-               add_bad(fctx, rctx->query->rmessage, addrinfo,
-                       rctx->broken_server, rctx->broken_type);
+               add_bad(fctx, message, addrinfo, rctx->broken_server,
+                       rctx->broken_type);
        }
 
        if (rctx->get_nameservers) {
@@ -9650,13 +9655,12 @@ rctx_next(respctx_t *rctx) {
  * Look up the parent zone's NS records so that DS records can be fetched.
  */
 static void
-rctx_chaseds(respctx_t *rctx, dns_adbaddrinfo_t *addrinfo,
-            isc_result_t result) {
+rctx_chaseds(respctx_t *rctx, dns_message_t *message,
+            dns_adbaddrinfo_t *addrinfo, isc_result_t result) {
        fetchctx_t *fctx = rctx->fctx;
        unsigned int n;
 
-       add_bad(fctx, rctx->query->rmessage, addrinfo, result,
-               rctx->broken_type);
+       add_bad(fctx, message, addrinfo, result, rctx->broken_type);
        fctx_cancelqueries(fctx, true, false);
        fctx_cleanupfinds(fctx);
        fctx_cleanupforwaddrs(fctx);
@@ -9696,6 +9700,14 @@ rctx_done(respctx_t *rctx, isc_result_t result) {
        resquery_t *query = rctx->query;
        fetchctx_t *fctx = rctx->fctx;
        dns_adbaddrinfo_t *addrinfo = query->addrinfo;
+       /*
+        * Need to attach to the message until the scope
+        * of this function ends, since there are many places
+        * where te message is used and/or may be destroyed
+        * before this function ends.
+        */
+       dns_message_t *message = NULL;
+       dns_message_attach(query->rmessage, &message);
 
        FCTXTRACE4("query canceled in response(); ",
                   rctx->no_response ? "no response" : "responding", result);
@@ -9724,13 +9736,13 @@ rctx_done(respctx_t *rctx, isc_result_t result) {
 #endif /* ifdef ENABLE_AFL */
                if (rctx->next_server)
        {
-               rctx_nextserver(rctx, addrinfo, result);
+               rctx_nextserver(rctx, message, addrinfo, result);
        } else if (rctx->resend) {
                rctx_resend(rctx, addrinfo);
        } else if (rctx->nextitem) {
                rctx_next(rctx);
        } else if (result == DNS_R_CHASEDSSERVERS) {
-               rctx_chaseds(rctx, addrinfo, result);
+               rctx_chaseds(rctx, message, addrinfo, result);
        } else if (result == ISC_R_SUCCESS && !HAVE_ANSWER(fctx)) {
                /*
                 * All has gone well so far, but we are waiting for the
@@ -9752,6 +9764,8 @@ rctx_done(respctx_t *rctx, isc_result_t result) {
                 */
                fctx_done(fctx, result, __LINE__);
        }
+
+       dns_message_detach(&message);
 }
 
 /*