diff --git a/Yubico.Core/src/Yubico/Core/Devices/Hid/MacOSHidIOReportConnection.cs b/Yubico.Core/src/Yubico/Core/Devices/Hid/MacOSHidIOReportConnection.cs index efc0d26b8..57413fc5c 100644 --- a/Yubico.Core/src/Yubico/Core/Devices/Hid/MacOSHidIOReportConnection.cs +++ b/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 _reportsQueue; + private GCHandle _pinnedReportsQueue; /// /// 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(); + _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(RemovalCallback); IOHIDDeviceRegisterRemovalCallback(_deviceHandle, callback, _deviceHandle); @@ -150,49 +160,50 @@ private void SetupConnection() /// 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; } /// @@ -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)GCHandle.FromIntPtr(context).Target; + reportsQueue.Enqueue(report); } /// @@ -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); diff --git a/Yubico.Core/src/Yubico/PlatformInterop/macOS/IOKitFramework/IOKitHidConstants.cs b/Yubico.Core/src/Yubico/PlatformInterop/macOS/IOKitFramework/IOKitHidConstants.cs index afc2a4c2c..f39866573 100644 --- a/Yubico.Core/src/Yubico/PlatformInterop/macOS/IOKitFramework/IOKitHidConstants.cs +++ b/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; }