Skip to content

MacOS: fix for crash in SDL_Quit w/ gamepad input#16487

Merged
Megamouse merged 1 commit into
RPCS3:masterfrom
gdawg:teardown-sdl-on-pad-thread
Jan 2, 2025
Merged

MacOS: fix for crash in SDL_Quit w/ gamepad input#16487
Megamouse merged 1 commit into
RPCS3:masterfrom
gdawg:teardown-sdl-on-pad-thread

Conversation

@gdawg

@gdawg gdawg commented Jan 1, 2025

Copy link
Copy Markdown
Contributor

This PR refactors logic associated with SDL gamepads so that SDL_Quit is called on the same thread which calls SDL_Init as a fix for crash at exit on macOS.

The use-after-free bug this addresses occurs because some of the IOKit objects created during SDL_Init are automatically torn down when the creating thread finishes leaving SDL with dangling pointers which may cause memory access faults during shutdown.

For me this usually results in the error/backtrace below:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x691b6edcc010)
    frame #0: 0x000000018072d020 libobjc.A.dylib`objc_msgSend + 32
libobjc.A.dylib`objc_msgSend:
->  0x18072d020 <+32>: ldr    x10, [x16, #0x10]
    0x18072d024 <+36>: lsr    x11, x10, #48
    0x18072d028 <+40>: and    x10, x10, #0xffffffffffff
    0x18072d02c <+44>: and    w12, w1, w11
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x691b6edcc010)
  * frame #0: 0x000000018072d020 libobjc.A.dylib`objc_msgSend + 32
    frame #1: 0x0000000180b70210 CoreFoundation`CFEqual + 748
    frame #2: 0x0000000184306ed8 IOKit`IOHIDManagerUnscheduleFromRunLoop + 72
    frame #3: 0x000000010d59afd4 libSDL2-2.0.0.dylib`DARWIN_JoystickQuit + 64
    frame #4: 0x000000010d4fee64 libSDL2-2.0.0.dylib`SDL_JoystickQuit + 80
    frame #5: 0x000000010d4d3c08 libSDL2-2.0.0.dylib`SDL_QuitSubSystem_REAL + 188
    frame #6: 0x000000010d4d3bcc libSDL2-2.0.0.dylib`SDL_QuitSubSystem_REAL + 128
    frame #7: 0x000000010d4d3e28 libSDL2-2.0.0.dylib`SDL_Quit_REAL + 32
    frame #8: 0x0000000100195124 rpcs3`sdl_instance::~sdl_instance(this=0x0000000102d88d70) at sdl_pad_handler.cpp:22:4
    frame #9: 0x000000010019508c rpcs3`sdl_instance::~sdl_instance(this=0x0000000102d88d70) at sdl_pad_handler.cpp:17:2
    frame #10: 0x00000001809c32e8 libsystem_c.dylib`__cxa_finalize_ranges + 476
    frame #11: 0x00000001809c3070 libsystem_c.dylib`exit + 44
    frame #12: 0x0000000180b1e850 libdyld.dylib`dyld4::LibSystemHelpers::exit(int) const + 20
    frame #13: 0x000000018077b1a0 dyld`start + 2552

There may be other paths which cause SDL to access the dangling pointers or occasions where the memory is reallocated leading to more obscure bugs.

I've tested this on macOS but unfortunately don't currently have any equipment to test windows or linux builds on.

@Megamouse

Copy link
Copy Markdown
Contributor

We only want to init sdl once and quit sdl when rpcs3 is terminated.
It's not really meant to be initialized and quit more than once.
If it really just needs to call init and quit from the same thread, you can probably just use Emu.BlockingCallFromMainThread

@gdawg

gdawg commented Jan 1, 2025

Copy link
Copy Markdown
Contributor Author

We only want to init sdl once and quit sdl when rpcs3 is terminated.

I agree this would be ideal.

If it really just needs to call init and quit from the same thread, you can probably just use Emu.BlockingCallFromMainThread

I'm concerned that many or all of the other SDL operations run on the pad handler thread may also not be safe unless run on the same thread which calls init/quit. Even if not today some minor macOS update or internal SDL change may alter that.

Any thoughts?

@Megamouse

Copy link
Copy Markdown
Contributor

I'm pretty sure that this is an upstream issue already.
I think it's also mostly an init and quit thing.
There shouldn't really be any issue since we only ever really run sdl on a single thread.

@gdawg

gdawg commented Jan 2, 2025

Copy link
Copy Markdown
Contributor Author

I can try that and see what happens, will report back

@gdawg gdawg force-pushed the teardown-sdl-on-pad-thread branch from 8376b2c to 7526845 Compare January 2, 2025 16:49
@gdawg

gdawg commented Jan 2, 2025

Copy link
Copy Markdown
Contributor Author

I can try that and see what happens, will report back

@Megamouse good news! simply calling init on the main thread as you suggest seems to work just fine for me, also solves the issue and is much simpler too. I've updated the PR with that change thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants