From c559bc1325ab6beb48e52d09975ffae33e0316f4 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Sun, 22 Oct 2017 09:23:52 -0400 Subject: [PATCH] Change lifetime of Socket used in NamedPipeServerStream on Unix We currently create and destroy the unix domain socket each time WaitForConnection{Async} is called, and it lives until the client connects. This causes problems when multiple clients try to connect before WaitForConnection{Async} is called; any number that's under the min listen limit applied by the OS will have their connections completed, even though Accept hasn't been called, and then when the socket is closed, the connections will all fail. This change moves the UDS creation to when the NamedPipeServerStream is created, deleting it when the NPSS is disposed. Commit migrated from https://github.com/dotnet/corefx/commit/16b29a8ca2f075edfb27a720782320e720bb8941 --- .../System/IO/Pipes/NamedPipeServerStream.Unix.cs | 96 +++++++++------------- .../src/System/IO/Pipes/PipeStream.Unix.cs | 14 ++-- .../src/System/IO/Pipes/PipeStream.Windows.cs | 8 +- .../src/System/IO/Pipes/PipeStream.cs | 3 +- .../tests/NamedPipeTests/NamedPipeTest.Specific.cs | 43 ++++++++++ 5 files changed, 97 insertions(+), 67 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 788177d..c6249e3 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 @@ -8,7 +8,6 @@ using System.Diagnostics.CodeAnalysis; using System.Net.Sockets; using System.Runtime.InteropServices; using System.Security; -using System.Security.Permissions; using System.Threading; using System.Threading.Tasks; @@ -19,6 +18,7 @@ namespace System.IO.Pipes /// public sealed partial class NamedPipeServerStream : PipeStream { + private Socket _socket; private string _path; private PipeDirection _direction; private PipeOptions _options; @@ -52,52 +52,36 @@ namespace System.IO.Pipes _inBufferSize = inBufferSize; _outBufferSize = outBufferSize; _inheritability = inheritability; - } - - [SecurityCritical] - [SuppressMessage("Microsoft.Security", "CA2122:DoNotIndirectlyExposeMethodsWithLinkDemands", Justification = "Security model of pipes: demand at creation but no subsequent demands")] - public void WaitForConnection() - { - CheckConnectOperationsServer(); - if (State == PipeState.Connected) - { - throw new InvalidOperationException(SR.InvalidOperation_PipeAlreadyConnected); - } // Binding to an existing path fails, so we need to remove anything left over at this location. // There's of course a race condition here, where it could be recreated by someone else between this // deletion and the bind below, in which case we'll simply let the bind fail and throw. Interop.Sys.Unlink(_path); // ignore any failures + + // Start listening for connections on the path. They'll only be accepted in WaitForConnection{Async}. var socket = new Socket(AddressFamily.Unix, SocketType.Stream, ProtocolType.Unspecified); try { socket.Bind(new UnixDomainSocketEndPoint(_path)); - socket.Listen(1); - - Socket acceptedSocket = socket.Accept(); - SafePipeHandle serverHandle = new SafePipeHandle(acceptedSocket); - try - { - ConfigureSocket(acceptedSocket, serverHandle, _direction, _inBufferSize, _outBufferSize, _inheritability); - } - catch - { - serverHandle.Dispose(); - acceptedSocket.Dispose(); - throw; - } - - InitializeHandle(serverHandle, isExposed: false, isAsync: (_options & PipeOptions.Asynchronous) != 0); - State = PipeState.Connected; + socket.Listen(int.MaxValue); } - finally + catch { - // Bind will have created a file. Now that the client is connected, it's no longer necessary, so get rid of it. - Interop.Sys.Unlink(_path); // ignore any failures; worst case is we leave a tmp file - - // Clean up the listening socket socket.Dispose(); + throw; } + _socket = socket; + } + + public void WaitForConnection() + { + CheckConnectOperationsServer(); + if (State == PipeState.Connected) + { + throw new InvalidOperationException(SR.InvalidOperation_PipeAlreadyConnected); + } + + HandleAcceptedSocket(_socket.Accept()); } public Task WaitForConnectionAsync(CancellationToken cancellationToken) @@ -111,37 +95,35 @@ namespace System.IO.Pipes return cancellationToken.IsCancellationRequested ? Task.FromCanceled(cancellationToken) : WaitForConnectionAsyncCore(); + + async Task WaitForConnectionAsyncCore() => + HandleAcceptedSocket(await _socket.AcceptAsync().ConfigureAwait(false)); } - private async Task WaitForConnectionAsyncCore() - { - // This is the same implementation as is in WaitForConnection(), but using Socket.AcceptAsync - // instead of Socket.Accept. - - // Binding to an existing path fails, so we need to remove anything left over at this location. - // There's of course a race condition here, where it could be recreated by someone else between this - // deletion and the bind below, in which case we'll simply let the bind fail and throw. - Interop.Sys.Unlink(_path); // ignore any failures - var socket = new Socket(AddressFamily.Unix, SocketType.Stream, ProtocolType.Unspecified); + private void HandleAcceptedSocket(Socket acceptedSocket) + { + var serverHandle = new SafePipeHandle(acceptedSocket); try { - socket.Bind(new UnixDomainSocketEndPoint(_path)); - socket.Listen(1); - - Socket acceptedSocket = await socket.AcceptAsync().ConfigureAwait(false); - SafePipeHandle serverHandle = new SafePipeHandle(acceptedSocket); ConfigureSocket(acceptedSocket, serverHandle, _direction, _inBufferSize, _outBufferSize, _inheritability); - - InitializeHandle(serverHandle, isExposed: false, isAsync: (_options & PipeOptions.Asynchronous) != 0); - State = PipeState.Connected; } - finally + catch { - // Bind will have created a file. Now that the client is connected, it's no longer necessary, so get rid of it. - Interop.Sys.Unlink(_path); // ignore any failures; worst case is we leave a tmp file + serverHandle.Dispose(); + acceptedSocket.Dispose(); + throw; + } - // Clean up the listening socket - socket.Dispose(); + InitializeHandle(serverHandle, isExposed: false, isAsync: (_options & PipeOptions.Asynchronous) != 0); + State = PipeState.Connected; + } + + internal override void DisposeCore(bool disposing) + { + Interop.Sys.Unlink(_path); // ignore any failures + if (disposing) + { + _socket?.Dispose(); } } diff --git a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Unix.cs b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Unix.cs index 84fc05a..0c540a2 100644 --- a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Unix.cs +++ b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Unix.cs @@ -49,10 +49,14 @@ namespace System.IO.Pipes throw new PlatformNotSupportedException(SR.PlatformNotSupproted_InvalidNameChars); } - // Return the pipe path. The pipe is created directly under %TMPDIR%. We don't bother - // putting it into subdirectories, as the pipe will only exist on disk for the - // duration between when the server starts listening and the client connects, after - // which the pipe will be deleted. + // Return the pipe path. The pipe is created directly under %TMPDIR%. We previously + // didn't put it into a subdirectory because it only existed on disk for the duration + // between when the server started listening in WaitForConnection and when the client + // connected, after which the pipe was deleted. We now create the pipe when the + // server stream is created, which leaves it on disk longer, but we can't change the + // naming scheme used as that breaks the ability for code running on an older + // runtime to connect to code running on the newer runtime. That means we're stuck + // with a tmp file for the lifetime of the server stream. return s_pipePrefix + pipeName; } @@ -83,7 +87,7 @@ namespace System.IO.Pipes // nop } - private void UninitializeAsyncHandle() + internal virtual void DisposeCore(bool disposing) { // nop } diff --git a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs index ade06d3..f35a589 100644 --- a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs +++ b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs @@ -47,10 +47,12 @@ namespace System.IO.Pipes _threadPoolBinding = ThreadPoolBoundHandle.BindHandle(handle); } - private void UninitializeAsyncHandle() + private void DisposeCore(bool disposing) { - if (_threadPoolBinding != null) - _threadPoolBinding.Dispose(); + if (disposing) + { + _threadPoolBinding?.Dispose(); + } } [SecurityCritical] diff --git a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.cs b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.cs index 928ad8a..27e4c90 100644 --- a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.cs +++ b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.cs @@ -6,7 +6,6 @@ using Microsoft.Win32.SafeHandles; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Diagnostics.Contracts; -using System.Runtime.InteropServices; using System.Security; using System.Threading; using System.Threading.Tasks; @@ -395,7 +394,7 @@ namespace System.IO.Pipes _handle.Dispose(); } - UninitializeAsyncHandle(); + DisposeCore(disposing); } finally { diff --git a/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.Specific.cs b/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.Specific.cs index caaf0a8..c8499e3 100644 --- a/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.Specific.cs +++ b/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.Specific.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Collections.Generic; using System.Runtime.InteropServices; using System.Security.Principal; using System.Threading; @@ -77,6 +78,48 @@ namespace System.IO.Pipes.Tests } [Theory] + [InlineData(1)] + [InlineData(3)] + public async Task MultipleWaitingClients_ServerServesOneAtATime(int numClients) + { + string name = GetUniquePipeName(); + using (NamedPipeServerStream server = new NamedPipeServerStream(name)) + { + var clients = new List(numClients); + var clientConnects = new List(); + for (int i = 0; i < numClients; i++) + { + clients.Add(new NamedPipeClientStream(name)); + clientConnects.Add(clients[i].ConnectAsync()); + } + + while (clientConnects.Count > 0) + { + Task serverWait = server.WaitForConnectionAsync(); + + int connectedIndex = clientConnects.IndexOf(await Task.WhenAny(clientConnects)); + NamedPipeClientStream client = clients[connectedIndex]; + clientConnects.RemoveAt(connectedIndex); + clients.RemoveAt(connectedIndex); + + await serverWait; + + Task writeAsync = client.WriteAsync(new byte[1], 0, 1); + Task readAsync = server.ReadAsync(new byte[1], 0, 1); + await Task.WhenAll(writeAsync, readAsync); + Assert.Equal(1, await readAsync); + + writeAsync = server.WriteAsync(new byte[1], 0, 1); + readAsync = client.ReadAsync(new byte[1], 0, 1); + await Task.WhenAll(writeAsync, readAsync); + Assert.Equal(1, await readAsync); + + server.Disconnect(); + } + } + } + + [Theory] [InlineData(PipeOptions.None)] [InlineData(PipeOptions.Asynchronous)] [PlatformSpecific(TestPlatforms.Windows)] // Unix currently doesn't support message mode -- 2.7.4