Skip to content

Coverity: Uinitialized variable fixes for Coverity 2026.3.0 Upgrade - #12209

Open
william-gallagher-nv wants to merge 2 commits into
openucx:masterfrom
william-gallagher-nv:coverity/coverity-fixes-02
Open

william-gallagher-nv wants to merge 2 commits into
openucx:masterfrom
william-gallagher-nv:coverity/coverity-fixes-02

Conversation

@william-gallagher-nv

@william-gallagher-nv william-gallagher-nv commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

What?

This PR provides fixes for errors flagged by Coverity 2026.3.0.
It focuses on fixing the instances of UNINIT, uninitialized variable usage. This PR mainly just initializes variables to 0 or default values.

Why?

Fixing these errors helps prepare for upgrading to Coverity 2026.3.0 in CI. This is one PR in a series of PRs to make all the code changes required for upgrading.

How?

Testing performed locally with master as the baseline shows numerous errors resolved. Some UNINIT errors remain. These are either resolved in #12177 or fixing them resulted in other Coverity errors and will be resolved in future PRs.

   checker                       master      PR    delta
  UNINIT                             79      32      -47

@william-gallagher-nv
william-gallagher-nv requested a review from a team as a code owner November 2, 2026 13:32
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/ucp/proto/proto_am.inl Resolved
Comment thread examples/ucp_hello_world.c Resolved
Comment thread src/uct/sm/mm/xpmem/mm_xpmem.c Resolved
Comment thread src/uct/sm/mm/xpmem/mm_xpmem.c Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/ucp/proto/proto_am.inl Resolved
Comment thread examples/ucp_hello_world.c Resolved
Comment thread src/uct/sm/mm/xpmem/mm_xpmem.c Resolved
Comment thread test/gtest/ucp/test_ucp_am.cc Resolved
Comment thread test/gtest/uct/ib/test_ib.cc Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

Coverage gap: No test accompanies the xpmem max_unreleased fix. docs/CodeStyle.md asks for a test per bugfix; a unit test for that field is impractical, so the author should state that explicitly if the hunk stays in this PR.

@svc-ucx

svc-ucx commented Nov 2, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests new on worker 3) · commit e48613f9

TL;DR: The gtest binary aborted on ucs_assert(!(region->flags & UCS_RCACHE_REGION_FLAG_PGTABLE)) (rcache.c:396) while ucp_mem_unmap() released the parent rcache region of a user memh — i.e. the region's refcount reached 0 while it was still in the page table, an rcache refcount imbalance in the ucs_rcache_region_put_unsafe() fast path, not anything the Coverity uninitialized-variable PR changes.

Full analysis

Summary: Azure job "Tests new on worker 3" (build 142125) failed in tcp/test_ucp_rma.put_nonblocking/2 <tcp,cuda_copy,rocm_copy/user_memh> with a UCS assertion abort (make: *** [Makefile:4971: test] Aborted (core dumped)).

Root cause: From the log backtrace:
test_ucp_memheap.cc:120 (ucp_mem_unmap) → ucp_mm.c:1291 ucp_mem_unmap → ucp_mm.c:470 ucp_memh_cleanup → ucp_mm.c:441 ucp_memh_put_rcache → rcache.inl:128 ucs_rcache_region_put_unsafe → rcache.c:396 ucs_mem_region_destroy_internal, failing Assertion '!(region->flags & UCS_RCACHE_REGION_FLAG_PGTABLE)'.
A region that is still inserted in the page table always holds one reference (region->refcount = 2; /* Page-table + user */, rcache.c:1028), so releasing the single user reference must never bring it to 0. The assertion therefore proves a lost/duplicated reference on the parent region. The fast path is asymmetric with the slow path: ucs_rcache_lookup_unsafe() does a plain region->refcount++ (rcache.inl:103) and ucs_rcache_region_put_unsafe() a plain --region->refcount (rcache.inl:127), whereas ucs_rcache_region_hold()/ucs_rcache_region_put_internal() use ucs_atomic_add32/ucs_atomic_fsub32 (rcache.c:1055, 450). The UCM unmap/invalidate and LRU-evict paths (rcache.c:481-526, 700-734) run the atomic variants concurrently with these non-atomic ones (the RMA test constantly allocates/frees mem_buffers, generating invalidation events), so a lost update drops the refcount by two and the region is freed while still in the pgtable. Nothing in the failing path relates to the PR's stack-variable initialization work: the last functional changes to src/ucs/memory/rcache.c|.inl are from March 2026 (311e3506, inv_q → interval tree), and the only recent ucp_mm.c change is the already-merged c11daa09 (uct_md_mem_reg_params_t reg_params = {0}, which is redundant with the explicit field assignments at ucp_mm.c:614-621). This looks like a pre-existing/intermittent rcache refcount bug, unrelated to PR #12209. (The ~45 s gaps in the timestamps after the assertion are only the backtrace/addr2line resolution, not a hang.)

Implicated commit: unknown (not caused by [REDACTED:Hex High Entropy String]/PR #12209; closest functional change to the refcounting/invalidation logic is 311e350 "UCS/RCACHE: Change inv_q type to interval tree", nbellalou)

File: src/ucs/memory/rcache.inl:103,127 (assertion fires at src/ucs/memory/rcache.c:396; caller src/ucp/core/ucp_mm.c:441)

Suggested fix:

  1. Re-run the job to confirm this is not reproducible on the PR branch — the diagnosed path is untouched by PR Coverity: Uinitialized variable fixes for Coverity 2026.3.0 Upgrade #12209, so it should not block the merge.
  2. Real fix for the bug: make the "unsafe" fast path refcount updates atomic and consistent with the slow path — use ucs_atomic_add32(&region->refcount, 1) in ucs_rcache_lookup_unsafe() and ucs_atomic_fsub32(&region->refcount, 1) in ucs_rcache_region_put_unsafe() (rcache.inl:103/127), and strengthen diagnosis by replacing ucs_assert at rcache.c:396 with a ucs_assertv that prints region, flags, refcount, lru_flags and the rcache name so the next occurrence identifies the leaking/extra put.
  3. Optionally add a debug-build check in ucs_rcache_region_put_unsafe() that a region with UCS_RCACHE_REGION_FLAG_PGTABLE never reaches refcount 0, to catch the imbalance at the offending put rather than at destroy time.

Related: PR #12209 (this build); no existing issue found for this assertion signature.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 3335c6f3-230d-4299-81d9-f2599cb7744a 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 examples/ucp_hello_world.c Resolved
Comment thread src/ucp/proto/proto_am.inl Resolved
Comment thread src/uct/sm/mm/xpmem/mm_xpmem.c Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/ucp/core/ucp_mm.c Resolved
Comment thread src/ucp/proto/proto_am.inl Resolved
Comment thread examples/ucp_hello_world.c Resolved
Comment thread test/gtest/ucp/test_ucp_am.cc Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

Residual coverage note: the only behavioral hunk (xpmem max_unreleased) remains untested, which the previous review already raised.

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread test/gtest/ucp/test_ucp_device.cc Resolved
Comment thread src/uct/ib/base/ib_device.c Resolved
@svc-ucx

svc-ucx commented Nov 6, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (AddressSanitizer gpu on worker 1) · commit 87b267e7

TL;DR: The gtest run died because a GPU-side "illegal memory access" was raised during shm_ib_ipc/test_ucp_am_nbx_rndv_memtype_disable_zcopy.rndv_disable_get_zcopy/13 (cuda-async → host), which poisoned the CUDA context so every later CUDA call failed and the unwound progress loop then tripped ucp_worker.c:3335; this is a CUDA stream-ordered ("cuda-async"/mempool) memory handling bug, not something PR #12209 (uninitialized-variable Coverity fixes) touches — re-run and fix the mempool range handling in cuda_copy_md.c.

Full analysis

Summary: make test aborted after mem_buffer.cc:879 cudaMemcpy(... ) failed: an illegal memory access was encountered: dst=0x4302000000 src=0x631001748800 length=65536, followed by 14 cascading CUDA failures and the fatal assertion worker->inprogress++ == 0.

Root cause: Evidence chain from the log:

  1. 04:06:13.70 — rndv_disable_get_zcopy/13 <.../proto_v1/cuda-async/host> starts (sender buffer from cudaMallocAsync, i.e. stream-ordered mempool memory; rndv get/put zcopy disabled, so the data path goes through the cuda_copy memtype pipeline).
  2. 04:06:14.13 — the first CUDA error surfaces at mem_buffer.cc:879 on a cudaMemcpy into the device/mempool pointer 0x4302000000. cudaErrorIllegalAddress is sticky and context-wide, and asynchronous — it was raised by a GPU operation issued during that rndv transfer on the cuda-async buffer, not by the memcpy itself.
  3. 04:06:14–04:06:21 — every following test fails inside plain cudaMalloc / cudaMallocManaged / cudaMallocAsync with the same sticky error (14 tests), confirming the context is dead rather than each test being buggy.
  4. 04:06:21.28 — the gtest failure in the middle of a progress loop unwinds without decrementing the counter, so the next ucp_worker_progress() hits ucs_assert(--worker->inprogress == 0) (src/ucp/core/ucp_worker.c:3335) and aborts the binary (make: *** [Makefile:4992: test] Aborted (core dumped)). That assertion and the Bash exited with code '2' are cascade symptoms, not the cause.

The suspicious code is the registration/range-widening path for stream-ordered CUDA memory: in uct_cuda_copy_md_query_attributes() a cudaMallocAsync pointer takes the cuda_mem_ctx == NULL branch and is flagged is_async_managed with is_vmm = 0 (cuda_copy_md.c:721-731), then uct_cuda_copy_md_sync_memops_get_address_range() widens base_address/alloc_length via cuMemGetAddressRange() and caches it in the memtype cache (cuda_copy_md.c:595-621, 1052). The guard that rejects a range which is not fully backed — (is_vmm && ((base_address + alloc_length) < (address + length))) at line 616 — only applies when is_vmm is set, so for mempool allocations UCX can end up using/exposing a VA range larger than the committed mempool mapping, and a GPU access over it faults exactly as seen. Note this combination passed several times earlier in the same run (/45, /29, /53, /61, put_zcopy/13), so the fault is intermittent/timing-dependent (likely a free/reuse race of stream-ordered memory while UCX still accesses it), consistent with an existing latent bug rather than a regression from this PR.

Implicated commit: Not the PR commit (87b267e7, Coverity uninitialized-variable fixes — no CUDA data-path change is visible in the failure). Nearest related change to the implicated code: 4cdc1712 "UCT/CUDA_COPY/MD: Don't widen VMM range crossing a handle boundary (#12186)" by Tristan Rice, 2026-10-30, which added the partial-mapping guard only for is_vmm.

File: src/uct/cuda/cuda_copy/cuda_copy_md.c:590-621 (widening guard) and src/uct/cuda/cuda_copy/cuda_copy_md.c:721-731 (async/mempool detection); failure observed at test/gtest/common/mem_buffer.cc:879; cascade assert at src/ucp/core/ucp_worker.c:3335.

Suggested fix:

  1. Re-run this job on PR Coverity: Uinitialized variable fixes for Coverity 2026.3.0 Upgrade #12209 — the failure is unrelated to the uninitialized-variable fixes, so a green re-run should unblock the PR.
  2. Real fix: apply the "range not fully backed" check to stream-ordered/mempool allocations too, e.g. change line 615-618 to skip widening whenever the queried range does not cover [address, address+length) regardless of is_vmm, and skip cuMemGetAddressRange()-based widening entirely when is_async_managed is set (mempool VA reservations are not a safe proxy for the committed allocation).
  3. Test-harness hardening so one GPU fault doesn't produce 14 bogus failures plus an unrelated fatal assert: in mem_buffer.cc, treat a sticky CUDA error (cudaErrorIllegalAddress) as fatal for the whole run (abort immediately instead of continuing), and make the gtest progress helpers exception-safe (RAII decrement of worker->inprogress, or reset it on test teardown) so an aborted progress loop doesn't trip ucp_worker.c:3335.

Related: PR #12209 (the PR under test), PR #12186 (4cdc1712, VMM range widening guard), PR #11922 (2054662d, async managed memory handling), issue #10039 (worker->inprogress++ == 0 assertion), issue #8523 (gtest instability on the same swx-rdmz-ucx-gpu-01 worker).

Fix Coverity UNINIT errors by initializing structures before their first use.
Fix Coverity UNINIT errors by initializing structures before their first
use
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/ucp/proto/proto_am.inl Resolved
Comment thread src/uct/sm/mm/xpmem/mm_xpmem.c Resolved
@william-gallagher-nv

Copy link
Copy Markdown
Contributor Author

@gleon99 Another round of fixes prepping for upgrade to Coverity 2026.3.0. Can you take a look?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants