From f89689a2a088548840b0b1d5c32ac3db32e17874 Mon Sep 17 00:00:00 2001 From: Miod Vallat Date: Thu, 3 Apr 2025 07:44:22 +0200 Subject: [PATCH] Better error reporting interface for checkRRSet(). --- pdns/check-zone.cc | 41 +++++++++++++++++++++++++++++------------ pdns/check-zone.hh | 9 +-------- pdns/pdnsutil.cc | 13 ++++++++----- pdns/ws-auth.cc | 11 ++++++----- 4 files changed, 44 insertions(+), 30 deletions(-) diff --git a/pdns/check-zone.cc b/pdns/check-zone.cc index a8c1f839a..beccefbfc 100644 --- a/pdns/check-zone.cc +++ b/pdns/check-zone.cc @@ -28,12 +28,7 @@ namespace Check { -static void rejectRecord(const DNSResourceRecord& rec, const std::string& why) -{ - throw CheckException("RRset " + rec.qname.toString() + " IN " + rec.qtype.toString() + why); -} - -void checkRRSet(vector& records, const ZoneName& zone) +void checkRRSet(vector& records, const ZoneName& zone, vector>& errors, bool stopAtFirstError) { // QTypes that MUST NOT have multiple records of the same type in a given RRset. static const std::set onlyOneEntryTypes = {QType::CNAME, QType::DNAME, QType::SOA}; @@ -42,6 +37,10 @@ void checkRRSet(vector& records, const ZoneName& zone) // QTypes that are NOT allowed at apex. static const std::set nonApexTypes = {QType::DS}; + if (stopAtFirstError) { + errors.reserve(1); + } + sort(records.begin(), records.end(), [](const DNSResourceRecord& rec_a, const DNSResourceRecord& rec_b) -> bool { /* we need _strict_ weak ordering */ @@ -53,27 +52,42 @@ void checkRRSet(vector& records, const ZoneName& zone) if (previous.qname == rec.qname) { if (previous.qtype == rec.qtype) { if (onlyOneEntryTypes.count(rec.qtype.getCode()) != 0) { - rejectRecord(rec, ": only one such record allowed"); + errors.emplace_back(std::make_pair(rec, ": only one such record allowed")); + if (stopAtFirstError) { + break; + } } if (previous.content == rec.content) { - rejectRecord(rec, std::string{": duplicate record with content \""} + rec.content + "\""); + errors.emplace_back(std::make_pair(rec, std::string{": duplicate record with content \""} + rec.content + "\"")); + if (stopAtFirstError) { + break; + } } } else { if (QType::exclusiveEntryTypes.count(rec.qtype.getCode()) != 0 || QType::exclusiveEntryTypes.count(previous.qtype.getCode()) != 0) { - rejectRecord(rec, std::string{": conflicts with existing "} + previous.qtype.toString() + " RRset of the same name"); + errors.emplace_back(std::make_pair(rec, std::string{": conflicts with existing "} + previous.qtype.toString() + " RRset of the same name")); + if (stopAtFirstError) { + break; + } } } } if (rec.qname == zone.operator const DNSName&()) { if (nonApexTypes.count(rec.qtype.getCode()) != 0) { - rejectRecord(rec, ": is not allowed at apex"); + errors.emplace_back(std::make_pair(rec, ": is not allowed at apex")); + if (stopAtFirstError) { + break; + } } } else if (atApexTypes.count(rec.qtype.getCode()) != 0) { - rejectRecord(rec, ": is only allowed at apex"); + errors.emplace_back(std::make_pair(rec, ": is only allowed at apex")); + if (stopAtFirstError) { + break; + } } // Check if the DNSNames that should be hostnames, are hostnames @@ -81,7 +95,10 @@ void checkRRSet(vector& records, const ZoneName& zone) checkHostnameCorrectness(rec); } catch (const std::exception& e) { - rejectRecord(rec, std::string{": "} + e.what()); + errors.emplace_back(std::make_pair(rec, std::string{": "} + e.what())); + if (stopAtFirstError) { + break; + } } previous = rec; diff --git a/pdns/check-zone.hh b/pdns/check-zone.hh index 4bbcac4d2..d71b367dd 100644 --- a/pdns/check-zone.hh +++ b/pdns/check-zone.hh @@ -20,13 +20,6 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ -class CheckException : public runtime_error -{ -public: - CheckException(const string& what_arg) : - runtime_error(what_arg) {} -}; - namespace Check { @@ -38,6 +31,6 @@ namespace Check * *) no duplicates for QTypes that can only be present once per RRset * *) hostnames are hostnames */ -void checkRRSet(vector& records, const ZoneName& zone); +void checkRRSet(vector& records, const ZoneName& zone, vector>& errors, bool stopAtFirstError); } // namespace Check diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc index 54cae5e4b..47c08c3e8 100644 --- a/pdns/pdnsutil.cc +++ b/pdns/pdnsutil.cc @@ -2558,13 +2558,16 @@ static int addOrReplaceRecord(bool isAdd, const vector& cmds) newrrs = std::move(oldrrs); } - try { - Check::checkRRSet(newrrs, zone); - } - catch (CheckException& e) { - cerr << e.what() << endl; + std::vector> errors; + Check::checkRRSet(newrrs, zone, errors, false); + if (!errors.empty()) { + for (const auto& error : errors) { + const auto [rec, why] = error; + cerr << "RRset " << rec.qname.toString() << " IN " << rec.qtype.toString() << why << endl; + } return EXIT_FAILURE; } + if (isAdd) { // We had collected all record types earlier in order to be able to // perform the proper checks. Trim the list to only keep those of the diff --git a/pdns/ws-auth.cc b/pdns/ws-auth.cc index d83ccc33f..d02bbbd2d 100644 --- a/pdns/ws-auth.cc +++ b/pdns/ws-auth.cc @@ -1665,11 +1665,12 @@ static void gatherRecordsFromZone(const std::string& zonestring, vector& records, const ZoneName& zone) { - try { - Check::checkRRSet(records, zone); - } - catch (CheckException& e) { - throw ApiException(e.what()); + std::vector> errors; + + Check::checkRRSet(records, zone, errors, true); + if (!errors.empty()) { + const auto [rec, why] = errors.front(); + throw ApiException("RRset " + rec.qname.toString() + " IN " + rec.qtype.toString() + why); } } -- 2.47.3