Remove two-decade old hack.

answer_request() builds answers in the same packet buffer
as the request.  This means that any EDNS0 header from the
original request is overwritten. If the answer is in cache, that's
fine: dnsmasq adds its own EDNS0 header, but if the cache lookup fails
partially and the request needs to be sent upstream, it's a problem.

This was fixed a long, long time ago by running the cache
lookup twice if the request included an EDNS0 header. The first time,
nothing would be written to the answer packet, nad if the cache
lookup failed, the untouched question packet was still available
to forward upstream. If cache lookup succeeded, the whole thing
was done again, this time writing the data into the reply packet.
In a world where EDNS0 was rare and so was memory, this was a
reasonable solution. Today EDNS0 is ubiquitous so basically
every query is being looked up twice in the cache. There's also
the problem that any code change which makes successive cache lookups
for a query possibly return different answers adds a subtle hidden
bug, because this hack depends on absence of that behaviour.

This commit removes the lookup-twice hack entirely. answer_request()
can now return zero and overwrite the question packet. The code which
was previously added to support stale caching by saving a copy of the
query in the block-storage system is extended to always be active.
This handles the case where answer_request() returns no answer OR
a stale answer and a copy of the original query is needed to forward
upstream.

src/forward.c

@@ -1814,11 +1814,7 @@ void receive_query(struct listener *listen, time_t now)
       int stale, filtered;
       int ad_reqd = do_bit;
       int fd = listen->fd;
-      struct blockdata *saved_question = NULL;
+      struct blockdata *saved_question = blockdata_alloc((char *) header, (size_t)n);
-      
-      /* In case answer is stale */
-      if (daemon->cache_max_expiry != 0)
-	saved_question = blockdata_alloc((char *) header, (size_t)n);
       
       /* RFC 6840 5.7 */
       if (header->hb4 & HB4_AD)
@@ -1861,11 +1857,10 @@ void receive_query(struct listener *listen, time_t now)
 	    daemon->metrics[METRIC_DNS_STALE_ANSWERED]++;
 	}
       
-      if (stale && saved_question)
+      if (stale)
 	{
 	  /* We answered with stale cache data, so forward the query anyway to
-	     refresh that. Restore saved query. */
+	     refresh that. */
-	  blockdata_retrieve(saved_question, (size_t)n, header);
 	  m = 0;
 	  
 	  /* We've already answered the client, so don't send it the answer 
@@ -1873,16 +1868,21 @@ void receive_query(struct listener *listen, time_t now)
 	  fd = -1;
 	}
       
-      blockdata_free(saved_question);
+      if (saved_question)
-
+	{
 	  if (m == 0)
 	    {
+	      blockdata_retrieve(saved_question, (size_t)n, header);
+	      
 	      if (forward_query(fd, &source_addr, &dst_addr, if_index,
 				header, (size_t)n,  ((char *) header) + udp_size, now, NULL, ad_reqd, do_bit, 0))
 		daemon->metrics[METRIC_DNS_QUERIES_FORWARDED]++;
 	      else
 		daemon->metrics[METRIC_DNS_LOCAL_ANSWERED]++;
 	    }
+	  
+	  blockdata_free(saved_question);
+	}
     }
 }
 
@@ -2178,25 +2178,15 @@ unsigned char *tcp_request(int confd, time_t now,
     {
       int ede = EDE_UNSET;
 
-      if (do_stale)
+      if (!do_stale)
-	{
-	  /* We answered the last query with stale data. Now try and get fresh data.
-	     Restore saved query */
-	  if (!saved_question)
-	    break;
-
-	  blockdata_retrieve(saved_question, (size_t)saved_size, header);
-	  size = saved_size;
-	}
-      else
 	{
 	  if (query_count == TCP_MAX_QUERIES)
-	    return packet;
+	    break;
 	  
 	  if (!read_write(confd, &c1, 1, 1) || !read_write(confd, &c2, 1, 1) ||
 	      !(size = c1 << 8 | c2) ||
 	      !read_write(confd, payload, size, 1))
-	    return packet;
+	    break;
 	}
       
       if (size < (int)sizeof(struct dns_header))
@@ -2303,15 +2293,12 @@ unsigned char *tcp_request(int confd, time_t now,
 	   if (do_stale)
 	     m = 0;
 	   else
-	     {
-	       if (daemon->cache_max_expiry != 0)
 	     {
 	       if (saved_question)
 		 blockdata_free(saved_question);
 	       
 	       saved_question = blockdata_alloc((char *) header, (size_t)size);
 	       saved_size = size;
-		 }
 	       
 	       /* m > 0 if answered from cache */
 	       m = answer_request(header, ((char *) header) + 65536, (size_t)size, 
@@ -2320,11 +2307,14 @@ unsigned char *tcp_request(int confd, time_t now,
 	  /* Do this by steam now we're not in the select() loop */
 	  check_log_writer(1); 
 	  
-	  if (m == 0)
+	  if (m == 0 && saved_question)
 	    {
 	      struct server *master;
 	      int start;
 
+	      blockdata_retrieve(saved_question, (size_t)saved_size, header);
+	      size = saved_size;
+	      
 	      if (lookup_domain(daemon->namebuff, gotname, &first, &last))
 		flags = is_local_answer(now, first, daemon->namebuff);
 	      else
@@ -2486,7 +2476,7 @@ unsigned char *tcp_request(int confd, time_t now,
 	break;
       
       /* If we answered with stale data, this process will now try and get fresh data into
-	 the cache then and cannot therefore accept new queries. Close the incoming
+	 the cache and cannot therefore accept new queries. Close the incoming
 	 connection to signal that to the client. Then set do_stale and loop round
 	 once more to try and get fresh data, after which we exit. */
       if (stale)

src/rfc1035.c

@@ -1478,7 +1478,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
   int nameoffset;
   unsigned short flag;
   int q, ans, anscount = 0, addncount = 0;
-  int dryrun = 0;
   struct crec *crecp;
   int nxdomain = 0, notimp = 0, auth = 1, trunc = 0, sec_data = 1;
   struct mx_srv_record *rec;
@@ -1502,16 +1501,9 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
   if (header->hb4 & HB4_CD)
     sec_data = 0;
   
-  /* If there is an  additional data section then it will be overwritten by
-     partial replies, so we have to do a dry run to see if we can answer
-     the query. */
-  if (ntohs(header->arcount) != 0)
-    dryrun = 1;
-
   for (rec = daemon->mxnames; rec; rec = rec->next)
     rec->offset = 0;
   
- rerun:
   /* determine end of question section (we put answers there) */
   if (!(ansp = skip_questions(header, qlen)))
     return 0; /* bad packet */
@@ -1553,7 +1545,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 	      {
 		if (qtype == T_CNAME)
 		  {
-		   if (!dryrun)
 		    log_query(stale_flag | crecp->flags, name, NULL, record_source(crecp->uid), 0);
 		    auth = 0;
 		    nxdomain = 1;
@@ -1574,16 +1565,12 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 		if (!(crecp->flags & F_DNSSECOK))
 		  sec_data = 0;
 		
-		if (!dryrun)
-		  {
 		log_query(stale_flag | crecp->flags, name, NULL, record_source(crecp->uid), 0);
 		if (add_resource_record(header, limit, &trunc, nameoffset, &ansp, 
 					crec_ttl(crecp, now), &nameoffset,
 					T_CNAME, C_IN, "d", cname_target))
 		  anscount++;
 	      }
-		
-	      }
 	    else
 	      return 0; /* give up if any cached CNAME in chain can't be used for DNSSEC reasons. */
 	    
@@ -1599,12 +1586,11 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 	  for(t = daemon->txt; t ; t = t->next)
 	    {
 	      if (t->class == qclass && hostname_isequal(name, t->name))
-		{
-		  ans = 1, sec_data = 0;
-		  if (!dryrun)
 		{
 		  unsigned long ttl = daemon->local_ttl;
 		  int ok = 1;
+		  
+		  ans = 1, sec_data = 0;
 #ifndef NO_ID
 		  /* Dynamically generate stat record */
 		  if (t->stat != 0)
@@ -1625,7 +1611,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 		}
 	    }
 	}
-	}
 
       if (qclass == C_CHAOS)
 	{
@@ -1635,11 +1620,10 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 	      if (!ans)
 		{
 		  notimp = 1, auth = 0;
-		  if (!dryrun)
+		  
-		    {
 		  addr.log.rcode = NOTIMP;
 		  log_query(F_CONFIG | F_RCODE, name, &addr, NULL, 0);
-		    }
+		  
 		  ans = 1, sec_data = 0;
 		}
 	    }
@@ -1654,15 +1638,12 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 	      {
 		ans = 1;
 		sec_data = 0;
-		if (!dryrun)
-		  {
 		log_query(F_CONFIG | F_RRNAME, name, NULL, NULL, t->class);
 		if (add_resource_record(header, limit, &trunc, nameoffset, &ansp, 
 					daemon->local_ttl, NULL,
 					t->class, C_IN, "t", t->len, t->txt))
 		  anscount++;
 	      }
-	      }
 	  
 	  if (qtype == T_PTR || qtype == T_ANY)
 	    {
@@ -1710,21 +1691,16 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 		{
 		  sec_data = 0;
 		  ans = 1;
-		  if (!dryrun)
-		    {
 		  log_query(is_arpa | F_REVERSE | F_CONFIG, intr->name, &addr, NULL, 0);
 		  if (add_resource_record(header, limit, &trunc, nameoffset, &ansp, 
 					  daemon->local_ttl, NULL,
 					  T_PTR, C_IN, "d", intr->name))
 		    anscount++;
 		}
-		}
 	      else if (ptr)
 		{
 		  ans = 1;
 		  sec_data = 0;
-		  if (!dryrun)
-		    {
 		  log_query(F_CONFIG | F_RRNAME, name, NULL, "<PTR>", 0);
 		  for (ptr = daemon->ptr; ptr; ptr = ptr->next)
 		    if (hostname_isequal(name, ptr->name) &&
@@ -1734,7 +1710,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 		      anscount++;
 		  
 		}
-		}
 	      else if (is_arpa && (crecp = cache_find_by_addr(NULL, &addr, now, is_arpa)))
 		{
 		  /* Don't use cache when DNSSEC data required, unless we know that
@@ -1769,15 +1744,13 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 			      auth = 0;
 			      if (crecp->flags & F_NXDOMAIN)
 				nxdomain = 1;
-			      if (!dryrun)
 			      log_query(stale_flag | (crecp->flags & ~F_FORWARD), name, &addr, NULL, 0);
 			    }
 			  else
 			    {
 			      if (!(crecp->flags & (F_HOSTS | F_DHCP)))
 				auth = 0;
-			      if (!dryrun)
+			      
-				{
 			      log_query(stale_flag | (crecp->flags & ~F_FORWARD), cache_get_name(crecp), &addr, 
 					record_source(crecp->uid), 0);
 			      
@@ -1786,7 +1759,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 						      T_PTR, C_IN, "d", cache_get_name(crecp)))
 				anscount++;
 			    }
-			    }
 			} while ((crecp = cache_find_by_addr(crecp, &addr, now, is_arpa)));
 		    }
 		}
@@ -1794,8 +1766,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 		{
 		  ans = 1;
 		  sec_data = 0;
-		  if (!dryrun)
-		    {
 		  log_query(F_CONFIG | F_REVERSE | is_arpa, name, &addr, NULL, 0);
 		  
 		  if (add_resource_record(header, limit, &trunc, nameoffset, &ansp, 
@@ -1803,7 +1773,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 					  T_PTR, C_IN, "d", name))
 		    anscount++;
 		}
-		}
 	      else if (option_bool(OPT_BOGUSPRIV) &&
 		       ((is_arpa == F_IPV6 && private_net6(&addr.addr6, 1)) || (is_arpa == F_IPV4 && private_net(addr.addr4, 1))) &&
 		       !lookup_domain(name, F_DOMAINSRV, NULL, NULL))
@@ -1812,7 +1781,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 		  ans = 1;
 		  sec_data = 0;
 		  nxdomain = 1;
-		  if (!dryrun)
 		  log_query(F_CONFIG | F_REVERSE | is_arpa | F_NEG | F_NXDOMAIN,
 			    name, &addr, NULL, 0);
 		}
@@ -1866,8 +1834,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 
 			      ans = 1;	
 			      sec_data = 0;
-			      if (!dryrun)
-				{
 			      gotit = 1;
 			      log_query(F_FORWARD | F_CONFIG | flag, name, &addrlist->addr, NULL, 0);
 			      if (add_resource_record(header, limit, &trunc, nameoffset, &ansp, 
@@ -1876,15 +1842,14 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 				anscount++;
 			    }
 		      }
-		      }
 		  
-		  if (!dryrun && !gotit)
+		  if (!gotit)
 		    log_query(F_FORWARD | F_CONFIG | flag | F_NEG, name, NULL, NULL, 0);
 		     
 		  continue;
 		}
 
-	      if ((crecp = cache_find_by_name(NULL, name, now, flag | F_NXDOMAIN | (dryrun ? F_NO_RR : 0))))
+	      if ((crecp = cache_find_by_name(NULL, name, now, flag | F_NXDOMAIN)))
 		{
 		  int localise = 0;
 		 		  
@@ -1937,7 +1902,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 			    ans = 1;
 			    sec_data = 0;
 			    
-			    if (!dryrun)
 			    log_query(F_NEG | F_CONFIG | flag, name, NULL, NULL, 0);
 			    
 			    if (filtered)
@@ -1949,7 +1913,7 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 			    auth = 0;
 			    if (crecp->flags & F_NXDOMAIN)
 			      nxdomain = 1;
-			    if (!dryrun)
+			    
 			    log_query(stale_flag | crecp->flags, name, NULL, NULL, 0);
 			  }
 			else 
@@ -1962,8 +1926,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 			      continue;
 			    
 			    ans = 1;
-			    if (!dryrun)
-			      {
 			    log_query(stale_flag | (crecp->flags & ~F_REVERSE), name, &crecp->addr,
 				      record_source(crecp->uid), 0);
 			    
@@ -1972,21 +1934,17 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 						    type == T_A ? "4" : "6", &crecp->addr))
 			      anscount++;
 			  }
-			  }
 		      } while ((crecp = cache_find_by_name(crecp, name, now, flag)));
 		}
 	      else if (is_name_synthetic(flag, name, &addr))
 		{
 		  ans = 1, sec_data = 0;
-		  if (!dryrun)
-		    {
 		  log_query(F_FORWARD | F_CONFIG | flag, name, &addr, NULL, 0);
 		  if (add_resource_record(header, limit, &trunc, nameoffset, &ansp, 
 					  daemon->local_ttl, NULL, type, C_IN, type == T_A ? "4" : "6", &addr))
 		    anscount++;
 		}
 	    }
-	    }
 	  
 	  if (qtype == T_MX || qtype == T_ANY)
 	    {
@@ -1994,12 +1952,11 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 	      for (rec = daemon->mxnames; rec; rec = rec->next)
 		if (!rec->issrv && hostname_isequal(name, rec->name))
 		  {
+		    int offset;
+
 		    ans = found = 1;
 		    sec_data = 0;
 		    
-		    if (!dryrun)
-		      {
-			int offset;
 		    log_query(F_CONFIG | F_RRNAME, name, NULL, "<MX>", 0);
 		    if (add_resource_record(header, limit, &trunc, nameoffset, &ansp, daemon->local_ttl,
 					    &offset, T_MX, C_IN, "sd", rec->weight, rec->target))
@@ -2009,15 +1966,12 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 			  rec->offset = offset;
 		      }
 		  }
-		  }
 	      
 	      if (!found && (option_bool(OPT_SELFMX) || option_bool(OPT_LOCALMX)) &&
 		  cache_find_by_name(NULL, name, now, F_HOSTS | F_DHCP | F_NO_RR))
 		{ 
 		  ans = 1;
 		  sec_data = 0;
-		  if (!dryrun)
-		    {
 		  log_query(F_CONFIG | F_RRNAME, name, NULL, "<MX>", 0);
 		  if (add_resource_record(header, limit, &trunc, nameoffset, &ansp, daemon->local_ttl, NULL, 
 					  T_MX, C_IN, "sd", 1, 
@@ -2025,7 +1979,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 		    anscount++;
 		}
 	    }
-	    }
 	  	  
 	  if (qtype == T_SRV || qtype == T_ANY)
 	    {
@@ -2034,11 +1987,10 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 	      for (rec = daemon->mxnames; rec; rec = rec->next)
 		if (rec->issrv && hostname_isequal(name, rec->name))
 		  {
+		    int offset;
+
 		    ans = 1;
 		    sec_data = 0;
-		    if (!dryrun)
-		      {
-			int offset;
 		    log_query(F_CONFIG | F_RRNAME, name, NULL, "<SRV>", 0);
 		    if (add_resource_record(header, limit, &trunc, nameoffset, &ansp, daemon->local_ttl, 
 					    &offset, T_SRV, C_IN, "sssd", 
@@ -2048,7 +2000,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 			if (rec->target)
 			  rec->offset = offset;
 		      }
-		      }
 		    
 		    /* unlink first SRV record found */
 		    if (!move)
@@ -2078,8 +2029,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 		  {
 		    ans = 1;
 		    sec_data = 0;
-		    if (!dryrun)
-		      {
 		    log_query(F_CONFIG | F_RRNAME, name, NULL, "<NAPTR>", 0);
 		    if (add_resource_record(header, limit, &trunc, nameoffset, &ansp, daemon->local_ttl, 
 					    NULL, T_NAPTR, C_IN, "sszzzd", 
@@ -2087,7 +2036,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 		      anscount++;
 		  }
 	    }
-	    }
 	  
 	  if (qtype == T_MAILB)
 	    ans = 1, nxdomain = 1, sec_data = 0;
@@ -2096,13 +2044,12 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 	    {
 	      ans = 1;
 	      sec_data = 0;
-	      if (!dryrun)
 	      log_query(F_CONFIG | F_NEG, name, &addr, NULL, 0);
 	    }
 	  
 	  if (!ans)
 	    {
-	       if ((crecp = cache_find_by_name(NULL, name, now, F_RR | F_NXDOMAIN | (dryrun ? F_NO_RR : 0))) &&
+	       if ((crecp = cache_find_by_name(NULL, name, now, F_RR | F_NXDOMAIN)) &&
 		   rd_bit && (!do_bit || cache_validated(crecp)))
 		 do
 		   {
@@ -2116,6 +2063,9 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 
 		      if ((flags & F_NXDOMAIN) || rrtype == qtype)
 		       {
+			  char *rrdata = NULL;
+			  unsigned short rrlen = 0;
+
 			  if (crec_isstale(crecp, now))
 			   {
 			     if (stale)
@@ -2135,11 +2085,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 			 auth = 0;
 			 ans = 1;
 			 
-			 if (!dryrun)
-			   {
-			     char *rrdata = NULL;
-			     unsigned short rrlen = 0;
-
 			 if (!(flags & F_NEG))
 			   {
 			     if (flags & F_KEYTAG)
@@ -2165,7 +2110,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 			 else
 			   log_query(flags, name, &crecp->addr, NULL, 0);
 		       }
-		       }
 		   } while ((crecp = cache_find_by_name(crecp, name, now, F_RR)));
 	    }
 	  
@@ -2173,7 +2117,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 	    {
 	      ans = 1;
 	      sec_data = 0;
-	      if (!dryrun)
 	      log_query(F_CONFIG | F_NEG, name, NULL, NULL, 0);
 	    }
 	  
@@ -2192,7 +2135,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 		  ans = 1;
 		  sec_data = auth = 0;
 		  
-		  if (!dryrun)
 		  log_query(F_NEG | F_CONFIG | flag, name, NULL, NULL, 0);
 		  
 		  if (filtered)
@@ -2205,12 +2147,6 @@ size_t answer_request(struct dns_header *header, char *limit, size_t qlen,
 	return 0; /* failed to answer a question */
     }
   
-  if (dryrun)
-    {
-      dryrun = 0;
-      goto rerun;
-    }
-  
   /* create an additional data section, for stuff in SRV and MX record replies. */
   for (rec = daemon->mxnames; rec; rec = rec->next)
     if (rec->offset != 0)