Queue macOS input reports so that large responses aren't dropped

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;
     }