diff --git a/CMakePresets.json b/CMakePresets.json index 68bbb2d4e4a..fd504ff15f4 100644 --- a/CMakePresets.json +++ b/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", diff --git a/plugins/healthchecks/healthchecks.cc b/plugins/healthchecks/healthchecks.cc index c5da85da9da..ece4319aee2 100644 --- a/plugins/healthchecks/healthchecks.cc +++ b/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(bytes_read); + data->b_len = static_cast(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); diff --git a/src/iocore/hostdb/HostDB.cc b/src/iocore/hostdb/HostDB.cc index 6abda699462..6011d24b6b0 100644 --- a/src/iocore/hostdb/HostDB.cc +++ b/src/iocore/hostdb/HostDB.cc @@ -953,6 +953,10 @@ HostDBContinuation::dnsEvent(int event, HostEnt *e) ts::LocalBuffer q_buf(valid_records); SRV **q = q_buf.data(); ink_assert(valid_records <= static_cast(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]; } diff --git a/src/iocore/net/OCSPStapling.cc b/src/iocore/net/OCSPStapling.cc index c5fd6505eb0..358d7d88ffc 100644 --- a/src/iocore/net/OCSPStapling.cc +++ b/src/iocore/net/OCSPStapling.cc @@ -23,6 +23,7 @@ #include #include +#include #include #include @@ -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(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 rsp_buf(rsp_buf_len); + auto read_len = fread(rsp_buf.data(), 1, rsp_buf.size(), fp); + if (read_len == static_cast(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); } diff --git a/src/iocore/net/OpenSSLQUICNetVConnection.cc b/src/iocore/net/OpenSSLQUICNetVConnection.cc index d63003368d7..23fc1508e5b 100644 --- a/src/iocore/net/OpenSSLQUICNetVConnection.cc +++ b/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 { diff --git a/src/proxy/http/HttpBodyFactory.cc b/src/proxy/http/HttpBodyFactory.cc index e45d669ff09..0871be3dc92 100644 --- a/src/proxy/http/HttpBodyFactory.cc +++ b/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 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::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) { diff --git a/src/proxy/http/HttpProxyServerMain.cc b/src/proxy/http/HttpProxyServerMain.cc index 8272491436e..03a4537d876 100644 --- a/src/proxy/http/HttpProxyServerMain.cc +++ b/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}); diff --git a/src/proxy/http3/Http3Frame.cc b/src/proxy/http3/Http3Frame.cc index 2c15ee03ef3..54cd6355699 100644 --- a/src/proxy/http3/Http3Frame.cc +++ b/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) { ; } diff --git a/src/proxy/http3/Http3Session.cc b/src/proxy/http3/Http3Session.cc index 3b1985a5206..4c899f2e37f 100644 --- a/src/proxy/http3/Http3Session.cc +++ b/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(transaction->link.next); transaction->do_io_close(); delete transaction; + transaction = next; } } diff --git a/src/proxy/http3/QPACK.cc b/src/proxy/http3/QPACK.cc index 45da0384942..38aa3a5630f 100644 --- a/src/proxy/http3/QPACK.cc +++ b/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; } } diff --git a/src/proxy/logging/LogAccess.cc b/src/proxy/logging/LogAccess.cc index 1114af1b052..92f878622e8 100644 --- a/src/proxy/logging/LogAccess.cc +++ b/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"; diff --git a/src/proxy/logging/LogFieldFallback.cc b/src/proxy/logging/LogFieldFallback.cc index 876ac722060..9a98574e122 100644 --- a/src/proxy/logging/LogFieldFallback.cc +++ b/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; }