From 9eb44a18376a755f3a7765f5657a4357b47a13d4 Mon Sep 17 00:00:00 2001 From: Juan Pablo Acosta Date: Wed, 22 Jul 2026 16:32:46 -0700 Subject: [PATCH 1/4] =?UTF-8?q?Fixed=20a=20channel-open=20deadlock=20by=20?= =?UTF-8?q?disposing=20the=20cancellation=20registration=20after=20releasi?= =?UTF-8?q?ng=20the=20lock.=20ConnectionService.HandleMessageAsync=20was?= =?UTF-8?q?=20disposing=20a=20pending=20channel's=20CancellationTokenRegis?= =?UTF-8?q?tration=20while=20holding=20lockObject,=20but=20the=20callback?= =?UTF-8?q?=20registered=20in=20OpenChannelAsync=20also=20took=20lockObjec?= =?UTF-8?q?t,=20and=20Dispose()=20blocks=20until=20an=20in-flight=20callba?= =?UTF-8?q?ck=20completes=20=E2=80=94=20so=20a=20channel-open=20cancelling?= =?UTF-8?q?=20at=20that=20moment=20deadlocked=20the=20session=20and=20leak?= =?UTF-8?q?ed=20every=20later=20OpenChannelAsync=20thread.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/cs/Ssh/Services/ConnectionService.cs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/cs/Ssh/Services/ConnectionService.cs b/src/cs/Ssh/Services/ConnectionService.cs index d81c5d7..d66944e 100644 --- a/src/cs/Ssh/Services/ConnectionService.cs +++ b/src/cs/Ssh/Services/ConnectionService.cs @@ -492,6 +492,7 @@ private async Task HandleMessageAsync( TaskCompletionSource? completionSource = null; ChannelOpenMessage openMessage; + CancellationTokenRegistration cancellationRegistration = default; lock (this.lockObject) { if (this.pendingChannels.TryGetValue( @@ -499,7 +500,14 @@ private async Task HandleMessageAsync( { openMessage = pendingChannel.OpenMessage; completionSource = pendingChannel.CompletionSource; - pendingChannel.CancellationRegistration.Dispose(); + + // 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 +542,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 +575,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( From 00e876b836cbc427a4044ca920c9cbc731c1c711 Mon Sep 17 00:00:00 2001 From: Juan Pablo Acosta Date: Thu, 23 Jul 2026 14:51:50 -0700 Subject: [PATCH 2/4] Added deterministic unit and integration tests for the channel-open cancellation deadlock, using a small internal ConnectionService hook reached by reflection (matching how ReconnectTests reaches KeyRotationThreshold) to force the cancel-during-confirmation race. --- src/cs/Ssh/Services/ConnectionService.cs | 10 +++ test/cs/Ssh.Test/ChannelTests.cs | 102 +++++++++++++++++++++++ 2 files changed, 112 insertions(+) diff --git a/src/cs/Ssh/Services/ConnectionService.cs b/src/cs/Ssh/Services/ConnectionService.cs index d66944e..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(); @@ -501,6 +509,8 @@ private async Task HandleMessageAsync( openMessage = pendingChannel.OpenMessage; completionSource = pendingChannel.CompletionSource; + 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 diff --git a/test/cs/Ssh.Test/ChannelTests.cs b/test/cs/Ssh.Test/ChannelTests.cs index 7fb2227..459baa3 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,107 @@ 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 (whose callback, registered in OpenChannelAsync, also acquires that lock) from + /// another thread and then sleeps so the callback is in-flight and blocked on the lock. + /// 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. + /// + private static void InstallOpenCancelRaceHook( + SshSession session, CancellationTokenSource cancellationSource) + { + 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); + + // Cancel from another thread; the open's cancellation callback then contends for + // the ConnectionService lock, which the pump currently holds here. + Task.Run(() => cancellationSource.Cancel()); + + // Give the cancellation callback time to reach and block on the lock. + Thread.Sleep(500); + }; + + hookProperty.SetValue(connectionService, hook); + } + + [Fact] + public async Task OpenChannelCancelDuringConfirmationDoesNotDeadlock() + { + await this.sessionPair.ConnectAsync().WithTimeout(Timeout); + + using var cancellationSource = new CancellationTokenSource(); + InstallOpenCancelRaceHook(this.clientSession, cancellationSource); + + 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(); + InstallOpenCancelRaceHook(this.clientSession, cancellationSource); + + 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)] From fc7b42b940a85a662669d66138e7ec8bc62d672a Mon Sep 17 00:00:00 2001 From: Juan Pablo Acosta Date: Thu, 23 Jul 2026 15:34:46 -0700 Subject: [PATCH 3/4] =?UTF-8?q?Replace=20the=20timing-dependent=20Thread.S?= =?UTF-8?q?leep(500)=20in=20the=20deadlock=20test=20hook=20with=20an=20exp?= =?UTF-8?q?licit=20synchronization=20handshake=20using=20TaskCompletionSou?= =?UTF-8?q?rce=20and=20CancellationToken=20LIFO=20callback=20ordering.=20A?= =?UTF-8?q?=20second=20callback=20registered=20after=20the=20internal=20on?= =?UTF-8?q?e=20fires=20first=20(LIFO),=20and=20once=20it=20completes,=20Ca?= =?UTF-8?q?ncel()=20immediately=20proceeds=20to=20the=20internal=20callbac?= =?UTF-8?q?k=20on=20the=20same=20thread=20=E2=80=94=20which=20blocks=20on?= =?UTF-8?q?=20lockObject.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/cs/Ssh.Test/ChannelTests.cs | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/test/cs/Ssh.Test/ChannelTests.cs b/test/cs/Ssh.Test/ChannelTests.cs index 459baa3..f899b12 100644 --- a/test/cs/Ssh.Test/ChannelTests.cs +++ b/test/cs/Ssh.Test/ChannelTests.cs @@ -183,11 +183,11 @@ public async Task OpenChannelCancelByAcceptor() /// 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 (whose callback, registered in OpenChannelAsync, also acquires that lock) from - /// another thread and then sleeps so the callback is in-flight and blocked on the lock. - /// 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. + /// the open from another thread, using CancellationToken LIFO callback ordering to + /// deterministically confirm the internal callback is contending for the held lock before + /// returning. 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. /// private static void InstallOpenCancelRaceHook( SshSession session, CancellationTokenSource cancellationSource) @@ -210,12 +210,24 @@ private static void InstallOpenCancelRaceHook( // Fire only once, on the first (raced) open confirmation. hookProperty.SetValue(connectionService, null); - // Cancel from another thread; the open's cancellation callback then contends for - // the ConnectionService lock, which the pump currently holds here. + // We need the cancellation callback to be actively contending for our held lock + // before this hook returns. CancellationToken callbacks fire in LIFO order on the + // thread that calls Cancel(). The internal callback (registered in OpenChannelAsync) + // was registered BEFORE this one, so ours fires FIRST. Once ours completes, + // Cancel() immediately invokes the internal callback on the same thread — which + // blocks on lockObject that we hold here. So when callbackStarted completes, the + // internal callback is guaranteed to be contending for the lock (same-thread + // sequential execution within Cancel(), zero scheduling gap). + var callbackStarted = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + + // Registered AFTER the internal callback → fires FIRST (LIFO). + cancellationSource.Token.Register(() => callbackStarted.SetResult(true)); + Task.Run(() => cancellationSource.Cancel()); - // Give the cancellation callback time to reach and block on the lock. - Thread.Sleep(500); + // When this returns, the internal callback is already blocked on lockObject. + callbackStarted.Task.Wait(); }; hookProperty.SetValue(connectionService, hook); From e3344503c57b8e761aef7c975a9f86fda8e36363 Mon Sep 17 00:00:00 2001 From: Juan Pablo Acosta Date: Fri, 24 Jul 2026 11:49:33 -0700 Subject: [PATCH 4/4] Fix deadlock test to work on both .NET Core (LIFO) and .NET Framework (FIFO) by replacing Thread.Sleep with a dual-registration handshake using TaskCompletionSource. --- test/cs/Ssh.Test/ChannelTests.cs | 71 ++++++++++++++++++++++---------- 1 file changed, 49 insertions(+), 22 deletions(-) diff --git a/test/cs/Ssh.Test/ChannelTests.cs b/test/cs/Ssh.Test/ChannelTests.cs index f899b12..1c5fc0a 100644 --- a/test/cs/Ssh.Test/ChannelTests.cs +++ b/test/cs/Ssh.Test/ChannelTests.cs @@ -183,14 +183,24 @@ public async Task OpenChannelCancelByAcceptor() /// 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 CancellationToken LIFO callback ordering to - /// deterministically confirm the internal callback is contending for the held lock before - /// returning. 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; + /// 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. /// - private static void InstallOpenCancelRaceHook( - SshSession session, CancellationTokenSource cancellationSource) + /// 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"); @@ -210,27 +220,32 @@ private static void InstallOpenCancelRaceHook( // Fire only once, on the first (raced) open confirmation. hookProperty.SetValue(connectionService, null); - // We need the cancellation callback to be actively contending for our held lock - // before this hook returns. CancellationToken callbacks fire in LIFO order on the - // thread that calls Cancel(). The internal callback (registered in OpenChannelAsync) - // was registered BEFORE this one, so ours fires FIRST. Once ours completes, - // Cancel() immediately invokes the internal callback on the same thread — which - // blocks on lockObject that we hold here. So when callbackStarted completes, the - // internal callback is guaranteed to be contending for the lock (same-thread - // sequential execution within Cancel(), zero scheduling gap). - var callbackStarted = new TaskCompletionSource( - TaskCreationOptions.RunContinuationsAsynchronously); - - // Registered AFTER the internal callback → fires FIRST (LIFO). - cancellationSource.Token.Register(() => callbackStarted.SetResult(true)); + // 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()); - // When this returns, the internal callback is already blocked on lockObject. + // 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] @@ -239,7 +254,14 @@ public async Task OpenChannelCancelDuringConfirmationDoesNotDeadlock() await this.sessionPair.ConnectAsync().WithTimeout(Timeout); using var cancellationSource = new CancellationTokenSource(); - InstallOpenCancelRaceHook(this.clientSession, cancellationSource); + 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); @@ -264,7 +286,12 @@ public async Task SessionRemainsUsableAfterOpenCancelRace() await this.sessionPair.ConnectAsync().WithTimeout(Timeout); using var cancellationSource = new CancellationTokenSource(); - InstallOpenCancelRaceHook(this.clientSession, cancellationSource); + var callbackStarted = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + var registerEarlyCallback = InstallOpenCancelRaceHook( + this.clientSession, cancellationSource, callbackStarted); + + registerEarlyCallback(); var racedOpenTask = this.clientSession.OpenChannelAsync( (string)null, cancellationSource.Token);