Fix Clang 21 analyzer findings

CMakePresets.json

@@ -267,6 +267,8 @@
       "description": "CI Pipeline config for running clang-analyzer",
       "inherits": ["ci"],
       "cacheVariables": {
+        "CMAKE_C_COMPILER": "clang",
+        "CMAKE_CXX_COMPILER": "clang++",
         "CMAKE_EXPORT_COMPILE_COMMANDS": "ON",
         "ENABLE_CCACHE": "OFF",
         "ENABLE_EXAMPLE": "OFF",

plugins/healthchecks/healthchecks.cc

@@ -140,9 +140,9 @@ load_status_file(HCFileInfo *info)
 
   if (nullptr != (fd = fopen(info->fname, "r"))) {
     data->exists = 1;
-    size_t bytes_read;
-    while ((bytes_read = fread(data->body, 1, MAX_BODY_LEN, fd)) > 0) {
-      data->b_len = static_cast<int>(bytes_read);
+    data->b_len  = static_cast<int>(fread(data->body, 1, MAX_BODY_LEN, fd));
+    if (ferror(fd)) {
+      data->b_len = 0;
     }
     fclose(fd);
   }
@@ -333,68 +333,66 @@ parse_configs(const char *fname)
     return nullptr;
   }
 
-  while (!feof(fd)) {
+  while (fgets(buf, sizeof(buf) - 1, fd) != nullptr) {
     char *str, *save;
     char *ok = nullptr, *miss = nullptr, *mime = nullptr;
 
-    if (fgets(buf, sizeof(buf) - 1, fd)) {
-      finfo = new HCFileInfo();
-
-      str       = strtok_r(buf, SEPARATORS, &save);
-      int state = 0;
-      while (nullptr != str) {
-        if (strlen(str) > 0) {
-          switch (state) {
-          case 0:
-            if ('/' == *str) {
-              ++str;
-            }
-            strncpy(finfo->path, str, PATH_NAME_MAX - 1);
-            finfo->path[PATH_NAME_MAX - 1] = '\0';
-            finfo->p_len                   = strlen(finfo->path);
-            break;
-          case 1:
-            strncpy(finfo->fname, str, MAX_PATH_LEN - 1);
-            finfo->fname[MAX_PATH_LEN - 1] = '\0';
-            finfo->basename                = strrchr(finfo->fname, '/');
-            if (finfo->basename) {
-              ++(finfo->basename);
-              finfo->basename_len = strlen(finfo->basename);
-            }
-            break;
-          case 2:
-            mime = str;
-            break;
-          case 3:
-            ok = str;
-            break;
-          case 4:
-            miss = str;
-            break;
+    finfo = new HCFileInfo();
+
+    str       = strtok_r(buf, SEPARATORS, &save);
+    int state = 0;
+    while (nullptr != str) {
+      if (strlen(str) > 0) {
+        switch (state) {
+        case 0:
+          if ('/' == *str) {
+            ++str;
+          }
+          strncpy(finfo->path, str, PATH_NAME_MAX - 1);
+          finfo->path[PATH_NAME_MAX - 1] = '\0';
+          finfo->p_len                   = strlen(finfo->path);
+          break;
+        case 1:
+          strncpy(finfo->fname, str, MAX_PATH_LEN - 1);
+          finfo->fname[MAX_PATH_LEN - 1] = '\0';
+          finfo->basename                = strrchr(finfo->fname, '/');
+          if (finfo->basename) {
+            ++(finfo->basename);
+            finfo->basename_len = strlen(finfo->basename);
           }
-          ++state;
+          break;
+        case 2:
+          mime = str;
+          break;
+        case 3:
+          ok = str;
+          break;
+        case 4:
+          miss = str;
+          break;
         }
-        str = strtok_r(nullptr, SEPARATORS, &save);
+        ++state;
       }
+      str = strtok_r(nullptr, SEPARATORS, &save);
+    }
 
-      /* Fill in the info if everything was ok */
-      if (state > 4) {
-        Dbg(dbg_ctl, "Parsed: %s %s %s %s %s", finfo->path, finfo->fname, mime, ok, miss);
-        finfo->ok   = gen_header(ok, mime, &finfo->o_len);
-        finfo->miss = gen_header(miss, mime, &finfo->m_len);
-        finfo->set_data(load_status_file(finfo));
-
-        /* Add it the linked list */
-        Dbg(dbg_ctl, "Adding path=%s to linked list", finfo->path);
-        if (nullptr == head_finfo) {
-          head_finfo = finfo;
-        } else {
-          prev_finfo->_next = finfo;
-        }
-        prev_finfo = finfo;
+    /* Fill in the info if everything was ok */
+    if (state > 4) {
+      Dbg(dbg_ctl, "Parsed: %s %s %s %s %s", finfo->path, finfo->fname, mime, ok, miss);
+      finfo->ok   = gen_header(ok, mime, &finfo->o_len);
+      finfo->miss = gen_header(miss, mime, &finfo->m_len);
+      finfo->set_data(load_status_file(finfo));
+
+      /* Add it the linked list */
+      Dbg(dbg_ctl, "Adding path=%s to linked list", finfo->path);
+      if (nullptr == head_finfo) {
+        head_finfo = finfo;
       } else {
-        delete finfo;
+        prev_finfo->_next = finfo;
       }
+      prev_finfo = finfo;
+    } else {
+      delete finfo;
     }
   }
   fclose(fd);

src/iocore/hostdb/HostDB.cc

@@ -953,6 +953,10 @@ HostDBContinuation::dnsEvent(int event, HostEnt *e)
         ts::LocalBuffer<SRV *, 16> q_buf(valid_records);
         SRV                      **q = q_buf.data();
         ink_assert(valid_records <= static_cast<int>(hostdb_round_robin_max_count));
+        // The loop below assigns every element, but ts::LocalBuffer hands back raw storage and the
+        // static analyzer cannot follow the loop well enough to see that. Pre-fill so the sort below
+        // is never reported as reading an uninitialized pointer.
+        std::fill_n(q, valid_records, nullptr);
         for (int i = 0; i < valid_records; ++i) {
           q[i] = &e->srv_hosts.hosts[i];
         }

src/iocore/net/OCSPStapling.cc

@@ -23,6 +23,7 @@
 
 #include <memory>
 #include <mutex>
+#include <vector>
 
 #include <openssl/ssl.h>
 #include <openssl/x509v3.h>
@@ -899,19 +900,22 @@ ssl_stapling_init_cert(SSL_CTX *ctx, X509 *cert, const char *certname, const cha
     Dbg(dbg_ctl_ssl_ocsp, "using OCSP prefetched response file %s", rsp_file);
     FILE *fp = fopen(rsp_file, "r");
     if (fp) {
-      fseek(fp, 0, SEEK_END);
-      long rsp_buf_len = ftell(fp);
-      if (rsp_buf_len >= 0) {
-        rewind(fp);
-        unsigned char *rsp_buf  = static_cast<unsigned char *>(malloc(rsp_buf_len));
-        auto           read_len = fread(rsp_buf, 1, rsp_buf_len, fp);
+      long rsp_buf_len = -1;
+
+      if (fseek(fp, 0, SEEK_END) == 0) {
+        rsp_buf_len = ftell(fp);
+      }
+
+      if (rsp_buf_len > 0 && fseek(fp, 0, SEEK_SET) == 0) {
+        std::vector<unsigned char> rsp_buf(rsp_buf_len);
+        auto                       read_len = fread(rsp_buf.data(), 1, rsp_buf.size(), fp);
+
         if (read_len == static_cast<size_t>(rsp_buf_len)) {
-          const unsigned char *p = rsp_buf;
+          const unsigned char *p = rsp_buf.data();
           rsp                    = d2i_TS_OCSP_RESPONSE(nullptr, &p, rsp_buf_len);
         } else {
           Error("stapling_refresh_response: failed to read prefetched response file: %s", rsp_file);
         }
-        free(rsp_buf);
       } else {
         Error("stapling_refresh_response: failed to check the size of prefetched response file: %s", rsp_file);
       }

src/iocore/net/OpenSSLQUICNetVConnection.cc

@@ -389,7 +389,10 @@ QUICNetVConnection::acceptEvent(int event, Event *e)
 
   MUTEX_TRY_LOCK(lock, h->mutex, t);
   if (!lock.is_locked()) {
-    if (event == EVENT_NONE) {
+    // The event system always hands over a non-null Event, so @a e is only null if this is called
+    // directly, which pairs with EVENT_NONE. Reschedule on the thread in that case rather than
+    // dereferencing @a e.
+    if (event == EVENT_NONE || e == nullptr) {
       t->schedule_in(this, HRTIME_MSECONDS(net_retry_delay));
       return EVENT_DONE;
     } else {

src/proxy/http/HttpBodyFactory.cc

@@ -267,6 +267,7 @@ HttpBodyFactory::reconfigure()
     unlock();
     return;
   } // callbacks not setup right
+  unlock();
 
   ////////////////////////////////////////////
   // extract relevant records.yaml values //
@@ -278,14 +279,14 @@ HttpBodyFactory::reconfigure()
 
   // enable_customizations if records.yaml set
   auto e{RecGetRecordInt("proxy.config.body_factory.enable_customizations")};
-  enable_customizations = (e.has_value() ? e.value() : 0);
-  all_found             = all_found && e.has_value();
-  Dbg(dbg_ctl_body_factory, "enable_customizations = %d (found = %d)", enable_customizations, e.has_value());
+  int  new_enable_customizations = (e.has_value() ? e.value() : 0);
+  all_found                      = all_found && e.has_value();
+  Dbg(dbg_ctl_body_factory, "enable_customizations = %d (found = %d)", new_enable_customizations, e.has_value());
 
-  e              = RecGetRecordInt("proxy.config.body_factory.enable_logging");
-  enable_logging = (e.has_value() ? (e.value() ? true : false) : false);
-  all_found      = all_found && e.has_value();
-  Dbg(dbg_ctl_body_factory, "enable_logging = %d (found = %d)", enable_logging, e.has_value());
+  e                       = RecGetRecordInt("proxy.config.body_factory.enable_logging");
+  bool new_enable_logging = (e.has_value() ? (e.value() ? true : false) : false);
+  all_found               = all_found && e.has_value();
+  Dbg(dbg_ctl_body_factory, "enable_logging = %d (found = %d)", new_enable_logging, e.has_value());
 
   ats_scoped_str directory_of_template_sets;
 
@@ -311,21 +312,16 @@ HttpBodyFactory::reconfigure()
     Warning("config changed, but can't fetch all proxy.config.body_factory values");
   }
 
-  /////////////////////////////////////////////
-  // clear out previous template hash tables //
-  /////////////////////////////////////////////
-
-  nuke_template_tables();
-
-  /////////////////////////////////////////////////////////////
-  // at this point, the body hash table is gone, so we start //
-  // building a new one, by scanning the template directory. //
-  /////////////////////////////////////////////////////////////
-
+  std::unique_ptr<BodySetTable> new_table_of_sets;
   if (directory_of_template_sets) {
-    table_of_sets = load_sets_from_directory(directory_of_template_sets);
+    new_table_of_sets = load_sets_from_directory(directory_of_template_sets);
   }
 
+  lock();
+  enable_customizations = new_enable_customizations;
+  enable_logging        = new_enable_logging;
+  nuke_template_tables();
+  table_of_sets = std::move(new_table_of_sets);
   unlock();
 }
 
@@ -736,7 +732,6 @@ HttpBodyFactory::nuke_template_tables()
   }
 }
 
-// LOCKING: must be called with lock taken
 std::unique_ptr<HttpBodyFactory::BodySetTable>
 HttpBodyFactory::load_sets_from_directory(char *set_dir)
 {
@@ -806,7 +801,6 @@ HttpBodyFactory::load_sets_from_directory(char *set_dir)
   return new_table_of_sets;
 }
 
-// LOCKING: must be called with lock taken
 HttpBodySet *
 HttpBodyFactory::load_body_set_from_directory(char *set_name, char *tmpl_dir)
 {

src/proxy/http/HttpProxyServerMain.cc

@@ -194,19 +194,30 @@ MakeHttpProxyAcceptor(HttpProxyAcceptor &acceptor, HttpProxyPort &port, unsigned
 
   // XXX the protocol probe should be a configuration option.
 
-  ProtocolProbeSessionAccept *probe = new ProtocolProbeSessionAccept();
+  // A QUIC port is dispatched to the QUIC acceptor below, which has no probe fallback, so building a
+  // probe for one only leaks it. Every other port type ends up behind the probe, either directly or
+  // as the SSL acceptor's fallback. Without QUIC compiled in no port can be a QUIC port, so this is
+  // always true there.
+  bool const needs_probe = !port.isQUIC();
+
+  ProtocolProbeSessionAccept *probe = nullptr;
   HttpSessionAccept          *http  = nullptr; // don't allocate this unless it will be used.
-  probe->proxyPort                  = &port;
-  probe->proxy_protocol_ipmap       = &HttpConfig::m_master.config_proxy_protocol_ip_addrs;
 
-  if (port.m_session_protocol_preference.intersects(HTTP_PROTOCOL_SET)) {
-    http = new HttpSessionAccept(accept_opt, &port);
-    probe->registerEndpoint(ProtocolProbeSessionAccept::ProtoGroupKey::HTTP, http);
-  }
+  if (needs_probe) {
+    probe                       = new ProtocolProbeSessionAccept();
+    probe->proxyPort            = &port;
+    probe->proxy_protocol_ipmap = &HttpConfig::m_master.config_proxy_protocol_ip_addrs;
+
+    if (port.m_session_protocol_preference.intersects(HTTP_PROTOCOL_SET)) {
+      http = new HttpSessionAccept(accept_opt, &port);
+      probe->registerEndpoint(ProtocolProbeSessionAccept::ProtoGroupKey::HTTP, http);
+    }
 
-  if (port.m_session_protocol_preference.intersects(HTTP2_PROTOCOL_SET)) {
-    probe->registerEndpoint(ProtocolProbeSessionAccept::ProtoGroupKey::HTTP2, new Http2SessionAccept(accept_opt, &port));
+    if (port.m_session_protocol_preference.intersects(HTTP2_PROTOCOL_SET)) {
+      probe->registerEndpoint(ProtocolProbeSessionAccept::ProtoGroupKey::HTTP2, new Http2SessionAccept(accept_opt, &port));
+    }
   }
+
   ProtocolSessionCreateMap.insert({TS_ALPN_PROTOCOL_INDEX_HTTP_1_0, create_h1_server_session});
   ProtocolSessionCreateMap.insert({TS_ALPN_PROTOCOL_INDEX_HTTP_1_1, create_h1_server_session});
   ProtocolSessionCreateMap.insert({TS_ALPN_PROTOCOL_INDEX_HTTP_2_0, create_h2_server_session});

src/proxy/http3/Http3Frame.cc

@@ -599,8 +599,7 @@ Http3FrameFactory::create_headers_frame(IOBufferReader *header_block_reader, siz
 {
   ats_unique_buf buf = ats_unique_malloc(header_block_len);
 
-  int64_t nread;
-  while ((nread = header_block_reader->read(buf.get(), header_block_len)) > 0) {
+  while (header_block_reader->read(buf.get(), header_block_len) > 0) {
     ;
   }
 

src/proxy/http3/Http3Session.cc

@@ -65,11 +65,12 @@ HQSession::remove_transaction(HQTransaction *trans)
 void
 HQSession::_close_transactions()
 {
-  while (this->_transaction_list.head != nullptr) {
-    auto *transaction = this->_transaction_list.head;
+  for (auto *transaction = this->_transaction_list.head; transaction != nullptr;) {
+    auto *next = static_cast<HQTransaction *>(transaction->link.next);
 
     transaction->do_io_close();
     delete transaction;
+    transaction = next;
   }
 }
 

src/proxy/http3/QPACK.cc

@@ -291,11 +291,13 @@ QPACK::decode(uint64_t stream_id, const uint8_t *header_block, size_t header_blo
 
   if (largest_reference != 0 && (this->_dynamic_table.is_empty() || this->_dynamic_table.largest_index() < largest_reference)) {
     // Blocked
-    if (this->_add_to_blocked_list(
-          new DecodeRequest(largest_reference, thread, cont, stream_id, header_block, header_block_len, hdr))) {
+    auto *decode_request = new DecodeRequest(largest_reference, thread, cont, stream_id, header_block, header_block_len, hdr);
+
+    if (this->_add_to_blocked_list(decode_request)) {
       return 1;
     } else {
       // Number of blocked streams exceed the limit
+      delete decode_request;
       return -2;
     }
   }

src/proxy/logging/LogAccess.cc

@@ -1625,7 +1625,7 @@ LogAccess::marshal_version_string(char *buf)
 int
 LogAccess::marshal_proxy_protocol_version(char *buf)
 {
-  const char *version_str = "-";
+  const char *version_str;
   switch (m_data->get_pp_version()) {
   case 1:
     version_str = "V1";

src/proxy/logging/LogFieldFallback.cc

@@ -83,13 +83,13 @@ constexpr bool
 test_find_field_fallback_separator()
 {
   static_assert(find_field_fallback_separator("") == nullptr);
-  constexpr char const *text1 = "{field}??default";
+  [[maybe_unused]] constexpr char text1[] = "{field}??default";
   static_assert(find_field_fallback_separator(text1) == text1 + 7);
-  constexpr char const *text2 = "??default";
+  [[maybe_unused]] constexpr char text2[] = "??default";
   static_assert(find_field_fallback_separator(text2) == text2);
-  constexpr char const *text3 = "{field}??def??ault";
+  [[maybe_unused]] constexpr char text3[] = "{field}??def??ault";
   static_assert(find_field_fallback_separator(text3) == text3 + 7);
-  constexpr char const *text4 = "{field}??\"def??ault\"";
+  [[maybe_unused]] constexpr char text4[] = "{field}??\"def??ault\"";
   static_assert(find_field_fallback_separator(text4) == text4 + 7);
   return true;
 }