Coverity Fixes

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

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<std::string, std::string>;
-    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 <typename Func>
+    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<std::string_view> lookup(const std::string &name);
+    std::optional<std::string_view> lookup(const std::string &name) const;
 
   private:
     void _createString(const std::string &name, const std::string_view value);

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<value_type> operator()(swoc::TextView const &src) const noexcept;
+  Rv<value_type> operator()(swoc::TextView const &src) const;
 
 protected:
   bool  _unit_required_p = true; ///< Whether unitless values are allowed.

plugins/experimental/txn_box/plugin/src/Comparison.cc

@@ -833,6 +833,7 @@ Cmp_Rxp::expr_visitor::operator()(std::monostate)
 Rv<Comparison::Handle>
 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));
 }
 

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 {};
 }
 

plugins/regex_remap/regex_remap.cc

@@ -320,7 +320,7 @@ RemapRegex::initialize(const std::string &reg, 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());

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

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");

src/iocore/eventsystem/IOBuffer.cc

@@ -1046,25 +1046,26 @@ auto
 make_buffer_size_parser()
 {
   using L = swoc::Lexicon<int>;
-  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> {
-    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> {
+    int result = lexicon[esize];
     if (result == -1) {
       return std::nullopt;
     }

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<SSLSession> ssl_session(new SSLSession(id, buf, len, buf_exdata));

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

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<char *>(ats_malloc(128));

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")

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;
     }

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;

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);

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

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")

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);
         }
       }

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<char *>(value.c_str());
         callback(&tmp, data);
       }
-    }
+    });
   }
 
   int num_records = g_num_records;

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));
     }
 

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<std::string_view>
-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<std::string_view> result{};
 

src/tsutil/ts_unit_parser.cc

@@ -31,7 +31,7 @@ namespace ts
 {
 
 auto
-UnitParser::operator()(swoc::TextView const &src) const noexcept -> Rv<value_type>
+UnitParser::operator()(swoc::TextView const &src) const -> Rv<value_type>
 {
   value_type zret = 0;
   TextView   text = src; // Keep @a src around to report error offsets.