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 811a2731bbed65aad0f7adde351eae938c5e6818..979cbf52d7212bd357f965566f4edcb48fd2da45 100644 (file)
--- 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 df1609933964c1d183cf28b797a32bc886dac231..e7ccb99586d5b478749a76a61749358c85d38d8f 100644 (file)
--- 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));