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

zero-argument getindex method in Base incorrectly assumes 1-based indexing #39379

Closed
yurivish opened this issue Jan 24, 2021 · 5 comments · Fixed by #39393
Closed

zero-argument getindex method in Base incorrectly assumes 1-based indexing #39379

yurivish opened this issue Jan 24, 2021 · 5 comments · Fixed by #39393
Labels
arrays [a, r, r, a, y, s]

Comments

@yurivish
Copy link
Contributor

It seems that the bounds check used in Base for zero-dimensional indexing hardcodes a comparison to 1, assuming one-based indexing.

This means that an array with nonstandard axes can successfully pass the bounds check and then access out-of-bounds memory when indexing. I encountered this in an issue on OffsetArrays and @jishnub tracked it down to this line in Base:

checkbounds_indices(::Type{Bool}, IA::Tuple, ::Tuple{}) = (@_inline_meta; all(x->unsafe_length(x)==1, IA))

@jishnub
Copy link
Contributor

jishnub commented Jan 25, 2021

The issue here is that A[] gets mapped to A[1], and linear indexing for AbstractVectors should use their axes to obtain bounds on permissible indices. A 1-element OffsetVector may have an axis n:n for an arbitrary n, in which case 1 is not a valid index. Perhaps the best solution is to add a special method checkbounds_indices(::Type{Bool}, IA::Tuple{Any}, ::Tuple{}) that ensures that IA[1] begins at 1? This is the solution chosen in the fix in OffsetArrays.

@JeffBezanson JeffBezanson added the arrays [a, r, r, a, y, s] label Jan 25, 2021
@mbauman
Copy link
Member

mbauman commented Jan 25, 2021

No, that's working correctly. That method gets hit if you index into a non-zero-dimensional array with zero indices. It asserts that all axes have length 1.

The problem is not in the bounds checking, but rather in the getindex.

@mbauman
Copy link
Member

mbauman commented Jan 25, 2021

Here's the root problem:

_to_linear_index(A::AbstractArray) = 1

Edit: This should probably be first(eachindex(A)) or somesuch.

@yurivish yurivish changed the title Bounds check in Base incorrectly assumes 1-based indexing zero-argument getindex method in Base incorrectly assumes 1-based indexing Jan 25, 2021
@yurivish
Copy link
Contributor Author

yurivish commented Jan 25, 2021

Thanks for debugging this further @mbauman. I think my original issue title is incorrect; I've changed it to what I think is an appropriate description. Feel free to change if there's a better or more correct title.

@jishnub
Copy link
Contributor

jishnub commented Jan 26, 2021

This is great! I wonder if a similar fix might be applied to #37274?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants