From 885268cac5d76cce60847cacac87380f9fbf651e Mon Sep 17 00:00:00 2001
From: Otto Moerbeek <otto.moerbeek@open-xchange.com>
Date: Mon, 4 Nov 2024 13:07:52 +0100
Subject: [PATCH] Replace a few asserts with throws and make sure we've seen
 the version and type fields in putRecordSet.

An exception will be handled by the catch at the end of putRecordSets()

Prompted by @rgacogne
---
 .../docs/lua-scripting/functions.rst          |  1 +
 pdns/recursordist/recursor_cache.cc           | 21 +++++++++++--------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/pdns/recursordist/docs/lua-scripting/functions.rst b/pdns/recursordist/docs/lua-scripting/functions.rst
index 79690f1099..628ff91805 100644
--- a/pdns/recursordist/docs/lua-scripting/functions.rst
+++ b/pdns/recursordist/docs/lua-scripting/functions.rst
@@ -44,6 +44,7 @@ These are some functions that don't really have a place in one of the other cate
 
    Some record sets might be skipped, for example when they are already present in the record cache or contain specific information not supported yet by this function.
    If the :program:`Recursor` determines the version of the data is not compatible, it will skip loading and log an error.
+   In that case 0 is returned.
 
 .. function:: getRecordCacheRecords(perShard, maxSize) -> str, int
 
diff --git a/pdns/recursordist/recursor_cache.cc b/pdns/recursordist/recursor_cache.cc
index 72aa20c7d8..c919b6cf57 100644
--- a/pdns/recursordist/recursor_cache.cc
+++ b/pdns/recursordist/recursor_cache.cc
@@ -955,9 +955,6 @@ static void encodeComboAddress(protozero::pbf_builder<T>& writer, T type, const
   else if (address.sin4.sin_family == AF_INET6) {
     message.add_bytes(PBComboAddress::required_bytes_address, reinterpret_cast<const char*>(&address.sin6.sin6_addr.s6_addr), sizeof(address.sin6.sin6_addr)); // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast): it's the API
   }
-  else {
-    assert(0);
-  }
 }
 
 template <typename T>
@@ -971,7 +968,7 @@ static void decodeComboAddress(protozero::pbf_message<T>& reader, ComboAddress&
     address.setPort(message.get_uint32());
   }
   else {
-    assert(0);
+    throw std::runtime_error("expected port in protobuf data");
   }
   constexpr auto inet4size = sizeof(address.sin4.sin_addr);
   constexpr auto inet6size = sizeof(address.sin6.sin6_addr);
@@ -987,11 +984,11 @@ static void decodeComboAddress(protozero::pbf_message<T>& reader, ComboAddress&
       memcpy(&address.sin6.sin6_addr, data.data(), data.size());
     }
     else {
-      assert(0);
+      throw std::runtime_error("unexpected address family in protobuf data");
     }
   }
   else {
-    assert(0);
+    throw std::runtime_error("expected address bytes in protobuf data");
   }
 }
 
@@ -1052,7 +1049,7 @@ size_t MemRecursorCache::getRecordSets(size_t perShard, size_t maxSize, std::str
   auto log = g_slog->withName("recordcache")->withValues("perShard", Logging::Loggable(perShard), "maxSize", Logging::Loggable(maxSize));
   log->info(Logr::Info, "Producing cache dump");
 
-  // A size estmate is hard: size() returns the number of record *sets*. Each record set can have
+  // A size estimate is hard: size() returns the number of record *sets*. Each record set can have
   // multiple records, plus other associated records like signatures. 150 seems to works ok.
   size_t estimate = maxSize == 0 ? size() * 150 : maxSize + 4096; // We may overshoot (will be rolled back)
 
@@ -1125,7 +1122,7 @@ static void putAuthRecord(protozero::pbf_message<PBCacheEntry>& message, const D
       authRecord.d_clen = auth.get_uint32();
       break;
     default:
-      assert(0);
+      break;
     }
   }
   authRecs.emplace_back(std::make_shared<DNSRecord>(authRecord));
@@ -1191,7 +1188,6 @@ bool MemRecursorCache::putRecordSet(T& message)
       cacheEntry.d_tooBig = message.get_bool();
       break;
     default:
-      assert(0);
       break;
     }
   }
@@ -1207,6 +1203,8 @@ size_t MemRecursorCache::putRecordSets(const std::string& pbuf)
   size_t count = 0;
   size_t inserted = 0;
   try {
+    bool protocolVersionSeen = false;
+    bool typeSeen = false;
     while (full.next()) {
       switch (full.tag()) {
       case PBCacheDump::required_string_version: {
@@ -1225,6 +1223,7 @@ size_t MemRecursorCache::putRecordSets(const std::string& pbuf)
         if (protocolVersion != 1) {
           throw std::runtime_error("Protocol version mismatch");
         }
+        protocolVersionSeen = true;
         break;
       }
       case PBCacheDump::required_int64_time: {
@@ -1237,9 +1236,13 @@ size_t MemRecursorCache::putRecordSets(const std::string& pbuf)
         if (type != "PBCacheDump") {
           throw std::runtime_error("Data type mismatch");
         }
+        typeSeen = true;
         break;
       }
       case PBCacheDump::repeated_message_cacheEntry: {
+        if (!protocolVersionSeen || !typeSeen) {
+          throw std::runtime_error("Required field missing");
+        }
         protozero::pbf_message<PBCacheEntry> message = full.get_message();
         if (putRecordSet(message)) {
           ++inserted;
-- 
2.47.2