Automatic cleanup after 1 second in SDL_FreeLater can easily cause use after free bugs

[Susko3]

The idea of SDL_FreeLater and its automatic cleanup raises (at least to me) immediate concern about memory bugs. The 1 second rule seems fine at first glance, the memory is called temporary and nobody would do expensive, lengthy processing with it. But the problem is that the 1 second is wall clock time. There are many things that can pause a thread for more then one second:

• SDL_Delay(1200) (this is unrealistic, but makes use after free bugs easily reproducible)
• hitting a debugger breakpoint
• putting the PC or console to sleep
• suspending the game on a console
• blocking calls to system window manager functions (e.g. when resizing a window on Windows)

Demonstration

The last one can be easily demonstrated with checkkeys on Windows:

1. Apply the following diff

   diff --git a/test/checkkeys.c b/test/checkkeys.c
   index 36aa88d43..2dd86a536 100644
   --- a/test/checkkeys.c
   +++ b/test/checkkeys.c
   @@ -357,7 +357,6 @@ static void loop(void)
            switch (event.type) {
            case SDL_EVENT_KEY_DOWN:
            case SDL_EVENT_KEY_UP:
   -            PrintKey(&event.key);
                if (event.type == SDL_EVENT_KEY_DOWN) {
                    switch (event.key.key) {
                    case SDLK_BACKSPACE:
   @@ -402,6 +401,7 @@ static void loop(void)
                break;
            }
            case SDL_EVENT_TEXT_INPUT:
   +            SDL_AllocateTemporaryMemory(1); // force a collect
                PrintText("INPUT", event.text.text);
                SDLTest_TextWindowAddText(GetTextWindowForWindowID(event.text.windowID), "%s", event.text.text);
                UpdateTextWindowInputRect(event.text.windowID);

2. Build and run checkkeys.exe --resizable

3. At the same time, enter some text and start resizing the window (enter the text just before you start resizing)

4. Keep resizing until 1 second elapses

5. Notice that the resulting text contains garbage data, indicative of a use after free bug

The buggy text consists of \xdd\xdd\xdd\xdd\xdd\xdd on debug builds, which indicates freed memory.

2024-07-26.02-01-04.mp4

The bug happens because the text string is temporarily allocated before the resize starts blocking. By the time the event is handled, 1 second would have already elapsed and the next call to SDL_CollectTemporaryMemory will free it, leaving a dangling pointer.

The workaround for the bug is to call SDL_ClaimTemporaryMemory as soon as possible, but that defeats the purpose of the temporary memory.

---

I started looking into this issue when SDL3-CS was changed to no longer free certain arrays to account for the change in #10311.

Please try to use a better heuristic for automatic cleanup. Or better yet, don't add functionality that, by definition, introduces use after free bugs. (I don't mind calling SDL_free on a returned array :)

[slouken]

I clarified that temporary memory doesn't last past the current event being handled or the current function scope:

SDL/include/SDL3/SDL_events.h

Lines 1437 to 1440 in a1a8278

	* Some functions return temporary memory which SDL will automatically clean
	* up. If you want to hold onto it past the current event being handled or
	* beyond the current function scope, you can call this function to get a
	* pointer that you own, and can free using SDL_free() when you're done.

You can always call free on the returned array. If you want, you can do this:

const void *data = SDL_SomeFunction();
if (data) {
    ...
    SDL_free(SDL_ClaimTemporaryMemory(data));
}

That will transfer ownership of the memory to you, and you have full control over when you want to free it.

[slouken]

Ah, I see that your repro case forces collection while the current event is still being processed. Hmmmmmmm

[Kaktus514]

Can't a context switch happen at any time? In that case I don't see how you can ever guarantee that 1 second has not passed from one instruction to the next which makes the "1 second cleanup rule" pretty useless.

It's not clear why "the current function scope" is relevant. You can run a lot of code that takes a lot of time and calls a lot of functions in the current function scope.

I feel it would almost be better to just let the caller free the memory. It increases the risk of memory leaks but those are pretty harmless, and easy to track down, compared to potential use-after-free bugs that only happen occasionally.

Or have the memory freed automatically next time the same (or a related) function gets called (e.g. memory for an event could be freed when you request the next event). That's more deterministic and easier to understand than having to guess what "it's freed later" means. Right now it feels like I would always want to use SDL_ClaimTemporaryMemory, just in case.

[slouken]

Yep, this is all good feedback, thanks!

We're trying to balance making the API easy to use and giving the application control over memory usage.

[slouken]

We've change it so SDL no longer automatically frees temporary memory, instead you can call SDL_FreeTemporaryMemory() whenever you want. In addition, functions that return temporary memory are marked up with SDL_DECLSPEC_TEMP, so it's easy to see where you might want to claim memory.

@icculus, note that SDL_DECLSPEC_TEMP becomes part of the ABI.