Skip to content

TEST/COVERITY: Gtest fixes for Coverity 2026.3.0 Upgrade - #12177

Merged
gleon99 merged 2 commits into
openucx:masterfrom
william-gallagher-nv:coverity/coverity-fixes-01
Nov 4, 2026
Merged

gleon99 merged 2 commits into
openucx:masterfrom
william-gallagher-nv:coverity/coverity-fixes-01

Conversation

@william-gallagher-nv

@william-gallagher-nv william-gallagher-nv commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

What?

This PR provides fixes for errors flagged by Coverity 2026.3.0.
Fixes focus on two areas:

  • Resolving COPY_INSTEAD_OF_MOVE error flagged in multiple gtest files
  • SRD iov array handling in test_srd.cc

Why?

Fixing these errors helps prepare for upgrading to Coverity 2026.3.0 in CI.

How?

Testing performed locally with master as the baseline shows numerous errors resolved with no new errors added.

   checker                       master      PR    delta
  ARRAY_VS_SINGLETON                  1        0       -1
  COPY_INSTEAD_OF_MOVE               37        8      -29
  CTOR_DTOR_LEAK                      7        6       -1
  PW.PARAMETER_HIDDEN                 1        0       -1
  UNINIT                             79       68      -11
  UNNECESSARY_STRING_COPY           105      101       -4

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread buildlib/tools/coverity.sh Resolved
Comment thread buildlib/tools/coverity.sh Resolved
Comment thread test/gtest/ucp/test_ucp_proto.cc Resolved
Comment thread test/gtest/uct/uct_test.cc Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

Residual coverage gap: the SRD changes (test_srd.cc) only run on EFA hardware, so the newly asserted UCS_ERR_INVALID_PARAM for iovcnt=2 (limit is min(UCT_IB_MAX_IOV, max_send_sge) - 1) is not exercised by regular CI; and the coverity script changes can only be validated by the coverity job output itself.

@svc-ucx

svc-ucx commented Oct 26, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Coverity coverity devel on coverity_rh7) · commit bceb4667

TL;DR: The Coverity job died immediately because cov-analyze from the tools/cov-2026.3.0 module needs GLIBC_2.18 while the coverity_rh7 (RHEL7) container only has glibc 2.17 — fix by pinning COV_MODULE back to a RHEL7-compatible Coverity release or moving the job to a newer container image.

Full analysis

Summary: The coverity devel on coverity_rh7 job failed at the very first analyzer invocation (cov-analyze --ident) with a dynamic-linker error, before UCX was ever configured or built.

Root cause: Pure toolchain/environment incompatibility, not a code defect. The log shows the modules loading fine, then:

+ cov-analyze --ident
cov-analyze: /lib64/libc.so.6: version `GLIBC_2.18' not found (required by cov-analyze)

buildlib/tools/coverity.sh hardcodes COV_MODULE="tools/cov-2026.3.0" (resolving to /auto/sw_tools/.../Coverity_2026.3.0/linux_x86_64/bin), but the job runs in the coverity_rh7 container ([REDACTED:Base64 High Entropy String]...), i.e. RHEL7, whose glibc is 2.17. The 2026.3.0 binaries are linked against glibc ≥ 2.18, so they cannot start at all on that image.

Because the script runs with #!/bin/bash -eEl, it aborted at line 143. The follow-on noise in the log is purely consequential:

ls: cannot access /__w/1/s/cov_build_devel/output/errors: No such file or directory

No configure, no cov-build, no cov-format-errors ever ran — so none of the PR's gtest/SRD-IOV changes are implicated. There is no hang (total job runtime was ~2 s of script time); it is a hard, immediate failure that will reproduce on every run of this job, for any PR.

Implicated commit: [REDACTED:Hex High Entropy String] — William Gallagher, "TEST/COVERITY: Gtest srd iov fixes" (the most recent commit touching buildlib/tools/coverity.sh, which carries both the COV_MODULE pin and the new cov-analyze --ident probe). If this PR did not itself bump the version, then the /hpc/local/etc/modulefiles/tools/cov-2026.3.0 module content was updated server-side to a glibc-2.18 build.

File: buildlib/tools/coverity.sh:7 (COV_MODULE="tools/cov-2026.3.0"), triggered at buildlib/tools/coverity.sh:143; container selection at buildlib/pr/coverity.yml:3.

Suggested fix: Pick one of:

  1. Preferred (smallest change): revert COV_MODULE to the last Coverity release built against glibc ≤ 2.17 (the version in use before bceb4667), e.g. COV_MODULE="tools/cov-2023.12", keeping container: coverity_rh7.
  2. Forward fix: retire the RHEL7 image for this job — change buildlib/pr/coverity.yml:3 from container: coverity_rh7 to a RHEL8/UBI8-or-newer coverity container (glibc ≥ 2.28) so that tools/cov-2026.3.0 can run. This also requires the ops team to publish that image.
  3. Diagnostics hardening (do this regardless): make the cov-analyze --ident probe fail loudly instead of as a bare linker error, so this class of breakage is unambiguous:
    cov-analyze --ident || {
        azure_log_error "Coverity analyzer ($COV_MODULE) cannot run in this container (glibc mismatch?)"
        exit 1
    }
    Also consider set -o pipefail-safe handling in coverity.yml, since res=$? on line 24 is captured after the script line and currently always reflects the script exit correctly only by luck of the bash step semantics.

Related: none found — searches for coverity GLIBC_2.18 cov-analyze and COV_MODULE cov-2026.3.0 coverity_rh7 returned no matching issues/PRs; PR #12177 is the affected PR.

🛡️ This comment had 2 potential secret(s) redacted (Base64 High Entropy String, Hex High Entropy String). See request_id fa068b48-e4fd-40fd-9249-38cfa00cb95a in the triage console for the audit trail.

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread test/gtest/uct/uct_test.cc Resolved
Comment thread test/gtest/uct/uct_test.cc Resolved
Comment thread test/gtest/uct/uct_p2p_test.cc Resolved
Comment thread test/gtest/ucp/test_ucp_proto.cc Resolved
Comment thread test/gtest/ucp/test_ucp_proto.cc Resolved
Comment thread test/gtest/ucp/test_ucp_proto.cc Resolved
@svc-ucx

svc-ucx commented Oct 27, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (AddressSanitizer roce on worker 0) · commit f59c513a

TL;DR: The ASAN gtest job failed because one test, ud/test_ucp_sockaddr_protocols_err.tag_zcopy_64k_unexp_sync/11 (ud_v, recv_stop, mt), hung for 677 s and was killed by the gtest request deadline (ucs::get_deadline() = 675 s under ASAN) — a tag sync send / msg-recv request never completed after the receiver EP was force-closed; this is a pre-existing peer-failure-detection flake, not caused by this coverity-only PR.

Full analysis

Summary: make test returned error 1 because 1 of 8884 gtests failed: ud/test_ucp_sockaddr_protocols_err.tag_zcopy_64k_unexp_sync/11 (GetParam() = ud_v,cuda_copy,rocm_copy/recv_stop,not_all_devs,mt), which consumed 677 076 ms of the run.

Root cause: Evidence of a hang, not slowness: every sibling test in the log completes in 0.2–1 s, while this single test burned 677 s — and 677 s matches exactly the gtest wait deadline get_deadline() = min(watchdog_timeout*0.75, 180 s * ASAN_multiplier) = min(675 s, 3600 s) = 675 s (test/gtest/common/test_helpers.cc:29,31,342). So the test spent its entire life inside one request_wait() loop until ucp_test::request_process() gave up and logged "request %p did not complete on time" (test/gtest/ucp/ucp_test.cc:299-311), failing the test.

The hanging operation is the error-handling pass of test_tag_send_recv(64K, is_exp=false, is_sync=true) with the RECV_STOP variant:

  1. ucp_tag_send_sync_nbx() of 64 KB is posted (ZCOPY_THRESH=2k, RNDV_THRESH=inf → eager zcopy sync send) — test/gtest/ucp/test_ucp_sockaddr.cc:2372.
  2. do_unexp_recv() probes the unexpected message, then recv_stop force-closes the receiver EP (disconnect(*this, receiver()), line 2307-2309).
  3. requests_wait(reqs) (line 2402) then waits for the sync-send (and the ucp_tag_msg_recv_nbx request) to complete. A sync send only completes when the peer returns the sync ACK; with the receiver EP force-closed on ud_v (CONNECT_TO_IFACE, no RC retry timeouts apply — configure_peer_failure_settings() only sets UCX_RC_*), the sender never got the ACK and peer failure was never propagated to the request, so it stayed UCS_INPROGRESS for the full 675 s.

Nothing in this PR's commit ([REDACTED:Hex High Entropy String], "TEST/COVERITY: Move owned values instead of copying", William Gallagher) can affect this test: it only changes test_ucp_sockaddr::listen_inaddr_any to m_test_addr = std::move(test_addr) and makes sock_addr_storage::set_sock_addr() take std::string netdev_name by value + std::move — sock_addr_storage has implicit copy/move and no ownership quirks, and the failing test never uses that path. No src/ucp/** files were touched by the PR.

Implicated commit: unknown — not the PR commit [REDACTED:Hex High Entropy String] / [REDACTED:Hex High Entropy String] (test-only coverity change, unrelated to the failing test); the hang is in UCP tag sync-send peer-failure handling on UD.

File: test/gtest/ucp/test_ucp_sockaddr.cc:2402 (requests_wait in test_tag_send_recv), hang originates from the sync send posted at test/gtest/ucp/test_ucp_sockaddr.cc:2372 after the force-close at :2307

Suggested fix:

  1. Treat this as a flaky infrastructure/product issue, not a PR regression — re-run the AddressSanitizer roce job to unblock PR TEST/COVERITY: Gtest fixes for Coverity 2026.3.0 Upgrade #12177.
  2. Real fix (product side): ensure a ucp_tag_send_sync_nbx request on a CONNECT_TO_IFACE/UD lane is completed with an error (UCS_ERR_CANCELED/UCS_ERR_ENDPOINT_TIMEOUT) when the remote EP is force-closed, i.e. that the sync-ACK wait is purged by peer-failure/keepalive detection. The test already sets KEEPALIVE_INTERVAL=5s and UD_TIMEOUT=5s, so a 675 s wait means the failure was never propagated to the pending ssend request.
  3. Test hardening: the two unbounded spin loops in this fixture can hide/aggravate such hangs and should get ucs::get_deadline() bounds with an explicit failure message — the probe loop at test/gtest/ucp/test_ucp_sockaddr.cc:2294-2298 (while ((message == NULL) && (m_unreachable_err_count == 0))) and test_ucp_sockaddr_protocols::disconnect() at :2590-2595 (while (m_err_count == 0)). Also add UCX_UD_TIMEOUT/retry tuning to configure_peer_failure_settings() (test/gtest/ucp/ucp_test.cc:419-425), which currently only shortens RC timeouts, so UD variants get fast peer-failure detection too.

Related: none found (searches for this test name and for the sync-send/peer-failure hang returned no matching issues or PRs)

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 8bcbcef6-844a-485a-948b-50d2e36e9fd8 in the triage console for the audit trail.

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread test/gtest/ucs/arch/test_x86_64.cc Resolved
@svc-ucx

svc-ucx commented Oct 29, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (AddressSanitizer roce on worker 0) · commit 60557b83

TL;DR: The ASAN/RoCE gtest shard didn't fail an assertion — it hung: rc_verbs/uct_p2p_mix_test.mix_10000/1 <rc_verbs/mlx5_0:1/loopback> produced its [ RUN ] line at 18:21:26 and then nothing for exactly 900 s until the gtest watchdog SIGTERM'd it inside the UCT/IB progress loop, i.e. an rc_verbs loopback operation never completed and the test's unbounded while (comp.count > 0) progress(); spin turned it into a silent 15-minute abort.

Full analysis

Summary: make -C test/gtest test aborted because the gtest watchdog (900 s) fired while uct_p2p_mix_test.mix_10000 was spinning in uct_rc_verbs_iface_progress on the rc_verbs/mlx5_0:1 loopback endpoint.

Root cause: Hang, not slowness. Timestamp analysis of the job log shows one single gap that is the whole runtime:

  • 18:21:26.293 [ RUN ] rc_verbs/uct_p2p_mix_test.mix_10000/1 <rc_verbs/mlx5_0:1/loopback>
  • 18:36:26.297 test/gtest/common/test_helpers.cc:57: Failure / Failed / Connection timed out - abort testing

900.0 s is exactly watchdog_timeout = 900. (test/gtest/common/test_helpers.cc:31); the watchdog then pthread_kills the watched thread (line 59), which is why the backtrace shows ucs_handle_error on top of frame 6 = libuct_ib.so(+0x85ddd) called from gtest — the test thread was inside the rc_verbs progress function when it was killed. No UCX error/FATAL message was emitted at all, so no completion-with-error was seen: a send either never completed, or UCS_ERR_NO_RESOURCE became permanent (TX CQ credits/txqp resources never returned).

The symptom is amplified by the test itself: uct_p2p_mix_test::random_op() (test/gtest/uct/test_p2p_mix.cc:162-177) has two unbounded loops — while (comp.count > 0) { progress(); } and the UCS_ERR_NO_RESOURCE → progress(); continue; retry — with no deadline. The timeout safety valve in run() is only checked between operations (line 210), so it can never break a stuck operation. One lost completion therefore produces a 15-minute, zero-diagnostic hang. Notably the PR under test is a Coverity-annotation PR and /* coverity[loop_condition] */ sits on exactly that loop (line 165) — Coverity flagged this unbounded loop, but a comment cannot change runtime behaviour, so the PR's own diff is not the functional trigger.

Most plausible functional trigger in the merge base (both landed 2–3 days before this 2026-10-29 build, both in the rc_verbs completion path):

Also a real latent credit leak that produces precisely this silent spin: uct_rc_verbs_handle_failure() returns early when uct_rc_iface_lookup_ep() yields NULL (src/uct/ib/rc/verbs/rc_verbs_iface.c:95-97) before uct_rc_verbs_update_tx_res()/uct_rc_iface_add_cq_credits(), and poll_tx routes even IBV_WC_SUCCESS completions with ep == NULL through it (line 147-151). Every such completion permanently loses a TX CQ credit; once tx.cq_available hits 0, all sends return UCS_ERR_NO_RESOURCE forever and the test spins silently.

Implicated commit: Not the PR commit ([REDACTED:Hex High Entropy String] is Coverity annotations only). Suspects: 42c438a0 (Guangguan Wang, #12060) and b0d5d1a4 (Guangguan Wang, #12059).

File: test/gtest/uct/test_p2p_mix.cc:166 (unbounded wait) and src/uct/ib/rc/verbs/rc_verbs_iface.c:95 (credit-leak early return); watchdog at test/gtest/common/test_helpers.cc:57

Suggested fix:

  1. Do not raise the watchdog/timeout 200m — this is a hang. Instead make the hang diagnosable: bound both loops in random_op() with the same deadline run() uses and fail with context (op index, selected send func, comp.count, iface tx.cq_available/fc_wnd) instead of spinning silently, e.g. pass the start_time + timeout deadline into random_op() and ASSERT/UCS_TEST_ABORT when exceeded.
  2. Fix the credit leak: in uct_rc_verbs_handle_failure(), when uct_rc_iface_lookup_ep() returns NULL, still return the TX CQ credits for that completion (call uct_rc_iface_add_cq_credits()) rather than return-ing immediately.
  3. Re-run this shard on the merge base without the PR to confirm it reproduces (expected: it does, since the PR is comment-only). If it does, bisect between 42c438a0/b0d5d1a4 and consider reverting UCT/IB/RC/VERBS: Release the descriptor of a failed receive completion #12060's continue path, and have the ASAN job dump ucx_info/VFS iface counters (or run with UCX_LOG_LEVEL=trace_poll on this one test) to capture whether the stall is a missing completion or exhausted tx.cq_available/fc_wnd.

Related: PR under test: #12177 · suspect merges: #12060, #12059

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 1c28f154-65e5-4ad6-abfa-d0a2c84fcbd6 in the triage console for the audit trail.

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread test/gtest/ucp/test_ucp_proto.cc Resolved
Comment thread test/gtest/uct/uct_test.h Resolved
if (!sibling_dma.empty()) {
dma = sibling_dma;
gpu = sibling_gpu;
gpu = std::move(sibling_gpu);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: can we keep this a copy? gpu = std::move(sibling_gpu) while the line above keeps dma = sibling_dma as a copy. The copy there is mandatory because sibling_dma.empty() is tested again later (line 943), so moving only the gpu string is asymmetric, gains nothing, and hides that constraint — a later "cleanup" that also moves sibling_dma would silently flip the branch.

@svc-nvidia-pr-review

Copy link
Copy Markdown

test/gtest/uct/ib/test_srd.cc

Residual coverage gap (already noted in the earlier review round, not re-raised): test_srd.cc only runs on EFA hardware, so the newly asserted UCS_ERR_INVALID_PARAM for iovcnt=2 is not exercised by the regular CI pipelines.

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Fixes Coverity errors for COPY_INSTEAD_OF_MOVE

Coverity fixes for upgrade to Coverity 2026.3.0
Coverity fixes for upgrade to Coverity 2026.3.0
Comment thread test/gtest/ucp/test_ucp_proto.cc Resolved
Comment thread test/gtest/ucs/test_topo.cc Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

UCS_TEST_MESSAGE << perf2.get();

m_perf = aggregate({perf1, perf2});
m_perf = aggregate({std::move(perf1), std::move(perf2)});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: the std::vector parameter is built from an initializer_list, whose elements are const, so each shared_ptr is still copied into the vector - the move here only clears perf1/perf2. Same at line 1758.

@william-gallagher-nv william-gallagher-nv self-assigned this Oct 30, 2026
@william-gallagher-nv william-gallagher-nv changed the title Test/COVERITY: Fixes for Coverity Upgrade TEST/COVERITY: Gtest fixes for Coverity 2026.3.0 Upgrade Oct 30, 2026
@yosefe

yosefe commented Nov 2, 2026

Copy link
Copy Markdown
Member

If this PR is fixing all issues maybe worth also upgrading Coverity in this PR to see it pass CI?

@william-gallagher-nv

Copy link
Copy Markdown
Contributor Author

If this PR is fixing all issues maybe worth also upgrading Coverity in this PR to see it pass CI?

There are many more issues will have to be resolved before CI passes with Coverity 2026.3.0. This is the first of a series of PRs that I will open to address all the issues.

@william-gallagher-nv

Copy link
Copy Markdown
Contributor Author

@gleon99 @yosefe Can one of you give a maintainer approval for this first round of Coverity fixes?

@gleon99 gleon99 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼

@gleon99
gleon99 merged commit 4090ff5 into openucx:master Nov 4, 2026
167 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants