diff --git a/include/proxy/http/HttpTransact.h b/include/proxy/http/HttpTransact.h index 64e13a991de..2fa4cc8a477 100644 --- a/include/proxy/http/HttpTransact.h +++ b/include/proxy/http/HttpTransact.h @@ -852,6 +852,7 @@ class HttpTransact // memset((void *)&host_db_info, 0, sizeof(host_db_info)); } + // coverity[exn_spec_violation] - destroy() only frees memory and does ref counting ~State() { destroy(); } void diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 4a847f397a1..630f079b4d4 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -536,11 +536,15 @@ class Metrics }; // class Counter + /** + * Static string metrics storage. + * + * All methods are thread-safe. + */ class StaticString { public: using StringStorage = std::unordered_map; - using iterator = StringStorage::iterator; static void createString(const std::string &name, const std::string_view value) @@ -551,18 +555,21 @@ class Metrics static StaticString &instance(); - iterator - begin() + /** + * Thread-safe iteration over all string metrics. + * The callback is invoked for each metric while holding the mutex. + */ + template + void + for_each(Func &&func) const { - return _strings.begin(); + std::lock_guard lock(_mutex); + for (const auto &[name, value] : _strings) { + func(name, value); + } } - iterator - end() - { - return _strings.end(); - }; - std::optional lookup(const std::string &name); + std::optional lookup(const std::string &name) const; private: void _createString(const std::string &name, const std::string_view value); diff --git a/include/tsutil/ts_unit_parser.h b/include/tsutil/ts_unit_parser.h index 29c15610c4d..16d6dfa90f6 100644 --- a/include/tsutil/ts_unit_parser.h +++ b/include/tsutil/ts_unit_parser.h @@ -80,7 +80,7 @@ class UnitParser * @param src Input string. * @return The computed value if the input is valid, or an error report. */ - Rv operator()(swoc::TextView const &src) const noexcept; + Rv operator()(swoc::TextView const &src) const; protected: bool _unit_required_p = true; ///< Whether unitless values are allowed. diff --git a/plugins/experimental/txn_box/plugin/src/Comparison.cc b/plugins/experimental/txn_box/plugin/src/Comparison.cc index aae89917b8d..85013dd98a8 100644 --- a/plugins/experimental/txn_box/plugin/src/Comparison.cc +++ b/plugins/experimental/txn_box/plugin/src/Comparison.cc @@ -833,6 +833,7 @@ Cmp_Rxp::expr_visitor::operator()(std::monostate) Rv Cmp_Rxp::expr_visitor::operator()(Expr::Direct &d) { + // coverity[use_after_move] - intentional: visitor consumes the variant alternative return Handle(new Cmp_RxpSingle(Expr{std::move(d)}, _rxp_opt)); } diff --git a/plugins/experimental/txn_box/plugin/src/ts_util.cc b/plugins/experimental/txn_box/plugin/src/ts_util.cc index 81da18c32fc..3df9c750ed3 100644 --- a/plugins/experimental/txn_box/plugin/src/ts_util.cc +++ b/plugins/experimental/txn_box/plugin/src/ts_util.cc @@ -805,7 +805,9 @@ ts::HttpTxn::outbound_remote_addr() const Errata ts::HttpTxn::cache_key_assign(TextView const &key) { - TSCacheUrlSet(_txn, key.data(), key.size()); + if (TS_ERROR == TSCacheUrlSet(_txn, key.data(), key.size())) { + return Errata(S_ERROR, R"(Failed to set cache key to "{}".)", key); + } return {}; } diff --git a/plugins/regex_remap/regex_remap.cc b/plugins/regex_remap/regex_remap.cc index 18838291e29..6eca1e8c33a 100644 --- a/plugins/regex_remap/regex_remap.cc +++ b/plugins/regex_remap/regex_remap.cc @@ -320,7 +320,7 @@ RemapRegex::initialize(const std::string ®, const std::string &sub, const std _lowercase_substitutions = true; } else if (opt.compare(start, 8, "strategy") == 0) { _has_strategy = true; - _strategy = opt_val; + _strategy = std::move(opt_val); } else if (opt_val.size() <= 0) { // All other options have a required value TSError("[%s] Malformed options: %s", PLUGIN_NAME, opt.c_str()); diff --git a/src/iocore/aio/test_AIO.cc b/src/iocore/aio/test_AIO.cc index 8a08c8445d9..2900c87f030 100644 --- a/src/iocore/aio/test_AIO.cc +++ b/src/iocore/aio/test_AIO.cc @@ -459,6 +459,7 @@ class IOUringLoopTailHandler : public EThread::LoopTailHandler int main(int argc, char *argv[]) { + // coverity[fun_call_w_exception] - called functions may throw but this is a test program int i; // Read the configuration file diff --git a/src/iocore/cache/Stripe.cc b/src/iocore/cache/Stripe.cc index ebd98d06429..0fed76adfb9 100644 --- a/src/iocore/cache/Stripe.cc +++ b/src/iocore/cache/Stripe.cc @@ -173,6 +173,7 @@ Stripe::_init_directory(std::size_t directory_size, int header_size, int footer_ Stripe::~Stripe() { if (this->directory.raw_dir != nullptr) { + // coverity[fun_call_w_exception] - ink_assert aborts (doesn't throw), Dbg is exception-safe // Debug logging to track cleanup - helps correlate with crash location Dbg(dbg_ctl_cache_free, "Stripe %s: freeing raw_dir=%p size=%zu huge=%s", hash_text.get() ? hash_text.get() : "(null)", this->directory.raw_dir, this->directory.raw_dir_size, this->directory.raw_dir_huge ? "true" : "false"); diff --git a/src/iocore/eventsystem/IOBuffer.cc b/src/iocore/eventsystem/IOBuffer.cc index 228353502e6..246d98b5d42 100644 --- a/src/iocore/eventsystem/IOBuffer.cc +++ b/src/iocore/eventsystem/IOBuffer.cc @@ -1046,25 +1046,26 @@ auto make_buffer_size_parser() { using L = swoc::Lexicon; - return [l = L{ - L::with_multi{{0, {"128"}}, - {1, {"256"}}, - {2, {"512"}}, - {3, {"1k", "1024"}}, - {4, {"2k", "2048"}}, - {5, {"4k", "4096"}}, - {6, {"8k", "8192"}}, - {7, {"16k"}}, - {8, {"32k"}}, - {9, {"64k"}}, - {10, {"128k"}}, - {11, {"256k"}}, - {12, {"512k"}}, - {13, {"1M", "1024k"}}, - {14, {"2M", "2048k"}}}, - -1 - }](swoc::TextView esize) -> std::optional { - int result = l[esize]; + static const L lexicon{ + L::with_multi{{0, {"128"}}, + {1, {"256"}}, + {2, {"512"}}, + {3, {"1k", "1024"}}, + {4, {"2k", "2048"}}, + {5, {"4k", "4096"}}, + {6, {"8k", "8192"}}, + {7, {"16k"}}, + {8, {"32k"}}, + {9, {"64k"}}, + {10, {"128k"}}, + {11, {"256k"}}, + {12, {"512k"}}, + {13, {"1M", "1024k"}}, + {14, {"2M", "2048k"}}}, + -1 + }; + return [](swoc::TextView esize) -> std::optional { + int result = lexicon[esize]; if (result == -1) { return std::nullopt; } diff --git a/src/iocore/net/SSLSessionCache.cc b/src/iocore/net/SSLSessionCache.cc index 7e848b41b43..9dcccd535ef 100644 --- a/src/iocore/net/SSLSessionCache.cc +++ b/src/iocore/net/SSLSessionCache.cc @@ -179,7 +179,8 @@ SSLSessionBucket::insertSession(const SSLSessionID &id, SSL_SESSION *sess, SSL * } else { std::string_view group_name = SSLGetGroupName(ssl); ink_release_assert(group_name.size() < sizeof(exdata->group_name)); - strcpy(exdata->group_name, group_name.data()); + memcpy(exdata->group_name, group_name.data(), group_name.size()); + exdata->group_name[group_name.size()] = '\0'; } std::unique_ptr ssl_session(new SSLSession(id, buf, len, buf_exdata)); diff --git a/src/iocore/net/unit_tests/test_ProxyProtocol.cc b/src/iocore/net/unit_tests/test_ProxyProtocol.cc index 56e3aed064d..70ead688be0 100644 --- a/src/iocore/net/unit_tests/test_ProxyProtocol.cc +++ b/src/iocore/net/unit_tests/test_ProxyProtocol.cc @@ -773,6 +773,7 @@ TEST_CASE("ProxyProtocol Rule of 5", "[ProxyProtocol]") std::string_view copy_tlv_data("\x02\x00\x04copy", 7); copy.set_additional_data(copy_tlv_data); + // coverity[copy_instead_of_move] - intentionally testing copy assignment copy = original; // After assignment, copy should have original's data diff --git a/src/proxy/Plugin.cc b/src/proxy/Plugin.cc index dfcaa1450e8..5e2d1e6bb70 100644 --- a/src/proxy/Plugin.cc +++ b/src/proxy/Plugin.cc @@ -254,7 +254,7 @@ plugin_expand(char *arg) } case RECD_COUNTER: { auto count_val{RecGetRecordCounter(arg)}; - if (count_val) { + if (!count_val) { goto not_found; } str = static_cast(ats_malloc(128)); diff --git a/src/proxy/hdrs/unit_tests/test_HeaderValidator.cc b/src/proxy/hdrs/unit_tests/test_HeaderValidator.cc index 71db0c0a57b..c11bc9d8d7a 100644 --- a/src/proxy/hdrs/unit_tests/test_HeaderValidator.cc +++ b/src/proxy/hdrs/unit_tests/test_HeaderValidator.cc @@ -56,6 +56,8 @@ TEST_CASE("testIsHeaderValid", "[proxy][hdrtest]") { HTTPHdr hdr; // extra to prevent proxy allocation. + // coverity[resource_leak] - heap is freed via hdr.destroy() which calls + // HdrHeapSDKHandle::destroy() -> m_heap->destroy() HdrHeap *heap = new_HdrHeap(HdrHeap::DEFAULT_SIZE + 64); SECTION("Test (valid) request with 4 required pseudo headers") diff --git a/src/proxy/http/HttpConfig.cc b/src/proxy/http/HttpConfig.cc index 9276abdaefa..3b7352b1fc6 100644 --- a/src/proxy/http/HttpConfig.cc +++ b/src/proxy/http/HttpConfig.cc @@ -721,7 +721,8 @@ ParsedConfigCache::lookup_impl(TSOverridableConfigKey key, std::string_view valu // Fast path: check cache under read lock. { ts::bravo::shared_lock lock(_mutex); - auto it = _cache.find(cache_key); + // coverity[missing_lock] - ts::bravo::shared_lock properly holds the mutex + auto it = _cache.find(cache_key); if (it != _cache.end()) { return it->second; } diff --git a/src/proxy/http/HttpSM.cc b/src/proxy/http/HttpSM.cc index 0a9ed2c9947..b592e3d227c 100644 --- a/src/proxy/http/HttpSM.cc +++ b/src/proxy/http/HttpSM.cc @@ -280,9 +280,17 @@ HttpSM::~HttpSM() { http_parser_clear(&http_parser); + // coverity[exn_spec_violation] - release() only does ref counting and delete on POD types HttpConfig::release(t_state.http_config_param); - m_remap->release(); + // m_remap->release() can allocate (new_Deleter), so catch potential bad_alloc + try { + m_remap->release(); + } catch (...) { + Error("Exception in ~HttpSM during m_remap->release"); + } + + // coverity[exn_spec_violation] - cancel_pending_action() only sets boolean flags cache_sm.cancel_pending_action(); mutex.clear(); @@ -293,6 +301,7 @@ HttpSM::~HttpSM() debug_on = false; if (_prewarm_sm) { + // coverity[exn_spec_violation] - destroy() only frees memory and resets shared_ptrs _prewarm_sm->destroy(); THREAD_FREE(_prewarm_sm, preWarmSMAllocator, this_ethread()); _prewarm_sm = nullptr; diff --git a/src/proxy/http/remap/RemapConfig.cc b/src/proxy/http/remap/RemapConfig.cc index a1ceac4e0ee..64d29d37c63 100644 --- a/src/proxy/http/remap/RemapConfig.cc +++ b/src/proxy/http/remap/RemapConfig.cc @@ -365,7 +365,7 @@ parse_include_directive(const char *directive, BUILD_TABLE_INFO *bti, char *errb path = RecConfigReadConfigPath(nullptr, bti->paramv[i]); if (ink_file_is_directory(path)) { - struct dirent **entrylist; + struct dirent **entrylist = nullptr; int n_entries; n_entries = scandir(path, &entrylist, nullptr, alphasort); diff --git a/src/proxy/http/remap/unit-tests/test_NextHopRoundRobin.cc b/src/proxy/http/remap/unit-tests/test_NextHopRoundRobin.cc index 2fc18755015..908dbc35e5b 100644 --- a/src/proxy/http/remap/unit-tests/test_NextHopRoundRobin.cc +++ b/src/proxy/http/remap/unit-tests/test_NextHopRoundRobin.cc @@ -287,6 +287,7 @@ SCENARIO("Testing NextHopRoundRobin class, using policy 'rr-ip'", "[NextHopRound build_request(10017, &sm, &sa2, "rabbit.net", nullptr); result->reset(); strategy->findNextHop(txnp); + REQUIRE(result->hostname != nullptr); CHECK(strcmp(result->hostname, "p3.foo.com") == 0); // call and test parentExists(), this call should not affect @@ -297,6 +298,7 @@ SCENARIO("Testing NextHopRoundRobin class, using policy 'rr-ip'", "[NextHopRound build_request(10018, &sm, &sa2, "rabbit.net", nullptr); result->reset(); strategy->findNextHop(txnp); + REQUIRE(result->hostname != nullptr); CHECK(strcmp(result->hostname, "p3.foo.com") == 0); // call and test parentExists(), this call should not affect diff --git a/src/proxy/http/remap/unit-tests/test_RemapPlugin.cc b/src/proxy/http/remap/unit-tests/test_RemapPlugin.cc index c81790bdfc7..d2aaf3d3bc7 100644 --- a/src/proxy/http/remap/unit-tests/test_RemapPlugin.cc +++ b/src/proxy/http/remap/unit-tests/test_RemapPlugin.cc @@ -395,6 +395,7 @@ SCENARIO("unloading the plugin", "[plugin][core]") { debugObject->clear(); + // coverity[wrapper_escape] - plugin pointer temporarily escapes to thread context but is reset before return plugin->doneInstance(INSTANCE_HANDLER); THEN("expect it to run and receive the right instance handler") diff --git a/src/proxy/http2/unit_tests/test_HpackIndexingTable.cc b/src/proxy/http2/unit_tests/test_HpackIndexingTable.cc index cba2ff5587c..ad373211fb8 100644 --- a/src/proxy/http2/unit_tests/test_HpackIndexingTable.cc +++ b/src/proxy/http2/unit_tests/test_HpackIndexingTable.cc @@ -208,6 +208,7 @@ TEST_CASE("HPACK low level APIs", "[hpack]") REQUIRE(len > 0); REQUIRE(len == literal_test_case[i].encoded_field_len); + // coverity[overrun-buffer-arg] - len is validated positive above REQUIRE(memcmp(buf, literal_test_case[i].encoded_field, len) == 0); } } diff --git a/src/records/RecCore.cc b/src/records/RecCore.cc index 6008ee4fec8..cf816550c4a 100644 --- a/src/records/RecCore.cc +++ b/src/records/RecCore.cc @@ -595,7 +595,7 @@ RecLookupMatchingRecords(unsigned rec_type, const char *match, void (*callback)( } } // Finally check string metrics - for (auto &&[name, value] : ts::Metrics::StaticString::instance()) { + ts::Metrics::StaticString::instance().for_each([&](const std::string &name, const std::string &value) { if (regex.exec(name)) { RecRecord tmp; @@ -608,7 +608,7 @@ RecLookupMatchingRecords(unsigned rec_type, const char *match, void (*callback)( tmp.data.rec_string = const_cast(value.c_str()); callback(&tmp, data); } - } + }); } int num_records = g_num_records; diff --git a/src/traffic_crashlog/traffic_crashlog.cc b/src/traffic_crashlog/traffic_crashlog.cc index 9354c5f838c..382c6f1bfb2 100644 --- a/src/traffic_crashlog/traffic_crashlog.cc +++ b/src/traffic_crashlog/traffic_crashlog.cc @@ -179,6 +179,7 @@ main(int /* argc ATS_UNUSED */, const char **argv) if (syslog_mode) { int facility = -1; if (auto name{RecGetRecordStringAlloc("proxy.config.syslog_facility")}; name) { + // coverity[fun_call_w_exception] - guarded by if(name) check above facility = facility_string_to_int(ats_as_c_str(name)); } diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index b19c61cf1b4..ae96c1dbbac 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -303,15 +303,14 @@ Metrics::StaticString::instance() void Metrics::StaticString::_createString(const std::string &name, const std::string_view value) { - std::lock_guard l(_mutex); - + std::lock_guard lock(_mutex); _strings[name] = value; } std::optional -Metrics::StaticString::lookup(const std::string &name) +Metrics::StaticString::lookup(const std::string &name) const { - std::lock_guard l(_mutex); + std::lock_guard lock(_mutex); auto it = _strings.find(name); std::optional result{}; diff --git a/src/tsutil/ts_unit_parser.cc b/src/tsutil/ts_unit_parser.cc index a5065345837..b4a620cab56 100644 --- a/src/tsutil/ts_unit_parser.cc +++ b/src/tsutil/ts_unit_parser.cc @@ -31,7 +31,7 @@ namespace ts { auto -UnitParser::operator()(swoc::TextView const &src) const noexcept -> Rv +UnitParser::operator()(swoc::TextView const &src) const -> Rv { value_type zret = 0; TextView text = src; // Keep @a src around to report error offsets.