Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "Use socketpair to implement Process.Start redirection" (#47461) #47644

Merged
merged 2 commits into from
Feb 10, 2021
Merged

Revert "Use socketpair to implement Process.Start redirection" (#47461) #47644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6798ffd
Revert "Use socketpair to implement Process.Start redirection" (#47461)
eiriktsarpalis Jan 29, 2021
31a10e2
Add test validating against regression in #46469 (#47643)
eiriktsarpalis Feb 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 13 additions & 37 deletions src/libraries/Native/Unix/System.Native/pal_process.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@
#if HAVE_CRT_EXTERNS_H
#include <crt_externs.h>
#endif
#if HAVE_PIPE2
#include <fcntl.h>
#include <sys/socket.h>
#endif
#include <pthread.h>

#if HAVE_SCHED_SETAFFINITY || HAVE_SCHED_GETAFFINITY
Expand All @@ -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

Expand Down Expand Up @@ -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[],
Expand All @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.Diagnostics.Process/src/System.Diagnostics.Process.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,6 @@
<Reference Include="System.IO.FileSystem.DriveInfo" />
<Reference Include="System.Linq" />
<Reference Include="System.Memory" />
<Reference Include="System.Net.Sockets" />
<Reference Include="System.Runtime" />
<Reference Include="System.Runtime.Extensions" />
<Reference Include="System.Runtime.InteropServices" />
Expand Down
28 changes: 6 additions & 22 deletions src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -754,31 +753,16 @@ internal static TimeSpan TicksToTimeSpan(double ticks)
return TimeSpan.FromSeconds(ticks / (double)ticksPerSecond);
}

/// <summary>Opens a stream around the specified socket file descriptor and with the specified access.</summary>
/// <param name="fd">The socket file descriptor.</param>
/// <summary>Opens a stream around the specified file descriptor and with the specified access.</summary>
/// <param name="fd">The file descriptor.</param>
/// <param name="access">The access mode.</param>
/// <returns>The opened stream.</returns>
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);
}

/// <summary>Parses a command-line argument string into a list of arguments.</summary>
Expand Down
3 changes: 2 additions & 1 deletion src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,8 @@ async private Task<bool> 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()
{
Expand Down
25 changes: 25 additions & 0 deletions src/libraries/System.Diagnostics.Process/tests/ProcessTests.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)]
Expand Down Expand Up @@ -940,6 +959,12 @@ private static unsafe HashSet<uint> 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)
Expand Down