From: Otto Moerbeek <otto@drijf.net>
Date: Wed, 11 Nov 2020 11:02:50 +0000 (+0100)
Subject: Do not add request to a wait chain that's already processed or being processed.
X-Git-Tag: dnsdist-1.6.0-alpha0~2^2
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=c647a254a0f863aabeaea9d33f673afa26c60457;p=thirdparty%2Fpdns.git

Do not add request to a wait chain that's already processed or being processed.

The following scenario can occur. Multiple concurrent clients doing the same query A
are needed to trigger it:

1. Incoming request A, which has a need for request X
2. Add request X to chain because we already have an identical outstanding request
3. We receive the reply for X
4. We process the chain
5. In the meantime a new request for X that's identical is added to the chain
6. The added id in step 5 is not being processed anymore -> timeout

This can happen if request X has TTL 0, otherwise the record cache would have a hit.
---

diff --git a/pdns/mtasker.hh b/pdns/mtasker.hh
index ed6b02d0c5..2b9301d9a1 100644
--- a/pdns/mtasker.hh
+++ b/pdns/mtasker.hh
@@ -81,7 +81,7 @@ public:
     EventKey key;
     std::shared_ptr<pdns_ucontext_t> context;
     struct timeval ttd;
-    int tid;    
+    int tid;
   };
 
   typedef multi_index_container<
diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index d22c8e9843..49cf2c463c 100644
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -653,7 +653,7 @@ LWResult::Result asendto(const char *data, size_t len, int flags,
   pair<MT_t::waiters_t::iterator, MT_t::waiters_t::iterator> chain=MT->d_waiters.equal_range(pident, PacketIDBirthdayCompare());
 
   for(; chain.first != chain.second; chain.first++) {
-    if(chain.first->key.fd > -1) { // don't chain onto existing chained waiter!
+    if(chain.first->key.fd > -1 && !chain.first->key.closed) { // don't chain onto existing chained waiter or a chain already processed
       /*
       cerr<<"Orig: "<<pident.domain<<", "<<pident.remote.toString()<<", id="<<id<<endl;
       cerr<<"Had hit: "<< chain.first->key.domain<<", "<<chain.first->key.remote.toString()<<", id="<<chain.first->key.id
@@ -3814,6 +3814,9 @@ static void handleTCPClientWritable(int fd, FDMultiplexer::funcparam_t& var)
 // resend event to everybody chained onto it
 static void doResends(MT_t::waiters_t::iterator& iter, PacketID resend, const string& content)
 {
+  // We close the chain for new entries, since they won't be processed anyway
+  iter->key.closed = true;
+
   if(iter->key.chain.empty())
     return;
   //  cerr<<"doResends called!\n";
diff --git a/pdns/syncres.hh b/pdns/syncres.hh
index b38203bada..ccb6cfd47b 100644
--- a/pdns/syncres.hh
+++ b/pdns/syncres.hh
@@ -908,7 +908,7 @@ LWResult::Result arecvtcp(string& data, size_t len, Socket* sock, bool incomplet
 
 struct PacketID
 {
-  PacketID() : id(0), type(0), sock(0), inNeeded(0), inIncompleteOkay(false), outPos(0), nearMisses(0), fd(-1)
+  PacketID() : id(0), type(0), sock(0), inNeeded(0), inIncompleteOkay(false), outPos(0), nearMisses(0), fd(-1), closed(false)
   {
     remote.reset();
   }
@@ -930,6 +930,7 @@ struct PacketID
   mutable chain_t chain;
   mutable uint32_t nearMisses; // number of near misses - host correct, id wrong
   int fd;
+  mutable bool closed; // Processing already started, don't accept new chained ids
 
   bool operator<(const PacketID& b) const
   {