fix: concurrency resiliency #528
Open
DennisDyallo wants to merge 10 commits into
Open
Conversation
Windows: - Declare CM_NOTIFY_FILTER with Size = 416 to match the Windows SDK layout; previous 32-byte marshaling made CM_Register_Notification fail so the listener never started (H1) - Emit fallback rescan hint when arrival processing throws (M1) - Use a weak GCHandle for the callback context and free registration state on failed Start (M2, M3) - Validate eventDataSize before reading the symbolic link (L5) - Add CM_NOTIFY_FILTER struct-size unit test macOS: - Remove IOHIDManagerOpen/Close: matching/removal callbacks do not require an opened manager and opening triggers Input Monitoring TCC denial (kIOReturnNotPermitted) on macOS 10.15+ (H2) - CFRetain the listener run loop while the thread uses it (M8) - Clean up stale native state on Start after Error; abandon (not free) native handles when the thread misses the join timeout (M6, M7) - Report entry ID only as PlatformDeviceId, not as DevicePath (L3) Linux: - Extract ILinuxHidEventSource seam wrapping udev/eventfd/poll native mechanics; listener now session-scoped so an abandoned thread can never touch a newer session's file descriptors (H5, M6, M7) - Emit Unknown rescan hint on receive failure or missing action instead of silently dropping (H3) - Delete write-only identity caches (M4), dead poll branch (L4); per-event exception handling keeps the loop alive (L7) Monitor service: - Cap debounce coalescing at MaxCoalesceInterval (5x throttle) so a sustained hint storm cannot starve rescans (M9) - Serialize RescanAsync and monitor-loop rescans behind a semaphore; DisposeAsync uses the shared stop path under the lifecycle lock (M10, M11) Base listener: - Volatile Status backing field (L11); document Stop/Dispose re-entrancy hazards from the DeviceEvent callback (M5) Tests: - Linux fault-injection suite via fake event source: poll/fd failures bound to a single Error transition, shutdown unblocks promptly, receive failures hint instead of suppress, hidraw readiness fallback re-hints, restart after Error works (H5, M13) - Hint-burst coalescing bound and storm-cap latency tests (H4) - Manager tests use fake listeners and interlocked counters so real hotplug events cannot perturb ScanCount assertions (M12) - RuntimeResilience traits on monitor loop tests (L8) Docs: - Migration map/changelog reference the real v1 develop API (HidDeviceListener Arrived/Removed EventHandler<HidDeviceEventArgs>) and record reviewed commit SHAs (H6, M14) - Compilable device-discovery examples; architecture doc matches implemented debounce/coalescing internals (H7, M15, L10) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…exit Address Copilot review findings on PR #526: - StartMonitoring: treat a completed monitoring task as not monitoring; tear down stale listeners/CTS and restart instead of returning silently - StopMonitoring: honor Task.Wait timeout result; on timeout log a warning and abandon the loop without disposing its CancellationTokenSource - DisposeAsync: bound the monitoring-task and rescan-gate waits with a shutdown timeout (default 10s); abandon the semaphore on timeout instead of disposing it under a still-running rescan - Add internal shutdownTimeout ctor parameter as a test seam - Add DisposeAsync_RescanHungIgnoringCancellation_CompletesWithinBoundedTime fault-injection test (FakeFindYubiKeys hang mode ignoring cancellation) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- ReadSymbolicLink uses the length-bounded PtrToStringUni overload and trims at the first NUL instead of trusting native NUL-termination - document why the notification GCHandle is Weak (finalizer must stay reachable to recover leaked registrations) and log dropped callbacks for collected listeners - add cross-platform unit tests for the payload bounds behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ll scans A card blocked in a long native operation (e.g. RSA-4096 keygen holding SCardTransmit for ~2 min) stalled FindYubiKeys.FindAllAsync while it held _scanLock, backing up the monitor pipeline. Identity reads had retries but no per-attempt timeout; metadata reads used CancelAfter, which cannot abort an in-flight Task.Run(SCardTransmit). - Add ProtocolDeviceInfo.ReadBoundedAsync: runs connect+read as one task, bounds the wait with Task.WaitAsync; on timeout/cancel the read is abandoned (not aborted), observed via continuation, and disposes its protocol/connection when the native call returns - DiscoveryIdentityReader: 2s per-attempt budget; timeout returns null immediately (no retry), other failures keep 3-attempt backoff retry - CompositeMetadataReader: hard total wall-clock budget across transports (Stopwatch remaining-budget) replacing soft per-read CancelAfter - FindYubiKeys: rename MetadataReadTimeout to MetadataReadBudget - Add unit proof (hanging connect must not stall TryReadAsync) and PIV integration proof (scan completes <4s during in-flight RSA-4096 keygen) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Discovery's cache-miss device-info reads opened a second shared-mode CCID handle and issued SELECT Management, deselecting the applet and destroying auth state of any open session on the same interface (proven on hardware: PIN-verified PIV sign failed SW=0x6D00 after a cold FindAllAsync). - Add DeviceConnectionRegistry: in-process live-connection refcounts keyed by per-interface DeviceId; ResolveInterfaceId mirrors CompositeYubiKey first-supporting-member routing - Register connections in PcscYubiKey/HidYubiKey.ConnectAsync via transparent decorators (RegisteredConnections) that release the registration exactly once on dispose - DiscoveryIdentityReader: skip in-use interfaces (serial unknown => conservative no-merge), no retries on skip - CompositeMetadataReader: skip only the in-use transport, still read free transports - ProtocolDeviceInfo: post-connect IsInUseByOther re-check closes the TOCTOU window between pre-check and first APDU; aborts via DiscoveryReadSkippedException before transmitting - Add 5 registry unit tests incl. TOCTOU abort-before-transmit; add hardware proof test FindAllAsync_WhilePivSessionHasVerifiedPin_ DoesNotClobberSessionState Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ections Two concurrent operations on one protocol interleaved APDUs on the wire, corrupting chained commands, chained responses (0xC0 SEND REMAINING), and SCP MAC chains. Unit proof showed wire order 0xA1, 0xB2, 0xC0 — a foreign APDU inside a chained-response exchange. - Add AsyncExchangeGate: SemaphoreSlim(1,1) gate whose delegate receives CancellationToken.None — caller tokens cancel only the wait to enter; an in-flight exchange always runs to completion so no partial chained state is left on the card - Gate PcscProtocol.TransmitAndReceiveAsync and SelectAsync around the full processor exchange (whole chained sequences are atomic) - Share the base protocol's gate in PcscProtocolScp: the SCP wrapper drives the same connection through its own processor chain and bypasses the base protocol's public methods - Wrap the SCP handshake in the gate in ScpExtensions.WithScpAsync - Document the concurrency contract on ISmartCardProtocol - Add 5 deterministic unit tests (interleave, SCP gate sharing, exception release, mid-exchange cancel, waiting cancel) and a hardware demo test (concurrent sign + 2.5KB object read on one PivSession, 10 iterations) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- FidoHidProtocol: gate SendVendorCommandAsync/TransmitAndReceiveAsync/SelectAsync through AsyncExchangeGate; replace sync-over-async AcquireCtapHidChannel with async lazy init inside the gate (fixes concurrent-first-use double-INIT) - OtpHidProtocol: gate SendAndReceiveAsync/ReadStatusAsync; split gated public wrappers from ungated SendAndReceiveCoreAsync so the NEO (FW 3.x) init quirk runs inside the held gate without self-deadlock; remove sync ReadFeatureReport - Document concurrency contract on IFidoHidProtocol/IOtpHidProtocol and classes: exchanges serialize; cancellation cancels only the wait for a turn - Add unit tests with fake CTAP/OTP device models (wire-order interleave, single-INIT first use, NEO deadlock guard) - Add Management hardware demo: concurrent GetDeviceInfoAsync over HidFido/HidOtp - Document concurrency model in src/Core/CLAUDE.md and README Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nge gate - FidoHidProtocol.Configure/OtpHidProtocol.Configure previously touched the wire (CTAPHID_INIT; OTP status read + NEO scan-map) outside AsyncExchangeGate, so Configure racing a first-use operation could interleave initialization - Route both Configure paths through RunExclusiveAsync via the ungated init cores - Add regression test: Configure racing a gate-held first-use op puts exactly one CTAPHID_INIT on the wire (wire-observation gate, proven RED against pre-fix code) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- FindAllAsync with an open authenticated session on key A: scan enumerates all present keys, session A survives; serial identification deferred until after session close (discovery skips in-use interfaces by design) - Concurrent PIV sign loops on two distinct physical keys via Task.WhenAll - Runtime Skip.If unless >=2 allow-listed SmartCard-capable keys are plugged in Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.