Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce size of ArrayView returned by subspan #1001

Merged
merged 4 commits into from
Jan 19, 2023

Conversation

gunney1
Copy link
Contributor

@gunney1 gunney1 commented Jan 18, 2023

The over-large size was allowing access of potentially invalid memory by the ArrayView. This can happen in the object returned by ArrayView::subspan.

  • This PR is a bugfix
  • It does the following:
    • Reduce the size of the new ArrayView by the offset amount, to make it NOT okay to access data past the end of the original view.
    • Make the returned view consistent with the doxygen comments.
    • Add unit test on the range of the created ArrayView.

I added the unit test because subspan isn't used anywhere in the library. This change may affect application code that makes use of the buggy feature. The change is indicated in the release notes.

The size was allowing access of potentially invalid memory.
@gunney1 gunney1 added bug Something isn't working Core Issues related to Axom's 'core' component Documentation Issues related to documentation labels Jan 18, 2023
@gunney1 gunney1 self-assigned this Jan 18, 2023
Copy link
Member

@rhornung67 rhornung67 left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you.

Copy link
Member

@agcapps agcapps left a comment

Choose a reason for hiding this comment

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

Thanks, Brian.

Copy link
Member

@kennyweiss kennyweiss left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @gunney1

Since you're already touching this function, it might be good to further improve the range checks and tests.

src/axom/core/ArrayView.hpp Outdated Show resolved Hide resolved
src/axom/core/tests/core_array.hpp Show resolved Hide resolved
src/axom/core/tests/core_array.hpp Show resolved Hide resolved
@gunney1 gunney1 merged commit 1f86e24 into develop Jan 19, 2023
@gunney1 gunney1 deleted the bugfix/gunney/array-view-subspan-range branch January 19, 2023 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Core Issues related to Axom's 'core' component Documentation Issues related to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants