From: Stephen Toub Date: Thu, 31 Jan 2019 14:46:39 +0000 (-0500) Subject: Fix several related inheritability bugs on Unix (dotnet/corefx#34967) X-Git-Tag: submit/tizen/20210909.063632~11031^2~2559 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=f927467d2f94cbe8ff8245d7acb3f9e17e837ca3;p=platform%2Fupstream%2Fdotnet%2Fruntime.git Fix several related inheritability bugs on Unix (dotnet/corefx#34967) - AnonymousPipeServerStream accepts a HandleInheritability value. When it's set to None, both ends of the pipe are correctly created as O_CLOEXEC to prevent them from being inherited. However, when it's set to Inheritable, we're not marking either end of the pipe as O_CLOEXEC, which means both file descriptors are inherited by the child process. While technically that meets the letter of the law as to what the API requires, in the 99.9% use case the server handle really isn't intended to be inherited, and rather it's only the client handle that is; by allowing the server handle to also be inherited, when the server closes its end of the pipe, the client doesn't get an EOF or broken pipe, because it still has its dup of the file descriptor open, and will for the lifetime of the process. The fix is to always mark the server file descriptor as O_CLOEXEC even if the inheritability is set to Inheritable. - NamedPipeServer/ClientStream is built on Socket. Previously, sockets defaulted to being inheritable, but we changed that for .NET Core 3.0, such that they're now not inherited. But the pipe code was written expecting the default of Inheritable, and has an if block that says "if inheritability requested was not inheritable, then set O_CLOEXEC"; that now needs to be flipped to say "if inheritability was requested, clear O_CLOEXEC". - MemoryMappedFile has a similar issue. It tries to use shm_open to create the backing store, but shm_open defaults to having O_CLOEXEC set, and so if Inheritabile was requested, we're neglecting to clear the O_CLOEXEC flag. Commit migrated from https://github.com/dotnet/corefx/commit/e5d6c6fc20e72c9d5ba3214f99c25f418bcff0a0 --- diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Fcntl.SetCloseOnExec.cs b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Fcntl.SetCloseOnExec.cs deleted file mode 100644 index ec0836bacb8..00000000000 --- a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Fcntl.SetCloseOnExec.cs +++ /dev/null @@ -1,19 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// 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; -using System.Runtime.InteropServices; -using Microsoft.Win32.SafeHandles; - -internal static partial class Interop -{ - internal static partial class Sys - { - internal static partial class Fcntl - { - [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_FcntlSetCloseOnExec", SetLastError=true)] - internal static extern int SetCloseOnExec(SafeHandle fd); - } - } -} diff --git a/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Fcntl.SetFD.cs b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Fcntl.SetFD.cs new file mode 100644 index 00000000000..d59f569b4e0 --- /dev/null +++ b/src/libraries/Common/src/Interop/Unix/System.Native/Interop.Fcntl.SetFD.cs @@ -0,0 +1,19 @@ +// Licensed to the .NET Foundation under one or more agreements. +// 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; +using System.Runtime.InteropServices; +using Microsoft.Win32.SafeHandles; + +internal static partial class Interop +{ + internal static partial class Sys + { + internal static partial class Fcntl + { + [DllImport(Libraries.SystemNative, EntryPoint = "SystemNative_FcntlSetFD", SetLastError=true)] + internal static extern int SetFD(SafeHandle fd, int flags); + } + } +} diff --git a/src/libraries/Native/Unix/System.Native/pal_io.c b/src/libraries/Native/Unix/System.Native/pal_io.c index 3c676a6c732..fe0a8545151 100644 --- a/src/libraries/Native/Unix/System.Native/pal_io.c +++ b/src/libraries/Native/Unix/System.Native/pal_io.c @@ -529,10 +529,10 @@ int32_t SystemNative_Pipe(int32_t pipeFds[2], int32_t flags) return result; } -int32_t SystemNative_FcntlSetCloseOnExec(intptr_t fd) +int32_t SystemNative_FcntlSetFD(intptr_t fd, int32_t flags) { int result; - while ((result = fcntl(ToFileDescriptor(fd), F_SETFD, FD_CLOEXEC)) < 0 && errno == EINTR); + while ((result = fcntl(ToFileDescriptor(fd), F_SETFD, ConvertOpenFlags(flags))) < 0 && errno == EINTR); return result; } diff --git a/src/libraries/Native/Unix/System.Native/pal_io.h b/src/libraries/Native/Unix/System.Native/pal_io.h index 79735cb877b..5806e3a8dd3 100644 --- a/src/libraries/Native/Unix/System.Native/pal_io.h +++ b/src/libraries/Native/Unix/System.Native/pal_io.h @@ -421,7 +421,7 @@ DLLEXPORT int32_t SystemNative_Pipe(int32_t pipefd[2], // [out] pipefds[0] gets * * Returns 0 for success; -1 for failure. Sets errno for failure. */ -DLLEXPORT int32_t SystemNative_FcntlSetCloseOnExec(intptr_t fd); +DLLEXPORT int32_t SystemNative_FcntlSetFD(intptr_t fd, int32_t flags); /** * Determines if the current platform supports getting and setting pipe capacity. diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System.IO.MemoryMappedFiles.csproj b/src/libraries/System.IO.MemoryMappedFiles/src/System.IO.MemoryMappedFiles.csproj index 38d4b38ea4c..a5fd32caaea 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System.IO.MemoryMappedFiles.csproj +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System.IO.MemoryMappedFiles.csproj @@ -125,8 +125,8 @@ Common\CoreLib\Interop\Unix\Interop.Errors.cs - - Common\Interop\Unix\Interop.Fcntl.SetCloseOnExec.cs + + Common\Interop\Unix\Interop.Fcntl.SetFD.cs Common\CoreLib\Interop\Unix\Interop.IOErrors.cs diff --git a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs index 172bf39afe6..7c78f4d695d 100644 --- a/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs +++ b/src/libraries/System.IO.MemoryMappedFiles/src/System/IO/MemoryMappedFiles/MemoryMappedFile.Unix.cs @@ -68,13 +68,7 @@ namespace System.IO.MemoryMappedFiles if ((protections & Interop.Sys.MemoryMappedProtections.PROT_WRITE) != 0 && capacity > 0) { ownsFileStream = true; - fileStream = CreateSharedBackingObject(protections, capacity); - - // If the MMF handle should not be inherited, mark the backing object fd as O_CLOEXEC. - if (inheritability == HandleInheritability.None) - { - Interop.CheckIo(Interop.Sys.Fcntl.SetCloseOnExec(fileStream.SafeFileHandle)); - } + fileStream = CreateSharedBackingObject(protections, capacity, inheritability); } } @@ -134,10 +128,10 @@ namespace System.IO.MemoryMappedFiles FileAccess.Read; } - private static FileStream CreateSharedBackingObject(Interop.Sys.MemoryMappedProtections protections, long capacity) + private static FileStream CreateSharedBackingObject(Interop.Sys.MemoryMappedProtections protections, long capacity, HandleInheritability inheritability) { - return CreateSharedBackingObjectUsingMemory(protections, capacity) - ?? CreateSharedBackingObjectUsingFile(protections, capacity); + return CreateSharedBackingObjectUsingMemory(protections, capacity, inheritability) + ?? CreateSharedBackingObjectUsingFile(protections, capacity, inheritability); } // ----------------------------- @@ -145,7 +139,7 @@ namespace System.IO.MemoryMappedFiles // ----------------------------- private static FileStream CreateSharedBackingObjectUsingMemory( - Interop.Sys.MemoryMappedProtections protections, long capacity) + Interop.Sys.MemoryMappedProtections protections, long capacity, HandleInheritability inheritability) { // The POSIX shared memory object name must begin with '/'. After that we just want something short and unique. string mapName = "/corefx_map_" + Guid.NewGuid().ToString("N"); @@ -195,6 +189,13 @@ namespace System.IO.MemoryMappedFiles // causing it to preemptively throw from SetLength. Interop.CheckIo(Interop.Sys.FTruncate(fd, capacity)); + // shm_open sets CLOEXEC implicitly. If the inheritability requested is Inheritable, remove CLOEXEC. + if (inheritability == HandleInheritability.Inheritable && + Interop.Sys.Fcntl.SetFD(fd, 0) == -1) + { + throw Interop.GetExceptionForIoErrno(Interop.Sys.GetLastErrorInfo()); + } + // Wrap the file descriptor in a stream and return it. return new FileStream(fd, TranslateProtectionsToFileAccess(protections)); } @@ -205,21 +206,20 @@ namespace System.IO.MemoryMappedFiles } } - private static FileStream CreateSharedBackingObjectUsingFile(Interop.Sys.MemoryMappedProtections protections, long capacity) + private static FileStream CreateSharedBackingObjectUsingFile(Interop.Sys.MemoryMappedProtections protections, long capacity, HandleInheritability inheritability) { // We create a temporary backing file in TMPDIR. We don't bother putting it into subdirectories as the file exists // extremely briefly: it's opened/created and then immediately unlinked. string path = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N")); - FileAccess access = - (protections & (Interop.Sys.MemoryMappedProtections.PROT_READ | Interop.Sys.MemoryMappedProtections.PROT_WRITE)) != 0 ? FileAccess.ReadWrite : - (protections & (Interop.Sys.MemoryMappedProtections.PROT_WRITE)) != 0 ? FileAccess.Write : - FileAccess.Read; + FileShare share = inheritability == HandleInheritability.None ? + FileShare.ReadWrite : + FileShare.ReadWrite | FileShare.Inheritable; // Create the backing file, then immediately unlink it so that it'll be cleaned up when no longer in use. // Then enlarge it to the requested capacity. const int DefaultBufferSize = 0x1000; - var fs = new FileStream(path, FileMode.CreateNew, TranslateProtectionsToFileAccess(protections), FileShare.ReadWrite, DefaultBufferSize); + var fs = new FileStream(path, FileMode.CreateNew, TranslateProtectionsToFileAccess(protections), share, DefaultBufferSize); try { Interop.CheckIo(Interop.Sys.Unlink(path)); diff --git a/src/libraries/System.IO.Pipes/src/System.IO.Pipes.csproj b/src/libraries/System.IO.Pipes/src/System.IO.Pipes.csproj index 845787cefb2..4e274eebbb7 100644 --- a/src/libraries/System.IO.Pipes/src/System.IO.Pipes.csproj +++ b/src/libraries/System.IO.Pipes/src/System.IO.Pipes.csproj @@ -180,8 +180,8 @@ Common\Interop\Unix\Interop.Fcntl.Pipe.cs - - Common\Interop\Unix\Interop.Fcntl.SetCloseOnExec.cs + + Common\Interop\Unix\Interop.Fcntl.SetFD.cs Common\Interop\Unix\Interop.FLock.cs diff --git a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/AnonymousPipeServerStream.Unix.cs b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/AnonymousPipeServerStream.Unix.cs index 0580eeb11bb..9797ab153b9 100644 --- a/src/libraries/System.IO.Pipes/src/System/IO/Pipes/AnonymousPipeServerStream.Unix.cs +++ b/src/libraries/System.IO.Pipes/src/System/IO/Pipes/AnonymousPipeServerStream.Unix.cs @@ -21,9 +21,26 @@ namespace System.IO.Pipes SafePipeHandle serverHandle, clientHandle; if (direction == PipeDirection.In) - CreateAnonymousPipe(inheritability, reader: out serverHandle, writer: out clientHandle); + { + CreateAnonymousPipe(reader: out serverHandle, writer: out clientHandle); + } else - CreateAnonymousPipe(inheritability, reader: out clientHandle, writer: out serverHandle); + { + CreateAnonymousPipe(reader: out clientHandle, writer: out serverHandle); + } + + // We always create pipes with both file descriptors being O_CLOEXEC. + // If inheritability is requested, we clear the O_CLOEXEC flag + // from the child descriptor so that it can be passed to a child process. + // We assume that the HandleInheritability only applies to the child fd, + // as if we allowed the server fd to be inherited, then when this process + // closes its end of the pipe, the client won't receive an EOF or broken + // pipe notification, as the child will still have open its dup of the fd. + if (inheritability == HandleInheritability.Inheritable && + Interop.Sys.Fcntl.SetFD(clientHandle, 0) == -1) + { + throw Interop.GetExceptionForIoErrno(Interop.Sys.GetLastErrorInfo()); + } // Configure the pipe. For buffer size, the size applies to the pipe, rather than to // just one end's file descriptor, so we only need to do this with one of the handles. 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 e0448e3588a..e1d098ea2ea 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 @@ -404,19 +404,20 @@ namespace System.IO.Pipes } /// Creates an anonymous pipe. - /// The inheritability to try to use. This may not always be honored, depending on platform. /// The resulting reader end of the pipe. /// The resulting writer end of the pipe. - internal static unsafe void CreateAnonymousPipe( - HandleInheritability inheritability, out SafePipeHandle reader, out SafePipeHandle writer) + internal static unsafe void CreateAnonymousPipe(out SafePipeHandle reader, out SafePipeHandle writer) { // Allocate the safe handle objects prior to calling pipe/pipe2, in order to help slightly in low-mem situations reader = new SafePipeHandle(); writer = new SafePipeHandle(); - // Create the OS pipe + // Create the OS pipe. We always create it as O_CLOEXEC (trying to do so atomically) so that the + // file descriptors aren't inherited. Then if inheritability was requested, we opt-in the child file + // descriptor later; if the server file descriptor was also inherited, closing the server file + // descriptor would fail to signal EOF for the child side. int* fds = stackalloc int[2]; - CreateAnonymousPipe(inheritability, fds); + Interop.CheckIo(Interop.Sys.Pipe(fds, Interop.Sys.PipeFlags.O_CLOEXEC)); // Store the file descriptors into our safe handles reader.SetHandle(fds[Interop.Sys.ReadEndOfPipe]); @@ -452,13 +453,6 @@ namespace System.IO.Pipes _outBufferSize; } - internal static unsafe void CreateAnonymousPipe(HandleInheritability inheritability, int* fdsptr) - { - var flags = (inheritability & HandleInheritability.Inheritable) == 0 ? - Interop.Sys.PipeFlags.O_CLOEXEC : 0; - Interop.CheckIo(Interop.Sys.Pipe(fdsptr, flags)); - } - internal static void ConfigureSocket( Socket s, SafePipeHandle pipeHandle, PipeDirection direction, int inBufferSize, int outBufferSize, HandleInheritability inheritability) @@ -473,9 +467,11 @@ namespace System.IO.Pipes s.SendBufferSize = outBufferSize; } - if (inheritability != HandleInheritability.Inheritable) + // Sockets are created with O_CLOEXEC. If inheritability has been requested, we need to unset the flag. + if (inheritability == HandleInheritability.Inheritable && + Interop.Sys.Fcntl.SetFD(s.SafeHandle, 0) == -1) { - Interop.Sys.Fcntl.SetCloseOnExec(pipeHandle); // ignore failures, best-effort attempt + throw Interop.GetExceptionForIoErrno(Interop.Sys.GetLastErrorInfo()); } switch (direction) 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 38671e676d9..25bc4bdcdc3 100644 --- a/src/libraries/System.IO.Pipes/tests/AnonymousPipeTests/AnonymousPipeTest.CrossProcess.cs +++ b/src/libraries/System.IO.Pipes/tests/AnonymousPipeTests/AnonymousPipeTest.CrossProcess.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System.Diagnostics; +using System.Threading; using Xunit; namespace System.IO.Pipes.Tests @@ -16,7 +17,7 @@ namespace System.IO.Pipes.Tests // Then spawn another process to communicate with. using (var outbound = new AnonymousPipeServerStream(PipeDirection.Out, HandleInheritability.Inheritable)) using (var inbound = new AnonymousPipeServerStream(PipeDirection.In, HandleInheritability.Inheritable)) - using (var remote = RemoteInvoke(new Func(PingPong_OtherProcess), outbound.GetClientHandleAsString(), inbound.GetClientHandleAsString())) + using (var remote = RemoteInvoke(new Func(ChildFunc), outbound.GetClientHandleAsString(), inbound.GetClientHandleAsString())) { // Close our local copies of the handles now that we've passed them of to the other process outbound.DisposeLocalCopyOfClientHandle(); @@ -30,23 +31,78 @@ namespace System.IO.Pipes.Tests Assert.Equal(i, received); } } + + int ChildFunc(string inHandle, string outHandle) + { + // Create the clients associated with the supplied handles + using (var inbound = new AnonymousPipeClientStream(PipeDirection.In, inHandle)) + using (var outbound = new AnonymousPipeClientStream(PipeDirection.Out, outHandle)) + { + // Repeatedly read then write a byte from and to the server + for (int i = 0; i < 10; i++) + { + int b = inbound.ReadByte(); + outbound.WriteByte((byte)b); + } + } + return SuccessExitCode; + } } - private static int PingPong_OtherProcess(string inHandle, string outHandle) + [Fact] + public void ServerClosesPipe_ClientReceivesEof() { - // Create the clients associated with the supplied handles - using (var inbound = new AnonymousPipeClientStream(PipeDirection.In, inHandle)) - using (var outbound = new AnonymousPipeClientStream(PipeDirection.Out, outHandle)) + using (var pipe = new AnonymousPipeServerStream(PipeDirection.Out, HandleInheritability.Inheritable)) + using (var remote = RemoteInvoke(new Func(ChildFunc), pipe.GetClientHandleAsString())) { - // Repeatedly read then write a byte from and to the server - for (int i = 0; i < 10; i++) + pipe.DisposeLocalCopyOfClientHandle(); + pipe.Write(new byte[] { 1, 2, 3, 4, 5 }, 0, 5); + + pipe.Dispose(); + + Assert.True(remote.Process.WaitForExit(30_000)); + } + + int ChildFunc(string clientHandle) + { + using (var pipe = new AnonymousPipeClientStream(PipeDirection.In, clientHandle)) { - int b = inbound.ReadByte(); - outbound.WriteByte((byte)b); + for (int i = 1; i <= 5; i++) + { + Assert.Equal(i, pipe.ReadByte()); + } + Assert.Equal(-1, pipe.ReadByte()); } + return SuccessExitCode; } - return SuccessExitCode; } + [Fact] + public void ClientClosesPipe_ServerReceivesEof() + { + using (var pipe = new AnonymousPipeServerStream(PipeDirection.In, HandleInheritability.Inheritable)) + using (var remote = RemoteInvoke(new Func(ChildFunc), pipe.GetClientHandleAsString(), new RemoteInvokeOptions { CheckExitCode = false })) + { + pipe.DisposeLocalCopyOfClientHandle(); + + for (int i = 1; i <= 5; i++) + { + Assert.Equal(i, pipe.ReadByte()); + } + Assert.Equal(-1, pipe.ReadByte()); + + remote.Process.Kill(); + } + + int ChildFunc(string clientHandle) + { + using (var pipe = new AnonymousPipeClientStream(PipeDirection.Out, clientHandle)) + { + pipe.Write(new byte[] { 1, 2, 3, 4, 5 }, 0, 5); + } + Thread.CurrentThread.Join(); + return SuccessExitCode; + } + } } } diff --git a/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CrossProcess.cs b/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CrossProcess.cs index bb3455b57b4..929f00e4323 100644 --- a/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CrossProcess.cs +++ b/src/libraries/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CrossProcess.cs @@ -3,7 +3,10 @@ // See the LICENSE file in the project root for more information. using System.Diagnostics; +using System.Globalization; +using System.Security.Principal; using System.Threading.Tasks; +using Microsoft.Win32.SafeHandles; using Xunit; namespace System.IO.Pipes.Tests @@ -11,6 +14,38 @@ namespace System.IO.Pipes.Tests [ActiveIssue(22271, TargetFrameworkMonikers.UapNotUapAot)] public sealed class NamedPipeTest_CrossProcess : RemoteExecutorTestBase { + [Fact] + public void InheritHandles_AvailableInChildProcess() + { + string pipeName = GetUniquePipeName(); + + using (var server = new NamedPipeServerStream(pipeName, PipeDirection.In)) + using (var client = new NamedPipeClientStream(".", pipeName, PipeDirection.Out, PipeOptions.None, TokenImpersonationLevel.None, HandleInheritability.Inheritable)) + { + Task.WaitAll(server.WaitForConnectionAsync(), client.ConnectAsync()); + using (RemoteInvoke(new Func(ChildFunc), client.SafePipeHandle.DangerousGetHandle().ToString())) + { + client.Dispose(); + for (int i = 0; i < 5; i++) + { + Assert.Equal(i, server.ReadByte()); + } + } + } + + int ChildFunc(string handle) + { + using (var childClient = new NamedPipeClientStream(PipeDirection.Out, isAsync: false, isConnected: true, new SafePipeHandle((IntPtr)long.Parse(handle, CultureInfo.InvariantCulture), ownsHandle: true))) + { + for (int i = 0; i < 5; i++) + { + childClient.WriteByte((byte)i); + } + } + return SuccessExitCode; + } + } + [Fact] public void PingPong_Sync() {