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.

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

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

Open
jkotas opened this issue Mar 10, 2018 · 4 comments
Open

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

jkotas opened this issue Mar 10, 2018 · 4 comments
Labels
area-System.Console bug help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@jkotas
Copy link
Member

jkotas commented Mar 10, 2018

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
Copy link
Member Author

jkotas commented Mar 10, 2018

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
Copy link
Member

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
Copy link

TheLastRar commented Aug 25, 2018

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.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@carlossanlop carlossanlop removed the untriaged New issue has not been triaged by the area owner label Apr 1, 2020
@adamsitnik
Copy link
Member

The Windows implementation is not closing the handle:

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);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Console bug help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

7 participants