From c2a2bb2c6d33450080e2a7e02463bc380a064186 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 18 Nov 2022 16:18:47 +0100 Subject: [PATCH 1/3] add a failing test case --- .../AnonymousPipeTest.CrossProcess.cs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.IO.Pipes/tests/AnonymousPipeTests/AnonymousPipeTest.CrossProcess.cs b/src/libraries/System.IO.Pipes/tests/AnonymousPipeTests/AnonymousPipeTest.CrossProcess.cs index 9a68529796319b..85dd1fac34bfbf 100644 --- a/src/libraries/System.IO.Pipes/tests/AnonymousPipeTests/AnonymousPipeTest.CrossProcess.cs +++ b/src/libraries/System.IO.Pipes/tests/AnonymousPipeTests/AnonymousPipeTest.CrossProcess.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Diagnostics; using System.Threading; using Microsoft.DotNet.RemoteExecutor; using Xunit; @@ -48,16 +47,23 @@ void ChildFunc(string inHandle, string outHandle) } } - [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] - public void ServerClosesPipe_ClientReceivesEof() + [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + [InlineData(true)] + [InlineData(false)] + public void ServerClosesPipe_ClientReceivesEof(bool callDisposeLocalCopyOfClientHandle) { using (var pipe = new AnonymousPipeServerStream(PipeDirection.Out, HandleInheritability.Inheritable)) using (var remote = RemoteExecutor.Invoke(new Action(ChildFunc), pipe.GetClientHandleAsString())) { - pipe.DisposeLocalCopyOfClientHandle(); + if (callDisposeLocalCopyOfClientHandle) + { + pipe.DisposeLocalCopyOfClientHandle(); + } + pipe.Write(new byte[] { 1, 2, 3, 4, 5 }, 0, 5); pipe.Dispose(); + Assert.True(pipe.ClientSafePipeHandle.IsClosed); Assert.True(remote.Process.WaitForExit(30_000)); } From b3de41eb5bedb23f6a420bc8087206b872e52b22 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 18 Nov 2022 16:22:28 +0100 Subject: [PATCH 2/3] When AnonymousPipeServerStream is created with HandleInheritability.Inheritable, its Dispose method should dispose the client no matter if it was exposed or not. This is going to allow users who did not call DisposeLocalCopyOfClientHandle to avoid resource leaks. --- .../src/System/IO/Pipes/AnonymousPipeServerStream.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/AnonymousPipeServerStream.cs b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/AnonymousPipeServerStream.cs index cdb54c60e37e9a..94a5f0a8e0fa48 100644 --- a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/AnonymousPipeServerStream.cs +++ b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/AnonymousPipeServerStream.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Diagnostics.CodeAnalysis; using Microsoft.Win32.SafeHandles; namespace System.IO.Pipes @@ -13,6 +12,7 @@ public sealed partial class AnonymousPipeServerStream : PipeStream { private SafePipeHandle _clientHandle = null!; private bool _clientHandleExposed; + private readonly HandleInheritability _inheritability; public AnonymousPipeServerStream() : this(PipeDirection.Out, HandleInheritability.None, 0) @@ -73,6 +73,7 @@ public AnonymousPipeServerStream(PipeDirection direction, HandleInheritability i } Create(direction, inheritability, bufferSize); + _inheritability = inheritability; } ~AnonymousPipeServerStream() @@ -121,10 +122,10 @@ protected override void Dispose(bool disposing) { try { - // We should dispose of the client handle if it was not exposed. - if (!_clientHandleExposed && _clientHandle != null && !_clientHandle.IsClosed) + // We should dispose of the client handle if it was created inheritable or it was not exposed at all. + if (_inheritability == HandleInheritability.Inheritable || !_clientHandleExposed) { - _clientHandle.Dispose(); + DisposeLocalCopyOfClientHandle(); } } finally From 5fd2b0eeac021cd77d06eefdb1d7a767e9903ad2 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 18 Nov 2022 20:17:29 +0100 Subject: [PATCH 3/3] address code review feedback: dispose client handle only when it was created with HandleInheritability.Inheritable and not exposed via SafePipeHandle property --- .../src/System/IO/Pipes/AnonymousPipeServerStream.cs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/AnonymousPipeServerStream.cs b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/AnonymousPipeServerStream.cs index 94a5f0a8e0fa48..d9c1221662b389 100644 --- a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/AnonymousPipeServerStream.cs +++ b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/AnonymousPipeServerStream.cs @@ -11,7 +11,7 @@ namespace System.IO.Pipes public sealed partial class AnonymousPipeServerStream : PipeStream { private SafePipeHandle _clientHandle = null!; - private bool _clientHandleExposed; + private bool _clientHandleExposed, _clientHandleExposedAsString; private readonly HandleInheritability _inheritability; public AnonymousPipeServerStream() @@ -85,7 +85,7 @@ public AnonymousPipeServerStream(PipeDirection direction, HandleInheritability i // processes. For now, people do it via command line arguments. public string GetClientHandleAsString() { - _clientHandleExposed = true; + _clientHandleExposedAsString =_clientHandleExposed = true; GC.SuppressFinalize(_clientHandle); return _clientHandle.DangerousGetHandle().ToString(); } @@ -122,8 +122,9 @@ protected override void Dispose(bool disposing) { try { - // We should dispose of the client handle if it was created inheritable or it was not exposed at all. - if (_inheritability == HandleInheritability.Inheritable || !_clientHandleExposed) + // We should dispose of the client handle when it was not exposed at all OR + // it was exposed as a string (handle finalization has been suppressed) and created inheritable (out-of-proc communication). + if (!_clientHandleExposed || (_clientHandleExposedAsString && _inheritability == HandleInheritability.Inheritable)) { DisposeLocalCopyOfClientHandle(); }