From: Michał Kępień <michal@isc.org>
Date: Mon, 8 Oct 2018 10:47:28 +0000 (+0200)
Subject: Do not set qctx->result to DNS_R_SERVFAIL unless necessary
X-Git-Tag: v9.13.4~116^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ba912435427cf884fdc1ca26743eba6c00439106;p=thirdparty%2Fbind9.git

Do not set qctx->result to DNS_R_SERVFAIL unless necessary

In some cases, setting qctx->result to DNS_R_SERVFAIL causes the value
of a 'result' variable containing a more specific failure reason to be
effectively discarded.  This may cause certain query error log messages
to lack specificity despite a more accurate problem cause being
determined during query processing.

In other cases, qctx->result is set to DNS_R_SERVFAIL even though a more
specific error (e.g. ISC_R_NOMEMORY) could be explicitly indicated.

Since the response message's RCODE is derived from qctx->result using
dns_result_torcode(), which handles a number of possible isc_result_t
values and returns SERVFAIL for anything not explicitly listed, it is
fine to set qctx->result to something more specific than DNS_R_SERVFAIL
(in fact, this is already being done in a few cases).  Modify most
QUERY_ERROR() calls so that qctx->result is set to a more specific error
code when possible.  Adjust query_error() so that statistics are still
calculated properly.  Remove the RECURSE_ERROR() macro which was
introduced exactly because qctx->result could be set to DNS_R_SERVFAIL
instead of DNS_R_DUPLICATE or DNS_R_DROP, which need special handling.
Modify dns_sdlz_putrr() so that it returns DNS_R_SERVFAIL when a DLZ
driver returns invalid RDATA, in order to prevent setting RCODE to
FORMERR (which is what dns_result_torcode() translates e.g. DNS_R_SYNTAX
to) while responding authoritatively.
---

diff --git a/lib/dns/sdlz.c b/lib/dns/sdlz.c
index 811a2731bbe..979cbf52d72 100644
--- a/lib/dns/sdlz.c
+++ b/lib/dns/sdlz.c
@@ -1934,8 +1934,10 @@ dns_sdlz_putrr(dns_sdlzlookup_t *lookup, const char *type, dns_ttl_t ttl,
 					    origin, false,
 					    mctx, rdatabuf,
 					    &lookup->callbacks);
-		if (result != ISC_R_SUCCESS)
+		if (result != ISC_R_SUCCESS) {
 			isc_buffer_free(&rdatabuf);
+			result = DNS_R_SERVFAIL;
+		}
 		if (size >= 65535)
 			break;
 		size *= 2;
diff --git a/lib/ns/query.c b/lib/ns/query.c
index df160993396..e7ccb99586d 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -94,14 +94,6 @@ do { \
 	qctx->line = __LINE__; \
 } while (0)
 
-#define RECURSE_ERROR(qctx, r) \
-do { \
-	if ((r) == DNS_R_DUPLICATE || (r) == DNS_R_DROP) \
-		QUERY_ERROR(qctx, r); \
-	else \
-		QUERY_ERROR(qctx, DNS_R_SERVFAIL); \
-} while (0)
-
 /*% Partial answer? */
 #define PARTIALANSWER(c)	(((c)->query.attributes & \
 				  NS_QUERYATTR_PARTIALANSWER) != 0)
@@ -522,12 +514,12 @@ static void
 query_error(ns_client_t *client, isc_result_t result, int line) {
 	int loglevel = ISC_LOG_DEBUG(3);
 
-	switch (result) {
-	case DNS_R_SERVFAIL:
+	switch (dns_result_torcode(result)) {
+	case dns_rcode_servfail:
 		loglevel = ISC_LOG_DEBUG(1);
 		inc_stats(client, ns_statscounter_servfail);
 		break;
-	case DNS_R_FORMERR:
+	case dns_rcode_formerr:
 		inc_stats(client, ns_statscounter_formerr);
 		break;
 	default:
@@ -5376,7 +5368,7 @@ ns__query_start(query_ctx_t *qctx) {
 		} else {
 			CCTRACE(ISC_LOG_ERROR,
 			       "ns__query_start: query_getdb failed");
-			QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+			QUERY_ERROR(qctx, result);
 		}
 		return (query_done(qctx));
 	}
@@ -5458,7 +5450,7 @@ query_lookup(query_ctx_t *qctx) {
 	if (ISC_UNLIKELY(qctx->dbuf == NULL)) {
 		CCTRACE(ISC_LOG_ERROR,
 		       "query_lookup: query_getnamebuf failed (2)");
-		QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+		QUERY_ERROR(qctx, ISC_R_NOMEMORY);
 		return (query_done(qctx));
 	}
 
@@ -5468,7 +5460,7 @@ query_lookup(query_ctx_t *qctx) {
 	if (ISC_UNLIKELY(qctx->fname == NULL || qctx->rdataset == NULL)) {
 		CCTRACE(ISC_LOG_ERROR,
 		       "query_lookup: query_newname failed (2)");
-		QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+		QUERY_ERROR(qctx, ISC_R_NOMEMORY);
 		return (query_done(qctx));
 	}
 
@@ -5479,7 +5471,7 @@ query_lookup(query_ctx_t *qctx) {
 		if (qctx->sigrdataset == NULL) {
 			CCTRACE(ISC_LOG_ERROR,
 			       "query_lookup: query_newrdataset failed (2)");
-			QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+			QUERY_ERROR(qctx, ISC_R_NOMEMORY);
 			return (query_done(qctx));
 		}
 	}
@@ -6008,7 +6000,7 @@ query_resume(query_ctx_t *qctx) {
 	if (qctx->dbuf == NULL) {
 		CCTRACE(ISC_LOG_ERROR,
 		       "query_resume: query_getnamebuf failed (1)");
-		QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+		QUERY_ERROR(qctx, ISC_R_NOMEMORY);
 		return (query_done(qctx));
 	}
 
@@ -6016,7 +6008,7 @@ query_resume(query_ctx_t *qctx) {
 	if (qctx->fname == NULL) {
 		CCTRACE(ISC_LOG_ERROR,
 		       "query_resume: query_newname failed (1)");
-		QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+		QUERY_ERROR(qctx, ISC_R_NOMEMORY);
 		return (query_done(qctx));
 	}
 
@@ -6033,7 +6025,7 @@ query_resume(query_ctx_t *qctx) {
 	if (result != ISC_R_SUCCESS) {
 		CCTRACE(ISC_LOG_ERROR,
 		       "query_resume: dns_name_copy failed");
-		QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+		QUERY_ERROR(qctx, result);
 		return (query_done(qctx));
 	}
 
@@ -6317,8 +6309,8 @@ query_checkrpz(query_ctx_t *qctx, isc_result_t result) {
 		qctx->client->query.attributes |= NS_QUERYATTR_RECURSING;
 		return (ISC_R_COMPLETE);
 	default:
-		RECURSE_ERROR(qctx, rresult);
-		return (ISC_R_COMPLETE);;
+		QUERY_ERROR(qctx, rresult);
+		return (ISC_R_COMPLETE);
 	}
 
 	if (qctx->rpz_st->m.policy != DNS_RPZ_POLICY_MISS) {
@@ -6760,7 +6752,7 @@ query_gotanswer(query_ctx_t *qctx, isc_result_t result) {
 			 */
 			return (query_lookup(qctx));
 		}
-		QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+		QUERY_ERROR(qctx, result);
 		return (query_done(qctx));
 	}
 }
@@ -6866,7 +6858,7 @@ query_respond_any(query_ctx_t *qctx) {
 	if (result != ISC_R_SUCCESS) {
 		CCTRACE(ISC_LOG_ERROR,
 		       "query_respond_any: allrdatasets failed");
-		QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+		QUERY_ERROR(qctx, result);
 		return (query_done(qctx));
 	}
 
@@ -7075,7 +7067,7 @@ query_respond_any(query_ctx_t *qctx) {
 	if (result != ISC_R_NOMORE) {
 		CCTRACE(ISC_LOG_ERROR,
 		       "query_respond_any: dns_rdatasetiter_destroy failed");
-		QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+		QUERY_ERROR(qctx, result);
 	} else {
 		query_addauth(qctx);
 	}
@@ -7261,7 +7253,7 @@ query_respond(query_ctx_t *qctx) {
 				qctx->client->query.attributes |=
 				      NS_QUERYATTR_DNS64EXCLUDE;
 		} else {
-			RECURSE_ERROR(qctx, result);
+			QUERY_ERROR(qctx, result);
 		}
 
 		return (query_done(qctx));
@@ -7778,14 +7770,15 @@ query_notfound(query_ctx_t *qctx) {
 				if (qctx->dns64_exclude)
 					qctx->client->query.attributes |=
 						NS_QUERYATTR_DNS64EXCLUDE;
-			} else
-				RECURSE_ERROR(qctx, result);
+			} else {
+				QUERY_ERROR(qctx, result);
+			}
 			return (query_done(qctx));
 		} else {
 			/* Unable to give root server referral. */
 			CCTRACE(ISC_LOG_ERROR,
 				"unable to give root server referral");
-			QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+			QUERY_ERROR(qctx, result);
 			return (query_done(qctx));
 		}
 	}
@@ -8041,10 +8034,8 @@ query_delegation(query_ctx_t *qctx) {
 			if (qctx->dns64_exclude)
 				qctx->client->query.attributes |=
 				      NS_QUERYATTR_DNS64EXCLUDE;
-		} else if (result == DNS_R_DUPLICATE || result == DNS_R_DROP) {
-			QUERY_ERROR(qctx, result);
 		} else {
-			RECURSE_ERROR(qctx, result);
+			QUERY_ERROR(qctx, result);
 		}
 
 		return (query_done(qctx));
@@ -8211,7 +8202,7 @@ query_nodata(query_ctx_t *qctx, isc_result_t result) {
 				CCTRACE(ISC_LOG_ERROR,
 				       "query_nodata: "
 				       "query_getnamebuf failed (3)");
-				QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+				QUERY_ERROR(qctx, ISC_R_NOMEMORY);
 				return (query_done(qctx));;
 			}
 			qctx->fname = query_newname(qctx->client,
@@ -8220,7 +8211,7 @@ query_nodata(query_ctx_t *qctx, isc_result_t result) {
 				CCTRACE(ISC_LOG_ERROR,
 				       "query_nodata: "
 				       "query_newname failed (3)");
-				QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+				QUERY_ERROR(qctx, ISC_R_NOMEMORY);
 				return (query_done(qctx));;
 			}
 		}
@@ -8366,7 +8357,7 @@ query_sign_nodata(query_ctx_t *qctx) {
 					       "query_sign_nodata: "
 					       "failure getting "
 					       "closest encloser");
-					QUERY_ERROR(qctx, DNS_R_SERVFAIL);
+					QUERY_ERROR(qctx, ISC_R_NOMEMORY);
 					return (query_done(qctx));
 				}
 				/*
@@ -9427,7 +9418,7 @@ query_cname(query_ctx_t *qctx) {
 				qctx->client->query.attributes |=
 					NS_QUERYATTR_DNS64EXCLUDE;
 		} else {
-			RECURSE_ERROR(qctx, result);
+			QUERY_ERROR(qctx, result);
 		}
 
 		return (query_done(qctx));