We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 5041 - Memory leak in minimal example program caused by SDL_ClearError
Summary: Memory leak in minimal example program caused by SDL_ClearError
Status: ASSIGNED
Alias: None
Product: SDL
Classification: Unclassified
Component: thread (show other bugs)
Version: 2.0.10
Hardware: All All
: P2 minor
Assignee: Ryan C. Gordon
QA Contact: Sam Lantinga
URL:
Keywords: target-2.0.16
: 2771 3317 4055 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-03-17 08:55 UTC by sdlbugs
Modified: 2020-07-16 18:29 UTC (History)
5 users (show)

See Also:

Attachments
Diff for fixing leak in SDL2-2.0.12 (2.33 KB, patch)
2020-03-17 08:55 UTC, sdlbugs 		Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description sdlbugs 2020-03-17 08:55:39 UTC 
Created attachment 4264 [details]
Diff for fixing leak in SDL2-2.0.12

Code to cause leak:

#include "SDL.h"

int main()
{
    SDL_SetMainReady();
    SDL_Init(0);
    SDL_Quit();

    return 0;
}

I use Visual Leak Detector (VLD) on Windows and it spots that SDL_ClearError does some malloc-without-free. This will happen on all OSes however.

The fix (patch attached) is to make SDL_Quit() call SDL_TLSCleanup(). 

I have added an 'extern' declaration for SDL_TLSCleanup() directly in SDL_Quit() rather than expose it in a header, because if a user tries to call it at any other time it'll cause mayhem and almost certainly crashes (cf. SDL_ExitProcess() which is also extern-only).

Bug was noticed in 2.0.10 (and was probably present earlier) and still there in 2.0.12, but I cannot enter that as a version in the reporting version field. 

Note - I believe this leak has also been reported in several other guises, but I can't be sure. This report is for a clean, multi-platform repro case and fix. Please revisit related bugs to see if it fixes them too.

https://bugzilla.libsdl.org/show_bug.cgi?id=3317
https://bugzilla.libsdl.org/show_bug.cgi?id=4055
https://bugzilla.libsdl.org/show_bug.cgi?id=2771
Comment 1 Ryan C. Gordon 2020-03-22 18:52:35 UTC 
So the concern is that we can't clean up the main thread's TLS data because it has to live outside of SDL_Init/SDL_Quit, as you can have an SDL error outside of a successful init; if nothing else, SDL_Init can fail and this code pattern is considered legal:

    if (SDL_Init(SDL_INIT_VIDEO) == -1) {
        // SDL_GetError() uses TLS internally to store an error per-thread.
        printf("SDL_Init failed! reason: %s\n", SDL_GetError());
    }


I suppose that SDL_Quit() could clear the main thread's TLS, if:

- we know SDL_Quit is being called on the main thread, because other threads will clear their TLS on termination. Even if we don't have a system API to decide if we're on the main thread, we _could_ hook into SDL_CreateThread's startup code to mark a thread as not-the-main-one. This will cause problems if someone spins a thread through a system API directly instead of SDL and then calls SDL_Quit, but that's an unlikely case that we can say they shouldn't do.

- If the app does something that generates an SDL error after Quit, or even calls SDL_GetError(), it's going to regenerate the TLS data, but that's now an application bug instead of an SDL bug.

I don't know the full ramifications of doing this, as I'm not super familiar with the TLS code. I'm just talking out loud, but this bug report does pop up with some frequency and I'd like to prevent that as much as the memory leak.  :)

--ryan.
Comment 2 sdlbugs 2020-03-23 12:19:55 UTC 
Could have an SDL_Cleanup() function that clears it?
Comment 3 Sam Lantinga 2020-03-23 18:55:17 UTC 
(In reply to Ryan C. Gordon from comment #1)
> So the concern is that we can't clean up the main thread's TLS data because
> it has to live outside of SDL_Init/SDL_Quit, as you can have an SDL error
> outside of a successful init; if nothing else, SDL_Init can fail and this
> code pattern is considered legal:
> 
>     if (SDL_Init(SDL_INIT_VIDEO) == -1) {
>         // SDL_GetError() uses TLS internally to store an error per-thread.
>         printf("SDL_Init failed! reason: %s\n", SDL_GetError());
>     }
> 
> 
> I suppose that SDL_Quit() could clear the main thread's TLS, if:
> 
> - we know SDL_Quit is being called on the main thread, because other threads
> will clear their TLS on termination. Even if we don't have a system API to
> decide if we're on the main thread, we _could_ hook into SDL_CreateThread's
> startup code to mark a thread as not-the-main-one. This will cause problems
> if someone spins a thread through a system API directly instead of SDL and
> then calls SDL_Quit, but that's an unlikely case that we can say they
> shouldn't do.
> 
> - If the app does something that generates an SDL error after Quit, or even
> calls SDL_GetError(), it's going to regenerate the TLS data, but that's now
> an application bug instead of an SDL bug.
> 
> I don't know the full ramifications of doing this, as I'm not super familiar
> with the TLS code. I'm just talking out loud, but this bug report does pop
> up with some frequency and I'd like to prevent that as much as the memory
> leak.  :)
> 
> --ryan.

Yes, this sounds like a good approach.
Comment 4 Ryan C. Gordon 2020-03-25 00:24:40 UTC 
*** Bug 3317 has been marked as a duplicate of this bug. ***
Comment 5 Ryan C. Gordon 2020-03-25 00:25:06 UTC 
*** Bug 4055 has been marked as a duplicate of this bug. ***
Comment 6 Ryan C. Gordon 2020-03-25 00:25:29 UTC 
*** Bug 2771 has been marked as a duplicate of this bug. ***