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

segfault when reading from mmap derived from a file that get closed(?) #54128

Closed
KristofferC opened this issue Apr 18, 2024 · 4 comments · Fixed by #54210
Closed

segfault when reading from mmap derived from a file that get closed(?) #54128

KristofferC opened this issue Apr 18, 2024 · 4 comments · Fixed by #54210
Labels
domain:filesystem Underlying file system and functions that use it
Milestone

Comments

@KristofferC
Copy link
Sponsor Member

KristofferC commented Apr 18, 2024

Vague title because I am not really sure what is going on.

Slightly reduced from https://s3.amazonaws.com/julialang-reports/nanosoldier/pkgeval/by_hash/c356e60_vs_bd47eca/LasIO.primary.log

Pasting the following into the REPL segfaults:

using LasIO
using Mmap
using Downloads

testfile = Downloads.download("https://github.com/visr/LasIO.jl/raw/d39266994f60213ce373ed7d30b449d5400dc6c5/test/libLAS_1.2.las")

io = open(testfile)
header, pointdata = try
    read(io, UInt32)
    header = read(io, LasHeader)

    n = header.records_count
    pointtype = LasIO.pointformat(header)

    pointsize = Int(header.data_record_length)
    pointbytes = Mmap.mmap(io, Vector{UInt8}, n*pointsize, position(io))
    pointdata = LasIO.PointVector{pointtype}(pointbytes, pointsize)
    header, pointdata
finally
    close(io)
end

pointdata
[41248] signal 11 (2): Segmentation fault: 11
in expression starting at none:0
unsafe_load at ./pointer.jl:153 [inlined]
unsafe_load at ./pointer.jl:153 [inlined]
peek at ./iobuffer.jl:205 [inlined]
read at ./iobuffer.jl:211 [inlined]
read at ./none:0 [inlined]
getindex at /Users/kristoffercarlsson/PkgEvalAnalysis/dev/LasIO.jl/src/point.jl:37
isassigned at ./multidimensional.jl:1601 [inlined]
isassigned at ./multidimensional.jl:1596
unknown function (ip: 0x11150c153)
alignment at ./arrayshow.jl:68
_print_matrix at ./arrayshow.jl:207
print_matrix at ./arrayshow.jl:171
print_matrix at ./arrayshow.jl:171 [inlined]
print_array at ./arrayshow.jl:358 [inlined]
show at ./arrayshow.jl:399
unknown function (ip: 0x11145c6ab)

Note that pointdata contains an IOBuffer wrapping the input pointbytes (https://github.com/visr/LasIO.jl/blob/0a9f58d78c5a68da42b467275cdeca83cc0c7450/src/point.jl#L15-L20)

Things that make it "work":

  • adding a error() call above the pointdata printing
  • Calling the close(io) outside the finally
  • Not using mmap and instead using pointbytes = read(io, n*pointsize)

Any idea @vtjnash?

@KristofferC KristofferC added this to the 1.11 milestone Apr 18, 2024
@KristofferC KristofferC added the domain:filesystem Underlying file system and functions that use it label Apr 18, 2024
@KristofferC KristofferC changed the title segfault when reading from mmap derived from a closed file(?) segfault when reading from mmap derived from a file that get closed(?) Apr 18, 2024
@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 22, 2024

I suspect Mmap is putting a finalizer on the wrong object (the Array) now that we have the Memory abstraction. The IOBuffer discards the unnecessary indirection, resulting in the Memory getting unmapped. Not using Mmap is also usually a good idea, as it doesn't suffer from this problem and generally performs better in well-written code (which is admittedly non-trivial to do). But we should fix Mmap to add the finalizer to the right object in v1.11

@KristofferC
Copy link
Sponsor Member Author

So is that just changing the finalizer from being on the returned array A to be on A.ref.mem?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 22, 2024

Yes

@oscardssmith
Copy link
Member

Does this mean that the code prior to 1.11 could also segfault if the user push!ed to the array causing it to change where it was storing the data?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:filesystem Underlying file system and functions that use it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants