Skip to content

Revert "Use socketpair to implement Process.Start redirection (#34861)" #40832

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

Closed
eiriktsarpalis wants to merge 1 commit into from
Closed

Revert "Use socketpair to implement Process.Start redirection (#34861)" #40832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 6 additions & 6 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,15 +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 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);
}

/// <summary>Parses a command-line argument string into a list of arguments.</summary>
Expand Down
36 changes: 0 additions & 36 deletions 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,42 +344,6 @@ async private Task<bool> 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<int> vt = p.StandardOutput.ReadAsync(new char[1].AsMemory(), cts.Token);
await Task.Delay(1);
Assert.False(vt.IsCompleted);
cts.Cancel();
await Assert.ThrowsAnyAsync<OperationCanceledException>(async () => await vt);
}

using (var cts = new CancellationTokenSource())
{
ValueTask<int> vt = p.StandardError.ReadAsync(new char[1].AsMemory(), cts.Token);
await Task.Delay(1);
Assert.False(vt.IsCompleted);
cts.Cancel();
await Assert.ThrowsAnyAsync<OperationCanceledException>(async () => await vt);
}
}
finally
{
p.Kill();
p.Dispose();
}
}

[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
public void TestSyncStreams()
{
Expand Down
55 changes: 23 additions & 32 deletions src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<byte> 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<byte> 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 { }
}
Expand Down