From 35a32c0f5d4021bc7ab533e197896d6711c936d3 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Fri, 14 Aug 2020 11:42:41 +0100 Subject: [PATCH] Revert "Use socketpair to implement Process.Start redirection (#34861)" This reverts commit c44dc40b763b7c74012622a0a6120cd8ffa35ce4. --- .../Native/Unix/System.Native/pal_process.c | 50 +++++------------ .../src/System.Diagnostics.Process.csproj | 1 - .../src/System/Diagnostics/Process.Unix.cs | 12 ++-- .../tests/ProcessStreamReadTests.cs | 36 ------------ .../src/System/Net/Sockets/Socket.cs | 55 ++++++++----------- 5 files changed, 42 insertions(+), 112 deletions(-) diff --git a/src/libraries/Native/Unix/System.Native/pal_process.c b/src/libraries/Native/Unix/System.Native/pal_process.c index 44cb59d4c1a2ae..285a94916c4c2d 100644 --- a/src/libraries/Native/Unix/System.Native/pal_process.c +++ b/src/libraries/Native/Unix/System.Native/pal_process.c @@ -20,8 +20,9 @@ #if HAVE_CRT_EXTERNS_H #include #endif +#if HAVE_PIPE2 #include -#include +#endif #include #if HAVE_SCHED_SETAFFINITY || HAVE_SCHED_GETAFFINITY @@ -44,7 +45,7 @@ c_static_assert(PAL_PRIO_PROCESS == (int)PRIO_PROCESS); c_static_assert(PAL_PRIO_PGRP == (int)PRIO_PGRP); c_static_assert(PAL_PRIO_USER == (int)PRIO_USER); -#ifndef SOCK_CLOEXEC +#if !HAVE_PIPE2 static pthread_mutex_t ProcessCreateLock = PTHREAD_MUTEX_INITIALIZER; #endif @@ -179,31 +180,6 @@ static int SetGroups(uint32_t* userGroups, int32_t userGroupsLength, uint32_t* p return rv; } -static int32_t SocketPair(int32_t sv[2]) -{ - int32_t result; - - int type = SOCK_STREAM; -#ifdef SOCK_CLOEXEC - type |= SOCK_CLOEXEC; -#endif - - while ((result = socketpair(AF_UNIX, type, 0, sv)) < 0 && errno == EINTR); - -#ifndef SOCK_CLOEXEC - if (result == 0) - { - while ((result = fcntl(sv[READ_END_OF_PIPE], F_SETFD, FD_CLOEXEC)) < 0 && errno == EINTR); - if (result == 0) - { - while ((result = fcntl(sv[WRITE_END_OF_PIPE], F_SETFD, FD_CLOEXEC)) < 0 && errno == EINTR); - } - } -#endif - - return result; -} - int32_t SystemNative_ForkAndExecProcess(const char* filename, char* const argv[], char* const envp[], @@ -222,7 +198,7 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, int32_t* stderrFd) { #if HAVE_FORK -#ifndef SOCK_CLOEXEC +#if !HAVE_PIPE2 bool haveProcessCreateLock = false; #endif bool success = true; @@ -278,11 +254,11 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, goto done; } -#ifndef SOCK_CLOEXEC - // We do not have SOCK_CLOEXEC; take the lock to emulate it race free. - // If another process were to be launched between the socket creation and the fcntl call to set CLOEXEC on it, that - // file descriptor would be inherited into the other child process, eventually causing a deadlock either in the loop - // below that waits for that socket to be closed or in StreamReader.ReadToEnd() in the calling code. +#if !HAVE_PIPE2 + // We do not have pipe2(); take the lock to emulate it race free. + // If another process were to be launched between the pipe creation and the fcntl call to set CLOEXEC on it, that + // file descriptor will be inherited into the other child process, eventually causing a deadlock either in the loop + // below that waits for that pipe to be closed or in StreamReader.ReadToEnd() in the calling code. if (pthread_mutex_lock(&ProcessCreateLock) != 0) { // This check is pretty much just checking for trashed memory. @@ -294,9 +270,9 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, // Open pipes for any requests to redirect stdin/stdout/stderr and set the // close-on-exec flag to the pipe file descriptors. - if ((redirectStdin && SocketPair(stdinFds) != 0) || - (redirectStdout && SocketPair(stdoutFds) != 0) || - (redirectStderr && SocketPair(stderrFds) != 0)) + if ((redirectStdin && SystemNative_Pipe(stdinFds, PAL_O_CLOEXEC) != 0) || + (redirectStdout && SystemNative_Pipe(stdoutFds, PAL_O_CLOEXEC) != 0) || + (redirectStderr && SystemNative_Pipe(stderrFds, PAL_O_CLOEXEC) != 0)) { success = false; goto done; @@ -447,7 +423,7 @@ int32_t SystemNative_ForkAndExecProcess(const char* filename, *stderrFd = stderrFds[READ_END_OF_PIPE]; done:; -#ifndef SOCK_CLOEXEC +#if !HAVE_PIPE2 if (haveProcessCreateLock) { pthread_mutex_unlock(&ProcessCreateLock); diff --git a/src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj b/src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj index 947d0f2a1bb0ca..05f3fba55529b2 100644 --- a/src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj +++ b/src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj @@ -337,7 +337,6 @@ - diff --git a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs index fee92705d93102..0cbcefdb4c919a 100644 --- a/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -5,7 +5,6 @@ using System.Collections.Generic; using System.ComponentModel; using System.IO; -using System.Net.Sockets; using System.Security; using System.Text; using System.Threading; @@ -754,15 +753,16 @@ internal static TimeSpan TicksToTimeSpan(double ticks) return TimeSpan.FromSeconds(ticks / (double)ticksPerSecond); } - /// Opens a stream around the specified socket file descriptor and with the specified access. - /// The socket file descriptor. + /// Opens a stream around the specified file descriptor and with the specified access. + /// The file descriptor. /// The access mode. /// The opened stream. - private static Stream OpenStream(int fd, FileAccess access) + private static FileStream OpenStream(int fd, FileAccess access) { Debug.Assert(fd >= 0); - var socket = new Socket(new SafeSocketHandle((IntPtr)fd, ownsHandle: true)); - return new NetworkStream(socket, access, ownsSocket: true); + return new FileStream( + new SafeFileHandle((IntPtr)fd, ownsHandle: true), + access, StreamBufferSize, isAsync: false); } /// Parses a command-line argument string into a list of arguments. diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs index 1e906e4a803c74..c30211baa0f86b 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs @@ -344,42 +344,6 @@ async private Task WaitPipeSignal(PipeStream pipe, int millisecond) } } - [PlatformSpecific(~TestPlatforms.Windows)] // currently on Windows these operations async-over-sync on Windows - [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] - public async Task ReadAsync_OutputStreams_Cancel_RespondsQuickly() - { - Process p = CreateProcessLong(); - try - { - p.StartInfo.RedirectStandardOutput = true; - p.StartInfo.RedirectStandardError = true; - Assert.True(p.Start()); - - using (var cts = new CancellationTokenSource()) - { - ValueTask vt = p.StandardOutput.ReadAsync(new char[1].AsMemory(), cts.Token); - await Task.Delay(1); - Assert.False(vt.IsCompleted); - cts.Cancel(); - await Assert.ThrowsAnyAsync(async () => await vt); - } - - using (var cts = new CancellationTokenSource()) - { - ValueTask vt = p.StandardError.ReadAsync(new char[1].AsMemory(), cts.Token); - await Task.Delay(1); - Assert.False(vt.IsCompleted); - cts.Cancel(); - await Assert.ThrowsAnyAsync(async () => await vt); - } - } - finally - { - p.Kill(); - p.Dispose(); - } - } - [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] public void TestSyncStreams() { diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs index 9fbc3c4c9a28b6..e05a9f8f9bb8dd 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs @@ -187,45 +187,36 @@ private unsafe Socket(SafeSocketHandle handle, bool loadPropertiesFromHandle) { try { - // Local and remote end points may be different sizes for protocols like Unix Domain Sockets. + // Local and Remote EP may be different sizes for something like UDS. bufferLength = buffer.Length; - switch (SocketPal.GetPeerName(handle, buffer, ref bufferLength)) + if (SocketPal.GetPeerName(handle, buffer, ref bufferLength) != SocketError.Success) { - case SocketError.Success: - switch (_addressFamily) - { - case AddressFamily.InterNetwork: - _remoteEndPoint = new IPEndPoint( - new IPAddress((long)SocketAddressPal.GetIPv4Address(buffer.Slice(0, bufferLength)) & 0x0FFFFFFFF), - SocketAddressPal.GetPort(buffer)); - break; - - case AddressFamily.InterNetworkV6: - Span address = stackalloc byte[IPAddressParserStatics.IPv6AddressBytes]; - SocketAddressPal.GetIPv6Address(buffer.Slice(0, bufferLength), address, out uint scope); - _remoteEndPoint = new IPEndPoint( - new IPAddress(address, scope), - SocketAddressPal.GetPort(buffer)); - break; - - case AddressFamily.Unix: - socketAddress = new Internals.SocketAddress(_addressFamily, buffer.Slice(0, bufferLength)); - _remoteEndPoint = new UnixDomainSocketEndPoint(IPEndPointExtensions.GetNetSocketAddress(socketAddress)); - break; - } + return; + } - _isConnected = true; + switch (_addressFamily) + { + case AddressFamily.InterNetwork: + _remoteEndPoint = new IPEndPoint( + new IPAddress((long)SocketAddressPal.GetIPv4Address(buffer.Slice(0, bufferLength)) & 0x0FFFFFFFF), + SocketAddressPal.GetPort(buffer)); + break; + + case AddressFamily.InterNetworkV6: + Span address = stackalloc byte[IPAddressParserStatics.IPv6AddressBytes]; + SocketAddressPal.GetIPv6Address(buffer.Slice(0, bufferLength), address, out uint scope); + _remoteEndPoint = new IPEndPoint( + new IPAddress(address, scope), + SocketAddressPal.GetPort(buffer)); break; - case SocketError.InvalidArgument: - // On some OSes (e.g. macOS), EINVAL means the socket has been shut down. - // This can happen if, for example, socketpair was used and the parent - // process closed its copy of the child's socket. Since we don't know - // whether we're actually connected or not, err on the side of saying - // we're connected. - _isConnected = true; + case AddressFamily.Unix: + socketAddress = new Internals.SocketAddress(_addressFamily, buffer.Slice(0, bufferLength)); + _remoteEndPoint = new UnixDomainSocketEndPoint(IPEndPointExtensions.GetNetSocketAddress(socketAddress)); break; } + + _isConnected = true; } catch { } }