diff --git a/src/cs/Ssh/Services/ConnectionService.cs b/src/cs/Ssh/Services/ConnectionService.cs index d81c5d7..3887379 100644 --- a/src/cs/Ssh/Services/ConnectionService.cs +++ b/src/cs/Ssh/Services/ConnectionService.cs @@ -49,6 +49,14 @@ public PendingChannel( private long channelCounter = -1; private Exception? closedException = null; + /// + /// Test-only hook invoked while holding in the channel-open + /// confirmation handler, right before the pending channel's cancellation registration is + /// captured. Tests use this to deterministically reproduce the channel-open cancellation + /// race. Always null (a no-op) in production. + /// + internal Action? TestHook_ChannelOpenResponseLocked { get; set; } + public ConnectionService(SshSession session) : base(session) { this.channels = new Dictionary(); @@ -492,6 +500,7 @@ private async Task HandleMessageAsync( TaskCompletionSource? completionSource = null; ChannelOpenMessage openMessage; + CancellationTokenRegistration cancellationRegistration = default; lock (this.lockObject) { if (this.pendingChannels.TryGetValue( @@ -499,7 +508,16 @@ private async Task HandleMessageAsync( { openMessage = pendingChannel.OpenMessage; completionSource = pendingChannel.CompletionSource; - pendingChannel.CancellationRegistration.Dispose(); + + this.TestHook_ChannelOpenResponseLocked?.Invoke(); + + // Capture the registration and dispose it AFTER releasing the lock. + // Disposing here would deadlock: the callback registered in + // OpenChannelAsync also acquires this.lockObject, and + // CancellationTokenRegistration.Dispose() blocks until an in-flight + // callback completes. Removing the pending channel under the lock + // makes the callback a no-op, so deferring the dispose is safe. + cancellationRegistration = pendingChannel.CancellationRegistration; this.pendingChannels.Remove(message.RecipientChannel); } else if (this.channels.ContainsKey(message.RecipientChannel)) @@ -534,6 +552,8 @@ private async Task HandleMessageAsync( this.channels.Add(channel.ChannelId, channel); } + cancellationRegistration.Dispose(); + var args = new SshChannelOpeningEventArgs(openMessage, channel, isRemoteRequest: false); await Session.OnChannelOpeningAsync(args, cancellation).ConfigureAwait(false); @@ -565,17 +585,24 @@ private Task HandleMessageAsync( cancellation.ThrowIfCancellationRequested(); TaskCompletionSource? completionSource = null; + CancellationTokenRegistration cancellationRegistration = default; lock (this.lockObject) { if (this.pendingChannels.TryGetValue( message.RecipientChannel, out var pendingChannel)) { completionSource = pendingChannel.CompletionSource; - pendingChannel.CancellationRegistration.Dispose(); + + // Capture the registration and dispose it after releasing the lock; + // disposing under the lock can deadlock with the cancellation callback + // (which also acquires this.lockObject). See HandleMessageAsync above. + cancellationRegistration = pendingChannel.CancellationRegistration; this.pendingChannels.Remove(message.RecipientChannel); } } + cancellationRegistration.Dispose(); + if (completionSource != null) { completionSource.TrySetException(new SshChannelException( diff --git a/test/cs/Ssh.Test/ChannelTests.cs b/test/cs/Ssh.Test/ChannelTests.cs index 7fb2227..1c5fc0a 100644 --- a/test/cs/Ssh.Test/ChannelTests.cs +++ b/test/cs/Ssh.Test/ChannelTests.cs @@ -4,6 +4,7 @@ using System.Globalization; using System.IO; using System.Linq; +using System.Reflection; using System.Text; using System.Threading; using System.Threading.Tasks; @@ -176,6 +177,146 @@ public async Task OpenChannelCancelByAcceptor() } } + /// + /// Installs a test hook on the client's internal ConnectionService that deterministically + /// reproduces the channel-open cancellation race. The internal hook is reached via + /// reflection (as does for KeyRotationThreshold) so the test + /// otherwise uses only the public session API. The hook runs on the receive pump thread + /// while it holds the ConnectionService lock during open-confirmation processing: it cancels + /// the open from another thread, using a dual-registration handshake to deterministically + /// confirm the internal callback is contending for the held lock before returning. + /// A monitoring callback is registered both BEFORE and AFTER the internal one (via + /// the returned action and inside the hook). CancellationToken callbacks fire in LIFO + /// order on .NET Core and FIFO on .NET Framework — so one of the two monitoring + /// callbacks always fires before the internal one. Since Cancel() executes callbacks + /// sequentially on the calling thread, once the monitoring callback completes, the + /// internal callback starts immediately on the same thread and blocks on lockObject. + /// Before the fix, the pump then disposed the cancellation registration while still + /// holding the lock, which blocks on the in-flight callback and deadlocks the session; + /// after the fix the registration is disposed after the lock is released. + /// + /// An action that must be invoked after the hook is installed but before + /// OpenChannelAsync, to register the "early" monitoring callback. + private static Action InstallOpenCancelRaceHook( + SshSession session, + CancellationTokenSource cancellationSource, + TaskCompletionSource callbackStarted) + { + var connectionServiceType = typeof(SshSession).Assembly.GetType( + "Microsoft.DevTunnels.Ssh.Services.ConnectionService"); + var activateMethod = typeof(SshSession).GetMethods() + .Single(m => m.Name == nameof(SshSession.ActivateService) && + m.IsGenericMethodDefinition && + m.GetParameters().Length == 0) + .MakeGenericMethod(connectionServiceType); + var connectionService = activateMethod.Invoke(session, null); + + var hookProperty = connectionServiceType.GetProperty( + "TestHook_ChannelOpenResponseLocked", + BindingFlags.NonPublic | BindingFlags.Instance); + + Action hook = () => + { + // Fire only once, on the first (raced) open confirmation. + hookProperty.SetValue(connectionService, null); + + // Register a "late" monitoring callback — AFTER the internal callback + // (which was registered inside OpenChannelAsync). On LIFO runtimes + // (.NET Core) this fires first; on FIFO runtimes (.NET Framework) the + // "early" callback registered below fires first. Either way, one of them + // signals before the internal callback executes. + cancellationSource.Token.Register( + () => callbackStarted.TrySetResult(true)); + + // Cancel on a background thread. Cancel() invokes all registered + // callbacks sequentially on its thread before returning. + Task.Run(() => cancellationSource.Cancel()); + + // Wait for a monitoring callback to fire. Since Cancel() executes + // callbacks sequentially, the internal callback starts immediately + // after the monitoring one completes — on the same thread — and blocks + // on lockObject (which the caller holds). So when Wait() returns, the + // internal callback is guaranteed to be contending for the lock. + callbackStarted.Task.Wait(); + }; + + hookProperty.SetValue(connectionService, hook); + + // Return an action the caller must invoke BEFORE OpenChannelAsync to register + // the "early" monitoring callback (covers FIFO runtimes like .NET Framework). + return () => cancellationSource.Token.Register( + () => callbackStarted.TrySetResult(true)); + } + + [Fact] + public async Task OpenChannelCancelDuringConfirmationDoesNotDeadlock() + { + await this.sessionPair.ConnectAsync().WithTimeout(Timeout); + + using var cancellationSource = new CancellationTokenSource(); + var callbackStarted = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + var registerEarlyCallback = InstallOpenCancelRaceHook( + this.clientSession, cancellationSource, callbackStarted); + + // Register "early" monitoring callback BEFORE OpenChannelAsync registers + // the internal one. On FIFO (.NET Framework) this fires first. + registerEarlyCallback(); + + var openTask = this.clientSession.OpenChannelAsync( + (string)null, cancellationSource.Token); + + // Before the fix this times out (the pump thread deadlocks); after the fix the open + // completes, whether it ends up cancelled or opened. + try + { + await openTask.WithTimeout(Timeout); + } + catch (OperationCanceledException) + { + } + catch (SshChannelException) + { + } + } + + [Fact] + public async Task SessionRemainsUsableAfterOpenCancelRace() + { + await this.sessionPair.ConnectAsync().WithTimeout(Timeout); + + using var cancellationSource = new CancellationTokenSource(); + var callbackStarted = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + var registerEarlyCallback = InstallOpenCancelRaceHook( + this.clientSession, cancellationSource, callbackStarted); + + registerEarlyCallback(); + + var racedOpenTask = this.clientSession.OpenChannelAsync( + (string)null, cancellationSource.Token); + + try + { + await racedOpenTask.WithTimeout(Timeout); + } + catch (OperationCanceledException) + { + } + catch (SshChannelException) + { + } + + // If the pump thread had deadlocked, the receive loop could no longer process + // messages, so this subsequent open would hang instead of completing end-to-end. + var serverChannelTask = this.serverSession.AcceptChannelAsync(); + var clientChannel = await this.clientSession.OpenChannelAsync().WithTimeout(Timeout); + var serverChannel = await serverChannelTask.WithTimeout(Timeout); + + Assert.NotNull(clientChannel); + Assert.NotNull(serverChannel); + } + [Theory] [InlineData(false, false)] [InlineData(false, true)]