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

hvncat miscomputes output bounds for empty Vectors #41047

Closed
mfalt opened this issue Jun 1, 2021 · 10 comments
Closed

hvncat miscomputes output bounds for empty Vectors #41047

mfalt opened this issue Jun 1, 2021 · 10 comments
Labels
arrays [a, r, r, a, y, s] bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@mfalt
Copy link
Contributor

mfalt commented Jun 1, 2021

On: Version 1.7.0-DEV.1222 (2021-06-01)
Commit 4a8572f

Tried to figure out the new syntax for hvncat and ran into this bug:

julia> [[];;7;8]
2×2 Matrix{Any}:
 1  #undef
 2  #undef

Not sure what the expected behavior is, but since [7;8;;[]] throws an error, I assume that some boundscheck is missing.

This seems to be an almost equivalent call

 julia> hvncat(((1,2),(3,)), false, zeros(Int,0,), 7, 8)
2×2 Matrix{Int64}:
 7  0
 8  0

Trying to debug, these works as expected:

julia> hvncat(((1,2),(3,)), false, zeros(Int,2,0), 7.0, 8.0)
2×1 Matrix{Float64}:
 7.0
 8.0
julia> hvncat(((1,2),(3,)), false, zeros(Int, 2,), 7, 8)
2×2 Matrix{Int64}:
 0  7
 0  8

However, you can quickly cause a segfault:

julia> hvncat(((1,2),(3,)), false, zeros(Int,0,0,0), 7, 8)
Trace:
julia> hvncat(((1,2),(3,)), false, zeros(Int,0,0,0), 7, 8)
signal (11): Segmentation fault
in expression starting at none:0
jl_gc_pool_alloc at /work/mattiasf/juliagit_dev/src/gc.c:1211
jl_gc_alloc_ at /work/mattiasf/juliagit_dev/src/julia_internal.h:315 [inlined]
jl_gc_alloc at /work/mattiasf/juliagit_dev/src/gc.c:3278
_new_array_ at /work/mattiasf/juliagit_dev/src/array.c:122 [inlined]
_new_array at /work/mattiasf/juliagit_dev/src/array.c:188 [inlined]
jl_alloc_array_1d at /work/mattiasf/juliagit_dev/src/array.c:459
Array at ./boot.jl:453 [inlined]
Array at ./boot.jl:472 [inlined]
getindex at ./array.jl:411 [inlined]
with_repl_linfo at /work/mattiasf/juliagit_dev/usr/share/julia/stdlib/v1.7/REPL/src/REPL.jl:503
_jl_invoke at /work/mattiasf/juliagit_dev/src/gf.c:2244 [inlined]
jl_apply_generic at /work/mattiasf/juliagit_dev/src/gf.c:2426
display at /work/mattiasf/juliagit_dev/usr/share/julia/stdlib/v1.7/REPL/src/REPL.jl:254
display at /work/mattiasf/juliagit_dev/usr/share/julia/stdlib/v1.7/REPL/src/REPL.jl:266
_jl_invoke at /work/mattiasf/juliagit_dev/src/gf.c:2244 [inlined]
jl_apply_generic at /work/mattiasf/juliagit_dev/src/gf.c:2426
display at ./multimedia.jl:328
_jl_invoke at /work/mattiasf/juliagit_dev/src/gf.c:2244 [inlined]
jl_apply_generic at /work/mattiasf/juliagit_dev/src/gf.c:2426
jl_apply at /work/mattiasf/juliagit_dev/src/julia.h:1760 [inlined]
jl_f__call_latest at /work/mattiasf/juliagit_dev/src/builtins.c:751
#invokelatest#2 at ./essentials.jl:726 [inlined]
invokelatest at ./essentials.jl:724 [inlined]
print_response at /work/mattiasf/juliagit_dev/usr/share/julia/stdlib/v1.7/REPL/src/REPL.jl:288
#45 at /work/mattiasf/juliagit_dev/usr/share/julia/stdlib/v1.7/REPL/src/REPL.jl:272
jfptr_YY.45_20661 at /work/mattiasf/juliagit_dev/usr/lib/julia/sys.so (unknown line)
_jl_invoke at /work/mattiasf/juliagit_dev/src/gf.c:2244 [inlined]
jl_apply_generic at /work/mattiasf/juliagit_dev/src/gf.c:2426
with_repl_linfo at /work/mattiasf/juliagit_dev/usr/share/julia/stdlib/v1.7/REPL/src/REPL.jl:505
_jl_invoke at /work/mattiasf/juliagit_dev/src/gf.c:2244 [inlined]
jl_apply_generic at /work/mattiasf/juliagit_dev/src/gf.c:2426
print_response at /work/mattiasf/juliagit_dev/usr/share/julia/stdlib/v1.7/REPL/src/REPL.jl:270
do_respond at /work/mattiasf/juliagit_dev/usr/share/julia/stdlib/v1.7/REPL/src/REPL.jl:841
jfptr_do_respond_20618 at /work/mattiasf/juliagit_dev/usr/lib/julia/sys.so (unknown line)
_jl_invoke at /work/mattiasf/juliagit_dev/src/gf.c:2244 [inlined]
jl_apply_generic at /work/mattiasf/juliagit_dev/src/gf.c:2426
jl_apply at /work/mattiasf/juliagit_dev/src/julia.h:1760 [inlined]
jl_f__call_latest at /work/mattiasf/juliagit_dev/src/builtins.c:751
#invokelatest#2 at ./essentials.jl:726 [inlined]
invokelatest at ./essentials.jl:724 [inlined]
run_interface at /work/mattiasf/juliagit_dev/usr/share/julia/stdlib/v1.7/REPL/src/LineEdit.jl:2493
jfptr_run_interface_20024 at /work/mattiasf/juliagit_dev/usr/lib/julia/sys.so (unknown line)
_jl_invoke at /work/mattiasf/juliagit_dev/src/gf.c:2244 [inlined]
jl_apply_generic at /work/mattiasf/juliagit_dev/src/gf.c:2426
run_frontend at /work/mattiasf/juliagit_dev/usr/share/julia/stdlib/v1.7/REPL/src/REPL.jl:1227
#49 at ./task.jl:411
jfptr_YY.49_20803 at /work/mattiasf/juliagit_dev/usr/lib/julia/sys.so (unknown line)
_jl_invoke at /work/mattiasf/juliagit_dev/src/gf.c:2244 [inlined]
jl_apply_generic at /work/mattiasf/juliagit_dev/src/gf.c:2426
jl_apply at /work/mattiasf/juliagit_dev/src/julia.h:1760 [inlined]
start_task at /work/mattiasf/juliagit_dev/src/task.c:822
Allocations: 10796169 (Pool: 10791934; Big: 4235); GC: 13
[1]    3143063 segmentation fault (core dumped)  ./julia

Edit: Bug generalizes to higher dims: [zeros(Int,2,0);;;7;8]

@simeonschaub simeonschaub added arrays [a, r, r, a, y, s] bug Indicates an unexpected problem or unintended behavior labels Jun 1, 2021
@simeonschaub simeonschaub added this to the 1.7 milestone Jun 1, 2021
@vtjnash vtjnash self-assigned this Jun 2, 2021
@JeffBezanson
Copy link
Member

With --check-bounds=yes:

julia> Base.hvncat(((1,2),(3,)), false, zeros(Int,0,0,0), 7, 8)
ERROR: BoundsError: attempt to access 2×1×0 Array{Int64, 3} at index [1]
Stacktrace:
 [1] setindex!
   @ ./array.jl:877 [inlined]
 [2] hvncat_fill!(A::Array{Int64, 3}, scratch1::Vector{Int64}, scratch2::Vector{Int64}, d1::Int64, d2::Int64, as::Tuple{Array{Int64, 3}, Int64, Int64})
   @ Base ./abstractarray.jl:2361
 [3] _typed_hvncat(::Type{Int64}, ::Tuple{Tuple{Int64, Int64}, Tuple{Int64}}, ::Bool, ::Array{Int64, 3}, ::Vararg{Any})
   @ Base ./abstractarray.jl:2339
 [4] _hvncat(::Tuple{Tuple{Int64, Int64}, Tuple{Int64}}, ::Bool, ::Array{Int64, 3}, ::Vararg{Any})
   @ Base ./abstractarray.jl:2108
 [5] hvncat(::Tuple{Tuple{Int64, Int64}, Tuple{Int64}}, ::Bool, ::Array{Int64, 3}, ::Vararg{Any})
   @ Base ./abstractarray.jl:2104
 [6] top-level scope
   @ REPL[1]:1

@vtjnash
Copy link
Member

vtjnash commented Jun 2, 2021

Looks like the new hvncat code is littered with @inbounds markers (half of which seem be on code that never even does indexing), yet it never bothers to make sure that those accesses are inbounds. This leads to arbitrary memory corruption, which leads to the crashes observed here. I'll make a PR to delete them, then someone can re-add corrected ones more methodically.

vtjnash added a commit that referenced this issue Jun 2, 2021
There are no boundschecks so these lead to problems such as #41047.
vtjnash added a commit that referenced this issue Jun 2, 2021
There are no boundschecks so these lead to problems such as #41047.
vtjnash added a commit that referenced this issue Jun 2, 2021
There are no boundschecks so these lead to problems such as #41047.
@vtjnash vtjnash removed their assignment Jun 2, 2021
vtjnash added a commit that referenced this issue Jun 3, 2021
There are no boundschecks so these lead to problems such as #41047.
@vtjnash vtjnash changed the title hvncat segfault and bug hvncat miscomputes output bounds for empty Vectors Jun 3, 2021
@vtjnash vtjnash removed this from the 1.7 milestone Jun 3, 2021
@BioTurboNick
Copy link
Contributor

BioTurboNick commented Jun 5, 2021

Just seeing this.

With respect to @inbounds I was experimenting and measuring what seemed to help performance; I know about lower functions being able to propagate inbounds, but I didn't bother looking to see if they all particularly did.

The loop discovering the array bounds is what (is supposed to) guarantee no out of bounds accesses, so I take issue with saying hvncat never bothers to check. But clearly, empty arrays break an assumption I was implicitly making that each item being concatenated has at least one element in it.

@BioTurboNick
Copy link
Contributor

BioTurboNick commented Jun 6, 2021

Turns out there were two separate issues with the shape variant of hvncat.

One was handling of zero-length arrays, leading to an underfilled array.

The second, which led to the segfault, was a dumb error on my part, where I did not expand the shape argument and check higher dimensions, to account for the case where one or more concatenated elements has ndims greater than the maximum concatenation dimension. Both are fixed in the linked PR.

@vtjnash
Copy link
Member

vtjnash commented Jun 8, 2021

@BioTurboNick to be clear, your PR and dedication are great and much appreciated! I'm sometimes annoyed at code, but not usually at the people behind it.

@BioTurboNick
Copy link
Contributor

Fair enough 😁

shirodkara pushed a commit to shirodkara/julia that referenced this issue Jun 9, 2021
There are no boundschecks so these lead to problems such as JuliaLang#41047.
@simeonschaub
Copy link
Member

@BioTurboNick Does one of your broken up PRs fix this issue? If so, would be good to link back to here.

@BioTurboNick
Copy link
Contributor

BioTurboNick commented Jun 11, 2021

Yes, partially. #41196 catches the one that leads to an underfilled array and works for dims and int cases, but I haven't found a general solution for detecting and erroring when improperly-shaped 0-size arrays are input for the shape form.

Currently, it treats them like they're nothing. e.g. [zeros(Int, 0,0,0) ;;; a b] == [a b;;;]. Which is not terrible but also not formally correct.

@simeonschaub simeonschaub added this to the 1.7 milestone Jun 11, 2021
johanmon pushed a commit to johanmon/julia that referenced this issue Jul 5, 2021
There are no boundschecks so these lead to problems such as JuliaLang#41047.
@JeffBezanson
Copy link
Member

I believe this is fixed by #41196; plz reopen if I'm mistaken.

vtjnash pushed a commit that referenced this issue Jul 15, 2021
@BioTurboNick
Copy link
Contributor

Yes. I'll open another issue for the zero-length array inputs.

KristofferC pushed a commit that referenced this issue Jul 19, 2021
KristofferC pushed a commit that referenced this issue Jul 20, 2021
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] bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants