From: Remi Gacogne Date: Fri, 8 Sep 2023 08:26:50 +0000 (+0200) Subject: dnsdist: Fix a few warnings from Coverity X-Git-Tag: dnsdist-1.9.0-alpha2~4^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a56b8c0fa33e253433a71b1bc171755b94712d03;p=thirdparty%2Fpdns.git 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 7b94e00e51..3d118508cd 100644 --- 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 7d6e5bac60..8ceb242346 100644 --- a/pdns/dnsdist.cc +++ b/pdns/dnsdist.cc @@ -1568,9 +1568,11 @@ bool assignOutgoingUDPQueryToBackend(std::shared_ptr& 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 432e9f22c9..7d9c52dd87 100644 --- a/pdns/dnsdistdist/dnsdist-nghttp2-in.cc +++ b/pdns/dnsdistdist/dnsdist-nghttp2-in.cc @@ -230,7 +230,9 @@ std::unique_ptr IncomingHTTP2Connection::getDOHUnit(uint32_t s void IncomingHTTP2Connection::restoreDOHUnit(std::unique_ptr&& unit) { auto context = std::unique_ptr(dynamic_cast(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 e3ee319b11..4c8c473238 100644 --- 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 { 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(*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 128bee4126..73cdb86f4f 100644 --- a/pdns/dnsdistdist/doh.cc +++ b/pdns/dnsdistdist/doh.cc @@ -606,7 +606,10 @@ public: std::shared_ptr getTCPQuerySender() override { - dynamic_cast(query.d_idstate.du.get())->downstream = downstream; + auto* unit = dynamic_cast(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 f9fce6be56..81f39e659d 100644 --- 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