From cce0983c9fca654ae785a32184deb15d0f8f3296 Mon Sep 17 00:00:00 2001 From: Charles-Henri Bruyand Date: Thu, 18 Jan 2024 13:50:44 +0100 Subject: [PATCH 1/4] dnsname: move len and offset from int to size_t --- pdns/dnsname.cc | 11 +++++------ pdns/dnsname.hh | 4 ++-- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/pdns/dnsname.cc b/pdns/dnsname.cc index 25a90078a890..78ac49abe7e0 100644 --- a/pdns/dnsname.cc +++ b/pdns/dnsname.cc @@ -99,8 +99,7 @@ DNSName::DNSName(const std::string_view sw) } } - -DNSName::DNSName(const char* pos, int len, int offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, uint16_t minOffset) +DNSName::DNSName(const char* pos, size_t len, size_t offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, uint16_t minOffset) { if (offset >= len) throw std::range_error("Trying to read past the end of the buffer ("+std::to_string(offset)+ " >= "+std::to_string(len)+")"); @@ -115,7 +114,7 @@ DNSName::DNSName(const char* pos, int len, int offset, bool uncompress, uint16_t } // this should be the __only__ dns name parser in PowerDNS. -void DNSName::packetParser(const char* qpos, int len, int offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, int depth, uint16_t minOffset) +void DNSName::packetParser(const char* qpos, size_t len, size_t offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, int depth, uint16_t minOffset) { const unsigned char* pos=(const unsigned char*)qpos; unsigned char labellen; @@ -123,7 +122,7 @@ void DNSName::packetParser(const char* qpos, int len, int offset, bool uncompres if (offset >= len) throw std::range_error("Trying to read past the end of the buffer ("+std::to_string(offset)+ " >= "+std::to_string(len)+")"); - if (offset < (int) minOffset) + if (offset < static_cast(minOffset)) throw std::range_error("Trying to read before the beginning of the buffer ("+std::to_string(offset)+ " < "+std::to_string(minOffset)+")"); const unsigned char* end = pos + len; @@ -134,10 +133,10 @@ void DNSName::packetParser(const char* qpos, int len, int offset, bool uncompres throw std::range_error("Found compressed label, instructed not to follow"); labellen &= (~0xc0); - int newpos = (labellen << 8) + *(const unsigned char*)pos; + size_t newpos = (labellen << 8) + *(const unsigned char*)pos; if(newpos < offset) { - if(newpos < (int) minOffset) + if(newpos < static_cast(minOffset)) throw std::range_error("Invalid label position during decompression ("+std::to_string(newpos)+ " < "+std::to_string(minOffset)+")"); if (++depth > 100) throw std::range_error("Abort label decompression after 100 redirects"); diff --git a/pdns/dnsname.hh b/pdns/dnsname.hh index 2350d2bd3b8a..b338a532958d 100644 --- a/pdns/dnsname.hh +++ b/pdns/dnsname.hh @@ -100,7 +100,7 @@ public: DNSName(DNSName&& a) = default; explicit DNSName(std::string_view sw); //!< Constructs from a human formatted, escaped presentation - DNSName(const char* p, int len, int offset, bool uncompress, uint16_t* qtype = nullptr, uint16_t* qclass = nullptr, unsigned int* consumed = nullptr, uint16_t minOffset = 0); //!< Construct from a DNS Packet, taking the first question if offset=12. If supplied, consumed is set to the number of bytes consumed from the packet, which will not be equal to the wire length of the resulting name in case of compression. + DNSName(const char* p, size_t len, size_t offset, bool uncompress, uint16_t* qtype = nullptr, uint16_t* qclass = nullptr, unsigned int* consumed = nullptr, uint16_t minOffset = 0); //!< Construct from a DNS Packet, taking the first question if offset=12. If supplied, consumed is set to the number of bytes consumed from the packet, which will not be equal to the wire length of the resulting name in case of compression. bool isPartOf(const DNSName& rhs) const; //!< Are we part of the rhs name? Note that name.isPartOf(name). inline bool operator==(const DNSName& rhs) const; //!< DNS-native comparison (case insensitive) - empty compares to empty @@ -216,7 +216,7 @@ public: private: string_t d_storage; - void packetParser(const char* p, int len, int offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, int depth, uint16_t minOffset); + void packetParser(const char* p, size_t len, size_t offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, int depth, uint16_t minOffset); static void appendEscapedLabel(std::string& appendTo, const char* orig, size_t len); static std::string unescapeLabel(const std::string& orig); static void throwSafeRangeError(const std::string& msg, const char* buf, size_t length); From 05fe7452e75e4600c37eafdac09a65ef9e0c9e86 Mon Sep 17 00:00:00 2001 From: Charles-Henri Bruyand Date: Fri, 19 Jan 2024 10:24:01 +0100 Subject: [PATCH 2/4] dnsname: clang-tidy fix a few missing braces --- pdns/dnsname.cc | 89 ++++++++++++++++++++++++++++++++----------------- pdns/dnsname.hh | 2 +- 2 files changed, 59 insertions(+), 32 deletions(-) diff --git a/pdns/dnsname.cc b/pdns/dnsname.cc index 78ac49abe7e0..e02f6d4015e6 100644 --- a/pdns/dnsname.cc +++ b/pdns/dnsname.cc @@ -120,10 +120,12 @@ void DNSName::packetParser(const char* qpos, size_t len, size_t offset, bool unc unsigned char labellen; const unsigned char *opos = pos; - if (offset >= len) + if (offset >= len) { throw std::range_error("Trying to read past the end of the buffer ("+std::to_string(offset)+ " >= "+std::to_string(len)+")"); - if (offset < static_cast(minOffset)) + } + if (offset < static_cast(minOffset)) { throw std::range_error("Trying to read before the beginning of the buffer ("+std::to_string(offset)+ " < "+std::to_string(minOffset)+")"); + } const unsigned char* end = pos + len; pos += offset; @@ -136,13 +138,16 @@ void DNSName::packetParser(const char* qpos, size_t len, size_t offset, bool unc size_t newpos = (labellen << 8) + *(const unsigned char*)pos; if(newpos < offset) { - if(newpos < static_cast(minOffset)) + if(newpos < static_cast(minOffset)) { throw std::range_error("Invalid label position during decompression ("+std::to_string(newpos)+ " < "+std::to_string(minOffset)+")"); - if (++depth > 100) + } + if (++depth > 100) { throw std::range_error("Abort label decompression after 100 redirects"); + } packetParser((const char*)opos, len, newpos, true, nullptr, nullptr, nullptr, depth, minOffset); - } else + } else { throw std::range_error("Found a forward reference during label decompression"); + } pos++; break; } else if(labellen & 0xc0) { @@ -151,14 +156,17 @@ void DNSName::packetParser(const char* qpos, size_t len, size_t offset, bool unc if (pos + labellen < end) { appendRawLabel((const char*)pos, labellen); } - else + else { throw std::range_error("Found an invalid label length in qname"); + } pos+=labellen; } - if(d_storage.empty()) + if(d_storage.empty()) { d_storage.append(1, (char)0); // we just parsed the root - if(consumed) + } + if(consumed) { *consumed = pos - opos - offset; + } if(qtype) { if (pos + 2 > end) { throw std::range_error("Trying to read qtype past the end of the buffer ("+std::to_string((pos - opos) + 2)+ " > "+std::to_string(len)+")"); @@ -224,8 +232,9 @@ std::string DNSName::toLogString() const std::string DNSName::toDNSString() const { - if (empty()) + if (empty()) { throw std::out_of_range("Attempt to DNSString an unset dnsname"); + } return std::string(d_storage.c_str(), d_storage.length()); } @@ -249,11 +258,13 @@ size_t DNSName::wirelength() const { // Are WE part of parent bool DNSName::isPartOf(const DNSName& parent) const { - if(parent.empty() || empty()) + if(parent.empty() || empty()) { throw std::out_of_range("empty dnsnames aren't part of anything"); + } - if(parent.d_storage.size() > d_storage.size()) + if(parent.d_storage.size() > d_storage.size()) { return false; + } // this is slightly complicated since we can't start from the end, since we can't see where a label begins/ends then for(auto us=d_storage.cbegin(); us l=getRawLabels(); @@ -342,14 +355,17 @@ void DNSName::appendRawLabel(const std::string& label) void DNSName::appendRawLabel(const char* start, unsigned int length) { - if(length==0) + if (length==0) { throw std::range_error("no such thing as an empty label to append"); - if(length > 63) + } + if (length > 63) { throw std::range_error("label too long to append"); - if(d_storage.size() + length > s_maxDNSNameLength - 1) // reserve one byte for the label length + } + if (d_storage.size() + length > s_maxDNSNameLength - 1) { // reserve one byte for the label length throw std::range_error("name too long to append"); + } - if(d_storage.empty()) { + if (d_storage.empty()) { d_storage.append(1, (char)length); } else { @@ -361,15 +377,19 @@ void DNSName::appendRawLabel(const char* start, unsigned int length) void DNSName::prependRawLabel(const std::string& label) { - if(label.empty()) + if (label.empty()) { throw std::range_error("no such thing as an empty label to prepend"); - if(label.size() > 63) + } + if (label.size() > 63) { throw std::range_error("label too long to prepend"); - if(d_storage.size() + label.size() > s_maxDNSNameLength - 1) // reserve one byte for the label length + } + if (d_storage.size() + label.size() > s_maxDNSNameLength - 1) { // reserve one byte for the label length throw std::range_error("name too long to prepend"); + } - if(d_storage.empty()) + if (d_storage.empty()) { d_storage.append(1, (char)0); + } string_t prep(1, (char)label.size()); prep.append(label.c_str(), label.size()); @@ -414,16 +434,18 @@ DNSName DNSName::getLastLabel() const bool DNSName::chopOff() { - if(d_storage.empty() || d_storage[0]==0) + if (d_storage.empty() || d_storage[0]==0) { return false; + } d_storage.erase(0, (unsigned int)d_storage[0]+1); return true; } bool DNSName::isWildcard() const { - if(d_storage.size() < 2) + if (d_storage.size() < 2) { return false; + } auto p = d_storage.begin(); return (*p == 0x01 && *++p == '*'); } @@ -453,8 +475,9 @@ unsigned int DNSName::countLabels() const void DNSName::trimToLabels(unsigned int to) { - while(countLabels() > to && chopOff()) + while(countLabels() > to && chopOff()) { ; + } } @@ -469,12 +492,15 @@ void DNSName::appendEscapedLabel(std::string& appendTo, const char* orig, size_t while (pos < len) { auto p = static_cast(orig[pos]); - if(p=='.') + if (p=='.') { appendTo+="\\."; - else if(p=='\\') + } + else if (p=='\\') { appendTo+="\\\\"; - else if(p > 0x20 && p < 0x7f) + } + else if (p > 0x20 && p < 0x7f) { appendTo.append(1, (char)p); + } else { char buf[] = "000"; auto got = snprintf(buf, sizeof(buf), "%03" PRIu8, p); @@ -497,11 +523,12 @@ bool DNSName::has8bitBytes() const for (size_t idx = 0; idx < length; idx++) { ++pos; char c = s.at(pos); - if(!((c >= 'a' && c <= 'z') || - (c >= 'A' && c <= 'Z') || - (c >= '0' && c <= '9') || - c =='-' || c == '_' || c=='*' || c=='.' || c=='/' || c=='@' || c==' ' || c=='\\' || c==':')) + if (!((c >= 'a' && c <= 'z') || + (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') || + c =='-' || c == '_' || c=='*' || c=='.' || c=='/' || c=='@' || c==' ' || c=='\\' || c==':')) { return true; + } } ++pos; length = s.at(pos); diff --git a/pdns/dnsname.hh b/pdns/dnsname.hh index b338a532958d..64c14732fe7c 100644 --- a/pdns/dnsname.hh +++ b/pdns/dnsname.hh @@ -216,7 +216,7 @@ public: private: string_t d_storage; - void packetParser(const char* p, size_t len, size_t offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, int depth, uint16_t minOffset); + void packetParser(const char* qpos, size_t len, size_t offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, int depth, uint16_t minOffset); static void appendEscapedLabel(std::string& appendTo, const char* orig, size_t len); static std::string unescapeLabel(const std::string& orig); static void throwSafeRangeError(const std::string& msg, const char* buf, size_t length); From 7cd99b086bc1ed191d7feef63d1e51b96a9664ec Mon Sep 17 00:00:00 2001 From: Charles-Henri Bruyand Date: Fri, 19 Jan 2024 11:47:16 +0100 Subject: [PATCH 3/4] dnsname: tidy some implicit conversion --- pdns/dnsname.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pdns/dnsname.cc b/pdns/dnsname.cc index e02f6d4015e6..0c45c96c51fb 100644 --- a/pdns/dnsname.cc +++ b/pdns/dnsname.cc @@ -161,13 +161,13 @@ void DNSName::packetParser(const char* qpos, size_t len, size_t offset, bool unc } pos+=labellen; } - if(d_storage.empty()) { + if (d_storage.empty()) { d_storage.append(1, (char)0); // we just parsed the root } - if(consumed) { + if (consumed != nullptr) { *consumed = pos - opos - offset; } - if(qtype) { + if (qtype != nullptr) { if (pos + 2 > end) { throw std::range_error("Trying to read qtype past the end of the buffer ("+std::to_string((pos - opos) + 2)+ " > "+std::to_string(len)+")"); } From 8e9a1388b99efecdef9a10479f52a5fcae193f3f Mon Sep 17 00:00:00 2001 From: Charles-Henri Bruyand Date: Tue, 23 Jan 2024 09:42:01 +0100 Subject: [PATCH 4/4] dnsname: remove unnecessary cast as suggested by Otto --- pdns/dnsname.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pdns/dnsname.cc b/pdns/dnsname.cc index 0c45c96c51fb..4534788b0da3 100644 --- a/pdns/dnsname.cc +++ b/pdns/dnsname.cc @@ -137,8 +137,8 @@ void DNSName::packetParser(const char* qpos, size_t len, size_t offset, bool unc labellen &= (~0xc0); size_t newpos = (labellen << 8) + *(const unsigned char*)pos; - if(newpos < offset) { - if(newpos < static_cast(minOffset)) { + if (newpos < offset) { + if (newpos < minOffset) { throw std::range_error("Invalid label position during decompression ("+std::to_string(newpos)+ " < "+std::to_string(minOffset)+")"); } if (++depth > 100) {