You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by "bryancall (via GitHub)" <gi...@apache.org> on 2026/09/15 04:24:42 UTC
[I] Coverity: clear the 74 defects in unit and regression test code [trafficserver]
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: issues-unsubscribe@trafficserver.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Coverity: clear the 74 defects in unit and regression test code [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on issue #13682:
URL: https://github.com/apache/trafficserver/issues/13682#issuecomment-5768434648
Looks like everything was fixed.
--
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: issues-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Coverity: clear the 74 defects in unit and regression test code [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on issue #13682:
URL: https://github.com/apache/trafficserver/issues/13682#issuecomment-5675263216
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.
--
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: issues-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Coverity: clear the 74 defects in unit and regression test code [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on issue #13682:
URL: https://github.com/apache/trafficserver/issues/13682#issuecomment-5674724440
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.
--
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: issues-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Coverity: clear the 74 defects in unit and regression test code [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall closed issue #13682: Coverity: clear the 74 defects in unit and regression test code
URL: https://github.com/apache/trafficserver/issues/13682
--
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: issues-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org