DTolm / VkFFT

Vulkan/CUDA/HIP/OpenCL/Level Zero/Metal Fast Fourier Transform library
MIT License
1.52k stars 91 forks source link

Undefined behaviour when initializing plan #134

Closed DejvBayer closed 11 months ago

DejvBayer commented 1 year ago

Hi,

when function setConfigurationVkFFT is called from initializeVkFFT in module [vkFFT_InitializeApp.h] https://github.com/DTolm/VkFFT/blob/master/vkFFT/vkFFT/vkFFT_AppManagement/vkFFT_InitializeApp.h), there is line that should reset the app configuration, however it is commented out, so the configuration just works with some random values that lead to segfault. Shouldn't it be uncommented?

static inline VkFFTResult setConfigurationVkFFT(VkFFTApplication* app, VkFFTConfiguration inputLaunchConfiguration)  {
    VkFFTResult resFFT = VKFFT_SUCCESS;
    //app->configuration = {};// inputLaunchConfiguration;                              <- this line
    if (inputLaunchConfiguration.doublePrecision != 0)  app->configuration.doublePrecision = inputLaunchConfiguration.doublePrecision;
    if (inputLaunchConfiguration.doublePrecisionFloatMemory != 0)   app->configuration.doublePrecisionFloatMemory = inputLaunchConfiguration.doublePrecisionFloatMemory;

        /* ... */
}

Thanks.

Dave

DTolm commented 1 year ago

Hello,

Originally I thought the user should zero-initialize the app struct before initialization as to not allow potential memory leaks (for example, if initializeVkFFT is called twice on the same app without freeing resources first).

Now I see that it is possible to add a full app memset to zero in the deleteVkFFT call and add a check for app bytes being zero in the initializeVkFFT call. This should be a better approach compared to setting configuration to zero.

I will add this in the next update.

Best regards, Dmitrii

DejvBayer commented 1 year ago

Hi Dmitrii,

thank you for your reply. I am sorry, it was my mistake, I accidentally forgot to zero initialize the app object when I moved from OpenCL to CUDA. After zero initialization everything works fine.

Once again thank you very much.

Best regards, David

DTolm commented 1 year ago

Hello,

I have added the checks for the app being zero and freeing the resources on being deleted. It will be soon merged into the master branch, thanks for suggesting it!

Best regards, Dmitrii