Skip to content

Fix issue 8127 - #8247

Merged
esc merged 2 commits into
numba:mainfrom
bszollosinagy:fix_issue_8127
Aug 8, 2022
Merged

esc merged 2 commits into
numba:mainfrom
bszollosinagy:fix_issue_8127

Conversation

@bszollosinagy

Copy link
Copy Markdown
Contributor

Fixes an issue with bounds checking when using advanced indexing, as described in Issue 8127

Added a unit test to reproduce the issue. The unit test looks to see if IndexError assertion is raised or not. It fails without the modification in numba\np\arrayobj.py, and passes if you use the version in this pull request.

Apparently the issue only manifests itself if you try to assign to the indexed array, and that is why it was not visible with the existing unit test "test_fancy_indexing_boundscheck"

Fixes #8127

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

This looks good to me. Thank you so much for the contribution.

Comment thread numba/tests/test_boundscheck.py

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

Thanks for the patch @bszollosinagy. Thanks for reviewing @njriasan.

Not for this PR, but a general note, compile_isolated should not be used as it isn't isolated or particularly safe, it's better to just use @njit etc. Sometime in the future test_boundscheck.py needs a large scale refactor.

@stuartarchibald

Copy link
Copy Markdown
Contributor

Apparently the issue only manifests itself if you try to assign to the indexed array, and that is why it was not visible with the existing unit test "test_fancy_indexing_boundscheck"

Adding the boundscheck kwarg to get_item_pointer2 was done in the function fancy_setslice which implements "fancy" indexing whilst "setting" items, hence an assignment was needed to trigger this code path via __setitem__.

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 3 - Ready for Review labels Aug 8, 2022
@stuartarchibald stuartarchibald added this to the Numba 0.57 RC milestone Aug 8, 2022
@stuartarchibald stuartarchibald added the Effort - short Short size effort needed label Aug 8, 2022
@stuartarchibald stuartarchibald self-assigned this Aug 8, 2022
@esc
esc merged commit fc28b29 into numba:main Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

5 - Ready to merge Review and testing done, is ready to merge Effort - short Short size effort needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No out of bounds check during advanced array indexing

5 participants