Coverity: bound guarded lengths in Catch2 test bodies - #13687
Conversation
log_entry_to_json returns -1 on failure, and Catch2's REQUIRE does not constrain the value for static analysis, so the int length reached std::string's size_t parameter still possibly negative. Convert once, after the check, through a named size_t. Coverity CID 1660665.
The decode_single_ip lambda returned std::string(out, n) where n is the int result of log_entry_to_json, which is -1 on failure; Catch2's REQUIRE does not constrain it for static analysis. Convert once, after the check, through a named size_t. Coverity CID 1660670.
The int returned by log_entry_to_json is -1 on failure and Catch2's REQUIRE does not constrain it for static analysis, so it reached std::string's size_t parameter still possibly negative. Convert once, after the check, through a named size_t. Coverity CID 1660666.
The int returned by log_entry_to_json is -1 on failure and Catch2's REQUIRE does not constrain it for static analysis, so it reached std::string's size_t parameter still possibly negative. Convert once, after the check, through a named size_t. Coverity CID 1660668.
The int returned by log_entry_to_json is -1 on failure and Catch2's REQUIRE does not constrain it for static analysis, so it reached std::string's size_t parameter still possibly negative. Convert once, after the check, through a named size_t. Coverity CID 1660669.
The int returned by log_entry_to_json is -1 on failure and Catch2's REQUIRE does not constrain it for static analysis, so it reached std::string's size_t parameter still possibly negative. Convert once, after the check, through a named size_t. Coverity CID 1660667.
The int returned by log_entry_to_json is -1 on failure and Catch2's REQUIRE does not constrain it for static analysis, so it reached std::string's size_t parameter still possibly negative. Convert once, after the check, through a named size_t. Coverity CID 1660671.
LogAccess::unmarshal_http_version returns -1 when the destination is too small, and Catch2's REQUIRE does not constrain the value for static analysis, so the int length reached std::string's size_t parameter still possibly negative. Convert once, after the check. Coverity CID 1685407.
encode_indexed_header_field() returns a signed length that is -1 on failure, and Catch2's REQUIRE does not constrain it, so the raw value reached memcmp's size_t parameter. Convert it once, after the checks, into a named unsigned local. Coverity CID 1644291.
The encode_literal_header_field_with_* helpers return -1 on failure, and the preceding REQUIRE does not constrain the value for the analyzer, so the signed length reached memcmp's size_t parameter. Convert it once, after the checks, into a named unsigned local. Coverity CID 1644272.
encode_test handed huffman_encode's raw return value to memcmp as the length, so an error return would be read as a huge unsigned count past the end of both the malloc'd destination and the expected literal. The return value is already asserted equal to the table's expected length, so compare that many bytes instead. Coverity CID 1644220, 1644238, 1644269.
decode_known_vectors used huffman_decode's raw return value as the memcmp length, so the documented negative error return would be read as a huge unsigned count past the end of the 64 byte destination. The return value is already asserted equal to the table's source length. Coverity CID 1660642.
The XPACK_Integer encoding test used xpack_encode_integer's return value as the memcmp length, and that value is XPACK_ERROR_COMPRESSION_ERROR on failure, which memcmp would read as a huge unsigned count past the end of both buffers. The expected length is already asserted equal. Coverity CID 1644253.
The XPACK_String encoding test used xpack_encode_string's int64_t return as the memcmp length, so the negative error return would be read as a huge unsigned count past the end of both the stack buffer and the expected literal. The expected length is already asserted equal. Coverity CID 1644266, 1644292.
The XPACK_String decoding test used the length xpack_decode_string writes back as the memcmp length, which is attacker-controlled in production and unconstrained here, so it could read past the end of the raw string literal. The expected length is already asserted equal. Coverity CID 1644236.
There was a problem hiding this comment.
🟡 Changes recommended
Update both HPACK memcmp bounds to use the known table length instead of the API return value.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR adjusts unit-test length handling to address Coverity warnings while preserving existing runtime assertions.
Changes:
- Converts guarded lengths to explicit bounds.
- Uses expected lengths for XPACK and Huffman comparisons.
- Preserves
REQUIREchecks; HPACK bounds still need adjustment.
File summaries
| File | Summary |
|---|---|
src/traffic_logcat/unit-tests/test_LogEntryJson.cc |
Bounds JSON output lengths. |
src/proxy/logging/unit-tests/test_LogAccess.cc |
Bounds HTTP version string lengths. |
src/proxy/http2/unit_tests/test_HpackIndexingTable.cc |
Adds HPACK comparison bounds; two bounds still derive from the API return. |
src/proxy/hdrs/unit_tests/test_XPACK.cc |
Uses expected XPACK lengths. |
src/proxy/hdrs/unit_tests/test_Huffmancode.cc |
Uses expected Huffman lengths. |
Review details
Suppressed comments (1)
src/proxy/http2/unit_tests/test_HpackIndexingTable.cc:247
- As in the indexed case above, this bound remains derived from the API return
len, so the opaqueREQUIRE(len == ...)does not give Coverity an upper bound. Use the expected length from the test table formemcmp; the assertion immediately above still detects a mismatched return at runtime.
size_t const encoded_len = static_cast<size_t>(len);
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Both sites still derived the memcmp length from the encoder's return value, which REQUIRE does not constrain for the analyzer, so a modeled large positive return still reads as an oversized access. Compare the table's own expected length and keep the REQUIRE as the runtime check, matching the other files in this change. Coverity CID 1644291, 1644272.
|
[approve ci autest 2] |
brbzull0
left a comment
brbzull0
left a comment
There was a problem hiding this comment.
The memcmp half of this PR is correct and does what it claims. Every rewritten bound is now a compile-time constant from a file-scope static const table rather than the API return value: test_Huffmancode.cc:187 bounds by i.expect_len (also the exact malloc size on line 184), :222 by i.src_len into a fixed char dst[64] with a max vector of 29; test_XPACK.cc:69 and :139 by encoded_field_len into buf[128] with a max of 22, :154 by i.raw_string_len; test_HpackIndexingTable.cc:97 and :246 by encoded_field_len into buf[128] with a max of 26. No assertion was removed or loosened and nothing becomes vacuous.
Two things I'd like to see addressed before this merges, neither functional.
The eight std::string commits look like no-ops. At test_LogEntryJson.cc:115/134/188/214/237/322/376 and test_LogAccess.cc:239 the length argument is still literally n / len — the raw int return of log_entry_to_json and LogAccess::unmarshal_http_version, both of which return -1 on error — now wrapped in an explicit static_cast<size_t>. std::basic_string(const CharT*, size_type) is the only viable constructor for std::string(out, n) (the iterator-pair template can't deduce a single type from char* and int), so that conversion was already being inserted implicitly. The emitted code is identical and Coverity gains no constraint it didn't already have, which means these eight almost certainly don't close the eight CIDs they're credited with. This is the same point the PR makes for the memcmp sites — REQUIRE is opaque to Coverity — applied to the other half: the fix is to bound by the test's own constant, not by the API result.
test_HpackIndexingTable.cc:244-245 — the // coverity[overrun-buffer-arg] - len is validated positive above annotation was left in place by the very commit that made it redundant. Annotations attach to the next statement, so it still covers the rewritten REQUIRE(memcmp(...)) on :246-247, which no longer mentions len at all — the rationale text is now false, and the suppression will silently hide a genuine finding if someone later reintroduces a runtime-computed length here. Deleting those two lines makes the site match the already-clean sibling at :97.
Two smaller notes:
test_HpackIndexingTable.cc:470has a third memcmp with the identicalREQUIRE(len > 0); REQUIRE(len == <table>.encoded_field_len); memcmp(..., len)shape, left unconverted. Not in the CID table today — presumably Coverity can boundhpack_encode_header_block's return by itsbuf_lenargument — but converting it would make the file uniform.- The Verification section lists
test_records, test_tsutil, test_proxy_hdrs, test_proxy_hdrs_xpack, test_cache, test_hostdb, test_tscore, test_tsconfig — 321 tests, all passing, but per the CMakeLists at this head none of those eight compiles three of the five files the PR touches, includingtest_HpackIndexingTable.cc— the one file whose assertion semantics actually changed. Worth restating so a committer doesn't read that line as runtime verification of all 15 sites.
Coverity reports "argument cannot be negative" and "out-of-bounds access" on values these tests already guard with
REQUIRE. This PR narrows each one explicitly. Part of #13682.The one thing to decide
Catch2's
REQUIREdoes not constrain values for Coverity.REQUIRE(expr)expands to:Catch::AssertionHandler catchAssertionHandler(...); catchAssertionHandler.handleExpr( Catch::Decomposer() <= expr ); catchAssertionHandler.complete();complete()is not[[noreturn]]— it returns normally when the assertion passes — and the expression itself disappears into an opaqueITransientExpression. So the analyzer has no path fromREQUIRE(n > 0)back to a constraint onn. (Catch::throw_test_failure_exception()is[[noreturn]], but it sits behindcomplete(), so aci/coverity-model.cppentry cannot express this without lying about the passing path.)Accept that, and the remaining 14 commits are the same shape.
The shape of every fix
Each site compares against the test table's own expected length rather than the API's return value, with the existing assertion left intact on the line above. The cast is never applied to the API return value, so a negative error return is still caught by the
REQUIRE— nothing is hidden, and no assertion was changed, weakened or removed.test_LogEntryJson.cctest_LogAccess.cctest_HpackIndexingTable.cctest_Huffmancode.cctest_XPACK.ccFour commits close more than one CID, because Coverity reports one CID per pointer argument to a
memcmpand a single bound fixes both.Also useful to record
Coverity names
TEST_CASE/SECTIONbodiesdummyFunctionN, numbered 0-based and stepping by 2 perTEST_CASE;SECTIONs are not separately numbered. That mapping was confirmed three independent ways (the only body containingrand(), the only bodies feeding an API return intomemcmp, and matching defect-type counts) before any edit was made.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.