search

nihui / waifu2x-ncnn-vulkan

waifu2x converter ncnn version, runs fast on intel / amd / nvidia / apple-silicon GPU with vulkan
MIT License
2.99k stars 205 forks source link

Fix double free in main.cpp's save #152

Closed utybo closed 3 years ago

utybo commented 3 years ago

I should preface this by saying that I do not know if this is the right solution. This is probably hacky, as I do not know why a free was done there in the first place.

This is a follow-up to this comment I made on #138.

This should fix #138 and fix #140.

utybo commented 3 years ago

@nihui Could you please give the workflow an approval? I'd like to know if this breaks the CI or not.

ArchieMeng commented 3 years ago

@utybo I am not sure, but I feel like you might just omit the release of ncnn::Mat or the image memory space. And I am afraid this may leads to memory leak issues. Additionally, I remember ncnn had fixed the problem in the recently commits. Could you try update ncnn to the latest version and build again to see if the problem still exists? Because I don't have the problem you have and I have no way to see if this works.

BTW, for CI, you can enable the Github action in your fork repo.

utybo commented 3 years ago

@ArchieMeng I can still repro using the latest version of ncnn (tag 20210507). The output is the same as my initial comment

I am not sure, but I feel like you might just omit the release of ncnn::Mat or the image memory space. And I am afraid this may leads to memory leak issues.

To be honest, I don't really know what the loop does in the first place, but from what I understand, the freeing is unecessary as it is already done by getting out of the scope where the Task variable is declared (which is a value and not a pointer). The (implicit) destructor of Task automatically calls ncnn::Mat's destructor, which in turns calls ncnn::Mat::release which then does the problematic free for us. stbi_image_free is a macro which ends up leading to a free, so all branches of both if/elses (the one in code and the one using the preprocessor) end up doing the same thing, which is freeing v.inimage.data.

TL;DR: the code frees an internal pointer of ncnn::Mat which is already destroyed by Task's destructor, which calls ncnn::Mat's destructor, which takes care of that internal pointer. The same thing happens on every branch of the problematic code section.

nihui commented 3 years ago

fixed in https://github.com/nihui/waifu2x-ncnn-vulkan/commit/f332f3c256d3a7f9b7c18a23a05a4e402ea9fd18