Coverity: fix real defects in test code - #13689
Conversation
When TSUrlCreate fails the test destroys the buffer inside the error branch, then falls through to the shared cleanup that releases an MLoc against that buffer and destroys it a second time. The stale url_loc from the previous iteration is used as well. Skip to the next URL instead. Coverity CID 1497313.
When TSMimeHdrCreate fails the test destroys bufp1 and then keeps using it for the remaining thirteen test cases, ending with a second TSMBufferDestroy in the shared cleanup. The header location is never assigned on that path either. Report the failure and return instead. Coverity CID 1497445.
dump_summary divides the operation counts by measured seconds without checking them, so a run that records no elapsed time reports inf and nan rates instead of saying it has nothing to measure. Coverity CID 1591523.
Passing &ep.sa hands ats_ip_copy a pointer whose static type promises only the 16 bytes of struct sockaddr, while the function memcpys up to sizeof(sockaddr_un) into it; the IpEndpoint overload names the union that actually backs the storage, as the rest of the tree does. Coverity CID 1660031.
prepare() strcat'd the caller-supplied input directory into a fixed PATH_MAX buffer and then indexed that buffer with the directory's length, neither of which is bounded by the buffer's size. Coverity CID 1523657.
The story counter tested st_mode without checking stat(), so an entry that cannot be stat'd was classified from an uninitialized struct and could be miscounted as a test story. Coverity CID 1523644.
encode_oversized_hpack_index() returns the signed xpack_encode_integer() length, whose only guard is a REQUIRE inside the helper. The raw value was used to derive the decoder's buf_end, so a negative or oversized length would form a pointer outside the buffer. Check both bounds and convert once into a named unsigned local. Coverity CID 1644260.
The value suffix was memcpy'd to buf plus the raw signed length from encode_oversized_hpack_index(), with nothing checking that the destination stays inside buf. Verify the suffix fits before copying and carry the offset in an unsigned local. Coverity CID 1644201.
write_to() returns -1 on failure and the old REQUIRE only excluded -1, so the signed length reached the read() and memcmp() sizes with neither a lower nor an upper bound against the 32-byte readback buffer or the expected-bytes array. Coverity CID 1644235.
values_test compared dst_start[0] against the expected character before asserting that the decoder reported one byte written, so a decode that returned an error or zero left the test reading a stack buffer the decoder never wrote. Coverity CID 1660641.
decoder_roundtrip_fuzz passed huffman_encode's int64_t return straight into the uint32_t src_len of huffman_decode, so a negative error return would have been read as a four gigabyte source length and an oversized one would have read past the encode buffer. Assert the length fits the buffer and narrow it explicitly. Coverity CID 1660644.
TSMimeHdrFieldNextDup can return TS_NULL_MLOC, and compare_field_names passes the handle straight to TSMimeHdrFieldNameGet, whose sdk_assert would abort the test process. The existing null test only ran later, when the handle was released. Coverity CID 1022107.
build_request() copied the client address with sizeof(struct sockaddr) as the length even though the source object is the caller's struct sockaddr_in, taking the read size from a different type than the object being read. Copy into the IpEndpoint's sin member with sizeof(*ip) so both sides of the copy are provably in bounds. Coverity CID 1544438.
debugObject points at a static object inside the dlopen'd plugin, so it is only valid while the owning unique_ptr lives - ~PluginDso() calls dlclose() and unmaps it. Declaring it at scenario scope let the stale pointer stay live across the GIVEN boundary, where it is dereferenced again; declaring it after the plugin makes it die first instead. Coverity CID 1644274.
The test dropped the value of issuer.release() on the floor, which reads as a leak, and released the certificate even on the path where SSL_CTX_add_extra_chain_cert() fails and therefore does not adopt it. Coverity CID 1664287.
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review issues remain, and all reviewed changes address the identified defects.
Pull request overview
Fixes Coverity-confirmed defects in ATS test and regression code, improving memory safety, bounds checking, ownership, and error handling.
Changes:
- Corrects resource lifetimes and typed address copying.
- Adds buffer and length validation for HTTP/2, HPACK, and Huffman tests.
- Hardens filesystem, statistics, parser, and certificate failure paths.
File summaries
| File | Description |
|---|---|
src/proxy/logging/unit-tests/test_LogAccess.cc |
Uses typed IP endpoint copying. |
src/proxy/http2/unit_tests/test_Http2Frame.cc |
Bounds frame readback. |
src/proxy/http2/unit_tests/test_HpackIndexingTable.cc |
Validates encoded lengths and buffer capacity. |
src/proxy/http2/test_HPACK.cc |
Uses safe path construction and checks stat(). |
src/proxy/http/remap/unit-tests/test_RemapPlugin.cc |
Corrects plugin pointer lifetime ordering. |
src/proxy/http/remap/unit-tests/nexthop_test_stubs.cc |
Copies matching socket address types. |
src/proxy/hdrs/unit_tests/test_Huffmancode.cc |
Validates Huffman lengths and output availability. |
src/iocore/net/unit_tests/test_OCSPStapling.cc |
Preserves certificate ownership on failure. |
src/iocore/aio/test_AIO.cc |
Avoids division by zero in summaries. |
src/api/InkAPITest.cc |
Fixes cleanup flow and null-location handling. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
brbzull0
left a comment
brbzull0
left a comment
There was a problem hiding this comment.
LGTM. The OCSP chain-cert ownership change in test_OCSPStapling.cc is correct on both paths, and I checked it rather than taking the CID on faith:
- Success:
issuer.release()hands the rawX509*toSSL_CTX_add_extra_chain_cert, which pushes it onto the CTX's extra-chain stack, so the cert has exactly one owner.ctxis declared beforeissuerin the TEST_CASE body, soSSL_CTX_freeruns after the now-emptyunique_ptr— no double free, no dangling. - Failure:
issuer.reset(issuer_raw)restores ownership beforeFAIL(...), and in the vendored Catch2 (v3.9,catch_test_macros.hpp:158)FAILexpands to a throw followed byUnreachable(), so the destructor frees it exactly once and control never reaches the later calls. SSL_CTX_add_extra_chain_certdoes adopt only on success — its failure modes aresk_X509_new_null()/sk_X509_push()failing, both before adoption — so reclaiming on failure is right.
One correction to the commit message, no code change needed: the pre-change code did not leak. It was REQUIRE(SSL_CTX_add_extra_chain_cert(...) == 1); issuer.release();, and REQUIRE also throws on failure, so release() was never reached and ~X509Ptr freed the cert during unwinding. Coverity simply can't model REQUIRE as aborting. The new form is clearer and analyzer-legible, so it's a good change — it's just a readability fix rather than the memory-safety bug the message describes. Worth adjusting since the PR body groups it under "Error handling" with a leak claim.
Two small things:
test_HPACK.cc:419 — this commit hardens error handling inside prepare(), but main() still discards prepare()'s own return value, so if the input directory is absent or unreadable both HPACK regression tests report PASSED after executing zero stories. Worth propagating the failure while you're in here — CMakeLists.txt:61 passes -i ${CMAKE_CURRENT_SOURCE_DIR}/hpack-tests, which doesn't resolve outside a source tree.
test_RemapPlugin.cc:367 — the "Scope the plugin debug object to the plugin owning its DSO" change is cosmetic rather than structural: PluginDebugObject* is a raw pointer with a trivial destructor, so moving the declaration only reorders a no-op destructor relative to the dlclose. Fine to take as-is since it silences the CID at no cost, but if the intent is structural it should be applied to the file's other four SCENARIOs so it reads consistently.
These are the Coverity test-code findings that turned out to be real defects rather than analyzer artifacts. Part of #13682.
Unlike the other test-code PRs in this series, there is no single shared idea here — each commit is its own argument, which is why they are separated out.
Memory safety
SDK_API_TSUrlParsedestroys the MBuffer inside theTSUrlCreateerror branch, then falls through to shared cleanup that releases an MLoc against it and destroys it a second time — using a staleurl_locfrom the previous loop iteration. Open since 2022-09-01.SDK_API_TSMimeHdrParsedestroysbufp1whenTSMimeHdrCreatefails and then keeps using it for the remaining thirteen test cases, ending in a secondTSMBufferDestroy. The header location is never assigned on that path either.test_RemapPlugin.ccheld aPluginDebugObject *that points into adlopen'd image, declared one scope outside theunique_ptrwhose destructordlcloses it. Harmless today only because Catch2 re-enters the body per section and reassigns it; the fix makes the ordering structural.nexthop_test_stubs.cc build_request()copied a client address withsizeof(struct sockaddr)out of an object that is astruct sockaddr_in. Now both sides of the copy are the same type.test_LogAccess.cccopied the client address as&x.sa, handingats_ip_copyan undersized pointee type. Switched to theIpEndpointoverload that production code uses everywhere (HttpSM.cc,HttpTransact.cc,PluginVC.cc); behaviour-identical.Unbounded lengths and buffers
test_Huffmancode.cc decoder_roundtrip_fuzzpassedhuffman_encode'sint64_treturn straight intohuffman_decode'suint32_t src_len. An error return would have been read as a ~4 GB source length, and the existingREQUIREnever bounded it above the encode buffer at all. This commit adds a real new assertion.test_HPACK.cc prepare()built paths with an unboundedstrcatinto aPATH_MAX + 1buffer, plus an unchecked index write. Safe only because of a 511-byte cap enforced inink_args.cc— an invariant expressed nowhere locally, and the adjacent line already used the boundedink_strlcat. Fixed at the root: no fixed buffer.test_HpackIndexingTable.cc: verify the value suffix fits beforememcpyinto the block buffer.buf_endwas derived from an unchecked signed length.test_Http2Frame.cc: bound the PUSH_PROMISE frame length against the readback buffer before reading it back.test_Huffmancode.cc: assert the decode produced a byte before reading the output buffer.Error handling
test_HPACK.cc prepare()classified directory entries from astruct statit never checkedstat()had filled in.test_AIO.cc dump_summary()divides operation counts by measured seconds without checking them, reportinginfandnanrates when a run records no elapsed time.SDK_API_TSMimeHdrParsepassed a possibly-null MLoc tocompare_field_names, aborting insideTSMimeHdrFieldNameGet'ssdk_assert; it now reportsTC_FAILinstead. The pass/fail outcome is unchanged.test_OCSPStapling.ccreleased a cert from itsunique_ptrbefore knowingSSL_CTX_add_extra_chain_cert()had adopted it, leaking on the allocation-failure path.Verification
test_records,test_tsutil,test_proxy_hdrs,test_proxy_hdrs_xpack,test_cache,test_hostdb,test_tscore,test_tsconfig— 321 tests, all passing. Every touched file compiles independently on this branch. No test assertion was changed, weakened or removed.