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

Integer overflow in skip(s::IOBuffer, typemax(Int64)) can cause seg fault #53908

Closed
nhz2 opened this issue Mar 31, 2024 · 3 comments · Fixed by #54070
Closed

Integer overflow in skip(s::IOBuffer, typemax(Int64)) can cause seg fault #53908

nhz2 opened this issue Mar 31, 2024 · 3 comments · Fixed by #54070

Comments

@nhz2
Copy link
Contributor

nhz2 commented Mar 31, 2024

Here is a MWE:

julia> s = IOBuffer()
IOBuffer(data=UInt8[...], readable=true, writable=true, seekable=true, append=false, size=0, maxsize=Inf, ptr=1, mark=-1)

julia> skip(s, typemax(Int64))
IOBuffer(data=UInt8[...], readable=true, writable=true, seekable=true, append=false, size=0, maxsize=Inf, ptr=-9223372036854775808, mark=-1)

julia> position(s)
9223372036854775807

julia> read(s, UInt8)

[807576] signal 11 (128): Segmentation fault
in expression starting at REPL[10]:1
getindex at ./essentials.jl:375 [inlined]
read at ./iobuffer.jl:240
unknown function (ip: 0x775177259d32)
...

This happens on nightly as well as 1.10.2, both installed using juliaup.

julia> versioninfo()
Julia Version 1.12.0-DEV.274
Commit a3f710e2a35 (2024-03-31 01:56 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
@giordano
Copy link
Contributor

giordano commented Apr 1, 2024

% julia-master --check-bounds=yes -q
julia> s = IOBuffer()
IOBuffer(data=UInt8[...], readable=true, writable=true, seekable=true, append=false, size=0, maxsize=Inf, ptr=1, mark=-1)

julia> skip(s, typemax(Int64))
IOBuffer(data=UInt8[...], readable=true, writable=true, seekable=true, append=false, size=0, maxsize=Inf, ptr=-9223372036854775808, mark=-1)

julia> position(s)
9223372036854775807

julia> read(s, UInt8)
ERROR: BoundsError: attempt to access MemoryRef{UInt8} at index [-9223372036854775808]
Stacktrace:
 [1] getindex
   @ ./essentials.jl:375 [inlined]
 [2] read(from::IOBuffer, ::Type{UInt8})
   @ Base ./iobuffer.jl:240
 [3] top-level scope
   @ REPL[4]:1

The culprit is an unsafe use of @inbounds:

@inbounds byte = from.data[ptr]::UInt8

@giordano
Copy link
Contributor

giordano commented Apr 1, 2024

I think there's some logic to handle overflows around

julia/base/iobuffer.jl

Lines 262 to 264 in e9d25ca

function skip(io::GenericIOBuffer, n::Integer)
seekto = io.ptr + n
n < 0 && return seek(io, seekto-1) # Does error checking
but this comment inside seek

julia/base/iobuffer.jl

Lines 274 to 277 in e9d25ca

# TODO: REPL.jl relies on the fact that this does not throw (by seeking past the beginning or end
# of an GenericIOBuffer), so that would need to be fixed in order to throw an error here
#(n < 0 || n > io.size) && throw(ArgumentError("Attempted to seek outside IOBuffer boundaries."))
#io.ptr = n+1
may suggest that this is actually not happening?

@nhz2
Copy link
Contributor Author

nhz2 commented Apr 12, 2024

I think some of the modular additions should be changed to saturating additions.
clamp(widen(x) + widen(y), Int) seems to optimize to %2 = call i64 @llvm.sadd.sat.i64(i64 %0, i64 %1)

KristofferC pushed a commit that referenced this issue Apr 17, 2024
Fixes #53908  by clamping before doing addition.

This also fixes an issue with negative skips if `io.offset` isn't zero.

I am assuming that `io.size+1` cannot overflow.
KristofferC pushed a commit that referenced this issue Apr 25, 2024
Fixes #53908  by clamping before doing addition.

This also fixes an issue with negative skips if `io.offset` isn't zero.

I am assuming that `io.size+1` cannot overflow.

(cherry picked from commit 306124c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants