SDL3: SDL_GetJoystickInstanceName string ownership

[icculus]

We talked about this at one point and never came to a satisfying resolution on the matter, but just want to make sure we settle this before we lock down the SDL3 API.

This comment in SDL_GetJoystickInstanceName() ...

    /* FIXME: Really we should reference count this name so it doesn't go away after unlock */

I'm doing the same thing in the audio subsystem rewrite (SDL_GetAudioDeviceName), but instead of returning a const char *, I'm currently returning a char *, and SDL_strdup'ing the name before returning it.

This requires the app to call SDL_free() on the results, and risks out-of-memory conditions in a pathological case, but it's clear they own the returned pointer and it won't go away without warning if someone unplugs a joystick.

This feels like the most expedient approach; I can't think of a more reasonable way to protect these strings; even if we say "you should make a copy right away" it's possible the device is getting unplugged between SDL_GetJoystickInstanceName's return and the app immediately making a copy. But if there's a better option here (App must hold a lock? Strings are promised to live until SDL_PumpEvents or SDL_Quit is called?), I'm up for anything.

[slouken]

This seems like a reasonable approach. APIs that previously returned const char *, always returned strduped memory and should be freed by the caller. There's no confusion about the memory lifetime, and it's clear and consistent.

A few downsides:

• Applications which aren't aware of this will suddenly start leaking memory like crazy
• You can't use == to compare previous and current pointers to see if names are the same. Applications shouldn't be doing this, but if they are, it used to work and now it doesn't.
• What do we do in sdl2-compat?

Should we change the name of the functions to indicate that returned memory should be freed? That seems like overkill, but would help the transition from SDL2.

[icculus]

Applications which aren't aware of this will suddenly start leaking memory like crazy

If they're querying the device name every frame, I feel like maybe they deserve it. :)

I do think this is a possibility, but it's trivial enough to fix in a codebase without any serious redesign. If you aren't saving the pointer, add an SDL_free after the API call. If you are, add an SDL_free when tearing down the data structure later.

I know that's totally hand-wavey of me, but it doesn't seem like much to ask of an app.

You can't use == to compare previous and current pointers to see if names are the same. Applications shouldn't be doing this, but if they are, it used to work and now it doesn't.

A device name wouldn't ever change after creation, right? Why would they be comparing pointers?

Should we change the name of the functions to indicate that returned memory should be freed?

I'm inclined to say no, because it's probably going to look awkward. Now, changing a name just so they're forced to notice there's been a change isn't the worst idea, but I'd also hate to give up a nice concise name just for this purpose, too.

What do we do in sdl2-compat?

On device add events: add an object to a list that has the SDL device IDs and a copy of their associated strings. When the app asks for these strings: return the internal copy in that list. On device remove events: free the strings and remove the object from the list. SDL2 apps are in the same position as before: the string can become invalid without warning.

[slouken]

Let's add a task for this and change it uniformly across the SDL 3.0 API.

[AntTheAlchemist]

I use SDL_GetGamepadInstanceName (14 references) and SDL_GetJoystickInstanceName (12 references) heavily in my Game Controller Tester app. Having to use SDL_free() wouldn't be the end of the world and it's my responsibility understand the function description properly. I don't thrash these functions - the name gets copied into a larger string that gets rendered as text every frame, so that's fine. Ideally I'd like the string to be cached SDL3-side at least until I handle the next event. Worst case for me is having to add 26 SDL_free()s.

Just my wee input. Don't rename anything. Don't worry about leaky memory - just make it clear when SDL_free is needed.

[AntTheAlchemist]

Also, forgot to mention, I use SDL_SetMemoryFunctions() at the start of my app and globally handle any out of memory situations - which is virtually impossible anyway. So that's not a concern for me personally. None of this would really affect a well developed app.

[flibitijibibo]

To add to this, wrappers around managed languages already inspect the returned pointers and free when it's non-const memory anyhow, so for #6337 this won't be an issue.

[slouken]

Hey... maybe we can use SDL_AllocateEventMemory() for these?

[slouken]

This is fixed in https://github.com/libsdl-org/SDL/commit/e23257307e220c7dd3d827e195f52adaabaaeeeb