From 8f67f0c283ad6408cdd8e1f836a51a9b98b436b0 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Mon, 20 Nov 2023 16:06:50 +0100 Subject: [PATCH 1/7] rec: fix time_t truncation warnings from coverity by annotating them. Mostly they are due to DNS using 32-bit timestamps. --- pdns/axfr-retriever.cc | 3 ++- pdns/ixfr.cc | 1 + pdns/recursordist/aggressive_nsec.cc | 4 ++++ pdns/recursordist/rec-zonetocache.cc | 1 + pdns/recursordist/rec_channel.cc | 1 + pdns/recursordist/recpacketcache.cc | 1 + pdns/recursordist/recursor_cache.cc | 2 ++ pdns/recursordist/reczones-helpers.cc | 1 + pdns/recursordist/rpzloader.cc | 1 + pdns/recursordist/syncres.cc | 1 + pdns/unix_utility.cc | 1 + 11 files changed, 16 insertions(+), 1 deletion(-) diff --git a/pdns/axfr-retriever.cc b/pdns/axfr-retriever.cc index ce67f06384b2..47a909618b7e 100644 --- a/pdns/axfr-retriever.cc +++ b/pdns/axfr-retriever.cc @@ -175,7 +175,8 @@ void AXFRRetriever::timeoutReadn(uint16_t bytes, uint16_t timeoutsec) int n=0; int numread; while(n, vector>> 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 diff --git a/pdns/recursordist/aggressive_nsec.cc b/pdns/recursordist/aggressive_nsec.cc index b419f5ef2548..f31d2a7704ef 100644 --- a/pdns/recursordist/aggressive_nsec.cc +++ b/pdns/recursordist/aggressive_nsec.cc @@ -505,6 +505,7 @@ bool AggressiveNSECCache::synthesizeFromNSEC3Wildcard(time_t now, const DNSName& addToRRSet(now, wcSet, 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 */ @@ -527,6 +528,7 @@ bool AggressiveNSECCache::synthesizeFromNSECWildcard(time_t now, const DNSName& } addToRRSet(now, wcSet, 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); @@ -752,6 +754,7 @@ bool AggressiveNSECCache::getNSEC3Denial(time_t now, std::shared_ptr= 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"); diff --git a/pdns/recursordist/recpacketcache.cc b/pdns/recursordist/recpacketcache.cc index 0b54090e058a..c372ee186221 100644 --- a/pdns/recursordist/recpacketcache.cc +++ b/pdns/recursordist/recpacketcache.cc @@ -123,6 +123,7 @@ bool RecursorPacketCache::checkResponseMatches(MapCombo::LockedContent& shard, s } if (now < iter->d_ttd) { // it is right, it is fresh! + // coverity[store_truncates_time_t] *age = static_cast(now - iter->d_creation); // we know ttl is > 0 auto ttl = static_cast(iter->d_ttd - now); diff --git a/pdns/recursordist/recursor_cache.cc b/pdns/recursordist/recursor_cache.cc index 95cfdc1658a2..fd4043401191 100644 --- a/pdns/recursordist/recursor_cache.cc +++ b/pdns/recursordist/recursor_cache.cc @@ -606,6 +606,7 @@ void MemRecursorCache::replace(time_t now, const DNSName& qname, const QType qty prior to calling this function, so the TTL actually holds a TTD. */ cacheEntry.d_ttd = min(maxTTD, static_cast(record.d_ttl)); // XXX this does weird things if TTLs differ in the set + // coverity[store_truncates_time_t] cacheEntry.d_orig_ttl = cacheEntry.d_ttd - ttl_time; // Even though we record the time the ttd was computed, there still seems to be a case where the computed // d_orig_ttl can wrap. @@ -707,6 +708,7 @@ bool MemRecursorCache::doAgeCache(time_t now, const DNSName& name, const QType q return false; // would be dead anyhow } + // coverity[store_truncates_time_t] auto maxTTL = static_cast(cacheEntry.d_ttd - now); if (maxTTL > newTTL) { lockedShard->d_cachecachevalid = false; diff --git a/pdns/recursordist/reczones-helpers.cc b/pdns/recursordist/reczones-helpers.cc index 12770b636d51..2326054f3f6d 100644 --- a/pdns/recursordist/reczones-helpers.cc +++ b/pdns/recursordist/reczones-helpers.cc @@ -133,6 +133,7 @@ void putDefaultHintsIntoCache(time_t now, std::vector& nsvec) arr.d_type = QType::A; aaaarr.d_type = QType::AAAA; nsrr.d_type = QType::NS; + // coverity[store_truncates_time_t] arr.d_ttl = aaaarr.d_ttl = nsrr.d_ttl = now + 3600000; string templ = "a.root-servers.net."; diff --git a/pdns/recursordist/rpzloader.cc b/pdns/recursordist/rpzloader.cc index 9ebae34c2174..62368bb519d4 100644 --- a/pdns/recursordist/rpzloader.cc +++ b/pdns/recursordist/rpzloader.cc @@ -217,6 +217,7 @@ static shared_ptr loadRPZFromServer(Logr::log_t plogger, time_t axfrStart = time(nullptr); time_t axfrNow = time(nullptr); shared_ptr sr; + // coverity[store_truncates_time_t] while (axfr.getChunk(nop, &chunk, (axfrStart + axfrTimeout - axfrNow))) { for (auto& dr : chunk) { if (dr.d_type == QType::NS || dr.d_type == QType::TSIG) { diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 9f2964fffb82..0e9fdc0a2bc6 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -3130,6 +3130,7 @@ static uint32_t getRRSIGTTL(const time_t now, const std::shared_ptrd_sigexpire) { + // coverity[store_truncates_time_t] res = static_cast(rrsig->d_sigexpire) - now; } return res; diff --git a/pdns/unix_utility.cc b/pdns/unix_utility.cc index 60a15d9eecb3..c58bf404221c 100644 --- a/pdns/unix_utility.cc +++ b/pdns/unix_utility.cc @@ -274,6 +274,7 @@ time_t Utility::timegm(struct tm *const t) /* day is now the number of days since 'Jan 1 1970' */ i = 7; + // coverity[store_truncates_time_t] t->tm_wday = (day + 4) % i; /* Sunday=0, Monday=1, ..., Saturday=6 */ i = 24; From 888efc7b11193a0b44698fe832890b0d8fbd68b6 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Mon, 20 Nov 2023 16:08:02 +0100 Subject: [PATCH 2/7] The nod code sets up the SBF DBs before starting the thraed, no need for protection --- pdns/recursordist/nod.cc | 18 +++++++----------- pdns/recursordist/nod.hh | 1 - 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/pdns/recursordist/nod.cc b/pdns/recursordist/nod.cc index e35fc699b002..811811ca6ac8 100644 --- a/pdns/recursordist/nod.cc +++ b/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 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 diff --git a/pdns/recursordist/nod.hh b/pdns/recursordist/nod.hh index ede60d31ddaa..a27b1d77fd0c 100644 --- a/pdns/recursordist/nod.hh +++ b/pdns/recursordist/nod.hh @@ -68,7 +68,6 @@ public: private: void remove_tmp_files(const boost::filesystem::path&, std::lock_guard&); - bool d_init{false}; LockGuarded d_sbf; // Stable Bloom Filter std::string d_cachedir; std::string d_prefix = sbf_prefix; From 17417d541991bb4459f99f5d0dfd5b0ebcd986ff Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Mon, 20 Nov 2023 16:36:11 +0100 Subject: [PATCH 3/7] Another bunch of coverity fixes --- pdns/recursordist/filterpo.hh | 4 ++-- pdns/recursordist/reczones.cc | 4 ++-- pdns/recursordist/secpoll-recursor.cc | 2 +- pdns/recursordist/settings/cxxsupport.cc | 16 ++++++++-------- pdns/recursordist/validate-recursor.cc | 2 +- pdns/recursordist/ws-recursor.cc | 4 ++-- pdns/tcounters.hh | 1 + 7 files changed, 17 insertions(+), 16 deletions(-) diff --git a/pdns/recursordist/filterpo.hh b/pdns/recursordist/filterpo.hh index b5aa306bf737..19bdcb87077b 100644 --- a/pdns/recursordist/filterpo.hh +++ b/pdns/recursordist/filterpo.hh @@ -142,7 +142,7 @@ public: newZoneData = std::make_shared(); } newZoneData->d_name = name; - d_zoneData = newZoneData; + d_zoneData = std::move(newZoneData); } const std::unordered_set& getTags() const @@ -420,7 +420,7 @@ public: if (newZone) { assureZones(zoneIdx); newZone->setPriority(zoneIdx); - d_zones[zoneIdx] = newZone; + d_zones[zoneIdx] = std::move(newZone); } } diff --git a/pdns/recursordist/reczones.cc b/pdns/recursordist/reczones.cc index 8e09f19cbfd9..fa763cd9ad70 100644 --- a/pdns/recursordist/reczones.cc +++ b/pdns/recursordist/reczones.cc @@ -335,7 +335,7 @@ static void processApiZonesFile(shared_ptr& newMap, shared static void processForwardZonesFile(shared_ptr& newMap, shared_ptr& newSet, Logr::log_t log) { - const auto filename = ::arg()["forward-zones-file"]; + const auto& filename = ::arg()["forward-zones-file"]; if (filename.empty()) { return; } @@ -493,7 +493,7 @@ static void processAllowNotifyFor(shared_ptr& newSet) static void processAllowNotifyForFile(shared_ptr& newSet, Logr::log_t log) { - const auto filename = ::arg()["allow-notify-for-file"]; + const auto& filename = ::arg()["allow-notify-for-file"]; if (filename.empty()) { return; } diff --git a/pdns/recursordist/secpoll-recursor.cc b/pdns/recursordist/secpoll-recursor.cc index 9c165cb82179..b54ae7dc5c71 100644 --- a/pdns/recursordist/secpoll-recursor.cc +++ b/pdns/recursordist/secpoll-recursor.cc @@ -89,7 +89,7 @@ void doSecPoll(time_t* last_secpoll, Logr::log_t log) return; } - g_security_message = security_message; + g_security_message = std::move(security_message); auto rlog = vlog->withValues("securitymessage", Logging::Loggable(g_security_message), "status", Logging::Loggable(security_status)); if (g_security_status != 1 && security_status == 1) { diff --git a/pdns/recursordist/settings/cxxsupport.cc b/pdns/recursordist/settings/cxxsupport.cc index 2e58343bf4fd..8a9e331ff52e 100644 --- a/pdns/recursordist/settings/cxxsupport.cc +++ b/pdns/recursordist/settings/cxxsupport.cc @@ -95,7 +95,7 @@ void pdns::settings::rec::oldStyleForwardsFileToBridgeStruct(const std::string& } ::rust::Vec<::rust::String> addresses; stringtok(addresses, instructions, ",; "); - ForwardZone forwardzone{domain, addresses, recurse, allowNotify}; + ForwardZone forwardzone{domain, std::move(addresses), recurse, allowNotify}; vec.push_back(std::move(forwardzone)); } } @@ -320,7 +320,7 @@ pdns::settings::rec::YamlSettingsStatus pdns::settings::rec::readYamlSettings(co mergeYamlSubFile(yamlfile, yamlstruct, false, log); } yamlstruct.validate(); - settings = yamlstruct; + settings = std::move(yamlstruct); return YamlSettingsStatus::OK; } catch (const ::rust::Error& ex) { @@ -348,7 +348,7 @@ void pdns::settings::rec::readYamlAllowFromFile(const std::string& filename, ::r auto data = string(std::istreambuf_iterator(file), std::istreambuf_iterator()); auto yamlvec = pdns::rust::settings::rec::parse_yaml_string_to_allow_from(data); pdns::rust::settings::rec::validate_allow_from(filename, yamlvec); - vec = yamlvec; + vec = std::move(yamlvec); } void pdns::settings::rec::readYamlForwardZonesFile(const std::string& filename, ::rust::Vec& vec, Logr::log_t log) @@ -362,7 +362,7 @@ void pdns::settings::rec::readYamlForwardZonesFile(const std::string& filename, auto data = string(std::istreambuf_iterator(file), std::istreambuf_iterator()); auto yamlvec = pdns::rust::settings::rec::parse_yaml_string_to_forward_zones(data); pdns::rust::settings::rec::validate_forward_zones("forward_zones", yamlvec); - vec = yamlvec; + vec = std::move(yamlvec); } void pdns::settings::rec::readYamlAllowNotifyForFile(const std::string& filename, ::rust::Vec<::rust::String>& vec, Logr::log_t log) @@ -376,7 +376,7 @@ void pdns::settings::rec::readYamlAllowNotifyForFile(const std::string& filename auto data = string(std::istreambuf_iterator(file), std::istreambuf_iterator()); auto yamlvec = pdns::rust::settings::rec::parse_yaml_string_to_allow_notify_for(data); pdns::rust::settings::rec::validate_allow_notify_for("allow-notify-for", yamlvec); - vec = yamlvec; + vec = std::move(yamlvec); } std::string pdns::settings::rec::to_arg(const AuthZone& authzone) @@ -467,7 +467,7 @@ void pdns::settings::rec::to_yaml(::rust::Vec& field, const std::st boost::trim(headers.second); ::rust::Vec<::rust::String> addresses; stringtok(addresses, headers.second, " ;"); - ForwardZone forwardzone{headers.first, addresses, recurse, false}; + ForwardZone forwardzone{headers.first, std::move(addresses), recurse, false}; field.push_back(std::move(forwardzone)); } } @@ -521,7 +521,7 @@ static void processLine(const std::string& arg, FieldMap& map, bool mainFile) pdns::rust::settings::rec::Value rustvalue = {false, 0, 0.0, "", {}, {}, {}}; if (pdns::settings::rec::oldKVToBridgeStruct(var, val, section, fieldname, type_name, rustvalue)) { auto overriding = !mainFile && !incremental && !simpleRustType(type_name); - auto [existing, inserted] = map.emplace(std::pair{std::pair{section, fieldname}, pdns::rust::settings::rec::OldStyle{section, fieldname, var, type_name, rustvalue, overriding}}); + auto [existing, inserted] = map.emplace(std::pair{std::pair{section, fieldname}, pdns::rust::settings::rec::OldStyle{section, fieldname, var, std::move(type_name), rustvalue, overriding}}); if (!inserted) { // Simple values overwrite always existing->second.value.bool_val = rustvalue.bool_val; @@ -622,7 +622,7 @@ std::string pdns::settings::rec::defaultsToYaml() string name = var; string val = arg().getDefault(var); if (pdns::settings::rec::oldKVToBridgeStruct(name, val, section, fieldname, type_name, rustvalue)) { - map.emplace(std::pair{std::pair{section, fieldname}, pdns::rust::settings::rec::OldStyle{section, fieldname, name, type_name, rustvalue, false}}); + map.emplace(std::pair{std::pair{section, fieldname}, pdns::rust::settings::rec::OldStyle{section, fieldname, name, std::move(type_name), std::move(rustvalue), false}}); } } // Convert the map to a vector, as CXX does not have any dictionary like support. diff --git a/pdns/recursordist/validate-recursor.cc b/pdns/recursordist/validate-recursor.cc index 1a676792a992..d7f2b99c909f 100644 --- a/pdns/recursordist/validate-recursor.cc +++ b/pdns/recursordist/validate-recursor.cc @@ -75,7 +75,7 @@ bool updateTrustAnchorsFromFile(const std::string& fname, map& } SLOG(g_log << Logger::Info << "Read changed Trust Anchors from file, updating" << endl, log->info(Logr::Info, "Read changed Trust Anchors from file, updating")); - dsAnchors = newDSAnchors; + dsAnchors = std::move(newDSAnchors); return true; } catch (const std::exception& e) { diff --git a/pdns/recursordist/ws-recursor.cc b/pdns/recursordist/ws-recursor.cc index 64c027da55bf..9baba3c5c69c 100644 --- a/pdns/recursordist/ws-recursor.cc +++ b/pdns/recursordist/ws-recursor.cc @@ -260,7 +260,7 @@ static void doCreateZone(const Json& document) pdns::rust::settings::rec::AuthZone authzone; authzone.zone = zonename; authzone.file = zonefilename; - pdns::rust::settings::rec::api_add_auth_zone(yamlAPiZonesFile, authzone); + pdns::rust::settings::rec::api_add_auth_zone(yamlAPiZonesFile, std::move(authzone)); } else { apiWriteConfigFile(confbasename, "auth-zones+=" + zonename + "=" + zonefilename); @@ -275,7 +275,7 @@ static void doCreateZone(const Json& document) for (const auto& value : document["servers"].array_items()) { forward.forwarders.emplace_back(value.string_value()); } - pdns::rust::settings::rec::api_add_forward_zone(yamlAPiZonesFile, forward); + pdns::rust::settings::rec::api_add_forward_zone(yamlAPiZonesFile, std::move(forward)); } else { string serverlist; diff --git a/pdns/tcounters.hh b/pdns/tcounters.hh index 9689de466996..5225d725ddd5 100644 --- a/pdns/tcounters.hh +++ b/pdns/tcounters.hh @@ -151,6 +151,7 @@ public: } template + // coverity[auto_causes_copy] auto snapAt(Enum index) { return d_snapshot.lock()->at(index); From 4c5a50dce9c57eb5e517313d71969deea16292be Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Mon, 20 Nov 2023 17:06:35 +0100 Subject: [PATCH 4/7] Another set of coverity fixes --- pdns/ixfr.cc | 2 +- pdns/recursordist/aggressive_nsec.cc | 10 +++++----- pdns/recursordist/lwres.cc | 25 +++++++++++-------------- pdns/recursordist/negcache.cc | 2 +- pdns/recursordist/pdns_recursor.cc | 6 +++--- pdns/recursordist/rec-lua-conf.cc | 6 +++--- pdns/recursordist/rec-main.cc | 6 +++--- pdns/recursordist/rec-tcp.cc | 6 +++--- pdns/recursordist/rec_channel.cc | 2 +- pdns/recursordist/rec_channel_rec.cc | 4 ++-- pdns/recursordist/rec_control.cc | 4 ++-- 11 files changed, 35 insertions(+), 38 deletions(-) diff --git a/pdns/ixfr.cc b/pdns/ixfr.cc index f2a6bcf7c4ef..ac041cb00905 100644 --- a/pdns/ixfr.cc +++ b/pdns/ixfr.cc @@ -272,7 +272,7 @@ vector, vector>> 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(r.first); diff --git a/pdns/recursordist/aggressive_nsec.cc b/pdns/recursordist/aggressive_nsec.cc index f31d2a7704ef..a91f64457519 100644 --- a/pdns/recursordist/aggressive_nsec.cc +++ b/pdns/recursordist/aggressive_nsec.cc @@ -345,7 +345,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 { @@ -354,7 +354,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}); } } } @@ -503,7 +503,7 @@ 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); @@ -527,7 +527,7 @@ 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); @@ -883,7 +883,7 @@ 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) { diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index d02e51eb54e3..010eaf480087 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -59,32 +59,29 @@ thread_local TCPOutConnectionManager t_tcp_manager; std::shared_ptr 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; } } diff --git a/pdns/recursordist/negcache.cc b/pdns/recursordist/negcache.cc index 9b0b347180fe..5bea1a06b934 100644 --- a/pdns/recursordist/negcache.cc +++ b/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; diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc index 9435f555e53f..6409d6a0bf5b 100644 --- a/pdns/recursordist/pdns_recursor.cc +++ b/pdns/recursordist/pdns_recursor.cc @@ -2351,9 +2351,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); diff --git a/pdns/recursordist/rec-lua-conf.cc b/pdns/recursordist/rec-lua-conf.cc index 83c48cd432c3..6b45c54a81c0 100644 --- a/pdns/recursordist/rec-lua-conf.cc +++ b/pdns/recursordist/rec-lua-conf.cc @@ -134,7 +134,7 @@ static void parseRPZParameters(rpzOptions_t& have, std::shared_ptr>>(have["tags"]); + const auto& tagsTable = boost::get>>(have["tags"]); std::unordered_set tags; for (const auto& tag : tagsTable) { tags.insert(tag.second); @@ -463,7 +463,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")); } @@ -557,7 +557,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, diff --git a/pdns/recursordist/rec-main.cc b/pdns/recursordist/rec-main.cc index 9cc1cde07ab4..478839ea41d5 100644 --- a/pdns/recursordist/rec-main.cc +++ b/pdns/recursordist/rec-main.cc @@ -590,7 +590,7 @@ void protobufLogResponse(const struct dnsheader* header, LocalStateHolderprotobufMaskV4 : luaconfsLocal->protobufMaskV6); - auto requestor = requestorNM.getMaskedNetwork(); + const auto& requestor = requestorNM.getMaskedNetwork(); pbMessage.setFrom(requestor); pbMessage.setFromPort(mappedSource.getPort()); } @@ -1348,12 +1348,12 @@ void parseACLs() } g_initialAllowFrom = allowFrom; - broadcastFunction([=] { return pleaseSupplantAllowFrom(allowFrom); }); + broadcastFunction([=] { return pleaseSupplantAllowFrom(std::move(allowFrom)); }); auto allowNotifyFrom = parseACL("allow-notify-from-file", "allow-notify-from", log); g_initialAllowNotifyFrom = allowNotifyFrom; - broadcastFunction([=] { return pleaseSupplantAllowNotifyFrom(allowNotifyFrom); }); + broadcastFunction([=] { return pleaseSupplantAllowNotifyFrom(std::move(allowNotifyFrom)); }); l_initialized = true; } diff --git a/pdns/recursordist/rec-tcp.cc b/pdns/recursordist/rec-tcp.cc index 7d1a8bb3d103..0eac47653b53 100644 --- a/pdns/recursordist/rec-tcp.cc +++ b/pdns/recursordist/rec-tcp.cc @@ -362,9 +362,9 @@ static void doProcessTCPQuestion(std::unique_ptr& 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(); } diff --git a/pdns/recursordist/rec_channel.cc b/pdns/recursordist/rec_channel.cc index 3d78cd96c14a..6abab97a35fc 100644 --- a/pdns/recursordist/rec_channel.cc +++ b/pdns/recursordist/rec_channel.cc @@ -217,5 +217,5 @@ RecursorControlChannel::Answer RecursorControlChannel::recv(int fd, unsigned int str.append(buffer, recvd); } - return {err, str}; + return {err, std::move(str)}; } diff --git a/pdns/recursordist/rec_channel_rec.cc b/pdns/recursordist/rec_channel_rec.cc index 186befa61c06..b096e2d9ce8b 100644 --- a/pdns/recursordist/rec_channel_rec.cc +++ b/pdns/recursordist/rec_channel_rec.cc @@ -152,7 +152,7 @@ std::atomic* getDynMetric(const std::string& str, const std::stri name = getPrometheusName(name); } - auto ret = dynmetrics{new std::atomic(), name}; + auto ret = dynmetrics{new std::atomic(), std::move(name)}; (*dm)[str] = ret; return ret.d_ptr; } @@ -1208,7 +1208,7 @@ static StatsMap toRPZStatsMap(const string& name, const std::unordered_map Date: Mon, 20 Nov 2023 17:15:53 +0100 Subject: [PATCH 5/7] Another set of coverity fixes, these are a bit more tricky --- pdns/recursordist/lua-recursor4.cc | 17 +++++++++-------- pdns/recursordist/rpzloader.cc | 8 ++++---- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/pdns/recursordist/lua-recursor4.cc b/pdns/recursordist/lua-recursor4.cc index 985f9bf043c4..fc08701f3d44 100644 --- a/pdns/recursordist/lua-recursor4.cc +++ b/pdns/recursordist/lua-recursor4.cc @@ -293,7 +293,7 @@ void RecursorLua4::postPrepareContext() return ret; }); - d_lw->registerFunction("getContent", [](const ProxyProtocolValue& value) { return value.content; }); + d_lw->registerFunction("getContent", [](const ProxyProtocolValue& value) -> std::string { return value.content; }); d_lw->registerFunction("getType", [](const ProxyProtocolValue& value) { return value.type; }); d_lw->registerFunction("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(p.data()), p.size()); - auto cbFunc = d_lw->readVariable>(dq.udpCallback).get_value_or(0); + // coverity[auto_causes_copy] not copying produces a dangling ref + const auto cbFunc = d_lw->readVariable&>(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")); diff --git a/pdns/recursordist/rpzloader.cc b/pdns/recursordist/rpzloader.cc index 62368bb519d4..549902c0586d 100644 --- a/pdns/recursordist/rpzloader.cc +++ b/pdns/recursordist/rpzloader.cc @@ -307,7 +307,7 @@ std::shared_ptr loadRPZFromFile(const std::string& fname sr = getRR(dr); domain = dr.d_name; zone->setDomain(domain); - soaRecord = dr; + soaRecord = std::move(dr); } else if (dr.d_type == QType::NS) { continue; @@ -324,7 +324,7 @@ std::shared_ptr loadRPZFromFile(const std::string& fname if (sr != nullptr) { zone->setRefresh(sr->d_st.refresh); - zone->setSOA(soaRecord); + zone->setSOA(std::move(soaRecord)); setRPZZoneNewState(zone->getName(), sr->d_st.serial, zone->size(), true, false); } return sr; @@ -568,7 +568,7 @@ void RPZIXFRTracker(const std::vector& primaries, const boost::opt auto tempSR = getRR(rr); // g_log<d_st.serial<& primaries, const boost::opt /* only update sr now that all changes have been converted */ if (currentSR) { - sr = currentSR; + sr = std::move(currentSR); } SLOG(g_log << Logger::Info << "Had " << totremove << " RPZ removal" << addS(totremove) << ", " << totadd << " addition" << addS(totadd) << " for " << zoneName << " New serial: " << sr->d_st.serial << endl, logger->info(Logr::Info, "RPZ mutations", "removals", Logging::Loggable(totremove), "additions", Logging::Loggable(totadd), "newserial", Logging::Loggable(sr->d_st.serial))); From 86d6338ec9fec3d0876fe2380033b095fc63a026 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 24 Nov 2023 11:20:19 +0100 Subject: [PATCH 6/7] Sometimes, coverity and clang-tidy do not agree --- pdns/axfr-retriever.cc | 2 +- pdns/recursordist/rec-main.cc | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/pdns/axfr-retriever.cc b/pdns/axfr-retriever.cc index 47a909618b7e..f2630c9aebf4 100644 --- a/pdns/axfr-retriever.cc +++ b/pdns/axfr-retriever.cc @@ -176,7 +176,7 @@ void AXFRRetriever::timeoutReadn(uint16_t bytes, uint16_t timeoutsec) int numread; while(n(timeoutsec - (time(nullptr) - start))); if(res<0) throw ResolverException("Reading data from remote nameserver over TCP: "+stringerror()); if(!res) diff --git a/pdns/recursordist/rec-main.cc b/pdns/recursordist/rec-main.cc index 478839ea41d5..fb88100af225 100644 --- a/pdns/recursordist/rec-main.cc +++ b/pdns/recursordist/rec-main.cc @@ -1348,12 +1348,14 @@ void parseACLs() } g_initialAllowFrom = allowFrom; - broadcastFunction([=] { return pleaseSupplantAllowFrom(std::move(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; - broadcastFunction([=] { return pleaseSupplantAllowNotifyFrom(std::move(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; } From f4cd754cdd930d234114d18f9d668fc512c5232c Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Wed, 6 Dec 2023 11:15:53 +0100 Subject: [PATCH 7/7] Remove redundant coverity annotation --- pdns/axfr-retriever.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/pdns/axfr-retriever.cc b/pdns/axfr-retriever.cc index f2630c9aebf4..09461f07a7de 100644 --- a/pdns/axfr-retriever.cc +++ b/pdns/axfr-retriever.cc @@ -175,7 +175,6 @@ void AXFRRetriever::timeoutReadn(uint16_t bytes, uint16_t timeoutsec) int n=0; int numread; while(n(timeoutsec - (time(nullptr) - start))); if(res<0) throw ResolverException("Reading data from remote nameserver over TCP: "+stringerror());