Unsafe use of low-level pointer operations

maleadt commented 5 months ago

Many routines in this package derive pointers from managed objects (by calling the pointer methods), without explicitly making sure that the objects pointed to are kept alive (using GC.@preserve).

Example: there's nothing keeping colnames alive until the call to ffcrtb https://github.com/JuliaAstro/FITSIO.jl/blob/69b9e77012a80da92b57d8a3f16acf339d52815b/src/table.jl#L292-L330

Most of the time, it suffices to make sure to:

buf = Vector{...}(...)
GC.@preserve buf begin
  ptr = pointer(buf)
  # only use ptr here
end

However, I also see ccalls into libcfitsio that leak those pointers to C, so care should be taken that those pointers are not stored over there (if they are, keeping buf alive usingGC.@preserve may not suffice, and you may have to root the object by storing it in an object that's guaranteed to be kept alive for the duration of the pointer being known to the C library).

This is very likely the cause for the segfaults seen on PkgEval, as reported in https://github.com/JuliaLang/julia/issues/52951, so for the time being I'll blacklist daily testing of FITSIO.jl and OIFITS.jl. Feel free to ping me or revert the blacklist PR tagged below when the issue is fixed.

sefffal commented 2 months ago

I'll start gathering info for what needs to be fixed. The majority are all in the table handling code.

Here is a list of pointer operations in the package:

table.jl

header.jl

[ ] https://github.com/JuliaAstro/FITSIO.jl/blob/69b9e77012a80da92b57d8a3f16acf339d52815b/src/header.jl#L108
[ ] Three lines in read_header https://github.com/JuliaAstro/FITSIO.jl/blob/69b9e77012a80da92b57d8a3f16acf339d52815b/src/header.jl#L318

sefffal commented 2 months ago

Looking at the unsafe operations in header.jl, we have

key = Vector{UInt8}(undef, 81)
# ...
str =  unsafe_string(pointer(key))

the same pattern is used 3 times. I think this can be replaced just by String(key), so those are easy to fix.

emmt commented 1 month ago

Hello,

What I have done in the EasyFITS package is to extend Base.unsafe_convert in a spectial way (see here) so that each time a fitsfile pointer is required by a cfitsio function, it is sufficient to directly pass the Julia structure owning this pointer (an instance of EasyFITS.FitsFile) to ccall or @ccall to automatically:

check wheher the pointer is valid (non-NULL);
have the structure owning the pointer be preverved from being garbage collected (this is with no extra efforts due to the default behavior of Base.cconvert, note the double cc); Except when opening or closing (explictly or when structure is finalized), I never directly deal with the fitsfile pointer in the code.

I hope this may help to solve the issue in a simple way.

JuliaAstro / FITSIO.jl

Unsafe use of low-level pointer operations #194