Fix fundamental misunderstanding of how HID I/O reports work on macOS

This addresses a bug where if a YubiKey command would result in multiple input reports being generated, we would get the callbacks but drop the results on the floor. This would generally result in the runloop timing out because we already had received the report via the callback but had no way of caching it.

Author: Greg Domzalski

Yubico.Core/src/Yubico/Core/Devices/Hid/MacOSHidIOReportConnection.cs

@@ -13,8 +13,8 @@
 // limitations under the License.
 
 using System;
+using System.Collections.Concurrent;
 using System.Globalization;
-using System.Linq;
 using System.Runtime.InteropServices;
 using System.Text;
 using Yubico.Core.Buffers;
@@ -35,8 +35,12 @@ internal sealed class MacOSHidIOReportConnection : IHidConnection
         private readonly MacOSHidDevice _device;
         private readonly IntPtr _loopId;
         private readonly Logger _log = Log.GetLogger();
+
         private readonly byte[] _readBuffer;
-        private readonly GCHandle _readHandle;
+        private GCHandle _readHandle;
+
+        private readonly ConcurrentQueue<byte[]> _reportsQueue;
+        private GCHandle _pinnedReportsQueue;
 
         /// <summary>
         /// The correct size, in bytes, for the data buffer to be transmitted to the device.
@@ -67,9 +71,15 @@ public MacOSHidIOReportConnection(MacOSHidDevice device, long entryId)
             byte[] cstr = Encoding.UTF8.GetBytes($"fido2-loopid-{entryId}");
             _loopId = CFStringCreateWithCString(IntPtr.Zero, cstr, 0);
 
+            // The following buffer must be pinned because the native function must retain a pointer (i.e. the address)
             _readBuffer = new byte[64];
             _readHandle = GCHandle.Alloc(_readBuffer, GCHandleType.Pinned);
 
+            // This object is marshalled using a normal handle since .NET is on the other side and is capable of using
+            // the GCHandle (i.e. we do not need the address)
+            _reportsQueue = new ConcurrentQueue<byte[]>();
+            _pinnedReportsQueue = GCHandle.Alloc(_reportsQueue);
+
             SetupConnection();
 
             InputReportSize = IOKitHelpers.GetIntPropertyValue(_deviceHandle, IOKitHidConstants.MaxInputReportSize);
@@ -124,7 +134,7 @@ private void SetupConnection()
                     _readBuffer,
                     _readBuffer.Length,
                     reportCallback,
-                    GCHandle.ToIntPtr(_readHandle));
+                    GCHandle.ToIntPtr(_pinnedReportsQueue));
 
                 IntPtr callback = Marshal.GetFunctionPointerForDelegate<IOHIDCallback>(RemovalCallback);
                 IOHIDDeviceRegisterRemovalCallback(_deviceHandle, callback, _deviceHandle);
@@ -150,49 +160,50 @@ private void SetupConnection()
         /// </exception>
         public byte[] GetReport()
         {
-            try
+            if (_reportsQueue.TryDequeue(out byte[] report))
             {
-                IntPtr runLoop = CFRunLoopGetCurrent();
+                // If there's already a report in the queue (i.e. the callback beat us to calling GetReport) return
+                // that one immediately.
+                _log.SensitiveLogInformation(
+                    "GetReport returned buffer: {Report}",
+                    Hex.BytesToHex(report));
 
-                IOHIDDeviceScheduleWithRunLoop(_deviceHandle, runLoop, _loopId);
+                return report;
+            }
 
-                // The YubiKey has a reclaim timeout of 3 seconds. This can cause the SDK some trouble if we just
-                // switched out of a different USB interface (like Keyboard or CCID). We previously used a fairly
-                // tight timeout of 4 seconds, but that seemed to not always work. 6 seconds (double the timeout)
-                // seems like a more reasonable timeout for the operating system.
-                int runLoopResult = CFRunLoopRunInMode(_loopId, 6, true);
+            // Otherwise start up the IO runloop and see if we find more reports to pick up.
+            IntPtr runLoop = CFRunLoopGetCurrent();
 
-                _device.LogDeviceAccessTime();
+            IOHIDDeviceScheduleWithRunLoop(_deviceHandle, runLoop, _loopId);
 
-                if (runLoopResult != kCFRunLoopRunHandledSource)
-                {
-                    throw new PlatformApiException(
-                        string.Format(
-                            CultureInfo.CurrentCulture,
-                            ExceptionMessages.WrongIOKitRunLoopMode,
-                            runLoopResult));
-                }
+            // The YubiKey has a reclaim timeout of 3 seconds. This can cause the SDK some trouble if we just
+            // switched out of a different USB interface (like Keyboard or CCID). We previously used a fairly
+            // tight timeout of 4 seconds, but that seemed to not always work. 6 seconds (double the timeout)
+            // seems like a more reasonable timeout for the operating system.
+            int runLoopResult = CFRunLoopRunInMode(_loopId, 6, true);
 
-                IOHIDDeviceUnscheduleFromRunLoop(_deviceHandle, runLoop, _loopId);
-
-                _log.SensitiveLogInformation(
-                    "GetReport returned buffer: {Report}",
-                    Hex.BytesToHex(_readBuffer));
+            _device.LogDeviceAccessTime();
 
-                // Return a copy of the report
-                return _readBuffer.ToArray();
-            }
-            finally
+            if (runLoopResult != kCFRunLoopRunHandledSource)
             {
-                IOHIDDeviceRegisterInputReportCallback(
-                    _deviceHandle,
-                    _readBuffer,
-                    _readBuffer.Length,
-                    IntPtr.Zero,
-                    IntPtr.Zero);
-
-                IOHIDDeviceRegisterRemovalCallback(_deviceHandle, IntPtr.Zero, IntPtr.Zero);
+                throw new PlatformApiException(
+                    string.Format(
+                        CultureInfo.CurrentCulture,
+                        ExceptionMessages.WrongIOKitRunLoopMode,
+                        runLoopResult));
             }
+
+            IOHIDDeviceUnscheduleFromRunLoop(_deviceHandle, runLoop, _loopId);
+
+            // We should be guaranteed to have a report here - otherwise the runloop would have timed out
+            // and the PlatformApiException above would have been thrown.
+            _ = _reportsQueue.TryDequeue(out report);
+
+            _log.SensitiveLogInformation(
+                "GetReport returned buffer: {Report}",
+                Hex.BytesToHex(report));
+
+            return report;
         }
 
         /// <summary>
@@ -232,7 +243,7 @@ private static void ReportCallback(
 
             log.LogInformation("MacOSHidIOReportConnection.ReportCallback has been called.");
 
-            if (result != 0 || type != IOKitHidConstants.kIOHidReportTypeOutput || reportId != 0 || reportLength < 0)
+            if (result != 0 || type != IOKitHidConstants.kIOHidReportTypeInput || reportId != 0 || reportLength < 0)
             {
                 // Something went wrong. We don't currently signal, just continue.
                 log.LogWarning(
@@ -242,19 +253,10 @@ private static void ReportCallback(
                     type,
                     reportId,
                     reportLength);
-
-                return;
             }
 
-            byte[] buffer = (byte[])GCHandle.FromIntPtr(context).Target;
-            long length = Math.Min(buffer.Length, reportLength);
-            log.LogInformation(
-                "Buffer length determined to be {Length} bytes. (buffer.Length was {BufferLength}, and reportLength was {ReportLength}",
-                length,
-                buffer.Length,
-                reportLength);
-
-            Array.Copy(report, buffer, length);
+            var reportsQueue = (ConcurrentQueue<byte[]>)GCHandle.FromIntPtr(context).Target;
+            reportsQueue.Enqueue(report);
         }
 
         /// <summary>
@@ -324,11 +326,25 @@ private void Dispose(bool disposing)
                 // Dispose managed state here
             }
 
+            IOHIDDeviceRegisterInputReportCallback(
+                _deviceHandle,
+                _readBuffer,
+                _readBuffer.Length,
+                IntPtr.Zero,
+                IntPtr.Zero);
+
+            IOHIDDeviceRegisterRemovalCallback(_deviceHandle, IntPtr.Zero, IntPtr.Zero);
+
             if (_readHandle.IsAllocated)
             {
                 _readHandle.Free();
             }
 
+            if (_pinnedReportsQueue.IsAllocated)
+            {
+                _pinnedReportsQueue.Free();
+            }
+
             if (_deviceHandle != IntPtr.Zero)
             {
                 _ = IOHIDDeviceClose(_deviceHandle, 0);

Yubico.Core/src/Yubico/PlatformInterop/macOS/IOKitFramework/IOKitHidConstants.cs

@@ -24,6 +24,7 @@ internal static class IOKitHidConstants
         public const string MaxInputReportSize = "MaxInputReportSize";
         public const string MaxOutputReportSize = "MaxOutputReportSize";
 
+        public const int kIOHidReportTypeInput = 0;
         public const int kIOHidReportTypeOutput = 1;
         public const int kIOHidReportTypeFeature = 2;
     }