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 4673

Summary: [patch] fix two memory leaks in SDL
Product: SDL Reporter: Dmitry Gapkalov <gapkalov>
Component: threadAssignee: Ryan C. Gordon <icculus>
Status: RESOLVED WONTFIX QA Contact: Sam Lantinga <slouken>
Severity: major    
Priority: P2 CC: bugzilla
Version: 2.0.9Keywords: target-2.0.12
Hardware: x86   
OS: Windows 10   
Attachments: patch

Description Dmitry Gapkalov 2019-06-18 16:53:59 UTC
Created attachment 3830 [details]
patch

this patch fixes two memory leaks in SDL_envmem and SDL_TLSData
1) leak SDL_envmem always present in Win32
2) second, presented on all platform if SDL created, using and destroyed on another thread instead of main
Comment 1 Sam Lantinga 2019-06-18 21:18:18 UTC
These are potentially allocated before SDL_Init() and should survive SDL_Init()/SDL_Quit() cycle.

Ryan, what are your thoughts?
Comment 2 Ryan C. Gordon 2019-06-18 23:46:16 UTC
Ugh, yeah, I think those probably need to live forever. I'll have to look more closely, though.

It's conceivable that we could keep an atomic counter of threads that set TLS data, and if it drops to one, we can clean out the TLS stuff, though, for a clean shutdown. Maybe.

--ryan.
Comment 3 Ryan C. Gordon 2019-07-30 17:49:35 UTC
(Sorry if you get several emails like this, we're marking a bunch of bugs.)

We're hoping to ship SDL 2.0.11 on a much shorter timeframe than we have historically done releases, so I'm starting to tag bugs we hope to have closed in this release cycle.

Note that this tag means we just intend to scrutinize this bug for the 2.0.11 release: we may fix it, reject it, or even push it back to a later release for now, but this helps give us both a goal and a wishlist for the next release.

If this bug has been quiet for a few months and you have new information (such as, "this is definitely still broken" or "this got fixed at some point"), please feel free to retest and/or add more notes to the bug.

--ryan.
Comment 4 Ryan C. Gordon 2019-09-20 20:47:39 UTC
We're changing how we do SDL release versions; now releases will be even numbers (2.0.10, 2.0.12, etc), and as soon as we tag a release, we'll move the internal version number to an odd number (2.0.12 ships, we tag the latest in revision control as 2.0.13 immediately, which will become 2.0.14 on release, etc).

As such, I'm moving the bugs tagged with target-2.0.11 to target 2.0.12. Sorry if you get a lot of email from this change!

Thanks,
--ryan.
Comment 5 Ryan C. Gordon 2019-09-20 20:48:41 UTC
We're changing how we do SDL release versions; now releases will be even numbers (2.0.10, 2.0.12, etc), and as soon as we tag a release, we'll move the internal version number to an odd number (2.0.12 ships, we tag the latest in revision control as 2.0.13 immediately, which will become 2.0.14 on release, etc).

As such, I'm moving the bugs tagged with target-2.0.11 to target 2.0.12. Sorry if you get a lot of email from this change!

Thanks,
--ryan.
Comment 6 Sam Lantinga 2019-09-22 17:54:39 UTC
*** Bug 4718 has been marked as a duplicate of this bug. ***
Comment 7 Ryan C. Gordon 2019-10-15 14:10:49 UTC
(In reply to Ryan C. Gordon from comment #2)
> It's conceivable that we could keep an atomic counter of threads that set
> TLS data, and if it drops to one, we can clean out the TLS stuff, though,
> for a clean shutdown. Maybe.

Looking at this closer, I see this was about freeing the main thread's TLS data and not the global TLS structures, and that's a non-starter for the reasons Sam mentioned. Likewise for the environment variable buffer.

I'm going to close this as WONTFIX for now, but I think in 2.1 we should be genuinely adamant that _no_ SDL call works outside of an SDL_Init/SDL_Quit pair, or at least that a successful SDL_Quit() always cleans everything to a default state with biblical vengeance.

--ryan.
Comment 8 Dmitry Gapkalov 2019-10-15 14:59:10 UTC
SDL has two memory leaks, 
https://bugzilla.libsdl.org/show_bug.cgi?id=4718 also report it.

I provide you a patch with fix, why you igonre it and close with want fix?
Comment 9 Dmitry Gapkalov 2019-10-15 15:01:08 UTC
SDL has two memory leaks, 
https://bugzilla.libsdl.org/show_bug.cgi?id=4718 also report it.

I provide you a patch with fix, why you igonre it and close with want fix?
Comment 10 Dmitry Gapkalov 2019-10-15 15:03:56 UTC
guy from stack overflow also reported it 2 years ago
https://stackoverflow.com/a/26964538/6855178
Comment 11 Ryan C. Gordon 2019-10-15 15:41:05 UTC
(In reply to Dmitry Gapkalov from comment #10)
> guy from stack overflow also reported it 2 years ago
> https://stackoverflow.com/a/26964538/6855178

This memory can not be freed in SDL_Quit(), as it needs to exist outside of the range of an SDL_Init/SDL_Quit pair. It's a design flaw we can revisit in 2.1, but it has to remain for now.

--ryan.