Incorrect @inbounds annotation in Base.last can result in out-of-bounds memory accesses

[yurivish]

The end keyword in array indexing expressions is lowered to lastindex. The implementation of last in Base calls lastindex inside an @inbounds expression:

julia/base/abstractarray.jl

Lines 452 to 456 in 6dfa690

	# Faster method for vectors
	function first(v::AbstractVector, n::Integer)
	n < 0 && throw(ArgumentError("Number of elements must be nonnegative"))
	@inbounds v[begin:min(begin + n - 1, end)]
	end

This use of @inbounds goes against the guidance in the @inbounds documentation to only use it when you can be sure from locally-available information that all array accesses are in bounds. In this case, since the array is only known to be a general subtype of AbstractVector and the index value comes from a potentially incorrectly-implemented function, this annotation escalates a buggy lastindex implementation into an out-of-bounds memory access.

Here is a simple example using OffsetArrays and a patched lastindex method, though the same will work for any array with an incorrect implementation of lastindex:

julia> using OffsetArrays

julia> a = OffsetArray([1, 2, 3], -1:1);

julia> Base.lastindex(a::OffsetArray) = 100;

julia> a[end] # no bug with plain indexing
ERROR: BoundsError: attempt to access 3-element OffsetArray(::Vector{Int64}, -1:1) with eltype Int64 with indices -1:1 at index [100]
Stacktrace:
 [1] throw_boundserror(A::OffsetVector{Int64, Vector{Int64}}, I::Tuple{Int64})
   @ Base ./abstractarray.jl:651
 [2] checkbounds
   @ ./abstractarray.jl:616 [inlined]
 [3] getindex(A::OffsetVector{Int64, Vector{Int64}}, i::Int64)
   @ OffsetArrays ~/.julia/packages/OffsetArrays/2Z6ls/src/OffsetArrays.jl:408
 [4] top-level scope
   @ REPL[9]:1

julia> last(a, 1) # silent misbehavior with `last`
1-element Vector{Int64}:
 4680496544

[Moelf]

Here is a simple example using OffsetArrays and a patched lastindex method, though the same will work for any array with an incorrect implementation of lastindex:

whoever owns the T <: AbstractArray (in your example T === OffsetArray) has the responsibility to implement lastindex() correctly, if you're looking for an evidence this is the case, I guess: https://docs.julialang.org/en/v1/manual/interfaces/#Indexing

if someone has a bug in lastindex there's nothing we can do, I mean, what if they have a bugged getindex() ?

[jakobnissen]

It's true that this is against the stated policy of

Only use @inbounds when it is certain from the information locally available that all accesses are in bounds.

That stated policy is generally not adhered to. For example, in the docstring of @inbounds, it relies on eachindex begin implemented correctly, which is not local information.
I've been looking through a few issues and can't find any other decision on this, so it looks like the policy stands. In that case, we might have a lot of methods to go through.

[jakobnissen]

Grepping through Base, it is full of methods that use @inbounds on abstract types. Quite often it's not clear whether it's safe or not, because it's some internal method where there it takes time to tell whether bounds have already been checked in another function.
It might make sense to make an effort to go through Base and stdlibs and review every use of @inbounds: Remove if not safe, and add a comment why is safe when it is.

[vchuravy]

Also with #42573 @inbounds should be less necessary.

[PallHaraldsson]

Grepping through Base, it is full of methods that use @inbounds on abstract types. Quite often it's not clear whether it's safe

If now "less necessary", sometimes unsafe, and I've seen talk about @inbounds (or was it just the global --check-bounds=no ?) going away, and actually slower sometimes, how about just dropping all of it? At least to see in a benchmark where it hurts?

[jishnub]

The @inbounds annotation has been removed now, so this specific issue should be resolved.

julia/base/abstractarray.jl

Lines 554 to 557 in 5f4dec1

	function last(v::AbstractVector, n::Integer)
	n < 0 && throw(ArgumentError("Number of elements must be non-negative"))
	v[range(stop=lastindex(v), length=min(n, checked_length(v)))]
	end