fix(clr): drain ROCr async thread in hip::Device::~Device() to prevent ASAN mempool UAF by anugodavar · Pull Request #8534 · ROCm/rocm-systems

anugodavar · 2026-07-14T10:55:48Z

Summary

Fixes ASAN use-after-free failures in hipGraphMemPool tests.

Root cause

Graph completion callbacks run on the ROCr async-events thread and can cascade into GraphMemAllocNode -> MemoryPool/Heap::DecrementRefCount. At process exit, hip::Device is released (via RuntimeTearDown's external-object loop) before amd::roc::Device is destroyed — so the drain in roc::Device::~Device() runs too late to protect graph_mem_pool_.

JIRA ID

ROCM-27628

Fix

Add WaitForHsaAsyncHandlersIdle() at the top of hip::Device::~Device() to guarantee the ROCr async thread is idle before any HIP-layer pools are freed.

The drain in roc::Device::~Device() is intentionally kept as an independent guard for backend-level resources and OpenCL builds (CLR_BUILD_OCL).

Related PRs

therock-pr-bot · 2026-07-14T10:57:00Z

❌ PR Check — Action Required

Check	Status	Details
🌿 Branch Name	✅ Pass	—
📝 PR Title/Description	❌ Fail	Error: Title is too long (87 characters). Expected: at most 80 characters. Desired format: `type(optional-scope): short description`
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	❌ Fail	Error: Source/code files changed without an accompanying unit test. Expected: add at least one test file named like `test_<name>.py` / `test_<name>.cpp` (or `<name>_test.`). Current:* code file(s) changed: `projects/clr/hipamd/src/profiler/hip_clr_profiler.cpp`, `projects/clr/rocclr/device/rocm/rocdevice.cpp`; no test file found
🔎 pre-commit	⏳ Pending	⏳ Still running…
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

⚠️ 2 policy check(s) failed. Please address the issues above before this PR can be Reviewed.

🚫 Please fix the failed policies

❌ PR Title/Description

❌ Unit Test

The Not ready to Review label was added to this PR. Once all policies pass, the label is removed automatically.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

therock-pr-bot · 2026-07-14T10:57:02Z

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

❌ PR Title/Description
❌ Unit Test

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

Copilot

Pull request overview

Fixes ASAN use-after-free during HIP graph teardown by ensuring ROCr async completion handlers are drained before HIP-layer pools are freed, and by correcting graph execution teardown/lifetime management around devices and parallel streams.

Changes:

Drain ROCr async handler thread(s) early in hip::Device::~Device() to prevent graph completion callbacks from touching freed HIP pools.
Remove g_devices[captureDeviceId_] retain/release usage from graph/graph-exec code paths to avoid refcount corruption.
Centralize parallel_streams_ teardown in GraphExecBase::~GraphExecBase() so both Classic and Segmented paths clean up streams consistently.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
projects/clr/hipamd/src/hip_graph_internal.hpp	Moves parallel stream teardown into `GraphExecBase` destructor; removes Classic-only teardown (but leaves commented block to clean up).
projects/clr/hipamd/src/hip_graph_internal.cpp	Removes `g_devices[captureDeviceId_]` retains across init/capture paths to avoid device refcount imbalance.
projects/clr/hipamd/src/hip_device.cpp	Adds an async-handler drain at the start of `hip::Device` destruction to prevent teardown-time callback races.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…t ASAN mempool UAF Graph completion callbacks run on the ROCr async-events thread and can cascade into GraphMemAllocNode -> MemoryPool/Heap::DecrementRefCount. At process exit, hip::Device is released (via RuntimeTearDown) *before* amd::roc::Device is destroyed, so the drain in roc::Device::~Device() runs too late to protect graph_mem_pool_. Add WaitForHsaAsyncHandlersIdle() at the top of hip::Device::~Device() to guarantee the async thread is idle before any HIP-layer pools are freed. The drain in roc::Device::~Device() is kept as an independent guard for backend-level resources and OpenCL builds (CLR_BUILD_OCL). Fixes ASAN failures in hipGraphMemPool tests. Related: #8254, #8400

…vice teardown Move WaitForHsaAsyncHandlersIdle() out of hip::Device::~Device() and into a RuntimeTearDown callback registered at init time. The teardown order is: 1. TearDownCallbacks (reverse registration order) ← drain fires here 2. external_ release loop ← hip::Device released 3. Runtime::tearDown() ← roc::Device deleted Draining in a callback is correct because: - roc::Device is still alive (step 3 hasn't run), so the call is safe - It fires once before any hip::Device is released, making the ordering explicit at the teardown level rather than buried in each destructor - hip::Device::~Device() no longer needs to touch roc::Device at all The previous approach drained inside hip::Device::~Device() which worked but was fragile — any future hip::Device field with the same async-thread race would not be protected. Fixes ASAN use-after-free in hipGraphMem tests (ROCM-27628 adjacent). Co-Authored-By: Claude <noreply@anthropic.com>

Co-Authored-By: Claude <noreply@anthropic.com>

…SAN UAF Replace the per-device drain in hip::Device::~Device() with an unconditional DrainAllDevices() in ProfilerAtExit, which is always registered at init time via HipProfilerInitExt(). This ensures SyncAllStreams + WaitForHsaAsyncHandlersIdle always runs before any hip::Device (and its memory pools) is released, regardless of whether a profiler is attached. Previously: - DrainAllDevices() was gated on IsProfilingActive() — skipped with no profiler - WaitForHsaAsyncHandlersIdle() in roc::Device::~Device() ran too late (after hip::Device pools were freed), causing ASAN heap-use-after-free in graph mem pool tests Now: - ProfilerAtExit always drains (SyncAllStreams first, then WaitForHsaAsyncHandlersIdle) before any teardown callback or hip::Device release runs - roc::Device::~Device() no longer needs its own drain (redundant) - Correct ordering: flush GPU work → drain async handlers → release hip::Device Co-Authored-By: Claude <noreply@anthropic.com>

… for OCL safety The drain was removed in a prior commit on the assumption that the RuntimeTearDown callback covers all cases, but that callback is HIP-only. OCL teardown goes through roc::Device::~Device() directly without any prior drain, so restoring it here keeps OCL safe. For HIP this is a harmless no-op since the drain already ran earlier. Co-Authored-By: Claude <noreply@anthropic.com>

anugodavar · 2026-07-17T07:14:17Z

PR fixed the Asan failures. unittest is not required.

…rdown

## Motivation On systems where `hipDeviceCanAccessPeer` returns true (e.g., gfx1250 with AIFM fabric), `Unit_hipGraphAddMemcpyNode1D_Functional`'s peer device section proceeds to call `hipGraphInstantiate`, which fails with `hipErrorNotSupported` (error 801) because multi-device graphs are not yet supported on either the classic or segmented graph scheduling paths in CLR. The test hard-fails rather than skipping, producing a misleading failure on otherwise healthy systems. ## Technical Details In `validateMemcpyNode1DArray`, replace the unconditional `HIP_CHECK(hipGraphInstantiate(...))` in the peer device path with an explicit return-code check. When `hipGraphInstantiate` returns `hipErrorNotSupported`, clean up allocated resources and call `SKIP()` (Catch2 v3.3+) instead of failing. All other error codes continue through `HIP_CHECK` as before. The root cause is a known CLR limitation documented in both `GraphExecClassic::Init()` and `GraphExecSegmented::FindStreamsReqPerDevForSegments()`: [hipGraph] Multi-device graph is not supported on the segment scheduling path The fix is test-side only. `validateMemcpyNode1DArray` is `static` and called from exactly three SECTION blocks within a single test case; the new code path triggers only on `hipErrorNotSupported`, leaving the single-device sections unaffected. ## JIRA ID ROCM-28108 ## Test Plan Rebuilt `GraphsTest1` from the patched source on a gfx1250 system with AIFM fabric active (4 GPUs, all ACCEL_STATE: READY), with PRs #8254 and #8534 applied to CLR/ROCr. Ran with AMD_COMGR_HOTSWAP_ENTRY_TRAMPOLINES=1 HSA_ENABLE_SDMA=1, all 4 GPUs visible. ## Test Result Test | Before | After --------------------------------------------------------------|--------------|---------- Unit_hipGraphAddMemcpyNode1D_Functional (default device) | Passed | Passed Unit_hipGraphAddMemcpyNode1D_Functional (peer device) | Failed (801) | Skipped ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

anugodavar requested a review from a team as a code owner July 14, 2026 10:55

Copilot AI review requested due to automatic review settings July 14, 2026 10:55

github-actions Bot added the project: clr label Jul 14, 2026

therock-pr-bot Bot added the Not ready to Review label Jul 14, 2026

Copilot started reviewing on behalf of anugodavar July 14, 2026 10:57 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

Comment thread projects/clr/hipamd/src/hip_device.cpp Outdated

Comment thread projects/clr/hipamd/src/hip_graph_internal.cpp Outdated

Comment thread projects/clr/hipamd/src/hip_graph_internal.hpp Outdated

anugodavar force-pushed the users/agodavar/fix-asan-graph-mempool-teardown branch from fbc1491 to 97a061e Compare July 14, 2026 11:02

anugodavar changed the title ~~fix(clr): drain ROCr async thread and fix graph refcount/stream teardown to prevent ASAN mempool UAF~~ fix(clr): drain ROCr async thread in hip::Device::~Device() to prevent ASAN mempool UAF Jul 14, 2026

systems-assistant Bot added the organization: ROCm label Jul 14, 2026

anugodavar and others added 2 commits July 15, 2026 07:48

fix(clr): reduce comment verbosity for TearDownCallback

68e3969

Co-Authored-By: Claude <noreply@anthropic.com>

jaydeeppatel1111 reviewed Jul 15, 2026

View reviewed changes

Comment thread projects/clr/hipamd/src/hip_context.cpp Outdated

jaydeeppatel1111 reviewed Jul 15, 2026

View reviewed changes

Comment thread projects/clr/hipamd/src/hip_context.cpp Outdated

jaydeeppatel1111 reviewed Jul 15, 2026

View reviewed changes

Comment thread projects/clr/hipamd/src/profiler/hip_clr_profiler.cpp

jaydeeppatel1111 reviewed Jul 15, 2026

View reviewed changes

Comment thread projects/clr/rocclr/device/rocm/rocdevice.cpp

saleelk approved these changes Jul 15, 2026

View reviewed changes

sunhmy-amd mentioned this pull request Jul 16, 2026

hip-tests: skip peer-device graph test when hipErrorNotSupported #8671

Merged

1 task

Merge branch 'develop' into users/agodavar/fix-asan-graph-mempool-tea…

0a010bf

…rdown

anugodavar merged commit cb8445d into develop Jul 19, 2026
35 of 40 checks passed

anugodavar deleted the users/agodavar/fix-asan-graph-mempool-teardown branch July 19, 2026 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(clr): drain ROCr async thread in hip::Device::~Device() to prevent ASAN mempool UAF - #8534

fix(clr): drain ROCr async thread in hip::Device::~Device() to prevent ASAN mempool UAF#8534
anugodavar merged 6 commits into
developfrom
users/agodavar/fix-asan-graph-mempool-teardown

anugodavar commented Jul 14, 2026 •

edited

Loading

Uh oh!

therock-pr-bot Bot commented Jul 14, 2026 •

edited

Loading

Uh oh!

therock-pr-bot Bot commented Jul 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anugodavar commented Jul 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

anugodavar commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root cause

JIRA ID

Fix

Related PRs

Uh oh!

therock-pr-bot Bot commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ PR Check — Action Required

Uh oh!

therock-pr-bot Bot commented Jul 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anugodavar commented Jul 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anugodavar commented Jul 14, 2026 •

edited

Loading

therock-pr-bot Bot commented Jul 14, 2026 •

edited

Loading