Unsafe use of low-level pointer operations

[maleadt]

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

FITSIO.jl/src/table.jl

Lines 292 to 330 in 69b9e77

	ttype = [pointer(name) for name in colnames]
	# determine which columns are requested to be variable-length
	isvarcol = zeros(Bool, ncols)
	if !isa(varcols, Nothing)
	for i=1:ncols
	isvarcol[i] = (i in varcols) || (colnames[i] in varcols)
	end
	end
	# create an array of tform strings (which we will create pointers to)
	tform_str = Vector{String}(undef, ncols)
	for i in 1:ncols
	if isvarcol[i]
	tform_str[i] = fits_tform_v(hdutype, coldata[i])
	else
	tform_str[i] = fits_tform(hdutype, coldata[i])
	end
	end
	tform = [pointer(s) for s in tform_str]
	# get units
	if isa(units, Nothing)
	tunit = C_NULL
	else
	tunit = Ptr{UInt8}[(haskey(units, n) ? pointer(units[n]) : C_NULL)
	for n in colnames]
	end
	# extension name
	name_ptr = (isa(name, Nothing) ? Ptr{UInt8}(C_NULL) :
	pointer(name))
	status = Ref{Cint}(0)
	ccall(("ffcrtb", libcfitsio), Cint,
	(Ptr{Cvoid}, Cint, Int64, Cint, Ptr{Ptr{UInt8}}, Ptr{Ptr{UInt8}},
	Ptr{Ptr{UInt8}}, Ptr{UInt8}, Ref{Cint}),
	f.fitsfile.ptr, table_type_code(hdutype), 0, ncols, # 0 = nrows
	ttype, tform, tunit, name_ptr, status)

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 JuliaLang/julia#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]

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

• FITSIO.jl/src/table.jl

  Line 178 in 69b9e77

  colnames = [unsafe_string(pointer(item)) for item in colnames_in]

• FITSIO.jl/src/table.jl

  Line 179 in 69b9e77

  coltforms = [unsafe_string(pointer(item)) for item in coltforms_in]

• FITSIO.jl/src/table.jl

  Line 265 in 69b9e77

  buffer[] = pointer(data[i])

• FITSIO.jl/src/table.jl

  Line 292 in 69b9e77

  ttype = [pointer(name) for name in colnames]

• FITSIO.jl/src/table.jl

  Line 311 in 69b9e77

  tform = [pointer(s) for s in tform_str]

• FITSIO.jl/src/table.jl

  Line 317 in 69b9e77

  tunit = Ptr{UInt8}[(haskey(units, n) ? pointer(units[n]) : C_NULL)

• FITSIO.jl/src/table.jl

  Line 322 in 69b9e77

  name_ptr = (isa(name, Nothing) ? Ptr{UInt8}(C_NULL) :

• FITSIO.jl/src/table.jl

  Line 445 in 69b9e77

  bufptr[] = pointer(buffer)

header.jl

• FITSIO.jl/src/header.jl

  Line 108 in 69b9e77

  return try_parse_hdrval(T, unsafe_string(pointer(value)))

• Three lines in read_header

  FITSIO.jl/src/header.jl

  Line 318 in 69b9e77

  keys[i] = unsafe_string(pointer(key))

[sefffal]

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]

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:

1. check wheher the pointer is valid (non-NULL);
2. 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.

[sefffal]

This has become somewhat critical now that 1.11 is released, and FITSIO is still segfaulting. I don't have the time at the moment to track this down. Would be great if someone was willing to step up and tackle this!

[cgarling]

I would like to help but I do not know the best practices surrounding calling external libraries.

It looks like your PR #196 fixes some of these issues but it is unclear what else needs to be addressed. Do you know what else remains to be done?