search

veandco / go-sdl2

SDL2 binding for Go
https://godoc.org/github.com/veandco/go-sdl2
BSD 3-Clause "New" or "Revised" License
2.17k stars 219 forks source link

Windows release crashing likely because C.CString deallocated with C.SDL_free #582

Closed jabolopes closed 6 months ago

jabolopes commented 7 months ago

Go version: go version go1.21.2 linux/amd64 Go-SDL2 version: master (today's latest commit) SDL2 version: 2.0.20 OS: Windows 10 (cross-compiling from Linux) Architecture: 64bit

Hi,

I have been working on a game for a while now and on Linux it's been working great.

More recently, I have been trying to make a Windows build. I cross compile from Linux to Windows following the instructions provided on the README.md. Unfortunately, the Windows version of the game crashes on startup very frequently.

Here is a screenshot with the stack trace of the crash:

screenshot

The stack trace shows that the crash occurs in the call to SDL_free of the sdl.SetHint function.

What is interesting about this code location is that there is a C.CString allocation being deallocated by a C.SDL_free.

The cgo documentation states that C.CString should be deallocated with C.free.

In this project, sometimes a C.CString is deallocated by C.SDL_free and sometimes by C.free. Perhaps on Linux the C.SDL_free is actually the same as C.free and therefore programs are not crashing, but it's possible that on Windows this is not the case.

I patched the sdl.SetHint function in my program to use C.free instead of C.SDL_free, made a new Windows build, and so far I haven not been able to reproduce the crash.

Based on the above, I suspect that on Windows the SDL_free is corrupting the program by trying to deallocate memory that was allocated by C.CString.

Even if my suspicion is wrong it might be safer to just stick to the documentation of C.CString and use C.free to deallocate the strings in all circumstances just to be sure there is no memory corruption.

If y'all agree with that assessment, then the fix would be review all calls to C.CString and make sure that they are being deallocated with C.free instead of C.SDL_free.

Any thoughts?

Thank you, Jose

veeableful commented 7 months ago

Hi @jabolopes, thanks for the reporting this issue! I think you are right that we should replace C.SDL_free() with C.free() in the C.CString() cases which I have done so in a new commit in the master branch. Could you please try again?

jabolopes commented 7 months ago

Hi @veeableful, thanks a lot for the quick reply. I took a look at the commit and it looks great!

I suspect there a few more call sites that need to be fixed. I will try to send a PR for those.

Thanks, Jose

jabolopes commented 6 months ago

Hi @veeableful, now that the fixes were submitted I am no longer able to reproduce the crash, so I think we can mark this issue as fixed.

Thanks, Jose