Coverity: clear the 74 defects in unit and regression test code

[bryancall]

Coverity Scan build master-20260914 reports a group of defects in unit-test, regression-test and example code — 74 of them, with no shipped behavior at risk. This issue covers clearing that group.

Scope

74 findings across 29 files under unit_tests/, unit-tests/, plus InkAPITest.cc, example/, and the older regression-test sources (CacheTest.cc, RegressionSM.cc, NetVCTest.cc, test_AIO.cc).

By type: 20 uncaught exception, 16 argument-cannot-be-negative, 10 out-of-bounds access, 6 resource leak, 3 dereference-before-null-check, 2 use-after-free, and a tail of singles.

Two systemic causes worth recording

1. Catch2 macro bodies. Coverity names TEST_CASE/SECTION bodies dummyFunctionN (0-based, stepping by 2 per TEST_CASE; SECTIONs are not separately numbered). More importantly, REQUIRE(expr) does not constrain values for the analyzer: it expands to handleExpr(...) followed by complete(), and complete() is not [[noreturn]] — it returns normally when the assertion passes. So REQUIRE(n > 0) never teaches Coverity anything about n, which is the source of most of the "argument cannot be negative" and "out-of-bounds access" reports here.

2. The throwing _ink_assert override. src/iocore/eventsystem/unit_tests/test_MIOBufferWriter.cc redefines _ink_assert to throw InkAssertExcept() (it already carries a // coverity[UNCAUGHT_EXCEPT:FALSE] noting the confusion). Coverity merges symbol definitions across the whole analysis, so every ink_assert() in the tree resolves to the throwing one. That is why destructors reaching an assert — directly, or via THREAD_FREE → thread_freeup(), or free_MIOBuffer — are reported as uncaught-exception escapes. The aborting _ink_assert from unit_tests/stub.cc is what actually links into those binaries.

Note for anyone fixing this family: adding noexcept to a destructor is a no-op. A user-provided destructor with no exception-specification is already implicitly noexcept(true) in C++20 even when its body calls a throwing function, so an explicit noexcept changes nothing and cannot clear the finding.

Defects that turned out to be real

• test_Huffmancode.cc decoder_roundtrip_fuzz passed huffman_encode's int64_t return straight into huffman_decode's uint32_t src_len. An error return would be read as a ~4 GB source length, and the existing REQUIRE never bounded it above the encode buffer.
• InkAPITest.cc SDK_API_TSUrlParse and SDK_API_TSMimeHdrParse both destroy the MBuffer inside an error branch and then keep using it, ending in a second TSMBufferDestroy (use-after-free plus double-free). Open since 2022.
• test_RemapPlugin.cc held a PluginDebugObject * pointing into a dlopen'd image, declared one scope outside the unique_ptr whose destructor dlcloses it.
• test_HPACK.cc prepare() built paths with an unbounded strcat into a PATH_MAX + 1 buffer, safe only because of a 511-byte cap enforced in ink_args.cc.
• RegressionSM.cc: leaf state machines were never deleted. ReRegressionSM::run() completes synchronously via done() and so never reaches regression_sm_waiting(), which is where composites self-delete. 12 leaf instances leak per run, 6 of them clones created internally by the repeat path. Composites are fine — deleting the tree from the test body would be a double-free.

Recommended for triage rather than a code change

These eight are false positives or intentional; they need marking in the Coverity UI, not a patch:

CID	Location	Why
1550403	query_remap.cc hash_fnv32	The uint32 wraparound is the FNV algorithm
1528571, 1528692	thread_pool/thread.cc	Every access to the queue fields is already inside TSMutexLock/TSMutexUnlock; there is no model for those in ci/coverity-model.cpp, so lock semantics cannot be inferred
1021840, 1021841	CacheTest.cc RegressionTest_cache	Nothing leaks — prototypes are stack objects, clones self-delete via complete(), composites self-delete
1644306	test_ProxyProtocol.cc	The post-move reads are the test deliberately asserting that a moved-from ProxyProtocol is a valid empty object
1660643	test_Huffmancode.cc decoder_parity_exhaustive_short	The only array is uint8_t src[2], written at indices 0 and 1, no computed index
1644208	test_PluginFactory.cc setupConfigPathTest	TOCTOU against a file the test just created in its own per-run temp sandbox

Adjacent bugs found, not in scope here

• example/plugins/c-api/thread_pool/thread.cc: thread_loop polls remove_from_queue outside cond_mutex and enters pthread_cond_wait with no predicate re-check, so a job enqueued in that window is missed until the next signal. Separately, cond_mutex is never pthread_mutex_inited.
• include/iocore/net/ProxyProtocol.h: move-assignment has no self-assignment guard while copy-assignment does, so pp = std::move(pp) would silently clear the data.
• test_RemapPlugin.cc:295: static char ARGC = sizeof ARGV yields 8, not the intended argument count of 3. The test is self-consistent, so changing it would alter what it asserts.

Verification note

RegressionSM and RegressionTest_cache are REGRESSION_TEST macros compiled into inkcache; no ctest case exercises them. Any change there needs a traffic_server -R run, not just the unit-test suites.

[bryancall]

Prior art, recorded after searching for duplicates — none of these is open or overlapping, noting them so the history is on the record:

• TS-1475 "Static analysis: clean up all clang and coverity reports" (2012, Closed/Fixed 2014-11-05) is the Jira-era umbrella for the same kind of sweep. Closed long before any of the CIDs in this issue were detected.
• TS-2867 "traffic_shell uses predictable file names in public writable directories" (2014, Closed/Fixed) references CID 1022101, which today is reported against InkAPITest.cc RegressionTest_SDK_API_TSfopen. The traffic_shell code it was filed about no longer exists; only the CID number carried forward.

No open GitHub issue in this repo covers any of the 74 CIDs (138 CID-titled issues checked, zero overlap), and no issue in the private security fork or its Security tracker references a CID or this test-code work.

[bryancall]

Pull requests for this work, split so each one asks a single reviewable question:

PR	Scope	CIDs
#13689	Real defects in test code — use-after-free, unbounded lengths, error handling	15
#13690	Delete leaf RegressionSM instances when they complete	3
#13688	Uncaught-exception reports (the throwing _ink_assert override)	20
#13687	Bound guarded lengths in Catch2 test bodies (REQUIRE does not constrain for Coverity)	18
#13686	Trivial cleanups — copies, dead null checks	10

66 of the 74 CIDs in this issue. The remaining eight are the triage recommendations listed above and need marking in the Coverity UI rather than a patch.

Suggested review order is #13686 and #13687 first (mechanical), then #13688 (one rationale), then #13689 (the one that deserves real scrutiny). #13690 is independent and needs a traffic_server -R run, so it does not block the others.