fix(clr): remove kernelNamesOwner to fix graph UAF (ROCM-27628) by anugodavar · Pull Request #8400 · ROCm/rocm-systems

anugodavar · 2026-07-10T12:32:49Z

Summary

Removes kernelNamesOwner_ from AccumulateCommand, eliminating the heap-use-after-free in Unit_hipGraphAddChildGraphNode_CmplxGrph_SchedLogic (ROCM-27628).

The previous code pre-updated lastEnqueueCommand_ before releasing the old command to guard against re-entrant queue access: releasing the old AccumulateCommand could trigger kernelNamesOwner_->release() -> ~GraphExecBase
-> finish() back into the queue while lastEnqueueCommand_ still pointed to the old command. That re-entrant path no longer exists since this PR removes kernelNamesOwner_ from AccumulateCommand entirely, so the workaround is
removed and the order restored to the straightforward release-then-retain.

Motivation

AccumulateCommand retained GraphExecBase* via kernelNamesOwner_ to keep borrowed kernel-name strings alive for ReportActivity(). When the command destructed on the ROCr async-events thread, it released GraphExecBase, triggering cascading graph destruction while updateCommandsState was still walking the command batch — heap-use-after-free.

Regression introduced by df88b76 (DFS stream assignment, #6799).

Technical Details

Remove kernelNamesOwner_, kernelNamesRef_, and the borrowing API from AccumulateCommand
Replace with kernelNames_ (vector<const char*>): stable pointers into Kernel::getDemangledName(), resolved at dispatch time from KernelMap()
kernelNames_ and timestamps_ are parallel vectors walked by the same index in ReportActivity
~AccumulateCommand no longer touches GraphExecBase; the UAF race is structurally eliminated
Remove kernelNames param from dispatchAqlPacketBatchFlat; extract kernel names from flat AQL packets via KernelMap at dispatch time

JIRA ID

ROCM-27628

Test Plan

Unit_hipGraphAddChildGraphNode_CmplxGrph_SchedLogic — UAF fixed

Test Result

'Unit_hipGraphAddChildGraphNode_CmplxGrph_SchedLogic' - passed without UAF

therock-pr-bot · 2026-07-10T12:34:01Z

❌ PR Check — Action Required

Check	Status	Details
🌿 Branch Name	✅ Pass	—
📝 PR Title/Description	✅ Pass	—
⛔ 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/hip_graph_internal.cpp`, `projects/clr/rocclr/device/device.hpp`, `projects/clr/rocclr/device/rocm/rocvirtual.cpp`, `projects/clr/rocclr/device/rocm/rocvirtual.hpp`, `projects/clr/rocclr/platform/activity.cpp` (+3 more); no test file found
🔎 pre-commit	⏳ Pending	⏳ Still running…
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

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

🚫 Please fix the failed policies

❌ 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-10T12:34:03Z

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

❌ 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

This PR removes AccumulateCommand’s ownership/borrowing of kernel-name storage from GraphExecBase to eliminate a heap-use-after-free during async completion, and instead records stable kernel-name pointers resolved from the device KernelMap at dispatch time.

Changes:

Removed kernelNamesOwner_ / borrowing APIs from AccumulateCommand so its destructor no longer touches GraphExecBase.
Collected kernel-name pointers at dispatch time in dispatchAqlPacketBatchFlat and aligned ReportActivity to use the new representation.
Simplified graph enqueue path by dropping the kernelNames parameter from dispatchAqlPacketBatchFlat.

Reviewed changes

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

Show a summary per file

File	Description
projects/clr/rocclr/platform/command.hpp	Replaces borrowed kernel-name storage with `std::vector<const char*>` and parallel timestamps.
projects/clr/rocclr/platform/command.cpp	Removes `GraphExecBase` retain/release from `AccumulateCommand` destructor.
projects/clr/rocclr/platform/activity.cpp	Updates activity reporting to use the new kernel-name/timestamp vectors.
projects/clr/rocclr/device/rocm/rocvirtual.hpp	Updates `dispatchAqlPacketBatchFlat` signature to drop `kernelNames` parameter.
projects/clr/rocclr/device/rocm/rocvirtual.cpp	Resolves kernel names from `KernelMap` during flat AQL batch dispatch; updates debug logging accordingly.
projects/clr/rocclr/device/device.hpp	Updates virtual dispatch API to match the new signature.
projects/clr/hipamd/src/hip_graph_internal.cpp	Stops pinning `GraphExecBase` via `AccumulateCommand` and removes kernel-names plumbing.

💡 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

anugodavar · 2026-07-15T07:07:47Z

We fix asan failures for unit test. So testcase is not required for this PR.

…t ASAN mempool UAF (#8534) ## 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 - #8254 - #8400 ---------

… UAF AccumulateCommand retained GraphExecBase* via kernelNamesOwner_ to keep borrowed kernel-name strings alive for ReportActivity(). This caused ~AccumulateCommand on the ROCr async-events thread to release GraphExecBase, triggering cascading graph destruction (including ChildGraphNode deletion) while updateCommandsState was still walking the command batch, resulting in a heap-use-after-free. Repro: Unit_hipGraphAddChildGraphNode_CmplxGrph_SchedLogic with ASAN. Regression introduced by df88b76 (DFS stream assignment, #6799). - Remove kernelNamesOwner_, kernelNamesRef_, and the borrowing API (setKernelNamesRef, addKernelNames) from AccumulateCommand - Replace with kernelNames_ (vector<const char*>): stable pointers into Kernel::getDemangledName(), resolved at dispatch time from KernelMap() - kernelNames_ and timestamps_ are parallel vectors (one entry each per dispatch slot), walked by the same index in ReportActivity — no null-skip - ~AccumulateCommand no longer touches GraphExecBase; the UAF race is structurally eliminated - dispatchAqlPacketBatchFlat: remove kernelNames param, extract kernel_object from flat AQL packets and look up name from KernelMap at dispatch time - Debug logging path uses KernelMap directly instead of kernelNames param ROCM-27628 - Unit_hipGraphAddChildGraphNode_CmplxGrph_SchedLogic (UAF fixed) - Unit_hipGraphAddChildGraphNode_SingleChildNode (stream leak fixed) - Unit_hipExecutionCtxStreamDetached_ActiveCapture_Invalidated - Unit_hipExecutionCtxStreamDetached_ParallelCapture_Invalidated - Unit_hipGraphMem_Alloc_Free_NodeGetParams_Functional (×4, no regression) Co-Authored-By: Claude <noreply@anthropic.com>

- Handle ext kernel dispatch (HSA_AMD_PACKET_TYPE_EXT_KERNEL_DISPATCH) in kernelNames_ collection so it stays parallel to timestamps_ - Replace nullptr fallback with "<unknown>" to avoid null kernel_name in downstream activity consumers - Clarify comments: kernelNames_ only covers dispatch slots (base + ext), not all AQL slots; addKernelName() must not be passed nullptr Co-Authored-By: Claude <noreply@anthropic.com>

Kernel name resolution (memcpy + KernelMap lookup per dispatch packet) was running unconditionally on every graph launch. Wrap in vcmd->profilingInfo().enabled_ check to preserve the fast path when no profiler is active, matching the original setKernelNamesRef behavior. Co-Authored-By: Claude <noreply@anthropic.com>

- Replace kBaseKernelObjectOffset/kExtKernelObjectOffset with a single kKernelObjectOffset: both hsa_kernel_dispatch_packet_t and hsa_amd_ext_kernel_dispatch_packet_t place kernel_object at offset 32. Added static_assert to enforce this invariant going forward. - Extract kernelObjectFromPacket lambda to avoid duplicating the memcpy. - Fix profiling gate from vcmd->profilingInfo().enabled_ (always 0 at dispatch time) to IsEnabled(OP_ID_DISPATCH): EnableProfiling() runs inside enqueue(), after all segments are dispatched, so the old gate never fired. Also gate on IsLogEnabled(LOG_DETAIL_DEBUG, LOG_KERN2) to match the log level used in the main dispatch loop. Co-Authored-By: Claude <noreply@anthropic.com>

Remove the separate upfront name-resolution pass (separate loop over flatPacketData with a memcpy per packet). Instead collect kernel names inside the existing per-packet fixup loop where slot->kernel_object is already accessible directly from queue memory, isKernelDispatch is already computed, and the KernelMap lookup is already done for debug logging. - Add needKernelNames = IsEnabled(OP_ID_DISPATCH) to the outer fixup- loop gate so the loop runs when profiling is active even when timestamp_ is null and debug logging is off. - Hoist the KernelMap::find() so the single result feeds both addKernelName() and the ClPrint — one lookup instead of two. - Move static_assert for equal kernel_object offsets to function entry so it is always checked regardless of which code paths are active. Co-Authored-By: Claude <noreply@anthropic.com>

The previous comment claimed kernel_names and timestamps are strictly parallel ("one entry each per dispatch slot"), which is only true when profiling is active at both dispatch time and signal completion. The min-size loop bound is the actual safety mechanism. Co-Authored-By: Claude <noreply@anthropic.com>

The previous code pre-updated lastEnqueueCommand_ before releasing the old command to guard against re-entrant queue access: releasing the old AccumulateCommand could trigger kernelNamesOwner_->release() -> ~GraphExecBase -> finish() back into the queue while lastEnqueueCommand_ still pointed to the old command. That re-entrant path no longer exists since this PR removes kernelNamesOwner_ from AccumulateCommand entirely, so the workaround is removed and the order restored to the straightforward release-then-retain. Co-Authored-By: Claude <noreply@anthropic.com>

addTimestamps() was changed to take 2 args (start, end) when KernelTimestamp was removed, but this call site was missed, causing a build failure. Co-Authored-By: Claude <noreply@anthropic.com>

## Summary Removes `kernelNamesOwner_` from `AccumulateCommand`, eliminating the heap-use-after-free in `Unit_hipGraphAddChildGraphNode_CmplxGrph_SchedLogic` (ROCM-27628). The previous code pre-updated lastEnqueueCommand_ before releasing the old command to guard against re-entrant queue access: releasing the old AccumulateCommand could trigger kernelNamesOwner_->release() -> ~GraphExecBase -> finish() back into the queue while lastEnqueueCommand_ still pointed to the old command. That re-entrant path no longer exists since this PR removes kernelNamesOwner_ from AccumulateCommand entirely, so the workaround is removed and the order restored to the straightforward release-then-retain. ## Motivation `AccumulateCommand` retained `GraphExecBase*` via `kernelNamesOwner_` to keep borrowed kernel-name strings alive for `ReportActivity()`. When the command destructed on the ROCr async-events thread, it released `GraphExecBase`, triggering cascading graph destruction while `updateCommandsState` was still walking the command batch — heap-use-after-free. Regression introduced by df88b76 (DFS stream assignment, #6799). ## Technical Details - Remove `kernelNamesOwner_`, `kernelNamesRef_`, and the borrowing API from `AccumulateCommand` - Replace with `kernelNames_` (`vector<const char*>`): stable pointers into `Kernel::getDemangledName()`, resolved at dispatch time from `KernelMap()` - `kernelNames_` and `timestamps_` are parallel vectors walked by the same index in `ReportActivity` - `~AccumulateCommand` no longer touches `GraphExecBase`; the UAF race is structurally eliminated - Remove `kernelNames` param from `dispatchAqlPacketBatchFlat`; extract kernel names from flat AQL packets via `KernelMap` at dispatch time ## JIRA ID ROCM-27628 ## Test Plan - `Unit_hipGraphAddChildGraphNode_CmplxGrph_SchedLogic` — UAF fixed ## Test Result - 'Unit_hipGraphAddChildGraphNode_CmplxGrph_SchedLogic' - passed without UAF

…ice owner (#8535) ## Summary Fixes ASAN failures in hipGraphMemcpy tests caused by `dev_id_` being set to the wrong device in `GraphMemcpyNode1D`. ### Root cause For `hipWriteBuffer` (H2D) and `hipReadBuffer` (D2H) cases, `dev_id_` defaulted to the device that was current at node-record time. This differs from the device that actually owns the device-side memory pointer when `hipSetDevice()` is called between the `hipMalloc` and the graph node's creation, leading to ASAN failures. ### Fix Override `dev_id_` from the actual device memory object: - `hipWriteBuffer` (H2D): use `dstMemory->GetDeviceById()->index()` - `hipReadBuffer` (D2H): use `srcMemory->GetDeviceById()->index()` ## Related PRs - #8400 - #8245

#8935) …ression) PR #8400 (7c23d18) removed KernelTimestamp.queue_index when fixing the kernelNamesOwner UAF, causing all graph kernel dispatches to be reported on the launch stream queue_id instead of the actual parallel stream they ran on. Profiling traces showed 1 stream for paths4/full4 graphs regardless of their topology. Restore: - KernelTimestamp struct with start/end/queue_index fields - queue_index parameter in addTimestamps() (UINT32_MAX = unknown) - signal->queue_index_ passed through addTimestamps() in rocvirtual.cpp - Per-packet queue_id assignment in ReportActivity so each kernel lands on its actual internal parallel stream Verified with GraphBench paths4/full4 — now shows 4 distinct stream queue_ids with correct event distribution. ## Motivation  ## Technical Details  ## Issue Tracking    ## Test Plan  ## Test Result  ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

…ression) PR ROCm#8400 (7c23d18) removed KernelTimestamp.queue_index when fixing the kernelNamesOwner UAF, causing all graph kernel dispatches to be reported on the launch stream queue_id instead of the actual parallel stream they ran on. Profiling traces showed 1 stream for paths4/full4 graphs regardless of their topology. Restore: - KernelTimestamp struct with start/end/queue_index fields - queue_index parameter in addTimestamps() (UINT32_MAX = unknown) - signal->queue_index_ passed through addTimestamps() in rocvirtual.cpp - Per-packet queue_id assignment in ReportActivity so each kernel lands on its actual internal parallel stream Verified with GraphBench paths4/full4 — now shows 4 distinct stream queue_ids with correct event distribution. Co-Authored-By: Claude <noreply@anthropic.com>

anugodavar requested a review from a team as a code owner July 10, 2026 12:32

Copilot AI review requested due to automatic review settings July 10, 2026 12:32

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

Copilot started reviewing on behalf of anugodavar July 10, 2026 12:33 View session

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

Copilot AI reviewed Jul 10, 2026

View reviewed changes

Comment thread projects/clr/rocclr/device/rocm/rocvirtual.cpp Outdated

Comment thread projects/clr/rocclr/platform/activity.cpp

Comment thread projects/clr/rocclr/platform/command.hpp Outdated

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

jaydeeppatel1111 reviewed Jul 10, 2026

View reviewed changes

Comment thread projects/clr/rocclr/device/rocm/rocvirtual.cpp Outdated

This was referenced Jul 14, 2026

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

Merged

fix(clr): fix dev_id_ for H2D/D2H GraphMemcpyNode1D to use actual device owner #8535

Merged

saleelk reviewed Jul 14, 2026

View reviewed changes

Comment thread projects/clr/rocclr/device/rocm/rocvirtual.cpp Outdated

saleelk reviewed Jul 14, 2026

View reviewed changes

Comment thread projects/clr/rocclr/device/rocm/rocvirtual.cpp Outdated

saleelk requested changes Jul 14, 2026

View reviewed changes

anugodavar changed the title ~~fix(clr): remove kernelNamesOwner from AccumulateCommand to fix graph UAF (ROCM-27628)~~ fix(clr): remove kernelNamesOwner to fix graph UAF (ROCM-27628) Jul 15, 2026

anugodavar removed the Not ready to Review label Jul 15, 2026

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

anugodavar force-pushed the users/agodavar/rocm27628-fix-kernelnames-uaf branch from 7ff358a to 567e242 Compare July 15, 2026 07:23

saleelk approved these changes Jul 15, 2026

View reviewed changes

anugodavar force-pushed the users/agodavar/rocm27628-fix-kernelnames-uaf branch from de3ec4a to fa648af Compare July 19, 2026 16:53

anugodavar and others added 6 commits July 19, 2026 16:54

anugodavar force-pushed the users/agodavar/rocm27628-fix-kernelnames-uaf branch from fa648af to 2d056a2 Compare July 19, 2026 16:54

fix(clr): drop queue_index_ from addTimestamps call

0fe4830

addTimestamps() was changed to take 2 args (start, end) when KernelTimestamp was removed, but this call site was missed, causing a build failure. Co-Authored-By: Claude <noreply@anthropic.com>

anugodavar merged commit 7c23d18 into develop Jul 20, 2026
35 of 40 checks passed

anugodavar deleted the users/agodavar/rocm27628-fix-kernelnames-uaf branch July 20, 2026 08:32

anugodavar mentioned this pull request Jul 21, 2026

fix(clr): restore per-stream queue_id in graph profiling (7c23d18 reg… #8935

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(clr): remove kernelNamesOwner to fix graph UAF (ROCM-27628) - #8400

fix(clr): remove kernelNamesOwner to fix graph UAF (ROCM-27628)#8400
anugodavar merged 8 commits into
developfrom
users/agodavar/rocm27628-fix-kernelnames-uaf

anugodavar commented Jul 10, 2026 •

edited

Loading

Uh oh!

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

edited

Loading

Uh oh!

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

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anugodavar commented Jul 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

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

Uh oh!

Summary

Motivation

Technical Details

JIRA ID

Test Plan

Test Result

Uh oh!

therock-pr-bot Bot commented Jul 10, 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 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

anugodavar commented Jul 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anugodavar commented Jul 10, 2026 •

edited

Loading

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

edited

Loading

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

edited

Loading