Merge pull request #13521 from omoerbeek/coverity20231120

rec: set of coverity fixes 20231120

pdns/axfr-retriever.cc

@@ -175,7 +175,7 @@ void AXFRRetriever::timeoutReadn(uint16_t bytes, uint16_t timeoutsec)
   int n=0;
   int numread;
   while(n<bytes) {
-    int res=waitForData(d_sock, timeoutsec-(time(nullptr)-start));
+    int res=waitForData(d_sock, static_cast<int>(timeoutsec - (time(nullptr) - start)));
     if(res<0)
       throw ResolverException("Reading data from remote nameserver over TCP: "+stringerror());
     if(!res)

pdns/ixfr.cc

@@ -185,6 +185,7 @@ vector<pair<vector<DNSRecord>, vector<DNSRecord>>> getIXFRDeltas(const ComboAddr
   s.connect(primary, xfrTimeout);
 
   time_t elapsed = timeoutChecker();
+  // coverity[store_truncates_time_t]
   s.writenWithTimeout(msg.data(), msg.size(), xfrTimeout - elapsed);
 
   // CURRENT PRIMARY SOA
@@ -271,7 +272,7 @@ vector<pair<vector<DNSRecord>, vector<DNSRecord>>> getIXFRDeltas(const ComboAddr
           // we are up to date
           return ret;
         }
-        primarySOA = sr;
+        primarySOA = std::move(sr);
         ++primarySOACount;
       } else if (r.first.d_type == QType::SOA) {
         auto sr = getRR<SOARecordContent>(r.first);

pdns/recursordist/aggressive_nsec.cc

@@ -349,7 +349,7 @@ void AggressiveNSECCache::insertNSEC(const DNSName& zone, const DNSName& owner,
         ++d_entriesCount;
       }
       else {
-        zoneEntry->d_entries.replace(pair.first, {record.getContent(), signatures, realOwner, next, record.d_ttl});
+        zoneEntry->d_entries.replace(pair.first, {record.getContent(), signatures, std::move(realOwner), next, record.d_ttl});
       }
     }
     else {
@@ -358,7 +358,7 @@ void AggressiveNSECCache::insertNSEC(const DNSName& zone, const DNSName& owner,
         ++d_entriesCount;
       }
       else {
-        zoneEntry->d_entries.replace(pair.first, {record.getContent(), signatures, owner, next, record.d_ttl});
+        zoneEntry->d_entries.replace(pair.first, {record.getContent(), signatures, owner, std::move(next), record.d_ttl});
       }
     }
   }
@@ -507,8 +507,9 @@ bool AggressiveNSECCache::synthesizeFromNSEC3Wildcard(time_t now, const DNSName&
     return false;
   }
 
-  addToRRSet(now, wcSet, wcSignatures, name, doDNSSEC, ret, DNSResourceRecord::ANSWER);
+  addToRRSet(now, wcSet, std::move(wcSignatures), name, doDNSSEC, ret, DNSResourceRecord::ANSWER);
   /* no need for closest encloser proof, the wildcard is there */
+  // coverity[store_truncates_time_t]
   addRecordToRRSet(nextCloser.d_owner, QType::NSEC3, nextCloser.d_ttd - now, nextCloser.d_record, nextCloser.d_signatures, doDNSSEC, ret);
   /* and of course we won't deny the wildcard either */
 
@@ -530,7 +531,8 @@ bool AggressiveNSECCache::synthesizeFromNSECWildcard(time_t now, const DNSName&
     return false;
   }
 
-  addToRRSet(now, wcSet, wcSignatures, name, doDNSSEC, ret, DNSResourceRecord::ANSWER);
+  addToRRSet(now, wcSet, std::move(wcSignatures), name, doDNSSEC, ret, DNSResourceRecord::ANSWER);
+  // coverity[store_truncates_time_t]
   addRecordToRRSet(nsec.d_owner, QType::NSEC, nsec.d_ttd - now, nsec.d_record, nsec.d_signatures, doDNSSEC, ret);
 
   VLOG(log, name << ": Synthesized valid answer from NSECs and wildcard!" << endl);
@@ -756,6 +758,7 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr<LockGuarded
     addRecordToRRSet(nextCloserEntry.d_owner, QType::NSEC3, nextCloserEntry.d_ttd - now, nextCloserEntry.d_record, nextCloserEntry.d_signatures, doDNSSEC, ret);
   }
   if (wcEntry.d_owner != closestNSEC3.d_owner && wcEntry.d_owner != nextCloserEntry.d_owner) {
+    // coverity[store_truncates_time_t]
     addRecordToRRSet(wcEntry.d_owner, QType::NSEC3, wcEntry.d_ttd - now, wcEntry.d_record, wcEntry.d_signatures, doDNSSEC, ret);
   }
 
@@ -884,10 +887,11 @@ bool AggressiveNSECCache::getDenial(time_t now, const DNSName& name, const QType
 
   ret.reserve(ret.size() + soaSet.size() + soaSignatures.size() + /* NSEC */ 1 + entry.d_signatures.size() + (needWildcard ? (/* NSEC */ 1 + wcEntry.d_signatures.size()) : 0));
 
-  addToRRSet(now, soaSet, soaSignatures, zone, doDNSSEC, ret);
+  addToRRSet(now, soaSet, std::move(soaSignatures), zone, doDNSSEC, ret);
   addRecordToRRSet(entry.d_owner, QType::NSEC, entry.d_ttd - now, entry.d_record, entry.d_signatures, doDNSSEC, ret);
 
   if (needWildcard) {
+    // coverity[store_truncates_time_t]
     addRecordToRRSet(wcEntry.d_owner, QType::NSEC, wcEntry.d_ttd - now, wcEntry.d_record, wcEntry.d_signatures, doDNSSEC, ret);
   }
 

pdns/recursordist/filterpo.hh

@@ -143,7 +143,7 @@ public:
         newZoneData = std::make_shared<PolicyZoneData>();
       }
       newZoneData->d_name = name;
-      d_zoneData = newZoneData;
+      d_zoneData = std::move(newZoneData);
     }
 
     const std::unordered_set<std::string>& getTags() const
@@ -426,7 +426,7 @@ public:
     if (newZone) {
       assureZones(zoneIdx);
       newZone->setPriority(zoneIdx);
-      d_zones[zoneIdx] = newZone;
+      d_zones[zoneIdx] = std::move(newZone);
     }
   }
 

pdns/recursordist/lua-recursor4.cc

@@ -293,7 +293,7 @@ void RecursorLua4::postPrepareContext()
       return ret;
     });
 
-  d_lw->registerFunction<const ProxyProtocolValue, std::string()>("getContent", [](const ProxyProtocolValue& value) { return value.content; });
+  d_lw->registerFunction<const ProxyProtocolValue, std::string()>("getContent", [](const ProxyProtocolValue& value) -> std::string { return value.content; });
   d_lw->registerFunction<const ProxyProtocolValue, uint8_t()>("getType", [](const ProxyProtocolValue& value) { return value.type; });
 
   d_lw->registerFunction<void(DNSRecord::*)(const std::string&)>("changeContent", [](DNSRecord& dr, const std::string& newContent) { dr.setContent(DNSRecordContent::make(dr.d_type, QClass::IN, newContent)); });
@@ -637,7 +637,7 @@ unsigned int RecursorLua4::gettag(const ComboAddress& remote, const Netmask& edn
 
     auto ret = d_gettag(remote, ednssubnet, local, qname, qtype, ednsOptions, tcp, proxyProtocolValuesMap);
 
-    if (policyTags) {
+    if (policyTags != nullptr) {
       const auto& tags = std::get<1>(ret);
       if (tags) {
         policyTags->reserve(policyTags->size() + tags->size());
@@ -646,25 +646,25 @@ unsigned int RecursorLua4::gettag(const ComboAddress& remote, const Netmask& edn
         }
       }
     }
-    const auto dataret = std::get<2>(ret);
+    const auto& dataret = std::get<2>(ret);
     if (dataret) {
       data = *dataret;
     }
-    const auto reqIdret = std::get<3>(ret);
+    const auto& reqIdret = std::get<3>(ret);
     if (reqIdret) {
       requestorId = *reqIdret;
     }
-    const auto deviceIdret = std::get<4>(ret);
+    const auto& deviceIdret = std::get<4>(ret);
     if (deviceIdret) {
       deviceId = *deviceIdret;
     }
 
-    const auto deviceNameret = std::get<5>(ret);
+    const auto& deviceNameret = std::get<5>(ret);
     if (deviceNameret) {
       deviceName = *deviceNameret;
     }
 
-    const auto routingTarget = std::get<6>(ret);
+    const auto& routingTarget = std::get<6>(ret);
     if (routingTarget) {
       routingTag = *routingTarget;
     }
@@ -745,7 +745,8 @@ bool RecursorLua4::genhook(const luacall_t& func, DNSQuestion& dq, int& ret) con
       else if (dq.followupFunction == "udpQueryResponse") {
         PacketBuffer p = GenUDPQueryResponse(dq.udpQueryDest, dq.udpQuery);
         dq.udpAnswer = std::string(reinterpret_cast<const char*>(p.data()), p.size());
-        auto cbFunc = d_lw->readVariable<boost::optional<luacall_t>>(dq.udpCallback).get_value_or(0);
+        // coverity[auto_causes_copy] not copying produces a dangling ref
+        const auto cbFunc = d_lw->readVariable<boost::optional<luacall_t>&>(dq.udpCallback).get_value_or(nullptr);
         if (!cbFunc) {
           SLOG(g_log << Logger::Error << "Attempted callback for Lua UDP Query/Response which could not be found" << endl,
                g_slog->withName("lua")->info(Logr::Error, "Attempted callback for Lua UDP Query/Response which could not be found"));

pdns/recursordist/lwres.cc

@@ -59,32 +59,29 @@ thread_local TCPOutConnectionManager t_tcp_manager;
 std::shared_ptr<Logr::Logger> g_slogout;
 bool g_paddingOutgoing;
 
-void remoteLoggerQueueData(RemoteLoggerInterface& r, const std::string& data)
+void remoteLoggerQueueData(RemoteLoggerInterface& rli, const std::string& data)
 {
-  auto ret = r.queueData(data);
+  auto ret = rli.queueData(data);
 
   switch (ret) {
   case RemoteLoggerInterface::Result::Queued:
     break;
   case RemoteLoggerInterface::Result::PipeFull: {
-    const auto msg = RemoteLoggerInterface::toErrorString(ret);
-    const auto name = r.name();
-    SLOG(g_log << Logger::Debug << name << ": " << msg << std::endl,
-         g_slog->withName(name)->info(Logr::Debug, msg));
+    const auto& msg = RemoteLoggerInterface::toErrorString(ret);
+    SLOG(g_log << Logger::Debug << rli.name() << ": " << msg << std::endl,
+         g_slog->withName(rli.name())->info(Logr::Debug, msg));
     break;
   }
   case RemoteLoggerInterface::Result::TooLarge: {
-    const auto msg = RemoteLoggerInterface::toErrorString(ret);
-    const auto name = r.name();
-    SLOG(g_log << Logger::Notice << name << ": " << msg << endl,
-         g_slog->withName(name)->info(Logr::Debug, msg));
+    const auto& msg = RemoteLoggerInterface::toErrorString(ret);
+    SLOG(g_log << Logger::Notice << rli.name() << ": " << msg << endl,
+         g_slog->withName(rli.name())->info(Logr::Debug, msg));
     break;
   }
   case RemoteLoggerInterface::Result::OtherError: {
-    const auto msg = RemoteLoggerInterface::toErrorString(ret);
-    const auto name = r.name();
-    SLOG(g_log << Logger::Warning << name << ": " << msg << std::endl,
-         g_slog->withName(name)->info(Logr::Warning, msg));
+    const auto& msg = RemoteLoggerInterface::toErrorString(ret);
+    SLOG(g_log << Logger::Warning << rli.name() << ": " << msg << std::endl,
+         g_slog->withName(rli.name())->info(Logr::Warning, msg));
     break;
   }
   }

pdns/recursordist/negcache.cc

@@ -65,7 +65,7 @@ bool NegCache::getRootNXTrust(const DNSName& qname, const struct timeval& now, N
   // An 'ENT' QType entry, used as "whole name" in the neg-cache context.
   auto exists = get(lastLabel, QType::ENT, now, found, true, serveStale, refresh);
   if (exists && found.d_auth.isRoot()) {
-    negEntry = found;
+    negEntry = std::move(found);
     return true;
   }
   return false;

pdns/recursordist/nod.cc

@@ -57,9 +57,6 @@ void PersistentSBF::remove_tmp_files(const filesystem::path& p, std::lock_guard<
 // instances iterating and writing to the cache dir at the same time
 bool PersistentSBF::init(bool ignore_pid)
 {
-  if (d_init)
-    return false;
-
   auto log = g_slog->withName("nod");
   std::lock_guard<std::mutex> lock(d_cachedir_mutex);
   if (d_cachedir.length()) {
@@ -111,20 +108,19 @@ bool PersistentSBF::init(bool ignore_pid)
       return false;
     }
   }
-  d_init = true;
   return true;
 }
 
 void PersistentSBF::setCacheDir(const std::string& cachedir)
 {
-  if (!d_init) {
-    filesystem::path p(cachedir);
-    if (!exists(p))
-      throw PDNSException("NODDB setCacheDir specified nonexistent directory: " + cachedir);
-    else if (!is_directory(p))
-      throw PDNSException("NODDB setCacheDir specified a file not a directory: " + cachedir);
-    d_cachedir = cachedir;
+  filesystem::path path(cachedir);
+  if (!exists(path)) {
+    throw PDNSException("NODDB setCacheDir specified nonexistent directory: " + cachedir);
+  }
+  if (!is_directory(path)) {
+    throw PDNSException("NODDB setCacheDir specified a file not a directory: " + cachedir);
   }
+  d_cachedir = cachedir;
 }
 
 // Dump the SBF to a file

pdns/recursordist/nod.hh

@@ -68,7 +68,6 @@ public:
 private:
   void remove_tmp_files(const boost::filesystem::path&, std::lock_guard<std::mutex>&);
 
-  bool d_init{false};
   LockGuarded<bf::stableBF> d_sbf; // Stable Bloom Filter
   std::string d_cachedir;
   std::string d_prefix = sbf_prefix;

pdns/recursordist/pdns_recursor.cc

@@ -2359,9 +2359,9 @@ static string* doProcessUDPQuestion(const std::string& question, const ComboAddr
   if (t_protobufServers.servers || t_outgoingProtobufServers.servers) {
     comboWriter->d_uuid = uniqueId;
   }
-  comboWriter->d_requestorId = requestorId;
-  comboWriter->d_deviceId = deviceId;
-  comboWriter->d_deviceName = deviceName;
+  comboWriter->d_requestorId = std::move(requestorId);
+  comboWriter->d_deviceId = std::move(deviceId);
+  comboWriter->d_deviceName = std::move(deviceName);
   comboWriter->d_kernelTimestamp = tval;
   comboWriter->d_proxyProtocolValues = std::move(proxyProtocolValues);
   comboWriter->d_routingTag = std::move(routingTag);

pdns/recursordist/rec-lua-conf.cc

@@ -134,7 +134,7 @@ static void parseRPZParameters(rpzOptions_t& have, std::shared_ptr<DNSFilterEngi
     }
   }
   if (have.count("tags") != 0) {
-    const auto tagsTable = boost::get<std::vector<std::pair<int, std::string>>>(have["tags"]);
+    const auto& tagsTable = boost::get<std::vector<std::pair<int, std::string>>>(have["tags"]);
     std::unordered_set<std::string> tags;
     for (const auto& tag : tagsTable) {
       tags.insert(tag.second);
@@ -466,7 +466,7 @@ void loadRecursorLuaConfig(const std::string& fname, luaConfigDelayedThreads& de
            log->info(Logr::Info, "Loading RPZ from file"));
       zone->setName(polName);
       loadRPZFromFile(filename, zone, defpol, defpolOverrideLocal, maxTTL);
-      lci.dfe.addZone(zone);
+      lci.dfe.addZone(std::move(zone));
       SLOG(g_log << Logger::Warning << "Done loading RPZ from file '" << filename << "'" << endl,
            log->info(Logr::Info,  "Done loading RPZ from file"));
     }
@@ -560,7 +560,7 @@ void loadRecursorLuaConfig(const std::string& fname, luaConfigDelayedThreads& de
         }
       }
 
-      lci.ztcConfigs[validZoneName] = conf;
+      lci.ztcConfigs[validZoneName] = std::move(conf);
     }
     catch (const std::exception& e) {
       SLOG(g_log << Logger::Error << "Problem configuring zoneToCache for zone '" << zoneName << "': " << e.what() << endl,

pdns/recursordist/rec-main.cc

@@ -590,7 +590,7 @@ void protobufLogResponse(const struct dnsheader* header, LocalStateHolder<LuaCon
   else {
     pbMessage.setSocketFamily(mappedSource.sin4.sin_family);
     Netmask requestorNM(mappedSource, mappedSource.sin4.sin_family == AF_INET ? luaconfsLocal->protobufMaskV4 : luaconfsLocal->protobufMaskV6);
-    auto requestor = requestorNM.getMaskedNetwork();
+    const auto& requestor = requestorNM.getMaskedNetwork();
     pbMessage.setFrom(requestor);
     pbMessage.setFromPort(mappedSource.getPort());
   }
@@ -1348,11 +1348,13 @@ void parseACLs()
   }
 
   g_initialAllowFrom = allowFrom;
+  // coverity[copy_contructor_call] maybe this can be avoided, but be careful as pointers get passed to other threads
   broadcastFunction([=] { return pleaseSupplantAllowFrom(allowFrom); });
 
   auto allowNotifyFrom = parseACL("allow-notify-from-file", "allow-notify-from", log);
 
   g_initialAllowNotifyFrom = allowNotifyFrom;
+  // coverity[copy_contructor_call] maybe this can be avoided, but be careful as pointers get passed to other threads
   broadcastFunction([=] { return pleaseSupplantAllowNotifyFrom(allowNotifyFrom); });
 
   l_initialized = true;

pdns/recursordist/rec-tcp.cc

@@ -362,9 +362,9 @@ static void doProcessTCPQuestion(std::unique_ptr<DNSComboWriter>& comboWriter, s
   const struct dnsheader* dnsheader = headerdata.get();
 
   if (t_protobufServers.servers || t_outgoingProtobufServers.servers) {
-    comboWriter->d_requestorId = requestorId;
-    comboWriter->d_deviceId = deviceId;
-    comboWriter->d_deviceName = deviceName;
+    comboWriter->d_requestorId = std::move(requestorId);
+    comboWriter->d_deviceId = std::move(deviceId);
+    comboWriter->d_deviceName = std::move(deviceName);
     comboWriter->d_uuid = getUniqueID();
   }
 

pdns/recursordist/rec-zonetocache.cc

@@ -152,6 +152,7 @@ pdns::ZoneMD::Result ZoneData::getByAXFR(const RecZoneToCache::Config& config, p
   time_t axfrStart = time(nullptr);
   time_t axfrNow = time(nullptr);
 
+  // coverity[store_truncates_time_t]
   while (axfr.getChunk(nop, &chunk, (axfrStart + axfrTimeout - axfrNow)) != 0) {
     for (auto& dnsRecord : chunk) {
       if (config.d_zonemd != pdns::ZoneMD::Config::Ignore) {

pdns/recursordist/rec_channel.cc

@@ -159,6 +159,7 @@ static void waitForRead(int fd, unsigned int timeout, time_t start)
   if (elapsed >= timeout) {
     throw PDNSException("Timeout waiting for control channel data");
   }
+  // coverity[store_truncates_time_t]
   int ret = waitForData(fd, timeout - elapsed, 0);
   if (ret == 0) {
     throw PDNSException("Timeout waiting for control channel data");
@@ -216,5 +217,5 @@ RecursorControlChannel::Answer RecursorControlChannel::recv(int fd, unsigned int
     str.append(buffer, recvd);
   }
 
-  return {err, str};
+  return {err, std::move(str)};
 }

pdns/recursordist/rec_channel_rec.cc

@@ -152,7 +152,7 @@ std::atomic<unsigned long>* getDynMetric(const std::string& str, const std::stri
     name = getPrometheusName(name);
   }
 
-  auto ret = dynmetrics{new std::atomic<unsigned long>(), name};
+  auto ret = dynmetrics{new std::atomic<unsigned long>(), std::move(name)};
   (*dm)[str] = ret;
   return ret.d_ptr;
 }
@@ -1208,7 +1208,7 @@ static StatsMap toRPZStatsMap(const string& name, const std::unordered_map<std::
       sname = name + "-rpz-" + key;
       pname = pbasename + "{type=\"rpz\",policyname=\"" + key + "\"}";
     }
-    entries.emplace(sname, StatsMapEntry{pname, std::to_string(count)});
+    entries.emplace(sname, StatsMapEntry{std::move(pname), std::to_string(count)});
     total += count;
   }
   entries.emplace(name, StatsMapEntry{pbasename, std::to_string(total)});

pdns/recursordist/rec_control.cc

@@ -222,7 +222,7 @@ static RecursorControlChannel::Answer showYAML(const std::string& path)
     msg += showAllowYAML(mainsettings.incoming.allow_from_file, "incoming", "allow_from_file", pdns::rust::settings::rec::validate_allow_from);
     msg += showAllowYAML(mainsettings.incoming.allow_notify_from_file, "incoming", "allow_notify_from_file", pdns::rust::settings::rec::validate_allow_from);
     msg += showAllowYAML(mainsettings.incoming.allow_notify_for_file, "incoming", "allow_notify_for_file", pdns::rust::settings::rec::validate_allow_for);
-    return {0, msg};
+    return {0, std::move(msg)};
   }
   catch (const rust::Error& err) {
     return {1, std::string(err.what())};
@@ -343,7 +343,7 @@ int main(int argc, char** argv)
     auto timeout = arg().asNum("timeout");
     RecursorControlChannel rccS;
     rccS.connect(arg()["socket-dir"], sockname);
-    rccS.send(rccS.d_fd, {0, command}, timeout, fd);
+    rccS.send(rccS.d_fd, {0, std::move(command)}, timeout, fd);
 
     auto receive = rccS.recv(rccS.d_fd, timeout);
     if (receive.d_ret != 0) {