Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crash owing to improperly releasing memory. #7809

Closed
jacobbethany opened this issue Jul 22, 2024 · 3 comments
Closed

Crash owing to improperly releasing memory. #7809

jacobbethany opened this issue Jul 22, 2024 · 3 comments

Comments

@jacobbethany
Copy link

Version/Branch of Dear ImGui:

Newest docking branch

Back-ends:

imgui_impl_sdl3.cpp

Compiler, OS:

Tested on both Windows 10 MSVC Community 2022 and CLion with Msys2/Mingw/Ucrt64

Full config/build information:

Irrelevant

Details:

SDL3 has updated its API. When querying for gamepads, the memory is now automatically freed by SDL. Calling SDL_Free on the memory returned results in undefined behavior (and for me crashes).

See the documentation:
https://wiki.libsdl.org/SDL3/SDL_GetGamepads

I suggest replacing SDL_Free with SDL_ClaimTemporaryMemory or allowing it to be freed automatically when the SDL context is destroyed. (Locally, I used SDL_ClaimTemporaryMemory, until a fix is made to the repository and things are working, again.)

See:
imgui_impl_sdl3 ->
ImGui_ImplSDL3_UpdateGamepads -->
SDL_free ( sdl_gamepads );

vs SDL_ClaimTemporaryMemory ( sdl_gamepads );

PS:
The memory returned is now const T* instead of T*, thus const SDL_JoystickID *sdl_gamepads should be used, instead to avoid compiler warnings.

PPS:
SDL_GetDisplays now returns "const SDL_DisplayID *" instead of "SDL_DisplayID."

PPPS:
I want to say that I really appreciate the time and effort that goes into working on ImGUI! Hopefully, this will prevent someone else from chasing this bug.

Screenshots/Video:

No response

Minimal, Complete and Verifiable Example code:

Not necessary.

@ocornut
Copy link
Owner

ocornut commented Jul 22, 2024

Hello,
The gamepad part have already been fixed in master today:
eb72b5a (#7807)
I merged to docking now and corrected the SDL_DisplayID change.
It's all due to SDL3 which is currently unstable and fast changing API, so I am surprised so many people are using it already.

@ocornut ocornut closed this as completed Jul 22, 2024
@ocornut
Copy link
Owner

ocornut commented Jul 22, 2024

Thank you for reporting!

@jacobbethany
Copy link
Author

jacobbethany commented Jul 22, 2024

You're welcome for the reporting! It's the least I can do, if I'm getting any benefit from the project. I'm sorry for not noticing that it had already been fixed in the master branch. I can imagine that it must be somewhat frustrating to keep everything updated with SDL3's changes, right now. I was hoping to help with that.

I actually have another issue that I'm looking into right now, but I'm holding off until I have more information about a potential cause/fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants