From: Remi Gacogne <remi.gacogne@powerdns.com>
Date: Sat, 29 Jun 2019 17:21:05 +0000 (+0200)
Subject: dnsdist: Insert the response into the ringbuffer right after sending it
X-Git-Tag: dnsdist-1.4.0-rc1~74^2~1
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2f9671330740d7eef2fcf5049df2c745ccbd8911;p=thirdparty%2Fpdns.git

dnsdist: Insert the response into the ringbuffer right after sending it

The current code could have tried to read a new query before coming
back to the insertion, resetting the state in the process and leading
to recording a wrong backend or worse, to a NULL-pointer dereference
if the new query was dropped or self-answered (no backend then).
---

diff --git a/pdns/dnsdist-tcp.cc b/pdns/dnsdist-tcp.cc
index 7f7bbf02a9..06174c3064 100644
--- a/pdns/dnsdist-tcp.cc
+++ b/pdns/dnsdist-tcp.cc
@@ -610,6 +610,7 @@ public:
   TCPIOHandler d_handler;
   std::unique_ptr<TCPConnectionToBackend> d_downstreamConnection{nullptr};
   std::shared_ptr<DownstreamState> d_ds{nullptr};
+  dnsheader d_cleartextDH;
   struct timeval d_connectionStartTime;
   struct timeval d_handshakeDoneTime;
   struct timeval d_firstQuerySizeReadTime;
@@ -649,6 +650,14 @@ static void handleResponseSent(std::shared_ptr<IncomingTCPConnectionState>& stat
     return;
   }
 
+  if (state->d_ds) {
+    /* if we have no downstream server selected, this was a self-answered response */
+    struct timespec answertime;
+    gettime(&answertime);
+    double udiff = state->d_ids.sentTime.udiff();
+    g_rings.insertResponse(answertime, state->d_ci.remote, state->d_ids.qname, state->d_ids.qtype, static_cast<unsigned int>(udiff), static_cast<unsigned int>(state->d_responseBuffer.size()), state->d_cleartextDH, state->d_ds->remote);
+  }
+
   if (g_maxTCPQueriesPerConn && state->d_queriesCount > g_maxTCPQueriesPerConn) {
     vinfolog("Terminating TCP connection from %s because it reached the maximum number of queries per conn (%d / %d)", state->d_ci.remote.toStringWithPort(), state->d_queriesCount, g_maxTCPQueriesPerConn);
     return;
@@ -703,8 +712,7 @@ static void handleResponse(std::shared_ptr<IncomingTCPConnectionState>& state, s
     addRoom = DNSCRYPT_MAX_RESPONSE_PADDING_AND_MAC_SIZE;
   }
 
-  dnsheader cleartextDH;
-  memcpy(&cleartextDH, dr.dh, sizeof(cleartextDH));
+  memcpy(&state->d_cleartextDH, dr.dh, sizeof(state->d_cleartextDH));
 
   std::vector<uint8_t> rewrittenResponse;
   size_t responseSize = state->d_responseBuffer.size();
@@ -727,13 +735,9 @@ static void handleResponse(std::shared_ptr<IncomingTCPConnectionState>& state, s
     state->d_xfrStarted = true;
   }
 
-  sendResponse(state, now);
-
   ++g_stats.responses;
-  struct timespec answertime;
-  gettime(&answertime);
-  double udiff = state->d_ids.sentTime.udiff();
-  g_rings.insertResponse(answertime, state->d_ci.remote, *dr.qname, dr.qtype, static_cast<unsigned int>(udiff), static_cast<unsigned int>(state->d_responseBuffer.size()), cleartextDH, state->d_ds->remote);
+
+  sendResponse(state, now);
 }
 
 static void sendQueryToBackend(std::shared_ptr<IncomingTCPConnectionState>& state, struct timeval& now)