Skip to content

Commit

Permalink
Revert "Use socketpair to implement Process.Start redirection" (dotne…
Browse files Browse the repository at this point in the history
…t#47461)

* Revert dotnet#34861

* reinstate removed test

* Update src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs

Co-authored-by: Stephen Toub <stoub@microsoft.com>

Co-authored-by: Stephen Toub <stoub@microsoft.com>
  • Loading branch information
eiriktsarpalis and stephentoub committed Jan 29, 2021
1 parent 0e9bab8 commit 6798ffd
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 61 deletions.
50 changes: 13 additions & 37 deletions src/libraries/Native/Unix/System.Native/pal_process.c
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
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
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
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

0 comments on commit 6798ffd

Please sign in to comment.