- Fix #946: Forwarder returns servfail on upstream response noerror no

data.

Makefile.in

@@ -793,7 +793,7 @@ iter_priv.lo iter_priv.o: $(srcdir)/iterator/iter_priv.c config.h $(srcdir)/iter
  $(srcdir)/util/data/msgparse.h $(srcdir)/sldns/pkthdr.h $(srcdir)/sldns/rrdef.h $(srcdir)/util/net_help.h \
  $(srcdir)/util/storage/dnstree.h $(srcdir)/sldns/str2wire.h $(srcdir)/sldns/sbuffer.h
 iter_resptype.lo iter_resptype.o: $(srcdir)/iterator/iter_resptype.c config.h \
- $(srcdir)/iterator/iter_resptype.h $(srcdir)/iterator/iter_delegpt.h $(srcdir)/util/log.h \
+ $(srcdir)/iterator/iter_resptype.h $(srcdir)/iterator/iter_delegpt.h $(srcdir)/iterator/iterator.h $(srcdir)/util/log.h \
  $(srcdir)/services/cache/dns.h $(srcdir)/util/storage/lruhash.h $(srcdir)/util/locks.h \
  $(srcdir)/util/data/msgreply.h $(srcdir)/util/data/packed_rrset.h $(srcdir)/util/net_help.h \
  $(srcdir)/util/data/dname.h $(srcdir)/sldns/rrdef.h $(srcdir)/sldns/pkthdr.h

doc/Changelog

@@ -1,3 +1,7 @@
+4 October 2023: Wouter
+	- Fix #946: Forwarder returns servfail on upstream response noerror no
+	  data.
+
 3 October 2023: George
 	- Merge #881: Generalise the proxy protocol code.
 

iterator/iter_resptype.c

@@ -42,6 +42,7 @@
 #include "config.h"
 #include "iterator/iter_resptype.h"
 #include "iterator/iter_delegpt.h"
+#include "iterator/iterator.h"
 #include "services/cache/dns.h"
 #include "util/net_help.h"
 #include "util/data/dname.h"
@@ -105,7 +106,8 @@ response_type_from_cache(struct dns_msg* msg,
 
 enum response_type 
 response_type_from_server(int rdset,
-	struct dns_msg* msg, struct query_info* request, struct delegpt* dp)
+	struct dns_msg* msg, struct query_info* request, struct delegpt* dp,
+	int* empty_nodata_found)
 {
 	uint8_t* origzone = (uint8_t*)"\000"; /* the default */
 	struct ub_packed_rrset_key* s;
@@ -284,13 +286,22 @@ response_type_from_server(int rdset,
 
 	/* If we've gotten this far, this is NOERROR/NODATA (which could 
 	 * be an entirely empty message) */
-	/* but ignore entirely empty messages, noerror/nodata has a soa
-	 * negative ttl value in the authority section, this makes it try
-	 * again at another authority. And turns it from a 5 second empty
-	 * message into a 5 second servfail response. */
+	/* For entirely empty messages, try again, at first, then accept
+	 * it it happens more. A regular noerror/nodata response has a soa
+	 * negative ttl value in the authority section. This makes it try
+	 * again at another authority. And decides between storing a 5 second
+	 * empty message or a 5 second servfail response. */
 	if(msg->rep->an_numrrsets == 0 && msg->rep->ns_numrrsets == 0 &&
-		msg->rep->ar_numrrsets == 0)
-		return RESPONSE_TYPE_THROWAWAY;
+		msg->rep->ar_numrrsets == 0) {
+		if(empty_nodata_found) {
+			/* detect as throwaway at first, but accept later. */
+			(*empty_nodata_found)++;
+			if(*empty_nodata_found < EMPTY_NODATA_RETRY_COUNT)
+				return RESPONSE_TYPE_THROWAWAY;
+			return RESPONSE_TYPE_ANSWER;
+		}
+		return RESPONSE_TYPE_ANSWER;
+	}
 	/* check if recursive answer; saying it has empty cache */
 	if( (msg->rep->flags&BIT_RA) && !(msg->rep->flags&BIT_AA) && !rdset)
 		return RESPONSE_TYPE_REC_LAME;

iterator/iter_resptype.h

@@ -119,9 +119,11 @@ enum response_type response_type_from_cache(struct dns_msg* msg,
  * @param request: the request that generated the response.
  * @param dp: The delegation point that was being queried
  *          when the response was returned.
+ * @param empty_nodata_found: flag to keep track of empty nodata detection.
  * @return the response type (CNAME or ANSWER).
  */
 enum response_type response_type_from_server(int rdset, 
-	struct dns_msg* msg, struct query_info* request, struct delegpt* dp);
+	struct dns_msg* msg, struct query_info* request, struct delegpt* dp,
+	int* empty_nodata_found);
 
 #endif /* ITERATOR_ITER_RESPTYPE_H */

iterator/iterator.c

@@ -2940,7 +2940,7 @@ static int
 processQueryResponse(struct module_qstate* qstate, struct iter_qstate* iq,
 	struct iter_env* ie, int id)
 {
-	int dnsseclame = 0, origtypecname = 0;
+	int dnsseclame = 0, origtypecname = 0, orig_empty_nodata_found;
 	enum response_type type;
 
 	iq->num_current_queries--;
@@ -2960,12 +2960,25 @@ processQueryResponse(struct module_qstate* qstate, struct iter_qstate* iq,
 		return next_state(iq, QUERYTARGETS_STATE);
 	}
 	iq->timeout_count = 0;
+	orig_empty_nodata_found = iq->empty_nodata_found;
 	type = response_type_from_server(
 		(int)((iq->chase_flags&BIT_RD) || iq->chase_to_rd),
-		iq->response, &iq->qinfo_out, iq->dp);
+		iq->response, &iq->qinfo_out, iq->dp, &iq->empty_nodata_found);
 	iq->chase_to_rd = 0;
 	/* remove TC flag, if this is erroneously set by TCP upstream */
 	iq->response->rep->flags &= ~BIT_TC;
+	if(orig_empty_nodata_found != iq->empty_nodata_found &&
+		iq->empty_nodata_found < EMPTY_NODATA_RETRY_COUNT) {
+		/* try to search at another server */
+		if(qstate->reply) {
+			struct delegpt_addr* a = delegpt_find_addr(
+				iq->dp, &qstate->reply->remote_addr,
+				qstate->reply->remote_addrlen);
+			/* make selection disprefer it */
+			if(a) a->lame = 1;
+		}
+		return next_state(iq, QUERYTARGETS_STATE);
+	}
 	if(type == RESPONSE_TYPE_REFERRAL && (iq->chase_flags&BIT_RD) &&
 		!iq->auth_zone_response) {
 		/* When forwarding (RD bit is set), we handle referrals 
@@ -3501,7 +3514,7 @@ processPrimeResponse(struct module_qstate* qstate, int id)
 	iq->response->rep->flags &= ~(BIT_RD|BIT_RA); /* ignore rec-lame */
 	type = response_type_from_server(
 		(int)((iq->chase_flags&BIT_RD) || iq->chase_to_rd), 
-		iq->response, &iq->qchase, iq->dp);
+		iq->response, &iq->qchase, iq->dp, NULL);
 	if(type == RESPONSE_TYPE_ANSWER) {
 		qstate->return_rcode = LDNS_RCODE_NOERROR;
 		qstate->return_msg = iq->response;

iterator/iterator.h

@@ -101,6 +101,8 @@ extern int BLACKLIST_PENALTY;
  * Chosen so that the UNKNOWN_SERVER_NICENESS falls within the band of a 
  * fast server, this causes server exploration as a side benefit. msec. */
 #define RTT_BAND 400
+/** Number of retries for empty nodata packets before it is accepted. */
+#define EMPTY_NODATA_RETRY_COUNT 2
 
 /**
  * Global state for the iterator.
@@ -415,6 +417,11 @@ struct iter_qstate {
 	 */
 	int refetch_glue;
 
+	/**
+	 * This flag detects that a completely empty nodata was received,
+	 * already so that it is accepted later. */
+	int empty_nodata_found;
+
 	/** list of pending queries to authoritative servers. */
 	struct outbound_list outlist;
 

testdata/iter_ignore_empty.rpl

@@ -78,6 +78,18 @@ example2.com.	IN NS	ns2.example2.com.
 SECTION ADDITIONAL
 ns2.example2.com.		IN 	A	1.2.3.5
 ENTRY_END
+
+ENTRY_BEGIN
+MATCH opcode subdomain
+ADJUST copy_id copy_query
+REPLY QR NOERROR
+SECTION QUESTION
+foo.com. IN NS
+SECTION AUTHORITY
+foo.com.	IN NS	ns.foo.com.
+SECTION ADDITIONAL
+ns.foo.com.		IN 	A	1.2.3.5
+ENTRY_END
 RANGE_END
 
 ; ns.example.com.
@@ -172,6 +184,27 @@ www.example.com. IN A
 SECTION ANSWER
 www.example.com. IN A	10.20.30.40
 ENTRY_END
+
+; foo.com
+ENTRY_BEGIN
+MATCH opcode qtype qname
+ADJUST copy_id
+REPLY QR AA NOERROR
+SECTION QUESTION
+www.foo.com. IN A
+SECTION ANSWER
+ENTRY_END
+
+ENTRY_BEGIN
+MATCH opcode qtype qname
+ADJUST copy_id
+REPLY QR AA NOERROR
+SECTION QUESTION
+ns.foo.com. IN AAAA
+SECTION ANSWER
+SECTION AUTHORITY
+;foo.com.	IN SOA ns2.foo.com root.foo.com 4 14400 3600 604800 3600
+ENTRY_END
 RANGE_END
 
 STEP 1 QUERY
@@ -195,4 +228,21 @@ ENTRY_END
 ; wait for pending nameserver lookups.
 STEP 20 TRAFFIC
 
+; Test that a nodata stays a nodata.
+STEP 30 QUERY
+ENTRY_BEGIN
+REPLY RD
+SECTION QUESTION
+www.foo.com. IN A
+ENTRY_END
+
+STEP 40 CHECK_ANSWER
+ENTRY_BEGIN
+MATCH all
+REPLY QR RD RA NOERROR
+SECTION QUESTION
+www.foo.com. IN A
+SECTION ANSWER
+ENTRY_END
+
 SCENARIO_END