From cde6227a6878d2fe161028d7ff684d1d0e4957d2 Mon Sep 17 00:00:00 2001 From: Diego Fronza Date: Mon, 21 Sep 2020 17:44:29 -0300 Subject: [PATCH] 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. --- lib/dns/resolver.c | 46 ++++++++++++++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 973c7d3bb3..a5f95e7f16 100644 --- 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); } /*