Skip to content

Reject zero-size operation name buffers#4973

Open
fallintoplace wants to merge 1 commit into
ROCm:developfrom
fallintoplace:fix-operation-name-zero-size
Open

Reject zero-size operation name buffers#4973
fallintoplace wants to merge 1 commit into
ROCm:developfrom
fallintoplace:fix-operation-name-zero-size

Conversation

@fallintoplace

Copy link
Copy Markdown

Summary

migraphx_operation_name accepted a non-null output pointer with out_size == 0, then computed out_size - 1 before copying the operation name. That underflowed the limit used by std::copy_n and could write past the caller's buffer.

This change rejects zero-size output buffers with migraphx_status_bad_param, updates the API generator so regenerated C API output keeps the same guard, and extends the C API operation-construction test to cover null output, zero size, truncation, and normal success.

Tests

  • git diff --check
  • python3 -m py_compile tools/api.py src/api/migraphx.py
  • python3 tools/api.py src/api/migraphx.py tools/api/api.cpp | rg -n "migraphx_operation_name|Bad parameter out|Bad parameter out_size|copy_n" -C 3
  • cc -fsyntax-only -I <temporary export-header include> -I src/api/include test/api/test_c_op_construct.c

Not run: full CMake/API test target. Local configure stops before target generation because ROCmCMakeBuildTools is not installed on this machine.

@fallintoplace fallintoplace requested a review from causten as a code owner June 16, 2026 18:05
@github-actions

Copy link
Copy Markdown
Contributor

Thank you for your contribution! Since this is an external pull request, a maintainer must review PR and add the "ok-to-test" label if it is approved for testing.

@causten causten requested a review from pfultz2 June 16, 2026 18:37
Comment thread tools/api.py
p.add_param(t)
p.add_param('size_t', p.name + '_size')
p.bad_param('${name} == nullptr', 'Null pointer')
p.bad_param('${name}_size == 0', 'zero', '${name}_size')

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

An empty string could be a valid input perhaps not in this case, but this makes it always a bad param everytime we take a string as input.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah wait, nevermind, this is for the output parameter not the input.

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4973   +/-   ##
========================================
  Coverage    92.73%   92.74%           
========================================
  Files          592      592           
  Lines        31289    31291    +2     
========================================
+ Hits         29015    29018    +3     
+ Misses        2274     2273    -1     
Files with missing lines Coverage Δ
src/api/api.cpp 74.00% <100.00%> (+0.11%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants