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

Backends: SDL3: Fix gamepad memory leak #7898

Closed
wants to merge 1 commit into from
Closed

Conversation

cheyao
Copy link
Contributor

@cheyao cheyao commented Aug 18, 2024

The pointer returned by SDL_GetGamepads should be freed (See: https://wiki.libsdl.org/SDL3/SDL_GetGamepads)

Leak Sanitizer log:

Direct leak of 4 byte(s) in 1 object(s) allocated from:
    #0 0x10479f848 in malloc+0x58 (libclang_rt.lsan_osx_dynamic.dylib:x86_64h+0x33848)
    #1 0x1028887a4 in real_malloc+0x14 (libSDL3.0.dylib:x86_64+0x1037a4)
    #2 0x1028889fb in SDL_malloc_REAL+0x2b (libSDL3.0.dylib:x86_64+0x1039fb)
    #3 0x1027ff5ab in SDL_GetJoysticks_REAL+0x6b (libSDL3.0.dylib:x86_64+0x7a5ab)
    #4 0x1027f9312 in SDL_GetGamepads_REAL+0x22 (libSDL3.0.dylib:x86_64+0x74312)
    #5 0x1027bae08 in SDL_GetGamepads+0x18 (libSDL3.0.dylib:x86_64+0x35e08)
    #6 0x101fee382 in ImGui_ImplSDL3_UpdateGamepads() imgui_impl_sdl3.cpp:643
    #7 0x101fee067 in ImGui_ImplSDL3_NewFrame() imgui_impl_sdl3.cpp:727
    #8 0x101d04c9d in Game::gui() game.cpp:243
    #9 0x101d04afa in Game::iterate() game.cpp:157
    #10 0x101d03114 in SDL_AppIterate main.cpp:79
    #11 0x10280e107 in SDL_IterateMainCallbacks+0x47 (libSDL3.0.dylib:x86_64+0x89107)
    #12 0x102a1efb0 in SDL_EnterAppMainCallbacks_REAL+0xa0 (libSDL3.0.dylib:x86_64+0x299fb0)
    #13 0x1027b972e in SDL_EnterAppMainCallbacks+0x3e (libSDL3.0.dylib:x86_64+0x3472e)
    #14 0x101d02de8 in SDL_main SDL_main_impl.h:59
    #15 0x10280e350 in SDL_RunApp_REAL+0x40 (libSDL3.0.dylib:x86_64+0x89350)
    #16 0x1027d0a03 in SDL_RunApp_DEFAULT+0x33 (libSDL3.0.dylib:x86_64+0x4ba03)
    #17 0x1027beb9e in SDL_RunApp+0x2e (libSDL3.0.dylib:x86_64+0x39b9e)
    #18 0x101d0343e in main SDL_main_impl.h:208
    #19 0x7fff20471f3c in start+0x0 (libdyld.dylib:x86_64+0x15f3c)
its<char>, std::__1::allocator<char> > const&>, std::__1::tuple<> >(std::__1::basic_string<char, std

PS. Maybe the pointer should be cast to a non-const pointer when invoking SDL_free?

@ocornut
Copy link
Owner

ocornut commented Aug 19, 2024

This seems to undo #7807 and #7809 (commit eb72b5a)
Also see libsdl-org/SDL@4961af4

It's very confusing because:

  • SDL3 is clearly not up to date.
  • We previously made the change linked above but the documentations talks about SDL_GetJoysticks(), NOT SDL_GetGamepads(), they never explicitly state how to handle SDL_GetGamespads() in docs/README-migration.md

But the code is calling SDL_GetJoysticks() and reusing that memory:

SDL_JoystickID *SDL_GetGamepads(int *count)
{
    int num_joysticks = 0;
    int num_gamepads = 0;
    SDL_JoystickID *joysticks = SDL_GetJoysticks(&num_joysticks);
    if (joysticks) {
        int i;
        for (i = num_joysticks - 1; i >= 0; --i) {
            if (SDL_IsGamepad(joysticks[i])) {
                ++num_gamepads;
            } else {
                SDL_memmove(&joysticks[i], &joysticks[i+1], (num_gamepads + 1) * sizeof(joysticks[i]));
            }
        }
    }
    if (count) {
        *count = num_gamepads;
    }
    return joysticks;
}

So I think you have an old version of SDL3 and need to update to latest. SDL3 is currently changing very fast.

@cheyao
Copy link
Contributor Author

cheyao commented Aug 19, 2024

Oh ok! Sorry for the false commit!

@ocornut
Copy link
Owner

ocornut commented Sep 3, 2024

You were right as SDL3 actually reverted their logic on July 27, so actually I was the one not up to date here and needing to revert the imgui backend side change! We're pushing this now.

ocornut pushed a commit that referenced this pull request Sep 3, 2024
@ocornut
Copy link
Owner

ocornut commented Sep 3, 2024

Merged as 6a73195. Thank you!

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

Successfully merging this pull request may close these issues.

2 participants