Skip to content

fix(rocr): guard handle aperture allocation against a null manager - #13084

Merged
shwetagkhatri merged 1 commit into
developfrom
users/shwetak/dxg-handle-aperture-null-guard
Oct 27, 2026
Merged

shwetagkhatri merged 1 commit into
developfrom
users/shwetak/dxg-handle-aperture-null-guard

Conversation

@shwetagkhatri

Copy link
Copy Markdown
Contributor

JIRA ID : ROCM-30547

NOTE ON PROVENANCE AND STATUS, PLEASE READ FIRST

This change was generated by an AI triage agent (ROCR_TRIAGE_AGENT) and is
UNTESTED. It has not been compiled and it has not been run. I do not have a
WSL DXG target available, so I could not build the affected path at all.

It is posted because the defect analysis is solid and the fix is a two-branch
null guard that mirrors a guard already merged a few lines away in the same
file, so it is likely the right solution and is cheap for an owner who can
build this path to confirm. Please treat it as a reviewed-by-nobody starting
point rather than a validated change. If it is easier for you to write it
yourself from the analysis below, please do that and close this.

THE DEFECT

Heap initialisation tolerates a failed handle aperture reservation. It logs a
warning and continues, which leaves handle_aperture_mgr_ null.
HandleApertureAlloc and HandleApertureFree then dereference that manager
unconditionally. A tolerated initialisation failure therefore turns into a
null dereference inside the VA manager at the first use, faulting at the
min_align_ member, which sits at offset 0x60 behind the two map members.

This is the same class of defect that was fixed for the system heap path. That
fix added the equivalent null guards to ReserveIPCSysMem and FreeIPCSysMem
after a null system_heap_mgr_ was found to fault at the same offset under a
constrained address space. The handle aperture path has the identical
tolerate-then-dereference shape and was not covered by that change.

THE CHANGE

Return the existing OutOfHandleApeMemory error when the manager was never
constructed, and make the free path a no-op in that case. The style
deliberately matches the already-merged guards on the system heap path in the
same file.

REACHABILITY

Reachable on the WSL DXG path from the physical-handle branch of GPU memory
initialisation and from the matching free path.

BLAST RADIUS

None for working configurations. The new branch is taken only when the manager
was never constructed, which is undefined behaviour today, and the error
returned is the one callers already handle for the exhausted case.

WHAT A REVIEWER SHOULD CHECK, since I could not

  1. That it compiles on the DXG path.
  2. That OutOfHandleApeMemory is the right code for never-constructed as
    opposed to exhausted, or whether a distinct code is preferred.
  3. Whether the better fix is upstream of this: making heap initialisation
    propagate the failure instead of swallowing it. That is a separate and
    larger change, and is an open ask on the linked ticket. This guard is
    defence in depth and does not substitute for it.

🤖 Generated with Claude Code

Heap initialisation tolerates a failed handle aperture reservation by
logging a warning and continuing, which leaves the handle aperture
manager unset. The allocate and free helpers dereference that manager
unconditionally, so a tolerated initialisation failure turns into a
null dereference inside the VA manager at the first use.

Return the existing out-of-handle-aperture error when the manager was
never constructed, and make the free path a no-op in that case. This
matches the guard already used on the system heap path in the same
file.

Reachable on the WSL DXG path from the physical-handle branch of GPU
memory initialisation and from the matching free. Configurations where
the reservation succeeds are unaffected, since the new branch is taken
only in a state that is otherwise undefined behaviour.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@therock-pr-bot

therock-pr-bot Bot commented Oct 8, 2026

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning 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/rocr-runtime/libhsakmt/src/dxg/openclose.cpp; no test file found
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

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

🙋 Wish to Override Policy?

Copilot AI 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.

🔵 Needs a closer look

The affected DXG path remains uncompiled and untested, requiring validation in a supported environment.

0 open findings

What changed in this PR

Prevents null-manager dereferences in ROCr’s WSL DXG handle-aperture path after a tolerated initialization failure.

Changes:

  • Returns OutOfHandleApeMemory and clears the output address when the manager is absent.
  • Makes freeing a handle-aperture address a no-op when the manager is absent.
File Description
projects/​rocr-runtime/​libhsakmt/​src/​dxg/​openclose.cpp Adds null guards to handle-aperture allocation and freeing.

🧠 Review effort: Balanced


Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.

@shwetagkhatri

Copy link
Copy Markdown
Contributor Author

Follow-up from ROCR_TRIAGE_AGENT on the uncompiled status, with a specific
reason rather than just a caveat.

I set up a local ROCr build to try to close this gap and confirmed the path
cannot be compiled on Linux at all: in libhsakmt/CMakeLists.txt the entire
src/dxg/*.cpp source set, including the file this PR touches, sits inside an
if (WIN32) block. So no Linux build and no Linux CI job will compile this
change, and a green pipeline here should not be read as coverage of it.

That means validation needs a Windows or WSL DXG build specifically. I do not
have one, so I cannot move this past "reviewed by reading". The change is two
null guards that mirror the already-merged guards on the system-heap path in the
same file, so the shape is conventional, but shape is not compilation.

For contrast, and so the difference in confidence is visible: the companion
change I raised today in #13085 is on a Linux-buildable path and does compile
clean with no new diagnostics. This one I cannot make that claim about.

@cfreeamd cfreeamd 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.

🤖 AI-assisted pre-review (for @cfreeamd)

libhsakmt/src/dxg/memory.cpp — null guards for handle aperture allocation are correct; return value and zeroing behavior mirror the existing allocation-failure path.

@shwetagkhatri
shwetagkhatri merged commit fab8420 into develop Oct 27, 2026
212 of 227 checks passed
@shwetagkhatri
shwetagkhatri deleted the users/shwetak/dxg-handle-aperture-null-guard branch October 27, 2026 17:05
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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