[PATCH 1/2] dinput: Factor out HID value retrieval in joystick_osx

[Tim Clem]

Signed-off-by: Tim Clem <tclem(a)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))
                             {
-- 
2.33.0

[PATCH 2/2] dinput: Lock around polling a HID device in joystick_osx.

[Tim Clem]

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(a)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;
 }
 
-- 
2.33.0