From bfb150e024b355283261ba6b4e82bbd12ce7bdd6 Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Sun, 16 May 2021 19:41:03 +0200 Subject: [PATCH 1/8] Fix WaitForConnectionAsync when NamedPipeServerStream is disposed --- .../IO/Pipes/NamedPipeServerStream.Unix.cs | 5 ++ .../tests/PipeStreamConformanceTests.cs | 67 +++++++++++++------ 2 files changed, 50 insertions(+), 22 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 8a309dc9d49288..79c81abeed6581 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 @@ -87,6 +87,11 @@ private void HandleAcceptedSocket(Socket acceptedSocket) try { + if (State == PipeState.Closed) + { + throw Error.GetPipeNotOpen(); + } + if (IsCurrentUserOnly) { uint serverEUID = Interop.Sys.GetEUid(); diff --git a/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs b/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs index 805bc2983efd7a..d1c0fd5126b351 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(); + var server = CreateServerStream(pipeName); + var client = CreateClientStream(pipeName); + return (server, client); + } protected sealed override async Task CreateConnectedStreamsAsync() { @@ -629,6 +638,26 @@ public async Task CancelTokenOn_Client_ReadWriteCancelledToken_Throws_OperationC await Assert.ThrowsAnyAsync(() => clientWriteToken); } } + + [Fact] + public async Task TwoServerInstances_OnceDisposed_Throws() + { + string pipeName = GetUniquePipeName(); + var server1 = CreateServerStream(pipeName, 2); + using var server2 = CreateServerStream(pipeName, 2); + + var wait1 = server1.WaitForConnectionAsync(); + var wait2 = server2.WaitForConnectionAsync(); + server1.Dispose(); + + using var client = CreateClientStream(pipeName); + await client.ConnectAsync(); + + await Assert.ThrowsAsync(() => wait1); + await ValidateDisposedExceptionsAsync(server1); + + await wait2; + } } public sealed class AnonymousPipeTest_ServerIn_ClientOut : AnonymousPipeStreamConformanceTests @@ -653,34 +682,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); } } From 4c025496d494a06263e1872abfa983097615fa72 Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Sun, 16 May 2021 23:19:45 +0200 Subject: [PATCH 2/8] Align Unix implementation on broken pipe IO exception as on Windows --- .../src/System/IO/Pipes/NamedPipeServerStream.Unix.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 79c81abeed6581..0af679541229d5 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 @@ -89,7 +89,8 @@ private void HandleAcceptedSocket(Socket acceptedSocket) { if (State == PipeState.Closed) { - throw Error.GetPipeNotOpen(); + // Pipe was closed/disposed during the connection phase, handle it as a broken pipe + throw new IOException(SR.IO_PipeBroken); } if (IsCurrentUserOnly) From 5ede4a3999bbde196e76bf417159ee30e6b2b869 Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Mon, 17 May 2021 00:56:37 +0200 Subject: [PATCH 3/8] Add missing methods to test against ObjectDisposedException --- .../System.IO.Pipes/tests/PipeStreamConformanceTests.cs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs b/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs index d1c0fd5126b351..de3daa2329e26a 100644 --- a/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs +++ b/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs @@ -97,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 /// From 7e5d486a700c0755f1d128b7f5cb3d136ab69427 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emmanuel=20Andr=C3=A9?= <2341261+manandre@users.noreply.github.com> Date: Wed, 26 May 2021 20:24:25 +0200 Subject: [PATCH 4/8] Apply suggestions from code review Co-authored-by: Stephen Toub --- .../tests/PipeStreamConformanceTests.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs b/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs index de3daa2329e26a..9dcb069c34c0f4 100644 --- a/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs +++ b/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs @@ -69,8 +69,8 @@ public abstract class NamedPipeStreamConformanceTests : PipeStreamConformanceTes protected (NamedPipeServerStream Server, NamedPipeClientStream Client) CreateServerAndClientStreams() { string pipeName = GetUniquePipeName(); - var server = CreateServerStream(pipeName); - var client = CreateClientStream(pipeName); + NamedPipeServerStream server = CreateServerStream(pipeName); + NamedPipeClientStream client = CreateClientStream(pipeName); return (server, client); } @@ -652,14 +652,14 @@ public async Task CancelTokenOn_Client_ReadWriteCancelledToken_Throws_OperationC public async Task TwoServerInstances_OnceDisposed_Throws() { string pipeName = GetUniquePipeName(); - var server1 = CreateServerStream(pipeName, 2); - using var server2 = CreateServerStream(pipeName, 2); + Stream server1 = CreateServerStream(pipeName, 2); + using Stream server2 = CreateServerStream(pipeName, 2); - var wait1 = server1.WaitForConnectionAsync(); - var wait2 = server2.WaitForConnectionAsync(); + Task wait1 = server1.WaitForConnectionAsync(); + Task wait2 = server2.WaitForConnectionAsync(); server1.Dispose(); - using var client = CreateClientStream(pipeName); + using Stream client = CreateClientStream(pipeName); await client.ConnectAsync(); await Assert.ThrowsAsync(() => wait1); From 64f7e780722db0e46c31981fe26659ca1acb4725 Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Wed, 26 May 2021 20:44:33 +0200 Subject: [PATCH 5/8] Rebase and fix suggestions --- .../System.IO.Pipes/tests/PipeStreamConformanceTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs b/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs index 9dcb069c34c0f4..cc9d2e9d846395 100644 --- a/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs +++ b/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs @@ -652,14 +652,14 @@ public async Task CancelTokenOn_Client_ReadWriteCancelledToken_Throws_OperationC public async Task TwoServerInstances_OnceDisposed_Throws() { string pipeName = GetUniquePipeName(); - Stream server1 = CreateServerStream(pipeName, 2); - using Stream server2 = CreateServerStream(pipeName, 2); + NamedPipeServerStream server1 = CreateServerStream(pipeName, 2); + using NamedPipeServerStream server2 = CreateServerStream(pipeName, 2); Task wait1 = server1.WaitForConnectionAsync(); Task wait2 = server2.WaitForConnectionAsync(); server1.Dispose(); - using Stream client = CreateClientStream(pipeName); + using NamedPipeClientStream client = CreateClientStream(pipeName); await client.ConnectAsync(); await Assert.ThrowsAsync(() => wait1); From ef60a2f194269baf1bfbabbea77710a359c2ea4a Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Tue, 1 Jun 2021 22:51:45 +0200 Subject: [PATCH 6/8] Cancel Accept on dispose --- .../IO/Pipes/NamedPipeServerStream.Unix.cs | 45 +++++++++++++++---- 1 file changed, 36 insertions(+), 9 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 0af679541229d5..a7bcee32d018c1 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,29 @@ 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 = null; + try + { + linkedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(_internalTokenSource.Token, cancellationToken); + acceptedSocket = await _instance!.ListeningSocket.AcceptAsync(linkedTokenSource.Token).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + //if cancellation was via external token, throw an OCE + cancellationToken.ThrowIfCancellationRequested(); + + throw new IOException(SR.IO_PipeBroken); + } + finally + { + linkedTokenSource?.Dispose(); + } + + HandleAcceptedSocket(acceptedSocket); + } } private void HandleAcceptedSocket(Socket acceptedSocket) @@ -87,12 +109,6 @@ private void HandleAcceptedSocket(Socket acceptedSocket) try { - if (State == PipeState.Closed) - { - // Pipe was closed/disposed during the connection phase, handle it as a broken pipe - throw new IOException(SR.IO_PipeBroken); - } - if (IsCurrentUserOnly) { uint serverEUID = Interop.Sys.GetEUid(); @@ -122,9 +138,20 @@ 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(); + } + _internalTokenSource.Dispose(); + } + } + public void Disconnect() { CheckDisconnectOperations(); From 8a6e80279e302513e5673d8adfd3973f4824f00d Mon Sep 17 00:00:00 2001 From: Emmanuel ANDRE <2341261+manandre@users.noreply.github.com> Date: Wed, 2 Jun 2021 22:43:42 +0200 Subject: [PATCH 7/8] Improve test --- .../tests/PipeStreamConformanceTests.cs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs b/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs index cc9d2e9d846395..930b875fe8e543 100644 --- a/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs +++ b/src/libraries/System.IO.Pipes/tests/PipeStreamConformanceTests.cs @@ -658,14 +658,25 @@ public async Task TwoServerInstances_OnceDisposed_Throws() 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 ValidateDisposedExceptionsAsync(server1); 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; + } } } From edf50b378ca0a608e6f13096c8e2ccfc9ec5fb44 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Sun, 18 Jul 2021 06:43:21 -0400 Subject: [PATCH 8/8] Apply suggestions from code review --- .../src/System/IO/Pipes/NamedPipeServerStream.Unix.cs | 11 +++-------- 1 file changed, 3 insertions(+), 8 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 a7bcee32d018c1..4744afa6104861 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 @@ -81,22 +81,18 @@ public Task WaitForConnectionAsync(CancellationToken cancellationToken) async Task WaitForConnectionAsyncCore() { Socket acceptedSocket; - CancellationTokenSource? linkedTokenSource = null; + CancellationTokenSource linkedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(_internalTokenSource.Token, cancellationToken); try { - linkedTokenSource = CancellationTokenSource.CreateLinkedTokenSource(_internalTokenSource.Token, cancellationToken); acceptedSocket = await _instance!.ListeningSocket.AcceptAsync(linkedTokenSource.Token).ConfigureAwait(false); } - catch (OperationCanceledException) + catch (OperationCanceledException) when (!cancellationToken.IsCancellationRequested) { - //if cancellation was via external token, throw an OCE - cancellationToken.ThrowIfCancellationRequested(); - throw new IOException(SR.IO_PipeBroken); } finally { - linkedTokenSource?.Dispose(); + linkedTokenSource.Dispose(); } HandleAcceptedSocket(acceptedSocket); @@ -148,7 +144,6 @@ internal override void DisposeCore(bool disposing) { _internalTokenSource.Cancel(); } - _internalTokenSource.Dispose(); } }