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

Jump to bottom

Memory leak in minimal example program caused by SDL_ClearError #3596

Closed
SDLBugzilla opened this issue Feb 11, 2021 · 0 comments
Closed

Memory leak in minimal example program caused by SDL_ClearError #3596

SDLBugzilla opened this issue Feb 11, 2021 · 0 comments

Comments

@SDLBugzilla
Copy link
Collaborator

SDLBugzilla commented Feb 11, 2021 •
edited by icculus

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: 2.0.10
Reported for operating system, platform: All, All

Comments on the original bug report:

On 2020-03-17 08:55:39 +0000, wrote:

Created attachment 4264
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

On 2020-03-22 18:52:35 +0000, Ryan C. Gordon wrote:

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.

On 2020-03-23 12:19:55 +0000, wrote:

Could have an SDL_Cleanup() function that clears it?

On 2020-03-23 18:55:17 +0000, Sam Lantinga wrote:

(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.

On 2020-03-25 00:24:40 +0000, Ryan C. Gordon wrote:

*** Bug 3317 has been marked as a duplicate of this bug. ***

On 2020-03-25 00:25:06 +0000, Ryan C. Gordon wrote:

*** Bug 4055 has been marked as a duplicate of this bug. ***

On 2020-03-25 00:25:29 +0000, Ryan C. Gordon wrote:

*** Bug 2771 has been marked as a duplicate of this bug. ***
jixingcn added a commit to jixingcn/SDL that referenced this issue Mar 16, 2021
@jixingcn
Fix libsdl-org#3596, free all TLS data at the end of SDL_Quit to av…
ce00b4e 
…oid memory leak
@jixingcn jixingcn mentioned this issue Mar 16, 2021
Closed
jixingcn added a commit to jixingcn/SDL that referenced this issue Mar 22, 2021
@jixingcn
Revert "Fix libsdl-org#3596, free all TLS data at the end of `SDL_Qui…
28c2c05 
…t` to avoid memory leak"

This reverts commit ce00b4e.
jixingcn added a commit to jixingcn/SDL that referenced this issue Mar 22, 2021
@jixingcn
Fix libsdl-org#3596, allow to call the SDL_TLSCleanup to cleanup th…
de234f0 
…e TLS data after `SDL_Quit`
jixingcn added a commit to jixingcn/SDL that referenced this issue Mar 22, 2021
@jixingcn
Fix libsdl-org#3596, allow to call the SDL_TLSCleanup to cleanup th…
8df332e 
…e TLS data after `SDL_Quit`
@jixingcn jixingcn mentioned this issue Mar 22, 2021
Closed
jixingcn added a commit to jixingcn/SDL that referenced this issue Mar 22, 2021
@jixingcn
Squashed commit of the following:
3e79da1 
commit 5a56e5a8227d9cff6b497b681c618a76bec1cae1
Author: Xing Ji <jixingcn@gmail.com>
Date:   Mon Mar 22 23:55:10 2021 +0800

    Fix libsdl-org#3596, can call the `SDL_TLSCleanup` to cleanup the TLS data when closing the application
@jixingcn jixingcn mentioned this issue Mar 22, 2021
Closed
jixingcn added a commit to jixingcn/SDL that referenced this issue Mar 24, 2021
@jixingcn
Squashed commit of the following:
3f2a4e6 
commit 6b8f933589aa3925978a23e77a305a7e89c6ae4a
Author: Xing Ji <jixingcn@gmail.com>
Date:   Wed Mar 24 22:31:29 2021 +0800

    update the dynapi by `gendynapi.pl`

commit ebd1790c19983b652713f40ab1e139e485e1a2b7
Author: Xing Ji <jixingcn@gmail.com>
Date:   Wed Mar 24 22:17:48 2021 +0800

    revert the change in src/dynapi

commit 734b5f85c1613070081e39238e84198128971b53
Merge: 5a56e5a8 5ac6bd5
Author: Xing Ji <jixingcn@gmail.com>
Date:   Wed Mar 24 22:14:40 2021 +0800

    Merge remote-tracking branch 'libsdl/main' into jixingcn

commit 5ac6bd5
Author: vanfanel <redwindwanderer@gmail.com>
Date:   Wed Mar 24 02:54:36 2021 +0100

    [KMSDRM] Ask for videomode on the correct display when creating a window.

commit 7c08b04
Author: Ryan C. Gordon <icculus@icculus.org>
Date:   Tue Mar 23 15:36:12 2021 -0400

    headers: a few minor documentation corrections.

commit b55b11a
Author: Fabrice Fontaine <fontaine.fabrice@gmail.com>
Date:   Tue Mar 23 08:07:56 2021 +0100

    src/thread/pthread/SDL_systhread.c: drop include of SDL_platform.h

    Drop include of SDL_platform.h as SDL_plaform.h is already included by
    SDL_internal.h -> SDL_config.h -> SDL_platform.h

    Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>

commit 0bdf4f9
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:19:01 2021 -0700

    Disable system gestures on MFi controllers while they're open, so we get access to the back button, etc.

commit 1133ea0
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:18:57 2021 -0700

    Fixed crash on macOS when AirPods are connected

commit 38b61a3
Merge: 258b7bc c12f46b
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:40 2021 -0700

    Merge commit 'c12f46b100d22a0e06a64c5b6d1baa3f446d34e6' into main

commit 258b7bc
Merge: f82aa7f 100166d
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:38 2021 -0700

    Merge commit '100166d7d7b9ed2e486841498bbc585975630e02' into main

commit f82aa7f
Merge: 9332006 3f40396
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:36 2021 -0700

    Merge commit '3f40396d33df64326756648c3b8e1e6c922efe5a' into main

commit 9332006
Merge: 8a6810e 3c78c21
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:34 2021 -0700

    Merge commit '3c78c211d57de4e9d953bf71d49d2ee313bbff34' into main

commit 8a6810e
Merge: b0a047e 599edaa
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:33 2021 -0700

    Merge commit '599edaaf935aab69a13b5643566adc652a27e268' into main

commit b0a047e
Merge: de83222 1899844
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:31 2021 -0700

    Merge commit '1899844952756e932ee29e887501a9b9e39066a6' into main

commit de83222
Merge: 4b0b39a cf7eef3
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:29 2021 -0700

    Merge commit 'cf7eef37b045bb3f841e26879fdc6d865c8aaf9a' into main

commit 4b0b39a
Merge: f68ba3c 4acd1dc
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:28 2021 -0700

    Merge commit '4acd1dcad41d154093ca14eb0adf35f4f99bd06a' into main

commit f68ba3c
Merge: 3ee89ac 8638674
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:26 2021 -0700

    Merge commit '8638674a87c5ea92a87240f8f562ed1c437d1e0c' into main

commit 3ee89ac
Merge: 4b1dfb1 82ff604
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:25 2021 -0700

    Merge commit '82ff6045fa0aa7ff2f861f20512e30688c7b51c3' into main

commit 4b1dfb1
Merge: d27c6c1 c35e718
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:23 2021 -0700

    Merge commit 'c35e71892e6aa7dc2ce697b9ac44e541b3f4caef' into main

commit d27c6c1
Merge: 4fa42ca 281a7bd
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:21 2021 -0700

    Merge commit '281a7bdbb32a2ba124f8a6f6f9555135fd529599' into main

commit 4fa42ca
Merge: f83ce7c e5821bf
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:19 2021 -0700

    Merge commit 'e5821bf27668a5e54c699743c2b97aa55e7bdd93' into main

commit f83ce7c
Merge: e62a251 e6b8700
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:18 2021 -0700

    Merge commit 'e6b87005c1da22a0d354619eebca53c6e2639cdd' into main

commit e62a251
Merge: 49eb7c6 7d1b9c9
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:16 2021 -0700

    Merge commit '7d1b9c9f15eb3a9f2f253e5b88e091192a894bcf' into main

commit 49eb7c6
Merge: e944e40 db2ad6f
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:15 2021 -0700

    Merge commit 'db2ad6fa73adec1ffa364d21d130b69533b30ade' into main

commit e944e40
Merge: 4c412d2 e7e519a
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:13 2021 -0700

    Merge commit 'e7e519a466167b7a3ef9aa9b28535e436139936a' into main

commit 4c412d2
Merge: 9ffd477 559be8a
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:11 2021 -0700

    Merge commit '559be8aab4a0e666fa6fc9104570c9c9d3c54f12' into main

commit 9ffd477
Merge: 7ed2009 07fc1bb
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:10 2021 -0700

    Merge commit '07fc1bb883f4c3d9b603d3a3be8c0f2dfa88c285' into main

commit 7ed2009
Merge: f5253b7 96cc498
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:08 2021 -0700

    Merge commit '96cc49857dcda68910b8ae068de41983da625de2' into main

commit f5253b7
Merge: 4ef1527 e14fb54
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:06 2021 -0700

    Merge commit 'e14fb54e3f409aaf1e10b94f681677c59b1b7e0d' into main

commit 4ef1527
Merge: 2b65588 7a2a1a8
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:05 2021 -0700

    Merge commit '7a2a1a85e9738f127d9f6cd600aa6e1c4459bf2e' into main

commit 2b65588
Merge: 0727acd 108bb5a
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:03 2021 -0700

    Merge commit '108bb5aabec27f41f977bdf7a4d2ed3f3a3962eb' into main

commit 0727acd
Merge: 5f596d1 e213f37
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 22 19:16:01 2021 -0700

    Merge commit 'e213f37a450ca6b0746aa3acd2e5e76635e13468' into main

commit c12f46b
Author: Paul Cercueil <paul@crapouillou.net>
Date:   Mon Mar 22 19:03:25 2021 +0000

    [KMSDRM] Fix segmentation fault

    Deference the windata pointer *after* checking that it's non-NULL.

    Signed-off-by: Paul Cercueil <paul@crapouillou.net>

commit 100166d
Author: vanfanel <redwindwanderer@gmail.com>
Date:   Mon Mar 22 18:00:41 2021 +0100

    [KMSDRM] Improve cursor management.

commit 5a56e5a8227d9cff6b497b681c618a76bec1cae1
Author: Xing Ji <jixingcn@gmail.com>
Date:   Mon Mar 22 23:55:10 2021 +0800

    Fix libsdl-org#3596, can call the `SDL_TLSCleanup` to cleanup the TLS data when closing the application

commit 5f596d1
Merge: b1b93df b98b5ad
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:44 2021 -0700

    Merge commit 'b98b5adcaea159fc6a9753f808875acf7d3ee945' into main

commit b1b93df
Merge: aa00fe4 8ba735c
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:42 2021 -0700

    Merge commit '8ba735c208388159477bf0ccb06a8573a273fb02' into main

commit aa00fe4
Merge: 7acafda 3853531
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:40 2021 -0700

    Merge commit '3853531f6d11d1824b6a0ce0212c06e79483d22d' into main

commit 7acafda
Merge: 30bef5d 9996cec
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:39 2021 -0700

    Merge commit '9996cecc726a8d8900fd817d58f9505b3490d1bc' into main

commit 30bef5d
Merge: 00f93e4 5f7eb88
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:37 2021 -0700

    Merge commit '5f7eb88ae0990f89ec3a4bf697ec03aafee1a9a8' into main

commit 00f93e4
Merge: 4a39d89 b49e095
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:35 2021 -0700

    Merge commit 'b49e0953b14be38cdeada86df84a59c92e38aeff' into main

commit 4a39d89
Merge: 3313c67 1957ffd
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:33 2021 -0700

    Merge commit '1957ffd21ab5a3be6f347def510fcb8f985d3b8b' into main

commit 3313c67
Merge: 2bd0a71 34de3b5
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:32 2021 -0700

    Merge commit '34de3b57a101f8d885041add5e21311a153bb1bf' into main

commit 2bd0a71
Merge: 082394c dacf6cf
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:30 2021 -0700

    Merge commit 'dacf6cfbaa8c66ea6150f95aadad2954f116836c' into main

commit 082394c
Merge: 7ca94f5 a5f3ea1
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:25 2021 -0700

    Merge commit 'a5f3ea14487f1a36b1d421d02e86f25704f8bdc8' into main

commit 7ca94f5
Merge: 713a047 4fbd60b
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:23 2021 -0700

    Merge commit '4fbd60b81714f8f93946520147cb0c1f05faee64' into main

commit 713a047
Merge: 6158946 cd3809c
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:22 2021 -0700

    Merge commit 'cd3809c03e6d41a73cdb7c4a2e84c56e3d45cd7a' into main

commit 6158946
Merge: b53f06c 9e23c65
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:20 2021 -0700

    Merge commit '9e23c65237b480c74a0a2537b9927f6e52ca22aa' into main

commit b53f06c
Merge: 5b34a26 72bcf54
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:18 2021 -0700

    Merge commit '72bcf546f90423f05eda0fa08510e340683ff6f4' into main

commit 5b34a26
Merge: d84df04 3d22731
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:16 2021 -0700

    Merge commit '3d22731d94d6259255ef3fd5006c8c6c80e377b4' into main

commit d84df04
Merge: 9d46bd2 5134562
Author: Sam Lantinga <slouken@libsdl.org>
Date:   Mon Mar 15 09:00:15 2021 -0700

    Merge commit '51345623e888d7bc4131cd8f015c339f757ea68a' into main
jixingcn added a commit to jixingcn/SDL that referenced this issue Mar 24, 2021
@jixingcn
Squashed commit of the following:
ed38e4f 
commit 6b8f933589aa3925978a23e77a305a7e89c6ae4a
Author: Xing Ji <jixingcn@gmail.com>
Date:   Wed Mar 24 22:31:29 2021 +0800

    update the dynapi by `gendynapi.pl`

commit ebd1790c19983b652713f40ab1e139e485e1a2b7
Author: Xing Ji <jixingcn@gmail.com>
Date:   Wed Mar 24 22:17:48 2021 +0800

    revert the change in src/dynapi

commit 734b5f85c1613070081e39238e84198128971b53
Merge: 5a56e5a8 5ac6bd5
Author: Xing Ji <jixingcn@gmail.com>
Date:   Wed Mar 24 22:14:40 2021 +0800

    Merge remote-tracking branch 'libsdl/main' into jixingcn

commit 5a56e5a8227d9cff6b497b681c618a76bec1cae1
Author: Xing Ji <jixingcn@gmail.com>
Date:   Mon Mar 22 23:55:10 2021 +0800

    Fix libsdl-org#3596, can call the `SDL_TLSCleanup` to cleanup the TLS data when closing the application
@jixingcn jixingcn mentioned this issue Mar 24, 2021
Merged
joolswills pushed a commit to RetroPie/SDL that referenced this issue May 22, 2021
@jixingcn @joolswills
Squashed commit of the following:
efbdbe5 
commit 6b8f933589aa3925978a23e77a305a7e89c6ae4a
Author: Xing Ji <jixingcn@gmail.com>
Date:   Wed Mar 24 22:31:29 2021 +0800

    update the dynapi by `gendynapi.pl`

commit ebd1790c19983b652713f40ab1e139e485e1a2b7
Author: Xing Ji <jixingcn@gmail.com>
Date:   Wed Mar 24 22:17:48 2021 +0800

    revert the change in src/dynapi

commit 734b5f85c1613070081e39238e84198128971b53
Merge: 5a56e5a8 5ac6bd5
Author: Xing Ji <jixingcn@gmail.com>
Date:   Wed Mar 24 22:14:40 2021 +0800

    Merge remote-tracking branch 'libsdl/main' into jixingcn

commit 5a56e5a8227d9cff6b497b681c618a76bec1cae1
Author: Xing Ji <jixingcn@gmail.com>
Date:   Mon Mar 22 23:55:10 2021 +0800

    Fix libsdl-org#3596, can call the `SDL_TLSCleanup` to cleanup the TLS data when closing the application
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@SDLBugzilla