From: Otto Date: Tue, 19 Jan 2021 13:12:12 +0000 (+0100) Subject: Add unit tests and simplify code. X-Git-Tag: dnsdist-1.6.0-alpha2~72^2~5 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=e51c2d64da9ac7ed122047b999ad59339b157bff;p=thirdparty%2Fpdns.git Add unit tests and simplify code. --- diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc index 422b7728a6..9ab9b9f00c 100644 --- a/pdns/pdns_recursor.cc +++ b/pdns/pdns_recursor.cc @@ -49,7 +49,6 @@ #include #include #include -#include #include "misc.hh" #include "mtasker.hh" #include @@ -136,7 +135,7 @@ std::unique_ptr g_negCache; thread_local std::unique_ptr t_packetCache; thread_local FDMultiplexer* t_fdm{nullptr}; thread_local std::unique_ptr t_remotes, t_servfailremotes, t_largeanswerremotes, t_bogusremotes; -thread_local std::unique_ptr>> t_queryring, t_servfailqueryring, t_bogusqueryring; +thread_local std::unique_ptr > > t_queryring, t_servfailqueryring, t_bogusqueryring; thread_local std::shared_ptr t_allowFrom; #ifdef NOD_ENABLED thread_local std::shared_ptr t_nodDBp; @@ -449,7 +448,7 @@ static void handleGenUDPQueryResponse(int fd, FDMultiplexer::funcparam_t& var) ComboAddress fromaddr; socklen_t addrlen=sizeof(fromaddr); - ssize_t ret=recvfrom(fd, resp, sizeof(resp), 0, (struct sockaddr *)&fromaddr, &addrlen); + ssize_t ret=recvfrom(fd, resp, sizeof(resp), 0, (sockaddr *)&fromaddr, &addrlen); if (fromaddr != pident.remote) { g_log< representing a packet */ template static bool sendResponseOverTCP(const std::unique_ptr& dc, const T& packet) { - char buf[2]; + uint8_t buf[2]; buf[0] = packet.size() / 256; buf[1] = packet.size() % 256; Utility::iovec iov[2]; - iov[0].iov_base=(void*)buf; iov[0].iov_len=2; - iov[1].iov_base=(void*)&*packet.begin(); iov[1].iov_len = packet.size(); + iov[0].iov_base = (void*)buf; iov[0].iov_len = 2; + iov[1].iov_base = (void*)&*packet.begin(); iov[1].iov_len = packet.size(); int wret = Utility::writev(dc->d_socket, iov, 2); bool hadError = true; @@ -2353,11 +2353,13 @@ static bool handleTCPReadResult(int fd, ssize_t bytes) static bool checkForCacheHit(bool qnameParsed, unsigned int tag, const string& data, DNSName& qname, uint16_t& qtype, uint16_t& qclass, const struct timeval& now, - string& response, uint32_t& age, vState& valState, uint32_t& qhash, + string& response, uint32_t& qhash, RecursorPacketCache::OptPBData& pbData, bool tcp, const ComboAddress& source) { bool cacheHit = false; - + uint32_t age; + vState valState; + if (qnameParsed) { cacheHit = !SyncRes::s_nopacketcache && t_packetCache->getResponsePacket(tag, data, qname, qtype, qclass, now.tv_sec, &response, &age, &valState, &qhash, &pbData, tcp); } else { @@ -2378,9 +2380,8 @@ static bool checkForCacheHit(bool qnameParsed, unsigned int tag, const string& d SyncRes::s_queries++; ageDNSPacket(response, age); if (response.length() >= sizeof(struct dnsheader)) { - struct dnsheader tmpdh; - memcpy(&tmpdh, response.data(), sizeof(tmpdh)); // XXX Only needed if response.data() isn't aligned - updateResponseStats(tmpdh.rcode, source, response.length(), 0, 0); + const struct dnsheader *dh = reinterpret_cast(response.data()); + updateResponseStats(dh->rcode, source, response.length(), 0, 0); } g_stats.avgLatencyUsec = (1.0 - 1.0 / g_latencyStatSize) * g_stats.avgLatencyUsec + 0.0; // we assume 0 usec g_stats.avgLatencyOursUsec = (1.0 - 1.0 / g_latencyStatSize) * g_stats.avgLatencyOursUsec + 0.0; // we assume 0 usec @@ -2660,13 +2661,11 @@ static void handleRunningTCPQuestion(int fd, FDMultiplexer::funcparam_t& var) ++g_stats.tcpqcounter; string response; - uint32_t age; - vState valState; uint32_t qhash = 0; RecursorPacketCache::OptPBData pbData{boost::none}; - bool cacheHit = checkForCacheHit(qnameParsed, dc->d_tag, conn->data, qname, qtype, qclass, g_now, response, age, valState, qhash, pbData, true, dc->d_source); - dc->d_qhash=qhash; + bool cacheHit = checkForCacheHit(qnameParsed, dc->d_tag, conn->data, qname, qtype, qclass, g_now, response, qhash, pbData, true, dc->d_source); + dc->d_qhash = qhash; if (cacheHit) { if (t_protobufServers && dc->d_logResponse && !(luaconfsLocal->protobufExportConfig.taggedOnly && pbData && !pbData->d_tagged)) { @@ -2719,7 +2718,7 @@ static void handleRunningTCPQuestion(int fd, FDMultiplexer::funcparam_t& var) if (conn->d_requestsInFlight >= TCPConnection::s_maxInFlight) { t_fdm->removeReadFD(fd); // should no longer awake ourselves when there is data to read } else { - Utility::gettimeofday(&g_now, 0); // needed? + Utility::gettimeofday(&g_now, nullptr); // needed? struct timeval ttd = g_now; t_fdm->setReadTTD(fd, ttd, g_tcpTimeout); } @@ -2867,7 +2866,6 @@ static string* doProcessUDPQuestion(const std::string& question, const ComboAddr DNSName qname; uint16_t qtype=0; uint16_t qclass=0; - uint32_t age; bool qnameParsed=false; #ifdef MALLOC_TRACE /* @@ -2932,8 +2930,7 @@ static string* doProcessUDPQuestion(const std::string& question, const ComboAddr /* It might seem like a good idea to skip the packet cache lookup if we know that the answer is not cacheable, but it means that the hash would not be computed. If some script decides at a later time to mark back the answer as cacheable we would cache it with a wrong tag, so better safe than sorry. */ - vState valState; - bool cacheHit = checkForCacheHit(qnameParsed, ctag, question, qname, qtype, qclass, g_now, response, age, valState, qhash, pbData, false, source); + bool cacheHit = checkForCacheHit(qnameParsed, ctag, question, qname, qtype, qclass, g_now, response, qhash, pbData, false, source); if (cacheHit) { if (t_protobufServers && logResponse && !(luaconfsLocal->protobufExportConfig.taggedOnly && pbData && !pbData->d_tagged)) { protobufLogResponse(dh, pbData, tv, false, source, destination, ednssubnet, uniqueId, requestorId, deviceId, deviceName); @@ -2957,15 +2954,6 @@ static string* doProcessUDPQuestion(const std::string& question, const ComboAddr << (source != fromaddr ? " (via " + fromaddr.toStringWithPort() + ")" : "") << " failed with: " << strerror(sendErr) << endl; } -#if 0 - if(response.length() >= sizeof(struct dnsheader)) { - struct dnsheader tmpdh; - memcpy(&tmpdh, response.c_str(), sizeof(tmpdh)); - updateResponseStats(tmpdh.rcode, source, response.length(), 0, 0); - } - g_stats.avgLatencyUsec=(1-1.0/g_latencyStatSize)*g_stats.avgLatencyUsec + 0.0; // we assume 0 usec - g_stats.avgLatencyOursUsec=(1-1.0/g_latencyStatSize)*g_stats.avgLatencyOursUsec + 0.0; // we assume 0 usec -#endif return 0; } } diff --git a/pdns/recpacketcache.cc b/pdns/recpacketcache.cc index 570fc83013..0485419824 100644 --- a/pdns/recpacketcache.cc +++ b/pdns/recpacketcache.cc @@ -42,11 +42,10 @@ int RecursorPacketCache::doWipePacketCache(const DNSName& name, uint16_t qtype, return count; } -bool RecursorPacketCache::qrMatch(const packetCache_t::index::type::iterator& iter, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass, bool tcp) +bool RecursorPacketCache::qrMatch(const packetCache_t::index::type::iterator& iter, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass) { // this ignores checking on the EDNS subnet flags! - // XXX OM tcp check is likely not needed, enforced by index - if (qname != iter->d_name || iter->d_type != qtype || iter->d_class != qclass || iter->d_tcp != tcp) { + if (qname != iter->d_name || iter->d_type != qtype || iter->d_class != qclass) { return false; } @@ -54,11 +53,11 @@ bool RecursorPacketCache::qrMatch(const packetCache_t::index::type::ite return queryMatches(iter->d_query, queryPacket, qname, optionsToSkip); } -bool RecursorPacketCache::checkResponseMatches(std::pair::type::iterator, packetCache_t::index::type::iterator> range, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass, time_t now, std::string* responsePacket, uint32_t* age, vState* valState, OptPBData* pbdata, bool tcp) +bool RecursorPacketCache::checkResponseMatches(std::pair::type::iterator, packetCache_t::index::type::iterator> range, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass, time_t now, std::string* responsePacket, uint32_t* age, vState* valState, OptPBData* pbdata) { for(auto iter = range.first ; iter != range.second ; ++iter) { // the possibility is VERY real that we get hits that are not right - birthday paradox - if (!qrMatch(iter, queryPacket, qname, qtype, qclass, tcp)) { + if (!qrMatch(iter, queryPacket, qname, qtype, qclass)) { continue; } @@ -134,7 +133,7 @@ bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string& return false; } - return checkResponseMatches(range, queryPacket, qname, qtype, qclass, now, responsePacket, age, valState, pbdata, tcp); + return checkResponseMatches(range, queryPacket, qname, qtype, qclass, now, responsePacket, age, valState, pbdata); } bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string& queryPacket, DNSName& qname, uint16_t* qtype, uint16_t* qclass, time_t now, @@ -151,7 +150,7 @@ bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string& qname = DNSName(queryPacket.c_str(), queryPacket.length(), sizeof(dnsheader), false, qtype, qclass, 0); - return checkResponseMatches(range, queryPacket, qname, *qtype, *qclass, now, responsePacket, age, valState, pbdata, tcp); + return checkResponseMatches(range, queryPacket, qname, *qtype, *qclass, now, responsePacket, age, valState, pbdata); } @@ -162,8 +161,7 @@ void RecursorPacketCache::insertResponsePacket(unsigned int tag, uint32_t qhash, auto iter = range.first; for( ; iter != range.second ; ++iter) { - // XXX OM tcp check not needed? - if (iter->d_type != qtype || iter->d_class != qclass || iter->d_tcp != tcp || iter->d_name != qname ) { + if (iter->d_type != qtype || iter->d_class != qclass || iter->d_name != qname ) { continue; } diff --git a/pdns/recpacketcache.hh b/pdns/recpacketcache.hh index fa5d713649..de6e3c719b 100644 --- a/pdns/recpacketcache.hh +++ b/pdns/recpacketcache.hh @@ -126,8 +126,8 @@ private: packetCache_t d_packetCache; - static bool qrMatch(const packetCache_t::index::type::iterator& iter, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass, bool tcp); - bool checkResponseMatches(std::pair::type::iterator, packetCache_t::index::type::iterator> range, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass, time_t now, std::string* responsePacket, uint32_t* age, vState* valState, OptPBData* pbdata, bool tcp); + static bool qrMatch(const packetCache_t::index::type::iterator& iter, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass); + bool checkResponseMatches(std::pair::type::iterator, packetCache_t::index::type::iterator> range, const std::string& queryPacket, const DNSName& qname, uint16_t qtype, uint16_t qclass, time_t now, std::string* responsePacket, uint32_t* age, vState* valState, OptPBData* pbdata); public: void preRemoval(const Entry& entry) diff --git a/pdns/test-recpacketcache_cc.cc b/pdns/test-recpacketcache_cc.cc index 95107b5d61..f3e0a994d7 100644 --- a/pdns/test-recpacketcache_cc.cc +++ b/pdns/test-recpacketcache_cc.cc @@ -281,4 +281,116 @@ BOOST_AUTO_TEST_CASE(test_recPacketCache_Tags) { BOOST_CHECK_EQUAL(fpacket, r2packet); } +BOOST_AUTO_TEST_CASE(test_recPacketCache_TCP) { + /* Insert a response with UDP, the exact same query with a TCP flag + should lead to a miss. + */ + RecursorPacketCache rpc; + string fpacket; + uint32_t age=0; + uint32_t qhash=0; + uint32_t temphash=0; + uint32_t ttd=3600; + BOOST_CHECK_EQUAL(rpc.size(), 0U); + + ::arg().set("rng")="auto"; + ::arg().set("entropy-source")="/dev/urandom"; + + DNSName qname("www.powerdns.com"); + vector packet; + DNSPacketWriter pw(packet, qname, QType::A); + pw.getHeader()->rd=true; + pw.getHeader()->qr=false; + pw.getHeader()->id=dns_random_uint16(); + string qpacket(reinterpret_cast(&packet[0]), packet.size()); + pw.startRecord(qname, QType::A, ttd); + + /* Both interfaces (with and without the qname/qtype/qclass) should get the same hash */ + BOOST_CHECK_EQUAL(rpc.getResponsePacket(0, qpacket, time(nullptr), &fpacket, &age, &qhash), false); + BOOST_CHECK_EQUAL(rpc.getResponsePacket(0, qpacket, qname, QType::A, QClass::IN, time(nullptr), &fpacket, &age, nullptr, &temphash, nullptr, false), false); + BOOST_CHECK_EQUAL(qhash, temphash); + + /* Different tcp/udp, should still get get the same hash, for both interfaces */ + BOOST_CHECK_EQUAL(rpc.getResponsePacket(0, qpacket, time(nullptr), &fpacket, &age, &qhash), false); + BOOST_CHECK_EQUAL(rpc.getResponsePacket(0, qpacket, qname, QType::A, QClass::IN, time(nullptr), &fpacket, &age, nullptr, &temphash, nullptr, true), false); + BOOST_CHECK_EQUAL(qhash, temphash); + + { + ARecordContent ar("127.0.0.1"); + ar.toPacket(pw); + pw.commit(); + } + string r1packet(reinterpret_cast(&packet[0]), packet.size()); + + { + ARecordContent ar("127.0.0.2"); + ar.toPacket(pw); + pw.commit(); + } + string r2packet(reinterpret_cast(&packet[0]), packet.size()); + + BOOST_CHECK(r1packet != r2packet); + + /* inserting a response for udp */ + rpc.insertResponsePacket(0, qhash, string(qpacket), qname, QType::A, QClass::IN, string(r1packet), time(0), ttd, vState::Indeterminate, boost::none, false); + BOOST_CHECK_EQUAL(rpc.size(), 1U); + + /* inserting a different response for tcp, should not override the first one */ + rpc.insertResponsePacket(0, qhash, string(qpacket), qname, QType::A, QClass::IN, string(r2packet), time(0), ttd, vState::Indeterminate, boost::none, true); + BOOST_CHECK_EQUAL(rpc.size(), 2U); + + /* remove all responses from the cache */ + rpc.doPruneTo(0); + BOOST_CHECK_EQUAL(rpc.size(), 0U); + + /* reinsert both */ + rpc.insertResponsePacket(0, qhash, string(qpacket), qname, QType::A, QClass::IN, string(r1packet), time(0), ttd, vState::Indeterminate, boost::none, false); + BOOST_CHECK_EQUAL(rpc.size(), 1U); + + rpc.insertResponsePacket(0, qhash, string(qpacket), qname, QType::A, QClass::IN, string(r2packet), time(0), ttd, vState::Indeterminate, boost::none, true); + BOOST_CHECK_EQUAL(rpc.size(), 2U); + + /* remove the responses by qname, should remove both */ + rpc.doWipePacketCache(qname); + BOOST_CHECK_EQUAL(rpc.size(), 0U); + + /* insert the response for tcp */ + rpc.insertResponsePacket(0, qhash, string(qpacket), qname, QType::A, QClass::IN, string(r1packet), time(0), ttd, vState::Indeterminate, boost::none, true); + BOOST_CHECK_EQUAL(rpc.size(), 1U); + + vState vState; + /* we can retrieve it */ + BOOST_CHECK_EQUAL(rpc.getResponsePacket(0, qpacket, qname, QType::A, QClass::IN, time(nullptr), &fpacket, &age, &vState, &temphash, nullptr, true), true); + BOOST_CHECK_EQUAL(qhash, temphash); + BOOST_CHECK_EQUAL(fpacket, r1packet); + + /* first interface assumes udp */ + BOOST_CHECK_EQUAL(rpc.getResponsePacket(0, qpacket, time(nullptr), &fpacket, &age, &temphash), false); + BOOST_CHECK_EQUAL(qhash, temphash); + BOOST_CHECK_EQUAL(fpacket, r1packet); + + /* and not with expliclit udp */ + BOOST_CHECK_EQUAL(rpc.getResponsePacket(0, qpacket, qname, QType::A, QClass::IN, time(nullptr), &fpacket, &age, &vState, &temphash, nullptr, false), false); + /* we should still get the same hash */ + BOOST_CHECK_EQUAL(temphash, qhash); + + /* adding a response for udp */ + rpc.insertResponsePacket(0, qhash, string(qpacket), qname, QType::A, QClass::IN, string(r2packet), time(0), ttd, vState::Indeterminate, boost::none, false); + BOOST_CHECK_EQUAL(rpc.size(), 2U); + + /* We get the correct response for udp now */ + BOOST_CHECK_EQUAL(rpc.getResponsePacket(0, qpacket, time(nullptr), &fpacket, &age, &temphash), true); + BOOST_CHECK_EQUAL(qhash, temphash); + BOOST_CHECK_EQUAL(fpacket, r2packet); + + BOOST_CHECK_EQUAL(rpc.getResponsePacket(0, qpacket, qname, QType::A, QClass::IN, time(nullptr), &fpacket, &age, &temphash), true); + BOOST_CHECK_EQUAL(qhash, temphash); + BOOST_CHECK_EQUAL(fpacket, r2packet); + + /* and the correct response for tcp */ + BOOST_CHECK_EQUAL(rpc.getResponsePacket(0, qpacket, qname, QType::A, QClass::IN, time(nullptr), &fpacket, &age, &vState, &temphash, nullptr, true), true); + BOOST_CHECK_EQUAL(qhash, temphash); + BOOST_CHECK_EQUAL(fpacket, r1packet); +} + BOOST_AUTO_TEST_SUITE_END()