Memory unsafety after deepcopy of plan

[kbarros]

The following will segfault on my machine:

using FFTW

function make_plan()
    plan = FFTW.plan_fft(zeros(10))
    return deepcopy(plan)
end

plan = make_plan()
GC.gc() # presumably C datastructures are freed in this step, even though they remain live
plan * ones(10) # segfault

Since FFTW plans are immutable, a quick fix might be to have deepcopy return the original plan,

Base.deepcopy(plan::FFTW.cFFTWPlan) = plan
# ... repeat for all FFTW plan types

Alternatively, attempting to deepcopy an FFTW plan could throw an error explaining that deepcopy cannot be used in conjunction with bindings to C libraries.

Tested on FFTW v1.5.0 with Julia 1.9 beta 4:

julia> versioninfo()
Julia Version 1.9.0-beta4
Commit b75ddb787ff (2023-02-07 21:53 UTC)
Platform Info:
  OS: macOS (arm64-apple-darwin21.5.0)
  CPU: 8 × Apple M1 Pro
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, apple-m1)
  Threads: 1 on 6 virtual cores

[stevengj]

Yeah, I would just return the original plan object? (Except that the plans aren't quite immutable, they include e.g. a pinv field which is mutated to cache the inverse plan if it is needed? Still, they act immutable, I guess.)

See also FFTW/fftw3#106 which would be required for a "true" deepcopy.

[kbarros]

Could the mutable pinv field create issues with thread safety if the "copied" plan is attempted to be used simultaneously on another thread?

[stevengj]

Yes, seems like there could be a race condition if two threads try to update pinv simultaneously? Though I guess we could add a lock.

[kbarros]

I think I agree that the easy thing for now is to fix the crash by adding the methods:

Base.deepcopy(plan::FFTW.cFFTWPlan) = plan
# ...

Later you could consider adding a lock, or a "true" deepcopy into FFTW itself, to provide thread safety.

[kbarros]

After pondering this a bit more, perhaps implementing deepcopy as a shallow copy is not great idea. Mainly, there are the potential thread safety issues. But also, deepcopy is a surprisingly dangerous function, especially in conjunction with C libraries, and there is no warning about this in the Julia docs. An error along the lines "deepcopy not implemented for FFTW plans" is factually correct (for the moment) and could also be helpful for keeping people within 'safe Julia'.

[kbarros]

Now that FFTW/fftw3#106 is closed, is it possible to add deepcopy in a thread-safe way? The motivation would be sharing of plans across threads.

[kbarros]

Gentle ping. Can Base.deepcopy(plan::FFTW.cFFTWPlan) and friends be implemented now that fftw_clone is available? At the moment, calling Julia's deepcopy function on an FFTW plan will create nondeterministic memory corruption (triggered upon garbage collection).

[stevengj]

We need to build a new FFTW release.

[kbarros]

Gentle ping -- allowing (broken) deepcopy on an FFTW plan seems like a dangerous footgun to leave around.

[KristofferC]

I think this is not too unexpected. You really shouldn't use deepcopy and in combination with C interop it's very much asking for trouble. What's the reason for calling deepcopy, what property from it do you want?