[PATCH 1/5] dinput: Do not wait for internal hooks inside the HID joystick CS.
This can cause some deadlocks as the internal thread will acquire the device CS when reading the state. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- Fixing the same kind of issue as the series I sent with xinput, where cancelled IO may cause crashes if it completes after the device is gone, although the dinput code needed more fixes first to prevent deadlocks. dlls/dinput/joystick_hid.c | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/dlls/dinput/joystick_hid.c b/dlls/dinput/joystick_hid.c index 75bf66b7a51..208e73835e9 100644 --- a/dlls/dinput/joystick_hid.c +++ b/dlls/dinput/joystick_hid.c @@ -881,13 +881,18 @@ static HRESULT WINAPI hid_joystick_Acquire( IDirectInputDevice8W *iface ) { struct hid_joystick *impl = impl_from_IDirectInputDevice8W( iface ); ULONG report_len = impl->caps.InputReportByteLength; - HRESULT hr; + HRESULT hr = DI_OK; TRACE( "iface %p.\n", iface ); EnterCriticalSection( &impl->base.crit ); - hr = IDirectInputDevice2WImpl_Acquire( iface ); - if (hr == DI_OK) + if (impl->base.acquired) + hr = DI_NOEFFECT; + else if (!impl->base.data_format.user_df) + hr = DIERR_INVALIDPARAM; + else if ((impl->base.dwCoopLevel & DISCL_FOREGROUND) && impl->base.win != GetForegroundWindow()) + hr = DIERR_OTHERAPPHASPRIO; + else { memset( &impl->read_ovl, 0, sizeof(impl->read_ovl) ); impl->read_ovl.hEvent = impl->base.read_event; @@ -895,8 +900,13 @@ static HRESULT WINAPI hid_joystick_Acquire( IDirectInputDevice8W *iface ) impl->base.read_callback( iface ); IDirectInputDevice8_SendForceFeedbackCommand( iface, DISFFC_RESET ); + impl->base.acquired = TRUE; } LeaveCriticalSection( &impl->base.crit ); + if (hr != DI_OK) return hr; + + dinput_hooks_acquire_device( iface ); + check_dinput_hooks( iface, TRUE ); return hr; } @@ -904,21 +914,26 @@ static HRESULT WINAPI hid_joystick_Acquire( IDirectInputDevice8W *iface ) static HRESULT WINAPI hid_joystick_Unacquire( IDirectInputDevice8W *iface ) { struct hid_joystick *impl = impl_from_IDirectInputDevice8W( iface ); - HRESULT hr; + HRESULT hr = DI_OK; BOOL ret; TRACE( "iface %p.\n", iface ); EnterCriticalSection( &impl->base.crit ); - if (impl->base.acquired) + if (!impl->base.acquired) hr = DI_NOEFFECT; + else { ret = CancelIoEx( impl->device, &impl->read_ovl ); if (!ret) WARN( "CancelIoEx failed, last error %u\n", GetLastError() ); IDirectInputDevice8_SendForceFeedbackCommand( iface, DISFFC_RESET ); + impl->base.acquired = FALSE; } - hr = IDirectInputDevice2WImpl_Unacquire( iface ); LeaveCriticalSection( &impl->base.crit ); + if (hr != DI_OK) return hr; + + dinput_hooks_unacquire_device( iface ); + check_dinput_hooks( iface, FALSE ); return hr; } -- 2.33.0
The read buffer is only used by the reading thread but the device state is not, we should only update it while holding the CS. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/dinput/joystick_hid.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/dinput/joystick_hid.c b/dlls/dinput/joystick_hid.c index 208e73835e9..64eae772381 100644 --- a/dlls/dinput/joystick_hid.c +++ b/dlls/dinput/joystick_hid.c @@ -1495,6 +1495,7 @@ static HRESULT hid_joystick_read_state( IDirectInputDevice8W *iface ) } } + EnterCriticalSection( &impl->base.crit ); do { count = impl->usages_count; @@ -1530,6 +1531,7 @@ static HRESULT hid_joystick_read_state( IDirectInputDevice8W *iface ) memset( &impl->read_ovl, 0, sizeof(impl->read_ovl) ); impl->read_ovl.hEvent = impl->base.read_event; } while (ReadFile( impl->device, report_buf, report_len, &count, &impl->read_ovl )); + LeaveCriticalSection( &impl->base.crit ); return DI_OK; } -- 2.33.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99742 Your paranoid android. === debiant2 (32 bit report) === dinput8: driver_hid.c:106: Test failed: hid.c:6669 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5299 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5416 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5441 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5708 expect[0]: missing (code 0xb000f id 1 len 2) === debiant2 (32 bit Chinese:China report) === dinput8: driver_hid.c:106: Test failed: hid.c:6669 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5299 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5416 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5441 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5708 expect[0]: missing (code 0xb000f id 1 len 2) === debiant2 (64 bit WoW report) === dinput8: driver_hid.c:106: Test failed: hid.c:6669 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5299 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5416 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5441 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5708 expect[0]: missing (code 0xb000f id 1 len 2)
Otherwise we may later write the cancelled status to invalid memory. Also use a manual-reset event, as it should be for overlapped I/O, so all waiters are woken up on cancel. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/dinput/joystick_hid.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/dlls/dinput/joystick_hid.c b/dlls/dinput/joystick_hid.c index 64eae772381..2e26000d10e 100644 --- a/dlls/dinput/joystick_hid.c +++ b/dlls/dinput/joystick_hid.c @@ -543,7 +543,6 @@ static ULONG hid_joystick_private_decref( struct hid_joystick *impl ) HeapFree( GetProcessHeap(), 0, tmp.input_report_buf ); HeapFree( GetProcessHeap(), 0, tmp.input_extra_caps ); HidD_FreePreparsedData( tmp.preparsed ); - CancelIoEx( tmp.device, &tmp.read_ovl ); CloseHandle( tmp.base.read_event ); CloseHandle( tmp.device ); } @@ -925,7 +924,7 @@ static HRESULT WINAPI hid_joystick_Unacquire( IDirectInputDevice8W *iface ) { ret = CancelIoEx( impl->device, &impl->read_ovl ); if (!ret) WARN( "CancelIoEx failed, last error %u\n", GetLastError() ); - + else WaitForSingleObject( impl->base.read_event, INFINITE ); IDirectInputDevice8_SendForceFeedbackCommand( iface, DISFFC_RESET ); impl->base.acquired = FALSE; } @@ -2009,7 +2008,7 @@ static HRESULT hid_joystick_create_device( IDirectInputImpl *dinput, const GUID if (FAILED(hr)) return hr; impl->base.crit.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": hid_joystick.base.crit"); impl->base.dwCoopLevel = DISCL_NONEXCLUSIVE | DISCL_BACKGROUND; - impl->base.read_event = CreateEventA( NULL, FALSE, FALSE, NULL ); + impl->base.read_event = CreateEventW( NULL, TRUE, FALSE, NULL ); impl->base.read_callback = hid_joystick_read_state; hr = hid_joystick_device_open( -1, &instance, impl->device_path, &impl->device, &impl->preparsed, -- 2.33.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99743 Your paranoid android. === debiant2 (32 bit report) === dinput8: driver_hid.c:106: Test failed: hid.c:6669 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5299 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5416 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5441 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5708 expect[0]: missing (code 0xb000f id 1 len 2) === debiant2 (32 bit Chinese:China report) === dinput8: driver_hid.c:106: Test failed: hid.c:6669 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5299 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5416 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5441 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5708 expect[0]: missing (code 0xb000f id 1 len 2) === debiant2 (64 bit WoW report) === dinput8: driver_hid.c:106: Test failed: hid.c:6669 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5299 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5416 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5441 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5708 expect[0]: missing (code 0xb000f id 1 len 2)
And remove the device from the list of acquired devices if the callback indicates a read error. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/dinput/dinput_main.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/dlls/dinput/dinput_main.c b/dlls/dinput/dinput_main.c index 1cd6d390430..fd889222b39 100644 --- a/dlls/dinput/dinput_main.c +++ b/dlls/dinput/dinput_main.c @@ -1289,8 +1289,7 @@ static LRESULT CALLBACK callwndproc_proc( int code, WPARAM wparam, LPARAM lparam static DWORD WINAPI hook_thread_proc(void *param) { static HHOOK kbd_hook, mouse_hook; - IDirectInputDeviceImpl *impl; - IDirectInputDevice8W *iface; + IDirectInputDeviceImpl *impl, *next; SIZE_T events_count = 0; HANDLE finished_event; HANDLE events[128]; @@ -1310,24 +1309,17 @@ static DWORD WINAPI hook_thread_proc(void *param) if (ret < events_count) { - iface = NULL; EnterCriticalSection( &dinput_hook_crit ); - LIST_FOR_EACH_ENTRY( impl, &acquired_device_list, IDirectInputDeviceImpl, entry ) + LIST_FOR_EACH_ENTRY_SAFE( impl, next, &acquired_device_list, IDirectInputDeviceImpl, entry ) { if (impl->read_event == events[ret]) { - iface = &impl->IDirectInputDevice8W_iface; - IDirectInputDevice8_AddRef( iface ); + hr = impl->read_callback( &impl->IDirectInputDevice8W_iface ); + if (FAILED(hr)) list_remove( &impl->entry ); break; } } LeaveCriticalSection( &dinput_hook_crit ); - - if (iface) - { - impl->read_callback( iface ); - IDirectInputDevice8_Release( iface ); - } } while (PeekMessageW( &msg, 0, 0, 0, PM_REMOVE )) -- 2.33.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99744 Your paranoid android. === debiant2 (build log) === ../wine/dlls/dinput/dinput_main.c:1317:21: error: ‘hr’ undeclared (first use in this function) ../wine/dlls/dinput/dinput_main.c:1317:21: error: ‘hr’ undeclared (first use in this function) Task: The win32 Wine build failed === debiant2 (build log) === ../wine/dlls/dinput/dinput_main.c:1317:21: error: ‘hr’ undeclared (first use in this function) ../wine/dlls/dinput/dinput_main.c:1317:21: error: ‘hr’ undeclared (first use in this function) Task: The wow64 Wine build failed
Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/dinput/dinput_main.c | 1 + dlls/dinput/joystick_hid.c | 57 ++++++++++++++++++++++++++++---------- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/dlls/dinput/dinput_main.c b/dlls/dinput/dinput_main.c index fd889222b39..9c0e3875c00 100644 --- a/dlls/dinput/dinput_main.c +++ b/dlls/dinput/dinput_main.c @@ -1293,6 +1293,7 @@ static DWORD WINAPI hook_thread_proc(void *param) SIZE_T events_count = 0; HANDLE finished_event; HANDLE events[128]; + HRESULT hr; DWORD ret; MSG msg; diff --git a/dlls/dinput/joystick_hid.c b/dlls/dinput/joystick_hid.c index 2e26000d10e..d6105c12e4f 100644 --- a/dlls/dinput/joystick_hid.c +++ b/dlls/dinput/joystick_hid.c @@ -881,6 +881,7 @@ static HRESULT WINAPI hid_joystick_Acquire( IDirectInputDevice8W *iface ) struct hid_joystick *impl = impl_from_IDirectInputDevice8W( iface ); ULONG report_len = impl->caps.InputReportByteLength; HRESULT hr = DI_OK; + BOOL ret; TRACE( "iface %p.\n", iface ); @@ -891,15 +892,29 @@ static HRESULT WINAPI hid_joystick_Acquire( IDirectInputDevice8W *iface ) hr = DIERR_INVALIDPARAM; else if ((impl->base.dwCoopLevel & DISCL_FOREGROUND) && impl->base.win != GetForegroundWindow()) hr = DIERR_OTHERAPPHASPRIO; - else + else if (impl->device == INVALID_HANDLE_VALUE) + { + impl->device = CreateFileW( impl->device_path, GENERIC_READ | GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, + NULL, OPEN_EXISTING, FILE_FLAG_OVERLAPPED | FILE_FLAG_NO_BUFFERING, 0 ); + if (impl->device == INVALID_HANDLE_VALUE) hr = DIERR_INPUTLOST; + } + + if (hr == DI_OK) { memset( &impl->read_ovl, 0, sizeof(impl->read_ovl) ); impl->read_ovl.hEvent = impl->base.read_event; - if (ReadFile( impl->device, impl->input_report_buf, report_len, NULL, &impl->read_ovl )) - impl->base.read_callback( iface ); - - IDirectInputDevice8_SendForceFeedbackCommand( iface, DISFFC_RESET ); - impl->base.acquired = TRUE; + ret = ReadFile( impl->device, impl->input_report_buf, report_len, NULL, &impl->read_ovl ); + if (!ret && GetLastError() != ERROR_IO_PENDING) + { + CloseHandle( impl->device ); + impl->device = INVALID_HANDLE_VALUE; + hr = DIERR_INPUTLOST; + } + else + { + IDirectInputDevice8_SendForceFeedbackCommand( iface, DISFFC_RESET ); + impl->base.acquired = TRUE; + } } LeaveCriticalSection( &impl->base.crit ); if (hr != DI_OK) return hr; @@ -922,9 +937,12 @@ static HRESULT WINAPI hid_joystick_Unacquire( IDirectInputDevice8W *iface ) if (!impl->base.acquired) hr = DI_NOEFFECT; else { - ret = CancelIoEx( impl->device, &impl->read_ovl ); - if (!ret) WARN( "CancelIoEx failed, last error %u\n", GetLastError() ); - else WaitForSingleObject( impl->base.read_event, INFINITE ); + if (impl->device != INVALID_HANDLE_VALUE) + { + ret = CancelIoEx( impl->device, &impl->read_ovl ); + if (!ret) WARN( "CancelIoEx failed, last error %u\n", GetLastError() ); + else WaitForSingleObject( impl->base.read_event, INFINITE ); + } IDirectInputDevice8_SendForceFeedbackCommand( iface, DISFFC_RESET ); impl->base.acquired = FALSE; } @@ -1475,11 +1493,11 @@ static HRESULT hid_joystick_read_state( IDirectInputDevice8W *iface ) char *report_buf = impl->input_report_buf; USAGE_AND_PAGE *usages; NTSTATUS status; + HRESULT hr; BOOL ret; ret = GetOverlappedResult( impl->device, &impl->read_ovl, &count, FALSE ); - if (!ret) WARN( "ReadFile failed, error %u\n", GetLastError() ); - else if (TRACE_ON(dinput)) + if (ret && TRACE_ON(dinput)) { TRACE( "read size %u report:\n", count ); for (i = 0; i < report_len;) @@ -1495,7 +1513,7 @@ static HRESULT hid_joystick_read_state( IDirectInputDevice8W *iface ) } EnterCriticalSection( &impl->base.crit ); - do + while (ret) { count = impl->usages_count; memset( impl->usages_buf, 0, count * sizeof(*impl->usages_buf) ); @@ -1529,10 +1547,21 @@ static HRESULT hid_joystick_read_state( IDirectInputDevice8W *iface ) memset( &impl->read_ovl, 0, sizeof(impl->read_ovl) ); impl->read_ovl.hEvent = impl->base.read_event; - } while (ReadFile( impl->device, report_buf, report_len, &count, &impl->read_ovl )); + ret = ReadFile( impl->device, report_buf, report_len, &count, &impl->read_ovl ); + } + + if (GetLastError() == ERROR_IO_PENDING || GetLastError() == ERROR_OPERATION_ABORTED) hr = DI_OK; + else + { + WARN( "GetOverlappedResult/ReadFile failed, error %u\n", GetLastError() ); + CloseHandle(impl->device); + impl->device = INVALID_HANDLE_VALUE; + impl->base.acquired = FALSE; + hr = DIERR_INPUTLOST; + } LeaveCriticalSection( &impl->base.crit ); - return DI_OK; + return hr; } static DWORD device_type_for_version( DWORD type, DWORD version ) -- 2.33.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99745 Your paranoid android. === debiant2 (32 bit report) === dinput8: driver_hid.c:106: Test failed: hid.c:6669 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5299 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5416 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5441 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5708 expect[0]: missing (code 0xb000f id 1 len 2) === debiant2 (32 bit Chinese:China report) === dinput8: driver_hid.c:106: Test failed: hid.c:6669 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5299 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5416 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5441 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5708 expect[0]: missing (code 0xb000f id 1 len 2) === debiant2 (64 bit WoW report) === dinput8: driver_hid.c:106: Test failed: hid.c:6669 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5299 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5416 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5441 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5708 expect[0]: missing (code 0xb000f id 1 len 2)
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=99741 Your paranoid android. === debiant2 (32 bit report) === dinput8: driver_hid.c:106: Test failed: hid.c:6669 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5299 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5416 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5441 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5708 expect[0]: missing (code 0xb000f id 1 len 2) === debiant2 (32 bit Chinese:China report) === dinput8: driver_hid.c:106: Test failed: hid.c:6669 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5299 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5416 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5441 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5708 expect[0]: missing (code 0xb000f id 1 len 2) === debiant2 (64 bit WoW report) === dinput8: driver_hid.c:106: Test failed: hid.c:6669 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5299 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5416 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5441 expect[0]: missing (code 0xb000f id 1 len 2) driver_hid.c:106: Test failed: hid.c:5708 expect[0]: missing (code 0xb000f id 1 len 2)
participants (2)
-
Marvin -
Rémi Bernon