Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Object destruction issues... #313

Closed
icculus opened this issue Feb 4, 2025 · 14 comments · Fixed by #356
Closed

Object destruction issues... #313

icculus opened this issue Feb 4, 2025 · 14 comments · Fixed by #356

Comments

@icculus
Copy link
Collaborator

icculus commented Feb 4, 2025

Something I'm seeing in a lot of the apps we've fixed things in over the last several days:

  • They call SDL_Quit() when cleaning up.
  • They call exit() or return from main().
  • They then call SDL_FreeSurface or SDL_FreeAudioStream, etc, during static destructors.

SDL3 already destroyed these things during SDL_Quit, which I don't think SDL2 does, so when the static destructors run, they're already gone.

Here's The Battle For Wesnoth, just starting and clicking the window bar's close button on the main menu:

corrupted size vs. prev_size while consolidating

Thread 1 "wesnoth" received signal SIGABRT, Aborted.
__pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
warning: 44	./nptl/pthread_kill.c: No such file or directory
(gdb) bt
#0  __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
#1  __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
#2  __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
#3  0x00007ffff6f7326e in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
#4  0x00007ffff6f568ff in __GI_abort () at ./stdlib/abort.c:79
#5  0x00007ffff6f577b6 in __libc_message_impl (fmt=fmt@entry=0x7ffff70fc8d7 "%s\n") at ../sysdeps/posix/libc_fatal.c:132
#6  0x00007ffff6fd6fe5 in malloc_printerr (str=str@entry=0x7ffff70ffb30 "corrupted size vs. prev_size while consolidating") at ./malloc/malloc.c:5772
#7  0x00007ffff6fd9144 in _int_free_merge_chunk (av=0x7ffff7131ac0 <main_arena>, p=0x5555576ec380, size=14734880) at ./malloc/malloc.c:4695
#8  0x00007ffff6fd942a in _int_free (av=0x7ffff7131ac0 <main_arena>, p=<optimized out>, have_lock=<optimized out>) at ./malloc/malloc.c:4646
#9  0x00007ffff6fdbd9e in __GI___libc_free (mem=0x5555584accc0) at ./malloc/malloc.c:3398
#10 0x00007ffff3986748 in real_free (p=0x5555584accc0) at /home/icculus/projects/SDL/src/stdlib/SDL_malloc.c:6330
#11 0x00007ffff3986a4f in SDL_free_REAL (ptr=0x5555584accc0) at /home/icculus/projects/SDL/src/stdlib/SDL_malloc.c:6505
#12 0x00007ffff3a1b60d in SDL_DestroySurface_REAL (surface=0x55555760d360) at /home/icculus/projects/SDL/src/video/SDL_surface.c:2978
#13 0x00007ffff38ba7a1 in SDL_DestroySurface (a=0x55555760d360) at /home/icculus/projects/SDL/src/dynapi/SDL_dynapi_procs.h:182
#14 0x00007ffff7c831cc in SDL_FreeSurface_REAL (surface=0x5555576d0510) at /home/icculus/projects/sdl2-compat/src/sdl2_compat.c:3161
#15 0x00007ffff7ca11f7 in SDL_FreeSurface (a=0x5555576d0510) at /home/icculus/projects/sdl2-compat/src/dynapi/SDL_dynapi_procs.h:471
#16 0x00005555558e2b82 in ??? ()
#17 0x00007ffff6f75a66 in __run_exit_handlers (status=0, listp=<optimized out>, run_list_atexit=run_list_atexit@entry=true, run_dtors=run_dtors@entry=true) at ./stdlib/exit.c:108
#18 0x00007ffff6f75bae in __GI_exit (status=<optimized out>) at ./stdlib/exit.c:138
#19 0x00007ffff6f581d1 in __libc_start_call_main (main=main@entry=0x5555557f25b0 <main>, argc=argc@entry=1, argv=argv@entry=0x7fffffffdd98)
    at ../sysdeps/nptl/libc_start_call_main.h:74
#20 0x00007ffff6f5828b in __libc_start_main_impl
    (main=0x5555557f25b0 <main>, argc=1, argv=0x7fffffffdd98, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffdd88)
    at ../csu/libc-start.c:360
#21 0x000055555584cc95 in ??? ()

...and here it is again, it blew up on an AudioStream this time...

#0  0x0000000000000009 in ??? ()
#1  0x00007ffff3897957 in DestroyAudioTrack (queue=0x555556f7d3b0, track=0x5555573ea1f0) at /home/icculus/projects/SDL/src/audio/SDL_audioqueue.c:173
#2  0x00007ffff38979d0 in SDL_ClearAudioQueue (queue=0x555556f7d3b0) at /home/icculus/projects/SDL/src/audio/SDL_audioqueue.c:188
#3  0x00007ffff389783d in SDL_DestroyAudioQueue (queue=0x555556f7d3b0) at /home/icculus/projects/SDL/src/audio/SDL_audioqueue.c:143
#4  0x00007ffff3897004 in SDL_DestroyAudioStream_REAL (stream=0x555556d4d030) at /home/icculus/projects/SDL/src/audio/SDL_audiocvt.c:1323
#5  0x00007ffff38ba5e2 in SDL_DestroyAudioStream (a=0x555556d4d030) at /home/icculus/projects/SDL/src/dynapi/SDL_dynapi_procs.h:169
#6  0x00007ffff7c8bfb3 in SDL_FreeAudioStream_REAL (stream2=0x555556f836c0) at /home/icculus/projects/sdl2-compat/src/sdl2_compat.c:6340
#7  0x00007ffff7ca2fc1 in SDL_FreeAudioStream (a=0x555556f836c0) at /home/icculus/projects/sdl2-compat/src/dynapi/SDL_dynapi_procs.h:673
#8  0x00007ffff7c0f03b in OGG_Delete (context=0x555556d542c0) at ../src/codecs/music_ogg_stb.c:444
#9  0x00007ffff7c205cb in Mix_FreeMusic (music=0x555556d7f4f0) at ../src/music.c:819
#10 0x0000555555d210b2 in ??? ()
#11 0x00005555558ae72a in ??? ()
#12 0x0000555555d21334 in ??? ()
#13 0x00007ffff6f75a66 in __run_exit_handlers (status=0, listp=<optimized out>, run_list_atexit=run_list_atexit@entry=true, run_dtors=run_dtors@entry=true) at ./stdlib/exit.c:108
#14 0x00007ffff6f75bae in __GI_exit (status=<optimized out>) at ./stdlib/exit.c:138
#15 0x00007ffff6f581d1 in __libc_start_call_main (main=main@entry=0x5555557f25b0 <main>, argc=argc@entry=1, argv=argv@entry=0x7fffffffdd98)
    at ../sysdeps/nptl/libc_start_call_main.h:74
#16 0x00007ffff6f5828b in __libc_start_main_impl
    (main=0x5555557f25b0 <main>, argc=1, argv=0x7fffffffdd98, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffdd88)
    at ../csu/libc-start.c:360
#17 0x000055555584cc95 in ??? ()

Some things obviously shouldn't work after SDL_Quit()...like an SDL_Texture, if that wasn't something we clean up, or whatever, but one could argue a Surface or AudioStream might still be useful, so we might need a hint to tell SDL3 not to clean stuff up.

Alternately, we can add a hack to sdl2-compat's SDL_FreeSurface() to make it return immediately after SDL_Quit() has been called.
@slouken
Copy link
Collaborator

slouken commented Feb 4, 2025

Did you see my comment about exposing the SDL_ObjectValid() API?

@slouken
Copy link
Collaborator

slouken commented Feb 4, 2025

Actually, I don't see my comment either, but the idea was that maybe we should expose the SDL_ObjectValid() API and allow the application (and sdl2-compat) register new object types and we can use that to validate pointers before dereferencing them.

This might also help the window surface issue that you ran into earlier.

@icculus
Copy link
Collaborator Author

icculus commented Feb 4, 2025

Hmm, this could work.

@icculus
Copy link
Collaborator Author

icculus commented Feb 6, 2025

Okay for the sake of argument here, we just want a thing that a) registers a void* b) tells you if the pointer is registered and c) lets you unregister it. More or less, this is what the ObjectValid stuff does.

The SDL_ObjectType part makes this dicey for a public API, because now we need to let people track their own types, taking into account that the SDL_ObjectType enum will increase in future SDL3 releases.

Unless we either let them have a single SDL_OBJECT_TYPE_GENERIC_POINTER and they're on their own.

Also, do we want apps to be able to query if a window pointer is valid, etc? It seems harmless, but also like there might be ramifications I haven't thought of.

I think for now I might just drop a dirt-simple hash table into sdl2-compat and leave it at that. We can always expose this API in SDL3 later and take advantage of it in sdl2-compat then.

@slouken
Copy link
Collaborator

slouken commented Feb 6, 2025

Okay for the sake of argument here, we just want a thing that a) registers a void* b) tells you if the pointer is registered and c) lets you unregister it. More or less, this is what the ObjectValid stuff does.

The SDL_ObjectType part makes this dicey for a public API, because now we need to let people track their own types, taking into account that the SDL_ObjectType enum will increase in future SDL3 releases.

Unless we either let them have a single SDL_OBJECT_TYPE_GENERIC_POINTER and they're on their own.

Maybe a public API can just expose those three functions and use SDL_OBJECT_TYPE_GENERIC_POINTER internally?

I think for now I might just drop a dirt-simple hash table into sdl2-compat and leave it at that. We can always expose this API in SDL3 later and take advantage of it in sdl2-compat then.

That seems fine too. Might as well drop in the SDL3 hashtable implementation though, we know it's fast and good.

@icculus
Copy link
Collaborator Author

icculus commented Feb 7, 2025

That seems fine too. Might as well drop in the SDL3 hashtable implementation though, we know it's fast and good.

Should we just make the hashtable a public SDL3 API at this point...?

@slouken
Copy link
Collaborator

slouken commented Feb 7, 2025

That seems fine too. Might as well drop in the SDL3 hashtable implementation though, we know it's fast and good.

Should we just make the hashtable a public SDL3 API at this point...?

It would fix libsdl-org/SDL_ttf#508 ...

@icculus
Copy link
Collaborator Author

icculus commented Feb 7, 2025

Okay, I'm going to do that after lunch.

@slouken
Copy link
Collaborator

slouken commented Feb 7, 2025

Should it go in 3.4? I already broke the "no new functions in minor release" rule, but ...?

@icculus
Copy link
Collaborator Author

icculus commented Feb 7, 2025

What's the value of only adding them on minor version boundaries?

@slouken
Copy link
Collaborator

slouken commented Feb 7, 2025

You have forward and backward compatibility in minor versions.

@icculus
Copy link
Collaborator Author

icculus commented Feb 7, 2025

I'm cleaning this up for a PR, we can merge whenever it makes sense.

@sgpowers sgpowers mentioned this issue Feb 8, 2025
Open
@slouken
Copy link
Collaborator

slouken commented Feb 12, 2025

FYI, I just ran into this with py-sdl2 and the gui.py example. It loads a BMP file, then calls SDL_Quit(), then the python garbage collection runs and cleans up the SDL2 surface, which is no longer valid.

@icculus
Copy link
Collaborator Author

icculus commented Feb 12, 2025

Yeah, lots of things are hitting this. I'm almost done cleaning up the hash table stuff, and then this will be trivial to fix.

@Ghabry Ghabry mentioned this issue Feb 12, 2025
Open
slouken added a commit to slouken/sdl2-compat that referenced this issue Feb 14, 2025
@slouken
Fixed crashes if the application cleans up after SDL_Quit()
59faa6c 
The application might free surfaces and audio streams after SDL_Quit(), which is allowed.

Wesnoth and 'python py-sdl2/examples/gui.py' both exit cleanly now.

Fixes libsdl-org#313
@slouken slouken mentioned this issue Feb 14, 2025
Merged
slouken added a commit that referenced this issue Feb 14, 2025
@slouken
Fixed crashes if the application cleans up after SDL_Quit()
4ed42d7 
The application might free surfaces and audio streams after SDL_Quit(), which is allowed.

Wesnoth and 'python py-sdl2/examples/gui.py' both exit cleanly now.

Fixes #313
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixed crashes if the application cleans up after SDL_Quit() slouken/sdl2-compat
2 participants
@icculus @slouken