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

SDL_TLSData storage leak, probably? #12291

Open
Des-Nerger opened this issue Feb 14, 2025 · 7 comments
Open

SDL_TLSData storage leak, probably? #12291

Des-Nerger opened this issue Feb 14, 2025 · 7 comments
Milestone

Comments

@Des-Nerger
Copy link

Des-Nerger commented Feb 14, 2025

I was using SDL v3.2.4 in a Ziglang project, having called SDL_SetMemoryFunctions in my SDL_AppInit to switch to Zig's GeneralPurposeAllocator which, in debug build, automatically reports leaks at a deinitialization.

Having the latest master version of Zig (0.14.0-dev.3217+5b9b5e45c), I ran $ zig build run -- 3.pool-end-game, then closed the app and on exit got the following trace:

error(gpa): memory address 0x7f2a853c0800 leaked: 
intro-to-game-physics-with-Box2Dv3-and-SDLv3.zig/src/lib.zig:45:51: 0x1224543 in malloc (3.pool-end-game)
                const byte_slice = allocator.alloc(u8, @sizeOf(usize) + size) catch unreachable;
                                                  ^
intro-to-game-physics-with-Box2Dv3-and-SDLv3.zig/src/lib.zig:62:42: 0x1224940 in realloc (3.pool-end-game)
                    return @This().malloc(size);
                                         ^
~/.cache/zig/p/1220f653f5b656888b522bf5be06fc3062278767cfa7764e5d00eb559056d65b616f/src/stdlib/SDL_malloc.c:6489:11: 0x1448429 in SDL_realloc_REAL (~/.cache/zig/p/1220f653f5b656888b522bf5be06fc3062278767cfa7764e5d00eb559056d65b616f/src/stdlib/SDL_malloc.c)
    mem = s_mem.realloc_func(ptr, size);
          ^
~/.cache/zig/p/1220f653f5b656888b522bf5be06fc3062278767cfa7764e5d00eb559056d65b616f/src/thread/SDL_thread.c:93:38: 0x1449089 in SDL_SetTLS_REAL (~/.cache/zig/p/1220f653f5b656888b522bf5be06fc3062278767cfa7764e5d00eb559056d65b616f/src/thread/SDL_thread.c)
        new_storage = (SDL_TLSData *)SDL_realloc(storage, sizeof(*storage) + (newlimit - 1) * sizeof(storage->array[0]));
                                     ^
~/.cache/zig/p/1220f653f5b656888b522bf5be06fc3062278767cfa7764e5d00eb559056d65b616f/src/thread/SDL_thread.c:303:9: 0x144a152 in SDL_GetErrBuf (~/.cache/zig/p/1220f653f5b656888b522bf5be06fc3062278767cfa7764e5d00eb559056d65b616f/src/thread/SDL_thread.c)
        SDL_SetTLS(&tls_errbuf, errbuf, SDL_FreeErrBuf);
        ^
~/.cache/zig/p/1220f653f5b656888b522bf5be06fc3062278767cfa7764e5d00eb559056d65b616f/src/SDL_error.c:43:28: 0x14485e4 in SDL_SetErrorV_REAL (~/.cache/zig/p/1220f653f5b656888b522bf5be06fc3062278767cfa7764e5d00eb559056d65b616f/src/SDL_error.c)
        SDL_error *error = SDL_GetErrBuf(true);
                           ^

The leaked allocation is the only one, as this kludge fixes it:

--- a/src/lib.zig
+++ b/src/lib.zig
@@ -29 +29 @@ pub const sdl = struct {
-        // lib.allocator.free(sdl.leaked_first_88); // a hacky way to stop up the leak. FIXME
+        lib.allocator.free(sdl.leaked_first_88); // a hacky way to stop up the leak. FIXME
@Des-Nerger Des-Nerger changed the title SDL_TLSData storage leak SDL_TLSData storage leak, probably? Feb 14, 2025
@madebr
Copy link
Contributor

madebr commented Feb 14, 2025

Does your app call SDL_Quit() before exiting?

@Des-Nerger
Copy link
Author

Yes, it does; I should have mentioned it. Without SDL_Quit() there are many more leaks reported at exit.

@madebr
Copy link
Contributor

madebr commented Feb 14, 2025

Is it possible that trace is incomplete? What is failing and trying to set an error? Also, don't call any SDL functions after SDL_Quit.

@Des-Nerger
Copy link
Author

Des-Nerger commented Feb 14, 2025

To confirm that it's not the Zig's allocator to blame, I just tried to switch to using the libc's one directly:

--- a/src/lib.zig
+++ b/src/lib.zig
@@ -34 +34,2 @@ pub const sdl = struct {
-        try sdl.expect(@call(.auto, c.SDL_SetMemoryFunctions, sdl.memFns(allocator)), "");
+        _ = .{allocator};
+        try sdl.expect(c.SDL_SetMemoryFunctions(std.c.malloc, std.c.calloc, std.c.realloc, std.c.free), "");

and ran the executable through valgrind:

$ zig build 3.pool-end-game
$ valgrind --leak-check=full \
         --show-leak-kinds=all \
         --track-origins=yes \
         --verbose \
         --log-file=sdl-tlsdata-leak-valgrind.log \
         ./zig-out/3.pool-end-game

I'm attaching the produced log: sdl-tlsdata-leak-valgrind.log. But long story short, valgrind does see the leak, too:

$ grep -F 'definitely lost' sdl-tlsdata-leak-valgrind.log
==82757== 145 (88 direct, 57 indirect) bytes in 1 blocks are definitely lost in loss record 147 of 210
==82757==    definitely lost: 88 bytes in 1 blocks

@slouken
Copy link
Collaborator

slouken commented Feb 14, 2025

This is usually caused by not properly cleaning up threads, or calling SDL functions from threads that aren't created by SDL. Do you use multiple threads in that program? What's the minimal program you can write that has that leak?

@slouken slouken added this to the 3.x milestone Feb 14, 2025
@Des-Nerger
Copy link
Author

Des-Nerger commented Feb 14, 2025

My own code doesn't use multiple threads. To make absolutely sure the Zig standard library doesn't use them either, I just tried to rebuild the app with the single-threaded flag enabled:

--- a/build.zig
+++ b/build.zig
@@ -90,0 +91 @@ pub fn build(b: *std.Build) void {
+        exe_mod.single_threaded = true;

But the leak is still reported.

@slouken
Copy link
Collaborator

slouken commented Feb 14, 2025

@icculus, that valgrind log is really interesting. We should check it out before the next release.

@slouken slouken modified the milestones: 3.x, 3.2.6 Feb 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants