From b4c8f5b918a6eb68bf99cb60871b3aa09e4b3dba Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Tue, 26 Jan 2021 14:38:49 +0000 Subject: [PATCH 1/3] Revert #34861 --- .../Native/Unix/System.Native/pal_process.c | 50 +++++-------------- .../src/System.Diagnostics.Process.csproj | 1 - .../src/System/Diagnostics/Process.Unix.cs | 28 +++-------- .../tests/ProcessStreamReadTests.cs | 36 ------------- 4 files changed, 19 insertions(+), 96 deletions(-) diff --git a/src/libraries/Native/Unix/System.Native/pal_process.c b/src/libraries/Native/Unix/System.Native/pal_process.c index 036aac97cfd33d..5e97d958f74d17 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 @@ -55,7 +56,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 @@ -190,31 +191,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[], @@ -233,7 +209,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; @@ -289,11 +265,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. @@ -305,9 +281,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; @@ -458,7 +434,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 28e419f230f0e4..8896c6b0bfd16c 100644 --- a/src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj +++ b/src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj @@ -342,7 +342,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 587d4bcc3532af..fb52afebdf2a20 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; @@ -752,31 +751,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 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() { From e43ad7be7bef062fa1bd3623eddc6563eb3e5418 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Tue, 26 Jan 2021 23:46:45 +0000 Subject: [PATCH 2/3] reinstate removed test --- .../tests/ProcessStreamReadTests.cs | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs index c30211baa0f86b..f3810837546c7d 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs @@ -344,6 +344,43 @@ async private Task WaitPipeSignal(PipeStream pipe, int millisecond) } } + [ActiveIssue("https://github.com/dotnet/runtime/issues/46489")] + [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() { From fe71f18d0a9976d840ae7c978a271e4fd222a4b3 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Fri, 29 Jan 2021 14:50:11 +0000 Subject: [PATCH 3/3] Update src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs Co-authored-by: Stephen Toub --- .../System.Diagnostics.Process/tests/ProcessStreamReadTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs b/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs index f3810837546c7d..577a9f1a061373 100644 --- a/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs +++ b/src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs @@ -344,7 +344,7 @@ async private Task WaitPipeSignal(PipeStream pipe, int millisecond) } } - [ActiveIssue("https://github.com/dotnet/runtime/issues/46489")] + [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()