SDL
SDL copied to clipboard
libsdl-org
WGI callbacks can be invoked after WGI has been destroyed
I tracked down the cause of https://github.com/moonlight-stream/moonlight-qt/issues/856 which can be easily reproduced by installing DS4Windows and connected a PS4 controller. Git bisect blames 5aa438e80aabe3570c0ef807d9b22bcd9835ced6.
It appears that the crash is happening in EnumJoystickPresentCallback() because the joystick subsystem has actually been deinitialized during the call to SDL_DINPUT_JoystickPresent(). So we have a valid dinput pointer when we check in SDL_DINPUT_JoystickPresent(), but it's gone by the time DInput calls EnumJoystickPresentCallback().
Here is the main thread reaching the SDL_zero(wgi) at the end of WGI_JoystickQuit():
Breakpoint 2 hit
SDL2!WGI_JoystickQuit+0x185:
00007ffa`1ca25455 41b860000000 mov r8d,60h
0:000> k
# Child-SP RetAddr Call Site
00 00000029`db4fe520 00007ffa`1ca13812 SDL2!WGI_JoystickQuit+0x185 [C:\Users\camer\SDL\src\joystick\windows\SDL_windows_gaming_input.c @ 887]
01 00000029`db4fe550 00007ffa`1cacb824 SDL2!SDL_JoystickQuit+0x82 [C:\Users\camer\SDL\src\joystick\SDL_joystick.c @ 1214]
02 00000029`db4fe590 00007ffa`1c9b7ed2 SDL2!SDL_QuitSubSystem_REAL+0x94 [C:\Users\camer\SDL\src\SDL.c @ 395]
03 00000029`db4fe5c0 00007ff6`8d20871f SDL2!SDL_QuitSubSystem+0x12 [C:\Users\camer\SDL\src\dynapi\SDL_dynapi_procs.h @ 90]
04 00000029`db4fe5f0 00007ff6`8d216bfc Moonlight!SdlInputHandler::getUnmappedGamepads+0x1bf [C:\projects\moonlight-qt\app\streaming\input\gamepad.cpp @ 606]
05 00000029`db4fe6c0 00007ff6`8d1e273b Moonlight!SystemProperties::SystemProperties+0x21c [C:\projects\moonlight-qt\app\backend\systemproperties.cpp @ 67]
meanwhile a WGI device add notification is still in progress on another thread:
0:029> k
# Child-SP RetAddr Call Site
00 00000029`dcbacb28 00007ffa`e276df5e ntdll!NtOpenFile+0x14
01 00000029`dcbacb30 00007ffa`e276dd5c KERNELBASE!InternalFindFirstFileExW+0x1b6
02 00000029`dcbacef0 00007ffa`e1f9d8f4 KERNELBASE!FindFirstFileW+0x2c
03 00000029`dcbacf40 00007ffa`e1f9d4a3 cfgmgr32!pSetupFileExists+0x4c
04 00000029`dcbad1d0 00007ffa`e1f9928d cfgmgr32!SpInfLoadInfFile+0x73
05 00000029`dcbade60 00007ffa`e1f9335d cfgmgr32!LoadIndirectInfString+0x2c1
06 00000029`dcbae5d0 00007ffa`e1f931ae cfgmgr32!SpInfGetIndirectString+0x55
07 00000029`dcbae610 00007ffa`e1f930b0 cfgmgr32!SpInfResolveIndirection+0x76
08 00000029`dcbae680 00007ffa`e1f95ab9 cfgmgr32!SpInfGetString+0x54
09 00000029`dcbae6b0 00007ffa`e1f97a5a cfgmgr32!GetRegistryProperty+0x279
0a 00000029`dcbae770 00007ffa`e1f957ce cfgmgr32!Local_CM_Get_DevNode_Registry_Property+0x76
0b 00000029`dcbae810 00007ffa`1950b594 cfgmgr32!CM_Get_DevNode_Registry_PropertyW+0xde
0c 00000029`dcbaea30 00007ffa`1950adf4 dinput8!DIHid_GetDevInfo+0xcc
0d 00000029`dcbaed60 00007ffa`194f94c3 dinput8!DIHid_BuildHidListEntry+0x17c
0e 00000029`dcbaf090 00007ffa`194f46ad dinput8!DIHid_BuildHidList+0x4963
0f 00000029`dcbaf110 00007ffa`194f24ed dinput8!CDIDEnum_New+0x2d
10 00000029`dcbaf150 00007ffa`1ca1a44b dinput8!CDIObj_EnumDevicesW+0xad
11 00000029`dcbaf610 00007ffa`1ca2401a SDL2!SDL_DINPUT_JoystickPresent+0x6b [C:\Users\camer\SDL\src\joystick\windows\SDL_dinputjoystick.c @ 596]
12 00000029`dcbaf660 00007ffa`caca6850 SDL2!IEventHandler_CRawGameControllerVtbl_InvokeAdded+0x4ba [C:\Users\camer\SDL\src\joystick\windows\SDL_windows_gaming_input.c @ 335]
13 00000029`dcbaf7a0 00007ffa`caca7072 Windows_Gaming_Input!Microsoft::WRL::Details::DelegateArgTraits<long (__cdecl Windows::Foundation::IEventHandler_impl<Windows::Foundation::Internal::AggregateType<Windows::Gaming::Input::RawGameController * __ptr64,Windows::Gaming::Input::IRawGameController * __ptr64> >::*)(IInspectable * __ptr64,Windows::Gaming::Input::IRawGameController * __ptr64) __ptr64>::DelegateInvokeHelper<Microsoft::WRL::Implements<Microsoft::WRL::RuntimeClassFlags<2>,Windows::Foundation::IEventHandler<Windows::Gaming::Input::RawGameController * __ptr64>,Microsoft::WRL::FtmBase>,<lambda_6da117dbca11818c9940be4048cacede>,-1,IInspectable * __ptr64,Windows::Gaming::Input::IRawGameController * __ptr64>::Invoke+0xa0
14 00000029`dcbaf7d0 00007ffa`caca7301 Windows_Gaming_Input!Microsoft::WRL::InvokeTraits<-2>::InvokeDelegates<<lambda_ef5aba923f374d9535c9704c5548f434>,Windows::Foundation::IEventHandler<Windows::Gaming::Input::RawGameController * __ptr64> >+0x86
15 00000029`dcbaf830 00007ffa`cac5b105 Windows_Gaming_Input!Windows::Gaming::Input::Custom::Details::CustomGameControllerFactoryBase<Windows::Gaming::Input::RawGameController,Windows::Gaming::Input::RawGameController,Windows::Gaming::Input::IRawGameController,Windows::Gaming::Input::IRawGameControllerStatics,Microsoft::WRL::Details::Nil>::UpdateGameControllerCollection+0x16d
16 00000029`dcbaf8c0 00007ffa`e3c092ba Windows_Gaming_Input!FactoryManager::SendControllerNotifications+0x171
17 00000029`dcbaf900 00007ffa`e3c08126 shcore!WorkThreadManager::CThread::ThreadProc+0x26a
18 00000029`dcbafba0 00007ffa`e3c07f31 shcore!WorkThreadManager::CThread::s_ExecuteThreadProc+0x22
19 00000029`dcbafbe0 00007ffa`e3b4244d shcore!<lambda_9844335fc14345151eefcc3593dd6895>::<lambda_invoker_cdecl>+0x11
1a 00000029`dcbafc10 00007ffa`e4eedf78 KERNEL32!BaseThreadInitThunk+0x1d
1b 00000029`dcbafc40 00000000`00000000 ntdll!RtlUserThreadStart+0x28
That same WGI notification thread later crashes either inside DInput itself or in our EnumJoystickPresentCallback() callback because DInput has been cleaned up:
0:029> k
# Child-SP RetAddr Call Site
00 00000029`dcbaf0c0 00007ffa`194f2508 dinput8!CDIDEnum_Next+0x9f
01 00000029`dcbaf150 00007ffa`1ca1a44b dinput8!CDIObj_EnumDevicesW+0xc8
02 00000029`dcbaf610 00007ffa`1ca2401a SDL2!SDL_DINPUT_JoystickPresent+0x6b [C:\Users\camer\SDL\src\joystick\windows\SDL_dinputjoystick.c @ 596]
03 00000029`dcbaf660 00007ffa`caca6850 SDL2!IEventHandler_CRawGameControllerVtbl_InvokeAdded+0x4ba [C:\Users\camer\SDL\src\joystick\windows\SDL_windows_gaming_input.c @ 335]
04 00000029`dcbaf7a0 00007ffa`caca7072 Windows_Gaming_Input!Microsoft::WRL::Details::DelegateArgTraits<long (__cdecl Windows::Foundation::IEventHandler_impl<Windows::Foundation::Internal::AggregateType<Windows::Gaming::Input::RawGameController * __ptr64,Windows::Gaming::Input::IRawGameController * __ptr64> >::*)(IInspectable * __ptr64,Windows::Gaming::Input::IRawGameController * __ptr64) __ptr64>::DelegateInvokeHelper<Microsoft::WRL::Implements<Microsoft::WRL::RuntimeClassFlags<2>,Windows::Foundation::IEventHandler<Windows::Gaming::Input::RawGameController * __ptr64>,Microsoft::WRL::FtmBase>,<lambda_6da117dbca11818c9940be4048cacede>,-1,IInspectable * __ptr64,Windows::Gaming::Input::IRawGameController * __ptr64>::Invoke+0xa0
05 00000029`dcbaf7d0 00007ffa`caca7301 Windows_Gaming_Input!Microsoft::WRL::InvokeTraits<-2>::InvokeDelegates<<lambda_ef5aba923f374d9535c9704c5548f434>,Windows::Foundation::IEventHandler<Windows::Gaming::Input::RawGameController * __ptr64> >+0x86
06 00000029`dcbaf830 00007ffa`cac5b105 Windows_Gaming_Input!Windows::Gaming::Input::Custom::Details::CustomGameControllerFactoryBase<Windows::Gaming::Input::RawGameController,Windows::Gaming::Input::RawGameController,Windows::Gaming::Input::IRawGameController,Windows::Gaming::Input::IRawGameControllerStatics,Microsoft::WRL::Details::Nil>::UpdateGameControllerCollection+0x16d
07 00000029`dcbaf8c0 00007ffa`e3c092ba Windows_Gaming_Input!FactoryManager::SendControllerNotifications+0x171
08 00000029`dcbaf900 00007ffa`e3c08126 shcore!WorkThreadManager::CThread::ThreadProc+0x26a
09 00000029`dcbafba0 00007ffa`e3c07f31 shcore!WorkThreadManager::CThread::s_ExecuteThreadProc+0x22
0a 00000029`dcbafbe0 00007ffa`e3b4244d shcore!<lambda_9844335fc14345151eefcc3593dd6895>::<lambda_invoker_cdecl>+0x11
0b 00000029`dcbafc10 00007ffa`e4eedf78 KERNEL32!BaseThreadInitThunk+0x1d
0c 00000029`dcbafc40 00000000`00000000 ntdll!RtlUserThreadStart+0x28
I'll take a look at this as part of the joystick locking review this milestone. Thanks for the detailed info!
fwiw https://github.com/libsdl-org/SDL/commit/5aa438e80aabe3570c0ef807d9b22bcd9835ced6 basically corrects imbalanced AddRef/Release on the callback objects that SDL registers. so the issue was probably present before, but the imbalanced refcount caused it to not be surfaced.
I originally added refcounting to the object in SDL to ensure the change was correct - maybe it's worth adding to SDL to detect such issues.
In other words, change controller_added and controller_removed to be a new struct type, which has __FIEventHandler_1_Windows__CGaming__CInput__CRawGameController as first member and adds a refcount field, then properly inc/dec the refcount field in QueryInterface and AddRef/Release methods and assert if it would be free'd (can't be valid since it's static object).
The reason I didn't commit that code is because I suspect there's a "proper" struct type to use there, but I'm not sure what it is (if you look at disasm of "real" c++/winrt code, you can see the refcount is at offset 0xc). Just adding refcount field does work, though.
fwiw here is how the c++/winrt code looks:
int __fastcall winrt::impl::implements_delegate_winrt::Windows::Foundation::EventHandler_winrt::Windows::Gaming::Input::RawGameController___ciface::WGInput::Init_::_6_::_lambda_2___::QueryInterface(
winrt::impl::abi<winrt::Windows::Foundation::IUnknown,void>::type *id,
void **result,
void **a3)
{
if ( *result == *(void **)&winrt::impl::guid_v<winrt::Windows::Foundation::EventHandler<winrt::Windows::Gaming::Input::RawGameController>>.Data1
&& result[1] == *(void **)winrt::impl::guid_v<winrt::Windows::Foundation::EventHandler<winrt::Windows::Gaming::Input::RawGameController>>.Data4
|| *result == *(void **)&winrt::impl::guid_v<winrt::Windows::Foundation::IUnknown>.Data1
&& result[1] == *(void **)winrt::impl::guid_v<winrt::Windows::Foundation::IUnknown>.Data4
|| *result == *(void **)&winrt::impl::guid_v<winrt::impl::IAgileObject>.Data1
&& result[1] == *(void **)winrt::impl::guid_v<winrt::impl::IAgileObject>.Data4 )
{
*a3 = id;
_InterlockedExchangeAdd((volatile signed __int32 *)&id[1].__vftable + 1, 1u); // refcount
return 0;
}
else if ( *result == *(void **)&winrt::impl::guid_v<winrt::impl::IMarshal>.Data1
&& result[1] == *(void **)winrt::impl::guid_v<winrt::impl::IMarshal>.Data4 )
{
return winrt::impl::make_marshaler(id, a3);
}
else
{
*a3 = 0i64;
return -2147467262;
}
}
__int64 __fastcall winrt::impl::implements_delegate_winrt::Windows::Foundation::EventHandler_winrt::Windows::Gaming::Input::RawGameController___ciface::WGInput::Init_::_6_::_lambda_2___::AddRef(
__int64 a1)
{
return (unsigned int)_InterlockedIncrement((volatile signed __int32 *)(a1 + 12)); // refcount
}
__int64 __fastcall winrt::impl::implements_delegate_winrt::Windows::Foundation::EventHandler_winrt::Windows::Gaming::Input::RawGameController___ciface::WGInput::Init_::_6_::_lambda_2___::Release(
volatile signed __int32 *a1)
{
int v1; // ebx
int v2; // edx
v1 = _InterlockedDecrement(a1 + 3); // refcount
if ( v1 < 0 )
goto LABEL_9;
if ( v1 || !a1 )
return (unsigned int)v1;
v2 = _InterlockedDecrement(&`winrt::get_module_lock'::`2'::s_lock);
if ( !v2 )
{
operator delete((void *)a1);
return 0i64;
}
if ( v2 < 0 )
LABEL_9:
abort();
operator delete((void *)a1);
return (unsigned int)v1;
}
// where it's allocated. note refcount is u32 at + 0xc
v2 = (winrt::impl::abi<winrt::Windows::Foundation::IUnknown,void>::type *)operator new(0x10ui64);
_InterlockedExchangeAdd(&`winrt::get_module_lock'::`2'::s_lock, 1u);
HIDWORD(v2[1].__vftable) = 1; // refcount
v2->__vftable = (winrt::impl::abi<winrt::Windows::Foundation::IUnknown,void>::type_vtbl *)winrt::impl::delegate_winrt::Windows::Foundation::EventHandler_winrt::Windows::Gaming::Input::RawGameController___ciface::WGInput::Init_::_6_::_lambda_1___::_vftable_;
... which is this code from <winrt/base.h>
template <typename T, typename H>
struct implements_delegate : abi_t<T>, H, update_module_lock
{
implements_delegate(H&& handler) : H(std::forward<H>(handler))
{
}
int32_t __stdcall QueryInterface(guid const& id, void** result) noexcept final
{
if (is_guid_of<T>(id) || is_guid_of<Windows::Foundation::IUnknown>(id) || is_guid_of<IAgileObject>(id))
{
*result = static_cast<abi_t<T>*>(this);
AddRef();
return 0;
}
if (is_guid_of<IMarshal>(id))
{
return make_marshaler(this, result);
}
*result = nullptr;
return error_no_interface;
}
uint32_t __stdcall AddRef() noexcept final
{
return ++m_references;
}
uint32_t __stdcall Release() noexcept final
{
auto const remaining = --m_references;
if (remaining == 0)
{
delete static_cast<delegate<T, H>*>(this);
}
return remaining;
}
private:
atomic_ref_count m_references{ 1 };
};
so anyway, the extra 4 bytes i was worried about is just because atomic_ref_count is std::atomic<int32_t> which happens to be composed of (simplistically) { u32 spinlock; u32 value; } in the MS STL i'm compiling against. So nothing to worry about.
Can you retest this issue after https://github.com/libsdl-org/SDL/commit/40bd4feedcca779a4600743c27a786b436edc8f7?
No response, closing. Please feel free to reopen if this is still an issue.