dnsdist: Fix a few warnings from Coverity

Mostly false positives, but also a real issue with `QueryProcessingResult::Empty` which was processed twice for incoming DoH queries with nghttp2.

diff --git a/pdns/dnsdist-tcp.cc b/pdns/dnsdist-tcp.cc
index 7b94e00e51092fe321c16bfb70c8701c045f3112..3d118508cd3e26d92b6aebe35f6dcb1a2f719e5c 100644 (file)
--- a/pdns/dnsdist-tcp.cc
+++ b/pdns/dnsdist-tcp.cc
@@ -677,12 +677,15 @@ IncomingTCPConnectionState::QueryProcessingResult IncomingTCPConnectionState::ha
       TCPResponse response;
       dh->rcode = RCode::NotImp;
       dh->qr = true;
+      auto queryID = dh->id;
+      response.d_idstate = std::move(ids);
+      response.d_idstate.origID = queryID;
       response.d_idstate.selfGenerated = true;
       response.d_buffer = std::move(query);
       d_state = State::idle;
       ++d_currentQueriesCount;
       queueResponse(state, now, std::move(response), false);
-      return QueryProcessingResult::Empty;
+      return QueryProcessingResult::SelfAnswered;
     }
   }
 

diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc
index 7d6e5bac60bef21ef7b5515e9b7e598be694b137..8ceb242346feedf8d237c066b4b8383c09e9b2f6 100644 (file)
--- a/pdns/dnsdist.cc
+++ b/pdns/dnsdist.cc
@@ -1568,9 +1568,11 @@ bool assignOutgoingUDPQueryToBackend(std::shared_ptr<DownstreamState>& ds, uint1
 
     vinfolog("Got query for %s|%s from %s%s, relayed to %s", dq.ids.qname.toLogString(), QType(dq.ids.qtype).toString(), dq.ids.origRemote.toStringWithPort(), (doh ? " (https)" : ""), ds->getNameWithAddr());
 
+    /* make a copy since we cannot touch dq.ids after the move */
+    auto proxyProtocolPayloadSize = dq.ids.d_proxyProtocolPayloadSize;
     auto idOffset = ds->saveState(std::move(dq.ids));
     /* set the correct ID */
-    memcpy(query.data() + dq.ids.d_proxyProtocolPayloadSize, &idOffset, sizeof(idOffset));
+    memcpy(query.data() + proxyProtocolPayloadSize, &idOffset, sizeof(idOffset));
 
     /* you can't touch ids or du after this line, unless the call returned a non-negative value,
        because it might already have been freed */

diff --git a/pdns/dnsdistdist/dnsdist-nghttp2-in.cc b/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
index 432e9f22c94d41a11ff9759f6595d2e5f977ea9f..7d9c52dd87b4daee0e2ca49b7a92a7acf19262e8 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
+++ b/pdns/dnsdistdist/dnsdist-nghttp2-in.cc
@@ -230,7 +230,9 @@ std::unique_ptr<DOHUnitInterface> IncomingHTTP2Connection::getDOHUnit(uint32_t s
 void IncomingHTTP2Connection::restoreDOHUnit(std::unique_ptr<DOHUnitInterface>&& unit)
 {
   auto context = std::unique_ptr<IncomingDoHCrossProtocolContext>(dynamic_cast<IncomingDoHCrossProtocolContext*>(unit.release()));
-  d_currentStreams.at(context->d_streamID) = std::move(context->d_query);
+  if (context) {
+    d_currentStreams.at(context->d_streamID) = std::move(context->d_query);
+  }
 }
 
 IncomingHTTP2Connection::IncomingHTTP2Connection(ConnectionInfo&& connectionInfo, TCPClientThreadData& threadData, const struct timeval& now) :
@@ -906,9 +908,6 @@ void IncomingHTTP2Connection::handleIncomingQuery(IncomingHTTP2Connection::Pendi
     case QueryProcessingResult::InvalidHeaders:
       handleImmediateResponse(400, "DoH invalid headers");
       break;
-    case QueryProcessingResult::Empty:
-      handleImmediateResponse(200, "DoH empty query", std::move(query.d_buffer));
-      break;
     case QueryProcessingResult::Dropped:
       handleImmediateResponse(403, "DoH dropped query");
       break;

diff --git a/pdns/dnsdistdist/dnsdist-tcp-upstream.hh b/pdns/dnsdistdist/dnsdist-tcp-upstream.hh
index e3ee319b11a1c7bb09368d4a2c9954aa79deb26c..4c8c47323880b5fcac279457ec9ab5ee6cae6bf1 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-tcp-upstream.hh
+++ b/pdns/dnsdistdist/dnsdist-tcp-upstream.hh
@@ -27,7 +27,7 @@ public:
 class IncomingTCPConnectionState : public TCPQuerySender, public std::enable_shared_from_this<IncomingTCPConnectionState>
 {
 public:
-  enum class QueryProcessingResult : uint8_t { Forwarded, TooSmall, InvalidHeaders, Empty, Dropped, SelfAnswered, NoBackend, Asynchronous };
+  enum class QueryProcessingResult : uint8_t { Forwarded, TooSmall, InvalidHeaders, Dropped, SelfAnswered, NoBackend, Asynchronous };
   enum class ProxyProtocolResult : uint8_t { Reading, Done, Error };
 
   IncomingTCPConnectionState(ConnectionInfo&& ci, TCPClientThreadData& threadData, const struct timeval& now): d_buffer(s_maxPacketCacheEntrySize), d_ci(std::move(ci)), d_handler(d_ci.fd, timeval{g_tcpRecvTimeout,0}, d_ci.cs->tlsFrontend ? d_ci.cs->tlsFrontend->getContext() : (d_ci.cs->dohFrontend ? d_ci.cs->dohFrontend->d_tlsContext.getContext() : nullptr), now.tv_sec), d_connectionStartTime(now), d_ioState(make_unique<IOStateHandler>(*threadData.mplexer, d_ci.fd)), d_threadData(threadData), d_creatorThreadID(std::this_thread::get_id())

diff --git a/pdns/dnsdistdist/doh.cc b/pdns/dnsdistdist/doh.cc
index 128bee4126d5d09839fb34f814186525535e2f76..73cdb86f4fe3b7652db190bd36eae0313a45f2e1 100644 (file)
--- a/pdns/dnsdistdist/doh.cc
+++ b/pdns/dnsdistdist/doh.cc
@@ -606,7 +606,10 @@ public:
 
   std::shared_ptr<TCPQuerySender> getTCPQuerySender() override
   {
-    dynamic_cast<DOHUnit*>(query.d_idstate.du.get())->downstream = downstream;
+    auto* unit = dynamic_cast<DOHUnit*>(query.d_idstate.du.get());
+    if (unit != nullptr) {
+      unit->downstream = downstream;
+    }
     return s_sender;
   }
 
@@ -812,13 +815,20 @@ static void processDOHQuery(DOHUnitUniquePtr&& unit, bool inMainThread = false)
       }
 
       if (inMainThread) {
-        // NOLINTNEXTLINE(bugprone-use-after-move): cpq is not altered if the call fails
-        unit = cpq->releaseDU();
+        // cpq is not altered if the call fails but linters are not smart enough to notice that
+        if (cpq) {
+          // NOLINTNEXTLINE(bugprone-use-after-move): cpq is not altered if the call fails
+          unit = cpq->releaseDU();
+        }
         unit->status_code = 502;
         handleImmediateResponse(std::move(unit), "DoH internal error");
       }
       else {
-        cpq->handleInternalError();
+        // cpq is not altered if the call fails but linters are not smart enough to notice that
+        if (cpq) {
+          // NOLINTNEXTLINE(bugprone-use-after-move): cpq is not altered if the call fails
+          cpq->handleInternalError();
+        }
       }
       return;
     }

diff --git a/regression-tests.dnsdist/test_DOH.py b/regression-tests.dnsdist/test_DOH.py
index f9fce6be567dbfb172223b8861b3f1d386f56b23..81f39e659dbb6f72ad94b5ff66ba6aebaacf128f 100644 (file)
--- a/regression-tests.dnsdist/test_DOH.py
+++ b/regression-tests.dnsdist/test_DOH.py
@@ -253,6 +253,22 @@ class DOHTests(object):
         rcode = conn.getinfo(pycurl.RESPONSE_CODE)
         self.assertEqual(rcode, 400)
 
+    def testDOHZeroQDCount(self):
+        """
+        DOH: qdcount == 0
+        """
+        if self._dohLibrary == 'h2o':
+            raise unittest.SkipTest('h2o tries to parse the qname early, so this check will fail')
+        name = 'zero-qdcount.doh.tests.powerdns.com.'
+        query = dns.message.Message()
+        query.id = 0
+        query.flags &= ~dns.flags.RD
+        expectedResponse = dns.message.make_response(query)
+        expectedResponse.set_rcode(dns.rcode.NOTIMP)
+
+        (_, receivedResponse) = self.sendDOHQuery(self._dohServerPort, self._serverName, self._dohBaseURL, caFile=self._caCert, query=query, response=None, useQueue=False)
+        self.assertEqual(receivedResponse, expectedResponse)
+
     def testDOHShortPath(self):
         """
         DOH: Short path in GET query