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

Console.Unix: for echoing, write back to the terminal instead of writing to Console.Out. #94414

Merged
merged 11 commits into from
Nov 16, 2023

Conversation

tmds
Copy link
Member

@tmds tmds commented Nov 6, 2023

On Unix .NET implements its own echo to make Console behave similar to Windows.

The echo implementation was writing to standard output, which has the unintended effect of writing the echo characters into a redirected standard output.

This changes to write the echo to the stdin terminal where they were read from.

Fixes #22314.

@adamsitnik @stephentoub ptal.

…ing to Console.Out.

On Unix .NET implements its own echo to make Console behave similar to Windows.

The echo implementation was writing to standard output, which has the unintended effect
of writing the echo characters into a redirected standard output.

This changes to write the echo to the stdin terminal where they were read from.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 6, 2023
@ghost
Copy link

ghost commented Nov 6, 2023

Tagging subscribers to this area: @dotnet/area-system-console
See info in area-owners.md if you want to be subscribed.

Issue Details

On Unix .NET implements its own echo to make Console behave similar to Windows.

The echo implementation was writing to standard output, which has the unintended effect of writing the echo characters into a redirected standard output.

This changes to write the echo to the stdin terminal where they were read from.

Fixes #22314.

@adamsitnik @stephentoub ptal.

Author: tmds
Assignees: -
Labels:

area-System.Console

Milestone: -

@tmds
Copy link
Member Author

tmds commented Nov 6, 2023

I need to make some additional changes so the characters that are written back for echo use the proper encoding.

@tmds
Copy link
Member Author

tmds commented Nov 7, 2023

I need to make some additional changes so the characters that are written back for echo use the proper encoding.

This is done. The PR is up for review.

@adamsitnik adamsitnik self-assigned this Nov 8, 2023
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is great! It's relatively simple for the bug that has been hunting us for years! 👍

I added some comments for improving perf. It would be great to address them before merging.

Thank you @tmds !

SetCursorPosition(Interop.Sys.FileDescriptors.STDOUT_FILENO, left, top);
}

internal static void SetCursorPosition(SafeFileHandle terminalHandle, int left, int top)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested all scenarios that include calling SetCursorPosition?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran the manual tests and I saw no regression.

I will try some additional scenarios this week.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the PR so it prefers using stdout as a terminal handle to remain closer to the current behavior.
I also made the cursor/window size getting to work against the stdin handle.

@adamsitnik adamsitnik added this to the 9.0.0 milestone Nov 14, 2023
{
return;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamsitnik does this look good to you? I wrote it this way to have the bytes Span is in the same scope as the WriteToTerminal call.

@tmds
Copy link
Member Author

tmds commented Nov 15, 2023

I want to highlight a specific special case which will now throw. When standard output is redirected, and standard input is a read-only terminal handle, ReadLine will now throw.

Unhandled exception. System.UnauthorizedAccessException: Access to the path is denied.
 ---> System.IO.IOException: Bad file descriptor
   --- End of inner exception stack trace ---
   at System.ConsolePal.Write(SafeFileHandle fd, ReadOnlySpan`1 buffer, Boolean mayChangeCursorPosition) in /home/tmds/repos/runtime/src/libraries/System.Console/src/System/ConsolePal.Unix.cs:line 1006
   at System.ConsolePal.WriteToTerminal(ReadOnlySpan`1 buffer, Boolean mayChangeCursorPosition) in /home/tmds/repos/runtime/src/libraries/System.Console/src/System/ConsolePal.Unix.cs:line 957
   at System.IO.StdInReader.EchoToTerminal(Char c) in /home/tmds/repos/runtime/src/libraries/System.Console/src/System/IO/StdInReader.cs:line 395
   at System.IO.StdInReader.ReadLineCore(Boolean consumeKeys) in /home/tmds/repos/runtime/src/libraries/System.Console/src/System/IO/StdInReader.cs:line 181
   at System.IO.StdInReader.ReadLine() in /home/tmds/repos/runtime/src/libraries/System.Console/src/System/IO/StdInReader.cs:line 95
   at System.IO.SyncTextReader.ReadLine() in /home/tmds/repos/runtime/src/libraries/System.Console/src/System/IO/SyncTextReader.cs:line 77
   at System.Console.ReadLine() in /home/tmds/repos/runtime/src/libraries/System.Console/src/System/Console.cs:line 752
   at Program.<Main>$(String[] args) in /tmp/console/Program.cs:line 7

This seems appropriate as we don't have a way to echo the characters to.
Previously this didn't throw as the characters got echoed to the standard output.
If desired, we could make this work by no longer throwing in this specific case which would result in the user not seeing an echo.

public static Stream OpenStandardInput()
{
return new UnixConsoleStream(Interop.CheckIo(Interop.Sys.FileDescriptors.STDIN_FILENO), FileAccess.Read,
return new UnixConsoleStream(Interop.Sys.FileDescriptors.STDIN_FILENO, FileAccess.Read,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the Interop.CheckIo is about, so I removed it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familiar with this particular overload, let me share it here think loud about that:

/// <summary>
/// Validates the result of system call that returns greater than or equal to 0 on success
/// and less than 0 on failure, with errno set to the error code.
/// If the system call failed for any reason, an exception is thrown. Otherwise, the system call succeeded.
/// </summary>
/// <returns>
/// On success, returns the valid SafeFileHandle that was validated.
/// </returns>
internal static TSafeHandle CheckIo<TSafeHandle>(TSafeHandle handle, string? path = null, bool isDirError = false)
where TSafeHandle : SafeHandle
{
if (handle.IsInvalid)
{
Exception e = Interop.GetExceptionForIoErrno(Sys.GetLastErrorInfo(), path, isDirError);
handle.Dispose();
throw e;
}
return handle;
}

  1. The xml comment does not describe what it does.
  2. If provided handle is invalid, then it gets last error for the current thread and throws an exception.

I don't think that we need such check here, as we have not performed any IO yet (the handle is created by providing a constant number: 0, 1 or 2, it's not a result of a sys-call), so there is no last error that makes sense. It can become invalid once we try to use it in a sys-call, but other layers are ready for that.

So I am fine with removing these checks (and the method itself if it's not used elsewhere) 👍

@@ -254,17 +250,21 @@ private static unsafe int Read(SafeFileHandle fd, Span<byte> buffer)
}
}

internal static unsafe void WriteFromConsoleStream(SafeFileHandle fd, ReadOnlySpan<byte> buffer)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These other changes sync with the .Unix file.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you very much @tmds!

public static Stream OpenStandardInput()
{
return new UnixConsoleStream(Interop.CheckIo(Interop.Sys.FileDescriptors.STDIN_FILENO), FileAccess.Read,
return new UnixConsoleStream(Interop.Sys.FileDescriptors.STDIN_FILENO, FileAccess.Read,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familiar with this particular overload, let me share it here think loud about that:

/// <summary>
/// Validates the result of system call that returns greater than or equal to 0 on success
/// and less than 0 on failure, with errno set to the error code.
/// If the system call failed for any reason, an exception is thrown. Otherwise, the system call succeeded.
/// </summary>
/// <returns>
/// On success, returns the valid SafeFileHandle that was validated.
/// </returns>
internal static TSafeHandle CheckIo<TSafeHandle>(TSafeHandle handle, string? path = null, bool isDirError = false)
where TSafeHandle : SafeHandle
{
if (handle.IsInvalid)
{
Exception e = Interop.GetExceptionForIoErrno(Sys.GetLastErrorInfo(), path, isDirError);
handle.Dispose();
throw e;
}
return handle;
}

  1. The xml comment does not describe what it does.
  2. If provided handle is invalid, then it gets last error for the current thread and throws an exception.

I don't think that we need such check here, as we have not performed any IO yet (the handle is created by providing a constant number: 0, 1 or 2, it's not a result of a sys-call), so there is no last error that makes sense. It can become invalid once we try to use it in a sys-call, but other layers are ready for that.

So I am fine with removing these checks (and the method itself if it's not used elsewhere) 👍

@adamsitnik adamsitnik added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 16, 2023
@adamsitnik
Copy link
Member

I want to highlight a specific special case which will now throw.

I am OK with that. I've applied needs-breaking-change-doc-created to ensure that we document that.

@adamsitnik adamsitnik merged commit ec0251b into dotnet:main Nov 16, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 17, 2023
@ericstj
Copy link
Member

ericstj commented Nov 26, 2024

@adamsitnik did you create a breaking change doc for this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Console community-contribution Indicates that the PR has been added by a community member needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

console redirection is broken
5 participants