Signed-off-by: Tim Clem tclem@codeweavers.com --- Ideally we would use IOHIDDeviceCopyValueMultiple or IOHIDTransactions to fetch all these element values at once, but my own testing and this message from an Apple engineer confirm that those don't work for input elements: https://www.mail-archive.com/usb%40lists.apple.com/msg00438.html
dlls/dinput/joystick_osx.c | 57 +++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 23 deletions(-)
diff --git a/dlls/dinput/joystick_osx.c b/dlls/dinput/joystick_osx.c index e8732bd696c0..2db9e521a1ed 100644 --- a/dlls/dinput/joystick_osx.c +++ b/dlls/dinput/joystick_osx.c @@ -110,6 +110,7 @@ struct JoystickImpl /* osx private */ int id; CFArrayRef elements; + int *element_values; ObjProps **propmap; FFDeviceObjectReference ff; struct list effects; @@ -753,6 +754,9 @@ static void get_osx_device_elements(JoystickImpl *device, uint64_t axis_map[8]) CFRelease(povs); CFRelease(buttons); CFRelease(elements); + + device->element_values = HeapAlloc(GetProcessHeap(), 0, + CFArrayGetCount(device->elements) * sizeof(int)); } else { @@ -786,6 +790,29 @@ static void get_osx_device_elements_props(JoystickImpl *device) } }
+static IOReturn get_element_values(IOHIDDeviceRef hid_device, JoystickImpl *device) +{ + CFIndex i, element_count = CFArrayGetCount(device->elements); + IOReturn ret = kIOReturnSuccess; + IOHIDElementRef element; + IOHIDValueRef valueRef; + + for (i = 0; i < element_count; i++) + { + element = (IOHIDElementRef)CFArrayGetValueAtIndex(device->elements, i); + ret = IOHIDDeviceGetValue(hid_device, element, &valueRef); + if (ret != kIOReturnSuccess) + { + ERR("error getting value of element %s: %08x\n", debugstr_element(element), ret); + break; + } + + device->element_values[i] = IOHIDValueGetIntegerValue(valueRef); + } + + return ret; +} + static void poll_osx_device_state( IDirectInputDevice8W *iface ) { JoystickImpl *device = impl_from_IDirectInputDevice8W( iface ); @@ -805,6 +832,9 @@ static void poll_osx_device_state( IDirectInputDevice8W *iface )
if (device->elements) { + if (get_element_values(hid_device, device) != kIOReturnSuccess) + return; + int button_idx = 0; int pov_idx = 0; int slider_idx = 0; @@ -813,8 +843,7 @@ static void poll_osx_device_state( IDirectInputDevice8W *iface )
for ( idx = 0; idx < cnt; idx++ ) { - IOHIDValueRef valueRef; - int val, oldVal, newVal; + int oldVal, newVal, val = device->element_values[idx]; IOHIDElementRef element = ( IOHIDElementRef ) CFArrayGetValueAtIndex( device->elements, idx ); int type = IOHIDElementGetType( element );
@@ -826,16 +855,10 @@ static void poll_osx_device_state( IDirectInputDevice8W *iface ) TRACE("kIOHIDElementTypeInput_Button\n"); if(button_idx < 128) { - valueRef = NULL; - if (IOHIDDeviceGetValue(hid_device, element, &valueRef) != kIOReturnSuccess) - return; - if (valueRef == NULL) - return; - val = IOHIDValueGetIntegerValue(valueRef); newVal = val ? 0x80 : 0x0; oldVal = device->generic.js.rgbButtons[button_idx]; device->generic.js.rgbButtons[button_idx] = newVal; - TRACE("valueRef %s val %d oldVal %d newVal %d\n", debugstr_cf(valueRef), val, oldVal, newVal); + TRACE("val %d oldVal %d newVal %d\n", val, oldVal, newVal); if (oldVal != newVal) { inst_id = DIDFT_MAKEINSTANCE(button_idx) | DIDFT_PSHBUTTON; @@ -855,19 +878,13 @@ static void poll_osx_device_state( IDirectInputDevice8W *iface ) case MAKEUINT64(kHIDPage_GenericDesktop, kHIDUsage_GD_Hatswitch): { TRACE("kIOHIDElementTypeInput_Misc / kHIDUsage_GD_Hatswitch\n"); - valueRef = NULL; - if (IOHIDDeviceGetValue(hid_device, element, &valueRef) != kIOReturnSuccess) - return; - if (valueRef == NULL) - return; - val = IOHIDValueGetIntegerValue(valueRef); oldVal = device->generic.js.rgdwPOV[pov_idx]; if ((val > device->generic.props[idx].lDevMax) || (val < device->generic.props[idx].lDevMin)) newVal = -1; else newVal = (val - device->generic.props[idx].lDevMin) * 4500; device->generic.js.rgdwPOV[pov_idx] = newVal; - TRACE("valueRef %s val %d oldVal %d newVal %d\n", debugstr_cf(valueRef), val, oldVal, newVal); + TRACE("val %d oldVal %d newVal %d\n", val, oldVal, newVal); if (oldVal != newVal) { inst_id = DIDFT_MAKEINSTANCE(pov_idx) | DIDFT_POV; @@ -890,12 +907,6 @@ static void poll_osx_device_state( IDirectInputDevice8W *iface ) { int wine_obj = -1;
- valueRef = NULL; - if (IOHIDDeviceGetValue(hid_device, element, &valueRef) != kIOReturnSuccess) - return; - if (valueRef == NULL) - return; - val = IOHIDValueGetIntegerValue(valueRef); newVal = joystick_map_axis(&device->generic.props[idx], val); switch (MAKEUINT64(usage_page, usage)) { @@ -945,7 +956,7 @@ static void poll_osx_device_state( IDirectInputDevice8W *iface ) slider_idx ++; break; } - TRACE("valueRef %s val %d oldVal %d newVal %d\n", debugstr_cf(valueRef), val, oldVal, newVal); + TRACE("val %d oldVal %d newVal %d\n", val, oldVal, newVal); if ((wine_obj != -1) && (oldVal != newVal)) {
If multiple threads try to read IOHIDElements and IOHIDValues from the same IOHIDDevice simultaneously, we sometimes crash deep in IOKit.
Fixes a crash in GTA 4 when using a PS4 controller.
Signed-off-by: Tim Clem tclem@codeweavers.com --- Testing with multiple controllers attached confirms that the per-HID device lock is appropriate. It looks like the kernel releases the underlying memory referenced by a IOHIDValueRef if you fetch another for the same element via IOHIDDeviceGetValue.
Note that the same issue is present in winejoystick.drv, though there it is mitigated by the fact that you can only open each controller main element once.
I'll write an analogous patch for winejoystick.drv after this one is reviewed. Even that won't help in the case of an app using both dinput and winmm to talk to the same device, but I imagine that's vanishingly rare.
The use of HID reports directly, as implemented in xinput and Remi's joystick_hid, should completely avoid this issue.
dlls/dinput/joystick_osx.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/dlls/dinput/joystick_osx.c b/dlls/dinput/joystick_osx.c index 2db9e521a1ed..9ee45e8a4456 100644 --- a/dlls/dinput/joystick_osx.c +++ b/dlls/dinput/joystick_osx.c @@ -100,6 +100,9 @@ WINE_DEFAULT_DEBUG_CHANNEL(dinput);
static CFMutableArrayRef device_main_elements = NULL;
+/* Maps IOHIDDeviceRefs to CRITICAL_SECTION pointers. */ +static CFMutableDictionaryRef hid_device_crits = NULL; + typedef struct JoystickImpl JoystickImpl; static const IDirectInputDevice8WVtbl JoystickWvt;
@@ -534,8 +537,11 @@ static int find_osx_devices(void) CFArraySortValues(devices, CFRangeMake(0, num_devices), device_location_name_comparator, NULL);
device_main_elements = CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks); - if (!device_main_elements) + hid_device_crits = CFDictionaryCreateMutable(kCFAllocatorDefault, 0, &kCFTypeDictionaryKeyCallBacks, NULL); + if (!device_main_elements || !hid_device_crits) { + if (device_main_elements) CFRelease(device_main_elements); + if (hid_device_crits) CFRelease(hid_device_crits); CFRelease( devices ); goto fail; } @@ -550,6 +556,15 @@ static int find_osx_devices(void) TRACE("hid_device %s\n", debugstr_device(hid_device)); top = find_top_level(hid_device, device_main_elements); num_main_elements += top; + + if ( top ) { + /* This HID device has relevent elements, so it needs a critical section. */ + CRITICAL_SECTION *cs = HeapAlloc(GetProcessHeap(), 0, sizeof(CRITICAL_SECTION)); + InitializeCriticalSection(cs); + cs->DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": hid_device_crits"); + + CFDictionaryAddValue(hid_device_crits, hid_device, cs); + } }
CFRelease(devices); @@ -797,6 +812,20 @@ static IOReturn get_element_values(IOHIDDeviceRef hid_device, JoystickImpl *devi IOHIDElementRef element; IOHIDValueRef valueRef;
+ /* If more than one thread is trying to poll the same HID device, we can + * crash deep inside IOKit. So we enter a per-IOHIDDevice critical section + * here. Note that it must be per-HID device and not just per-JoystickImpl, + * as there may be multiple JoystickImpls referencing the same underlying + * device. */ + + CRITICAL_SECTION *crit = (CRITICAL_SECTION *)CFDictionaryGetValue(hid_device_crits, hid_device); + if (!crit) { + ERR("missing critical section for device %s\n", debugstr_device(hid_device)); + return kIOReturnError; + } + + EnterCriticalSection(crit); + for (i = 0; i < element_count; i++) { element = (IOHIDElementRef)CFArrayGetValueAtIndex(device->elements, i); @@ -810,6 +839,7 @@ static IOReturn get_element_values(IOHIDDeviceRef hid_device, JoystickImpl *devi device->element_values[i] = IOHIDValueGetIntegerValue(valueRef); }
+ LeaveCriticalSection(crit); return ret; }
-
Tim Clem