9.2.x: latest clang-analyzer fixes

[bneradt]

The Ubuntu 26.04 clang-analyzer job exposes unchecked I/O, a rule leak,
and configuration reads under a mutex in the older 9.2.x code.

This branch backports the applicable fixes from #13593, #12226, #10273,
and #11386. The remaining changes handle file and socket failures,
keep session reuse configuration I/O outside the reader lock, and make
the nonblocking eventfd operation explicit to the analyzer.

The TLS client protocol test also isolates its OpenSSL configuration and
uses RSA key exchange for TLS 1.0 so system policy does not prevent its
SNI protocol checks from running.

The healthchecks backport retains the first 16 KiB of a status file,
rather than its tail, and fixes the empty response for an exactly
16 KiB file. A read error now yields an empty body with the existing OK
header and no read-error diagnostic, matching master, instead of
repeating the failed read indefinitely.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[bryancall]

❌ Request changes

One blocking item, a one character fix. The rest of the PR is careful work and I have verified a good part of it by building and running it.

Verified: the new ts_file test does fail without the fix

I built 9.2.x at the base commit with only test_ts_file.cc applied, then again with ts_file.cc added:

base + test only:  6 failures in [ts_file_copy]
                   EISDIR:               ec.value() 0, expected 21
                   /dev/full size 1:     ec.value() 0, expected 28
                   /dev/full size 65536: ec.value() 0, expected 28
base + test + fix: all 41 assertions pass

The nine pre-existing ts_file cases pass both ways, so return !ec breaks nothing that was already covered. ts::file::copy has exactly one caller outside the tests, proxy/http/remap/PluginDso.cc:78, and it does check the result, so this turns a truncated plugin copy that silently returned true into a reported strerror. That is a real fix rather than only quieting the analyzer.

Worth knowing about the test's shape: 35 of its 41 assertions pass against the unfixed code. The old loop was byte correct and wrong only in its reporting, so the six size sweep is a forward looking regression guard rather than proof of the fix, and all of the discriminating power sits inside the #ifdef __linux__ block. Splitting /dev/full into 1 byte and 65536 bytes is a nice touch, since it exercises the fclose flush path and the write through path separately.

Blocking: the auth key bound is one byte too loose

plugins/experimental/ssl_session_reuse/src/ssl_init.cc:118

if (read_len > 0 && read_len <= buffSize && static_cast<size_t>(read_len) <= key_data.length()) {
  memset(retKeyBuff, 0, buffSize);
  strncpy(retKeyBuff, key_data.c_str(), read_len);

read_len == buffSize passes. memset zeroes all 256 bytes, then strncpy with n == 256 fills all 256 and writes no terminator, because the source is at least that long. Both callers then convert the buffer to a std::string:

• publish.cc:84, redis_passwd = redis_auth_key;
• subscriber.cc:73, the same

redis_auth_key is char[MAX_REDIS_KEYSIZE] (256, redis_auth.h:26) on the stack, so strlen reads past the end of it until it meets a stray zero byte, which gives a garbage suffixed password and an ASan stack-buffer-overflow report.

To reach it: a redis_auth_key_file whose contents, after the trailing newlines are stripped, are exactly 256 bytes.

read_len < buffSize fixes it. The third clause is already implied, since read() filled a buffer of that size, so the bound is the only load bearing part.

This is still much better than what it replaces, where strncpy had no bound at all and a failed read() turned -1 into a huge length. It just needs the one character.

Non-blocking: a failed config load now retries on every lookup

plugins/experimental/ssl_session_reuse/src/config.cc:170-184

The old loadConfigOnChange() only retried inside if (configHasChanged()), which the m_lastCheck bucket throttles to once per cCheckDivisor seconds. The new flow consults checkConfigChange() only to clear m_alreadyLoaded, then falls through to loadConfig() whenever that flag is false. After a failed readConfig() it stays false permanently, so a missing or unreadable file means an open() and an fstat() per getValue() call with no throttle.

The impact today is bounded, and I checked rather than assumed: every getValue caller is a constructor (ssl_init.cc:54-57, publish.cc:67-74, subscriber.cc:60-63), and configHasChanged() has no callers anywhere in the plugin. So this is not a live problem, but it is a behavior regression sitting in a function whose next caller may well be on a request path.

Non-blocking: the new locking comment describes the wrong case

plugins/experimental/ssl_session_reuse/src/config.cc, the added line:

// Readers can continue using the previous configuration during file I/O.

That is true for a reload, and the swap under the lock makes it so. But m_loading makes a concurrent load a no-op rather than a wait: loadConfig returns m_alreadyLoaded, which is false during the very first load. A reader arriving then sees a load failure and an empty map, not the previous configuration, because there is no previous configuration yet. The three constructors run serially on plugin init so this is not reachable, but a locking comment that promises a guarantee the first load path does not provide is the kind of thing that gets trusted later.

Suggest saying what m_loading actually buys: the file is parsed into a local map with the lock released, then swapped in under the lock, so readers see either the old map or the new one and never a partial one, and a reader during the first load sees an empty one.

Agreed: the deleted LOCKING comments were right to go

proxy/http/HttpBodyFactory.cc:717 and :783

I read both helpers. Neither load_sets_from_directory nor load_body_set_from_directory touches a factory member; they work on locals and return a new table. Since the diff deliberately moves that call outside the lock, leaving "must be called with lock taken" in place would have been the hazard. Removing exactly the comments the restructure invalidated is the right instinct.

What is now missing is a note on the contract that replaced them. reconfigure() stages the config values in locals, builds the table unlocked, then publishes the values and the table together under one lock hold, and none of that is stated. One line above the re-lock would stop the next reader from re-adding a lock assumption.

I also checked the interleaving question, since dropping the lock invites it: two reconfigure() calls racing across the unlocked window is not reachable, because the only runtime caller is config_callback, driven by a single continuation on ET_TASK.

Non-blocking: the /dev/full case hard fails where /dev/full is absent

src/tscore/unit_tests/test_ts_file.cc:320

Docker's default device set includes /dev/full, so the common case is fine. Where it is missing, CHECK_FALSE(copy(...)) still passes, because the fopen fails and returns false, while CHECK(ec.value() == ENOSPC) fails with the wrong errno. That reads as a ts_file regression rather than a missing device. A guard on access("/dev/full", W_OK) would make it legible.

Two things I suspected and checked, both clean: ec does not carry state between the sub-cases, since copy, load and remove each reset it on entry; and mkdtemp(directory.data()) is legal, because C++17 data() is writable and mkdtemp only rewrites the trailing XXXXXX in place.

Non-blocking: two healthchecks behavior changes worth a line in the description

plugins/healthchecks/healthchecks.c:112-116

Both of these are faithful to master, which I diffed against rather than reasoned about, so there is nothing to change here. They are just invisible in the description:

• Retention moves from the tail of the file to the first 16KB, which is operator visible for any status file over MAX_BODY_LEN. It also fixes a real bug, where a file of exactly 16384 bytes made the second fread return 0 and serve an empty body.
• A read error sets b_len = 0 while data->exists stays 1, so the plugin serves the ok header with Content-Length: 0 and nothing in diags.log. That is inherited from master, and it replaces a loop that spun forever on the same error, so it is an improvement. If a node whose health file cannot be read should fail its check, that is an upstream conversation rather than a 9.2.x one.

Nit: dead return after Fatal

mgmt/ProcessManager.cc:347

Fatal exits the process, so the added return; is unreachable. Harmless, but it is now the only one of the five Fatal sites in initLMConnection with a return after it, which reads as though Fatal were recoverable there and not elsewhere. Marking Fatal [[noreturn]] would settle it for the analyzer everywhere at once.

Verified clean

I traced these and found nothing to report. Noting them so nobody has to re-derive them:

• InkAPI.cc:7773, HttpTransact.cc:7821, tsmemcache.cc:241. All three look like a safe default being replaced by null or by nothing at all, and all three have default: arms that assign before any use. Dead store removals.
• HttpProxyServerMain.cc:211. probe stays null for QUIC ports, but TRANSPORT_QUIC is only parseable under TS_USE_QUIC == 1 (lib/records/RecHttp.cc:410-413), so isQUIC() is always false in a non-QUIC build and the acceptor._accept = probe fallback is unreachable for QUIC. The change also removes a genuine leak of the probe and its endpoints on QUIC ports.
• header_rewrite.cc:182 and :232. add_rule takes ownership only on its true return, and release() is called only then. The error paths now free through the destructor, including the "hook condition not first" path that previously leaked outright.
• ink_queue.cc:148. ink_assert is a no-op in a release build, but __clang_analyzer__ is in its guard, so the analyzer does get the constraint, and no ink_freelist_init caller can pass type_size == 0.
• Cache.cc:2878 and healthchecks.c:312. Both look like gratuitous modernization on a release branch and both fix real leaks, the first because delete[] sat mid-loop with return -1 paths after it, the second because the old !feof loop allocated an finfo on its final pass and neither freed nor linked it.
• UnixNet.cc:230. eventfd_read is identical to the 8 byte read it replaces, inside the same HAVE_EVENTFD guard.

Reproducing the red and green runs

git worktree add --detach ~/ats-13692 f671e525110be50452f607fc1f2657f3dba7fe3b
cd ~/ats-13692 && autoreconf -if && ./configure
gh pr diff 13692 | git apply --include='*test_ts_file.cc'
printf '#define CATCH_CONFIG_MAIN\n#include "../../../tests/include/catch.hpp"\n' > src/tscore/unit_tests/catch_main.cc
g++ -std=gnu++17 -g -Iinclude -Ilib -Ilib/yamlcpp/include -Itests/include \
  src/tscore/unit_tests/catch_main.cc src/tscore/unit_tests/test_ts_file.cc \
  src/tscore/ts_file.cc src/tscpp/util/TextView.cc src/tscore/ink_memory.cc \
  -o /tmp/ts_file && /tmp/ts_file '[ts_file_copy]'

ink_abort needs a stub, since linking ink_error.cc pulls in Diags and the BufferWriter formatting chain. 9.2.x's cmake does not generate ink_config.h, which is why this goes through configure.

[bneradt]

@bryancall Fixed the blocking key bound in 9a6db9a: read_len < buffSize reserves the terminator. A focused ASan test reproduces the stack-buffer-overflow with the previous code and passes with the fix, including 255-, 256-, and 257-byte keys with and without trailing newlines.

I also addressed the non-blocking items:

• Restored timestamp-gated config reloads and kept missing-file checks within the five-second bucket. A focused test confirms that 100 lookups after a failed read do not reopen the file, readers retain the previous map, and a later detected change reloads successfully.
• Clarified that readers see a complete map, initially empty, and documented the body factory's atomic publication of settings and templates.
• Guarded /dev/full with access(..., W_OK) and an explicit warning when unavailable.
• Documented both healthchecks behavior changes in the PR description.

I kept the local return after Fatal. On this branch Fatal is a macro routed through LogMessage::message, which also handles nonfatal severities, so that shared function cannot be marked [[noreturn]]. A dedicated fatal API would be a broader change; the local return keeps the analyzer from following the invalid-descriptor path in this backport.

The asfats5 rebuild/install, formatting, all 41 file-copy assertions, config concurrency/reload checks, and all four tls_client_versions subtests passed. The latest commit was amended and pushed with a lease.

[bryancall]

All seven items addressed in 9a6db9a. I re-checked each rather than taking the
summary at its word:

• The blocking bound is right. read_len < buffSize with the preceding memset
  leaves at least one zero byte, so the std::string conversions in publish.cc:84
  and subscriber.cc:73 terminate inside the buffer.

• The retry fix is better targeted than what I asked for. Dropping
  0 == m_lastmtime from checkConfigChange removes exactly the unthrottled
  path: a missing file makes stat fail, m_lastmtime stays 0, and every getValue
  re-checked. It is now bucketed to cCheckDivisor, and the first load still
  fires because m_lastCheck starts at 0.

• The locking comment now says what m_loading actually buys, including the
  empty map on the first load.

• The /dev/full guard and the HttpBodyFactory contract line both do what I
  asked.

I accept the Fatal reasoning. On 9.2.x it routes through LogMessage::message,
which serves non-fatal severities too, so [[noreturn]] there is wrong and a
dedicated fatal API is out of scope for a backport.

One nit, not worth another round: test_ts_file.cc uses access() and W_OK
without including <unistd.h>. It compiles only because ts_file.h pulls in
ink_memory.h, which includes it under HAVE_UNISTD_H. An explicit include would
be more honest about the dependency.

Approving. Worth holding the merge until Clang-Analyzer reports, since that job
is the reason this branch exists.