Skip to content

Conversation

@shadidashmiz
Copy link
Contributor

Motivation

Technical Details

JIRA ID

Test Plan

Test Result

Submission Checklist

@shadidashmiz shadidashmiz requested a review from a team as a code owner January 30, 2026 19:56
Copilot AI review requested due to automatic review settings January 30, 2026 19:56
@shadidashmiz shadidashmiz requested a review from a team as a code owner January 30, 2026 19:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts HIP no-GPU tests and improves robustness around user object reference handling and fat binary loading, with an additional guard intended to avoid buffer over-reads when inspecting code object magic headers.

Changes:

  • Fix NoGpuTst_hipExtStreamGetCUMask to pass a properly sized CU mask array to hipExtStreamGetCUMask instead of a single uint32_t.
  • Reorder validation in hipUserObjectRelease so that user object validity is checked before accessing its reference count, avoiding potential dereference of invalid objects.
  • Add a minimum-size check before interpreting image_ in FatBinaryInfo::ExtractFatBinaryUsingCOMGR, though the current implementation incorrectly uses strlen on a binary buffer.

Reviewed changes

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

File Description
projects/hip-tests/catch/multiproc/hipNoGpuTsts.cc Updates the no-GPU test for hipExtStreamGetCUMask to allocate a CU mask array of 16 uint32_t entries and pass it correctly to the API.
projects/clr/hipamd/src/hip_graph.cpp Adjusts the condition in hipUserObjectRelease to validate the user object before comparing referenceCount() against count, reducing the risk of accessing invalid objects.
projects/clr/hipamd/src/hip_fatbin.cpp Introduces a guard on image_ size before magic string checks in ExtractFatBinaryUsingCOMGR, but currently relies on strlen over a binary buffer, which can itself cause out-of-bounds reads and needs to be reworked to use a real size value.

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

Comment on lines 482 to 483
size_t image_len = strlen(reinterpret_cast<const char*>(image_));
if (image_len < kMinMagicSize) {
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

image_ represents an arbitrary binary blob (mapped file or in-memory image), so calling strlen(reinterpret_cast<const char*>(image_)) is undefined behavior: the buffer is not guaranteed to be NUL-terminated and may contain embedded NUL bytes, which can cause out-of-bounds reads or incorrect lengths. To safely enforce the minimum size for the magic string comparisons, use a known size for the mapped region (e.g., ufd_->fsize_ when image_mapped_ is true, or an explicit size tracked alongside image_ for in-memory images) instead of strlen on binary data, and compare that size against kMinMagicSize before calling memcmp-based helpers.

Suggested change
size_t image_len = strlen(reinterpret_cast<const char*>(image_));
if (image_len < kMinMagicSize) {
size_t image_len = 0;
if (image_mapped_ && ufd_ != nullptr) {
image_len = ufd_->fsize_;
}
if (image_len != 0 && image_len < kMinMagicSize) {

Copilot uses AI. Check for mistakes.
@shadidashmiz shadidashmiz force-pushed the amd/sdashmiz/nullstring branch from d59820e to 44323ab Compare January 30, 2026 20:38
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.

2 participants