From 6798ffd87dda83809b8f016f8d2387018cd1dc60 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Fri, 29 Jan 2021 14:51:03 +0000 Subject: [PATCH 1/2] Revert "Use socketpair to implement Process.Start redirection" (#47461) * Revert #34861 * reinstate removed test * Update src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs Co-authored-by: Stephen Toub Co-authored-by: Stephen Toub --- .../Native/Unix/System.Native/pal_process.c | 50 +++++-------------- .../src/System.Diagnostics.Process.csproj | 1 - .../src/System/Diagnostics/Process.Unix.cs | 28 +++-------- .../tests/ProcessStreamReadTests.cs | 3 +- 4 files changed, 21 insertions(+), 61 deletions(-) diff --git a/src/libraries/Native/Unix/System.Native/pal_process.c b/src/libraries/Native/Unix/System.Native/pal_process.c index 44cb59d4c1a2a..285a94916c4c2 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 947d0f2a1bb0c..05f3fba55529b 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 5096a2adea86c..0cbcefdb4c919 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,31 +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 socketHandle = new SafeSocketHandle((IntPtr)fd, ownsHandle: true); - var socket = new Socket(socketHandle); - - if (!socket.Connected) - { - // WSL1 workaround -- due to issues with sockets syscalls - // socket pairs fd's are erroneously inferred as not connected. - // Fall back to using FileStream instead. - - GC.SuppressFinalize(socket); - GC.SuppressFinalize(socketHandle); - - return new FileStream( - new SafeFileHandle((IntPtr)fd, ownsHandle: true), - access, StreamBufferSize, isAsync: false); - } - - 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 1e906e4a803c7..577a9f1a06137 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs @@ -344,7 +344,8 @@ async private Task WaitPipeSignal(PipeStream pipe, int millisecond) } } - [PlatformSpecific(~TestPlatforms.Windows)] // currently on Windows these operations async-over-sync on Windows + [ActiveIssue("https://github.com/dotnet/runtime/issues/44329")] + [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() { From 31a10e211078e80dd93bd494049b7d026281bb53 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Mon, 1 Feb 2021 15:41:27 +0000 Subject: [PATCH 2/2] Add test validating against regression in #46469 (#47643) * Add test validating against regression in #46469 * fix test bug --- .../tests/ProcessTests.Unix.cs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index 9097dd7672a53..8b5d7b1d28558 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -4,6 +4,7 @@ using System.Collections; using System.Collections.Generic; using System.ComponentModel; +using System.Globalization; using System.IO; using System.Linq; using System.Reflection; @@ -810,6 +811,24 @@ public void TestProcessRecycledPid() Assert.True(foundRecycled); } + [PlatformSpecific(TestPlatforms.AnyUnix)] + [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] + [InlineData("/dev/stdin", O_RDONLY)] + [InlineData("/dev/stdout", O_WRONLY)] + [InlineData("/dev/stderr", O_WRONLY)] + public void ChildProcessRedirectedIO_FilePathOpenShouldSucceed(string filename, int flags) + { + var options = new RemoteInvokeOptions { StartInfo = new ProcessStartInfo { RedirectStandardOutput = true, RedirectStandardInput = true, RedirectStandardError = true }}; + using (RemoteInvokeHandle handle = RemoteExecutor.Invoke(ExecuteChildProcess, filename, flags.ToString(CultureInfo.InvariantCulture), options)) + { } + + static void ExecuteChildProcess(string filename, string flags) + { + int result = open(filename, int.Parse(flags, CultureInfo.InvariantCulture)); + Assert.True(result >= 0, $"failed to open file with {result} and errno {Marshal.GetLastWin32Error()}."); + } + } + [ConditionalTheory(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] [InlineData(true)] [InlineData(false)] @@ -940,6 +959,12 @@ private static unsafe HashSet GetGroups() [DllImport("libc", SetLastError = true)] private static extern int kill(int pid, int sig); + [DllImport("libc", SetLastError = true)] + private static extern int open(string pathname, int flags); + + private const int O_RDONLY = 0; + private const int O_WRONLY = 1; + private static readonly string[] s_allowedProgramsToRun = new string[] { "xdg-open", "gnome-open", "kfmclient" }; private string WriteScriptFile(string directory, string name, int returnValue)