bryancall opened a new issue, #13682: URL: https://github.com/apache/trafficserver/issues/13682
Coverity Scan build `master-20260914` reports 409 actionable defects on master. 74 of them are in unit-test, regression-test and example code — 18% of the backlog, 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`; `SECTION`s 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 `dlclose`s 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_init`ed. - `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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
