PyORBIT-Collaboration / py-orbit

Core of Py-ORBIT code
MIT License
25 stars 38 forks source link

Impedances causing segfault #56

Open azukov opened 3 years ago

azukov commented 3 years ago

Following function iterates over all elements of input sequence coming from python and puts all elements of the sequence into _zImped_n memory buffer allocated with new https://github.com/PyORBIT-Collaboration/py-orbit/blob/7916ee32bd92c554e386a3ea81c49e9df67e2a63/src/spacecharge/wrap_lspacechargecalc.cc#L63

https://github.com/PyORBIT-Collaboration/py-orbit/blob/7916ee32bd92c554e386a3ea81c49e9df67e2a63/src/spacecharge/LSpaceChargeCalc.cc#L43 If the length of the sequence is greater than nBins/2 - 1 then some elements are written out of buffer region. This happens because assignImpedanceValue function offsets incoming data by one https://github.com/PyORBIT-Collaboration/py-orbit/blob/7916ee32bd92c554e386a3ea81c49e9df67e2a63/src/spacecharge/LSpaceChargeCalc.cc#L83 It is probably done to match indexing of fftw library calls or coming from FORTRAN? Anyway it causes segfault during program completion (when destructor is called).

So the python code below segfaults because it has 32 impedances and 64 longitudinal slices

from spacecharge import LSpaceChargeCalc
from orbit.space_charge.sc1d import SC1D_AccNode

b_a = 10.0 / 3.0
length = 248.0
use_spacecharge = 1
min_n_macros = 1000
n_long_slices = 64
position = 124.0
Z = 32 * [complex(0., 0.)]
sc_node_long = SC1D_AccNode(b_a, length, min_n_macros, use_spacecharge, n_long_slices)
sc_node_long.assignImpedance(Z)

Same situation happens in LImpedance class https://github.com/PyORBIT-Collaboration/py-orbit/blob/7916ee32bd92c554e386a3ea81c49e9df67e2a63/src/orbit/Impedances/LImpedance.cc#L81

A workaround would be increasing buffer size by one, or removing increment in assignImpedanceValue. Consequences of either workaround are unclear. So one needs to fully test Fourier transform procedure to make sure that result is correct. Also a simple check would be nice to prevent calling assignImpedanceValue with unacceptably long sequence of impedances.

azukov commented 1 year ago

77 addresses this temporarily.