Don't dup stdin & stderr as this can break some std io redicection code

[jkotas]

From @TheLastRar on March 10, 2018 13:8

Consider a program that does the follow;

Create pipes and dup2 the write fd onto stdin, stderr,
Have a thread continually read the pipes until EOF is reached (at which point the thread will terminate).
Use coreclr.
...
Restore the original stdin, stderr,
Close the write fd of pipes,
Wait for the pipe reader thread to terminate.

As coreclr dups the fd for stdin & stderr, there is still a write fd for the pipe, meaning the read thread will never receive an EOF and the program to hang at the last step.

Copied from original issue: dotnet/coreclr#16880

[jkotas]

https://github.com/dotnet/corefx/blob/master/src/System.Console/src/System/ConsolePal.Unix.cs#L25

Why do we have to dup the stdin/out/err ? Comment would be nice.

[stephentoub]

That dup comes from this commit from several years ago:
dotnet/corefx@6525d2f

The commit includes the comment:
"Rather than opening e.g. "/dev/stdin", which effectively ends up dup'ing the corresponding file descriptor, we just dup the corresponding file descriptor directly, e.g. dup(STDIN_FILENO)."

I'd need to look in a bit more depth, but presumably the issue is we don't want the streams returned by Console.OpenStandardInput/Output/etc. to close these fds. There are other ways to avoid that, though (e.g. telling the SafeHandle it doesn't own the fd); don't know for sure that's the issue, nor whether such a change would break other things.

[TheLastRar]

As a quick test, I've changed the code here https://github.com/dotnet/corefx/blob/master/src/System.Console/src/System/ConsolePal.Unix.cs#L25 to not dup the handle (for out and err), but I'm still seeing duplicated stdin, stderr handles.

There must be another location where the handles are being duped.

[adamsitnik]

The Windows implementation is not closing the handle:

runtime/src/libraries/System.Console/src/System/ConsolePal.Windows.cs

Lines 1110 to 1119 in 4ae0dea

	protected override void Dispose(bool disposing)
	{
	// We're probably better off not closing the OS handle here. First,
	// we allow a program to get multiple instances of ConsoleStreams
	// around the same OS handle, so closing one handle would invalidate
	// them all. Additionally, we want a second AppDomain to be able to
	// write to stdout if a second AppDomain quits.
	_handle = IntPtr.Zero;
	base.Dispose(disposing);
	}

[adamsitnik]

After thinking more about it and reading the dup man page I am pretty sure that dup was used to ensure that given file descriptor is valid (it fails with EBADF when it's not).

I am going to remove it and use fcntl instead. So for processes with invalid STD IN/OUT/ERR we are going to keep throwing the error, but we are not going to create duplicates of the handles, just wrap them in SFH that does not own them.