From c5edf0e09541e2c9c4441c0abdf23f712507596c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emmanuel=20Andr=C3=A9?= <2341261+manandre@users.noreply.github.com> Date: Sun, 18 Jul 2021 19:30:16 +0200 Subject: [PATCH] Fix WaitForConnectionAsync when NamedPipeServerStream is disposed (#52825) * Fix WaitForConnectionAsync when NamedPipeServerStream is disposed * Align Unix implementation on broken pipe IO exception as on Windows * Add missing methods to test against ObjectDisposedException * Apply suggestions from code review Co-authored-by: Stephen Toub * Rebase and fix suggestions * Cancel Accept on dispose * Improve test * Apply suggestions from code review Co-authored-by: Stephen Toub --- .../IO/Pipes/NamedPipeServerStream.Unix.cs | 34 +++++++- .../tests/PipeStreamConformanceTests.cs | 87 ++++++++++++++----- 2 files changed, 96 insertions(+), 25 deletions(-) diff --git a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Unix.cs b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Unix.cs index 8a309dc9d4928..4744afa610486 100644 --- a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Unix.cs +++ b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Unix.cs @@ -20,6 +20,7 @@ public sealed partial class NamedPipeServerStream : PipeStream private int _inBufferSize; private int _outBufferSize; private HandleInheritability _inheritability; + private CancellationTokenSource _internalTokenSource = new CancellationTokenSource(); private void Create(string pipeName, PipeDirection direction, int maxNumberOfServerInstances, PipeTransmissionMode transmissionMode, PipeOptions options, int inBufferSize, int outBufferSize, @@ -77,8 +78,25 @@ public Task WaitForConnectionAsync(CancellationToken cancellationToken) Task.FromCanceled(cancellationToken) : WaitForConnectionAsyncCore(); - async Task WaitForConnectionAsyncCore() => - HandleAcceptedSocket(await _instance!.ListeningSocket.AcceptAsync(cancellationToken).ConfigureAwait(false)); + async Task WaitForConnectionAsyncCore() + { + Socket acceptedSocket; + CancellationTokenSource linkedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(_internalTokenSource.Token, cancellationToken); + try + { + acceptedSocket = await _instance!.ListeningSocket.AcceptAsync(linkedTokenSource.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) + { + throw new IOException(SR.IO_PipeBroken); + } + finally + { + linkedTokenSource.Dispose(); + } + + HandleAcceptedSocket(acceptedSocket); + } } private void HandleAcceptedSocket(Socket acceptedSocket) @@ -116,9 +134,19 @@ private void HandleAcceptedSocket(Socket acceptedSocket) State = PipeState.Connected; } - internal override void DisposeCore(bool disposing) => + internal override void DisposeCore(bool disposing) + { Interlocked.Exchange(ref _instance, null)?.Dispose(disposing); // interlocked to avoid shared state problems from erroneous double/concurrent disposes + if (disposing) + { + if (State != PipeState.Closed) + { + _internalTokenSource.Cancel(); + } + } + } + public void Disconnect() { CheckDisconnectOperations(); diff --git a/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs b/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs index 805bc2983efd7..930b875fe8e54 100644 --- a/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs +++ b/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs @@ -63,7 +63,16 @@ public abstract class NamedPipeStreamConformanceTests : PipeStreamConformanceTes { protected override bool BrokenPipePropagatedImmediately => OperatingSystem.IsWindows(); // On Unix, implemented on Sockets, where it won't propagate immediate - protected abstract (NamedPipeServerStream Server, NamedPipeClientStream Client) CreateServerAndClientStreams(); + protected abstract NamedPipeServerStream CreateServerStream(string pipeName, int maxInstances = 1); + protected abstract NamedPipeClientStream CreateClientStream(string pipeName); + + protected (NamedPipeServerStream Server, NamedPipeClientStream Client) CreateServerAndClientStreams() + { + string pipeName = GetUniquePipeName(); + NamedPipeServerStream server = CreateServerStream(pipeName); + NamedPipeClientStream client = CreateClientStream(pipeName); + return (server, client); + } protected sealed override async Task CreateConnectedStreamsAsync() { @@ -88,6 +97,15 @@ protected sealed override async Task CreateConnectedStreamsAsync() return ((NamedPipeServerStream)streams.Stream2, (NamedPipeClientStream)streams.Stream1); } + protected async Task ValidateDisposedExceptionsAsync(NamedPipeServerStream server) + { + Assert.Throws(() => server.Disconnect()); + Assert.Throws(() => server.GetImpersonationUserName()); + Assert.Throws(() => server.WaitForConnection()); + await Assert.ThrowsAsync(() => server.WaitForConnectionAsync()); + await ValidateDisposedExceptionsAsync(server as Stream); + } + /// /// Yields every combination of testing options for the OneWayReadWrites test /// @@ -629,6 +647,37 @@ public async Task CancelTokenOn_Client_ReadWriteCancelledToken_Throws_OperationC await Assert.ThrowsAnyAsync(() => clientWriteToken); } } + + [Fact] + public async Task TwoServerInstances_OnceDisposed_Throws() + { + string pipeName = GetUniquePipeName(); + NamedPipeServerStream server1 = CreateServerStream(pipeName, 2); + using NamedPipeServerStream server2 = CreateServerStream(pipeName, 2); + + Task wait1 = server1.WaitForConnectionAsync(); + Task wait2 = server2.WaitForConnectionAsync(); + server1.Dispose(); + await ValidateDisposedExceptionsAsync(server1); + + using NamedPipeClientStream client = CreateClientStream(pipeName); + await client.ConnectAsync(); + + await Assert.ThrowsAsync(() => wait1); + + await wait2; + + foreach ((Stream writeable, Stream readable) in GetReadWritePairs((server2, client))) + { + byte[] sent = new byte[] { 123 }; + byte[] received = new byte[] { 0 }; + + Task t = Task.Run(() => writeable.Write(sent, 0, sent.Length)); + Assert.Equal(sent.Length, readable.Read(received, 0, sent.Length)); + Assert.Equal(sent, received); + await t; + } + } } public sealed class AnonymousPipeTest_ServerIn_ClientOut : AnonymousPipeStreamConformanceTests @@ -653,34 +702,28 @@ protected override (AnonymousPipeServerStream Server, AnonymousPipeClientStream public sealed class NamedPipeTest_ServerOut_ClientIn : NamedPipeStreamConformanceTests { - protected override (NamedPipeServerStream Server, NamedPipeClientStream Client) CreateServerAndClientStreams() - { - string pipeName = PipeStreamConformanceTests.GetUniquePipeName(); - var server = new NamedPipeServerStream(pipeName, PipeDirection.Out, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous); - var client = new NamedPipeClientStream(".", pipeName, PipeDirection.In, PipeOptions.Asynchronous); - return (server, client); - } + protected override NamedPipeServerStream CreateServerStream(string pipeName, int maxInstances = 1) => + new NamedPipeServerStream(pipeName, PipeDirection.Out, maxInstances, PipeTransmissionMode.Byte, PipeOptions.Asynchronous); + + protected override NamedPipeClientStream CreateClientStream(string pipeName) => + new NamedPipeClientStream(".", pipeName, PipeDirection.In, PipeOptions.Asynchronous); } public sealed class NamedPipeTest_ServerIn_ClientOut : NamedPipeStreamConformanceTests { - protected override (NamedPipeServerStream Server, NamedPipeClientStream Client) CreateServerAndClientStreams() - { - string pipeName = PipeStreamConformanceTests.GetUniquePipeName(); - var server = new NamedPipeServerStream(pipeName, PipeDirection.In, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous); - var client = new NamedPipeClientStream(".", pipeName, PipeDirection.Out, PipeOptions.Asynchronous); - return (server, client); - } + protected override NamedPipeServerStream CreateServerStream(string pipeName, int maxInstances = 1) => + new NamedPipeServerStream(pipeName, PipeDirection.In, maxInstances, PipeTransmissionMode.Byte, PipeOptions.Asynchronous); + + protected override NamedPipeClientStream CreateClientStream(string pipeName) => + new NamedPipeClientStream(".", pipeName, PipeDirection.Out, PipeOptions.Asynchronous); } public sealed class NamedPipeTest_ServerInOut_ClientInOut : NamedPipeStreamConformanceTests { - protected override (NamedPipeServerStream Server, NamedPipeClientStream Client) CreateServerAndClientStreams() - { - string pipeName = PipeStreamConformanceTests.GetUniquePipeName(); - var server = new NamedPipeServerStream(pipeName, PipeDirection.InOut, 1, PipeTransmissionMode.Byte, PipeOptions.Asynchronous); - var client = new NamedPipeClientStream(".", pipeName, PipeDirection.InOut, PipeOptions.Asynchronous); - return (server, client); - } + protected override NamedPipeServerStream CreateServerStream(string pipeName, int maxInstances = 1) => + new NamedPipeServerStream(pipeName, PipeDirection.InOut, maxInstances, PipeTransmissionMode.Byte, PipeOptions.Asynchronous); + + protected override NamedPipeClientStream CreateClientStream(string pipeName) => + new NamedPipeClientStream(".", pipeName, PipeDirection.InOut, PipeOptions.Asynchronous); } }