Console.Unix: for echoing, write back to the terminal instead of writing to Console.Out. by tmds · Pull Request #94414 · dotnet/runtime

tmds · 2023-11-06T14:38:29Z

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 · 2023-11-06T14:38:47Z

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 · 2023-11-06T15:58:34Z

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

tmds · 2023-11-07T12:50:22Z

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

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 !

src/libraries/System.Console/src/System/IO/StdInReader.cs

adamsitnik · 2023-11-14T09:46:21Z

src/libraries/System.Console/src/System/ConsolePal.Unix.cs

+            SetCursorPosition(Interop.Sys.FileDescriptors.STDOUT_FILENO, left, top);
+        }
+
+        internal static void SetCursorPosition(SafeFileHandle terminalHandle, int left, int top)


Have you tested all scenarios that include calling SetCursorPosition?

I ran the manual tests and I saw no regression.

I will try some additional scenarios this week.

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.

src/libraries/System.Console/src/System/IO/StdInReader.cs

…or stdin terminal handle.

tmds · 2023-11-15T07:58:27Z

src/libraries/System.Console/src/System/IO/StdInReader.cs

+                {
+                    return;
+                }
+            }


@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 · 2023-11-15T08:04:09Z

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.

tmds · 2023-11-15T09:40:35Z

src/libraries/System.Console/src/System/ConsolePal.Wasi.cs

        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,


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

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

runtime/src/libraries/Common/src/Interop/Unix/Interop.IOErrors.cs

Lines 86 to 105 in 487d7f0

/// <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;

}

The xml comment does not describe what it does.

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) 👍

tmds · 2023-11-15T09:42:00Z

src/libraries/System.Console/src/System/ConsolePal.Wasi.cs

            }
        }

+        internal static unsafe void WriteFromConsoleStream(SafeFileHandle fd, ReadOnlySpan<byte> buffer)


These other changes sync with the .Unix file.

adamsitnik

LGTM, thank you very much @tmds!

adamsitnik · 2023-11-16T15:37:26Z

src/libraries/System.Console/src/System/ConsolePal.Wasi.cs

        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,


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

runtime/src/libraries/Common/src/Interop/Unix/Interop.IOErrors.cs

Lines 86 to 105 in 487d7f0

/// <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;

}

The xml comment does not describe what it does.

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 · 2023-11-16T15:44:26Z

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.

ericstj · 2024-11-26T17:30:27Z

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

ghost added area-System.Console community-contribution Indicates that the PR has been added by a community member labels Nov 6, 2023

tmds mentioned this pull request Nov 6, 2023

console redirection is broken #22314

Closed

This was referenced Nov 6, 2023

Crash in Microsoft.Extensions.Logging.Generators.Roslyn4.0.Tests.WorkItemExecution #90019

Open

MSBuild crashing in the build #92290

Open

Use console encoding for echoing characters.

6a66e2d

Fix compilation issue.

822df76

adamsitnik self-assigned this Nov 8, 2023

StdInReader is used only when stdin is a terminal.

624f51d

adamsitnik reviewed Nov 14, 2023

View reviewed changes

adamsitnik added this to the 9.0.0 milestone Nov 14, 2023

tmds added 2 commits November 15, 2023 08:28

Prefer stdout as terminal handle and enable cursor position caching f…

fc5fa66

…or stdin terminal handle.

PR feedback.

ad727c4

tmds commented Nov 15, 2023

View reviewed changes

tmds added 4 commits November 15, 2023 09:16

Clean up.

271a37f

Fix C errors for unused vars.

71789a7

Fix ConsolePal.Wasi.

dc2adb0

Clean up.

30400c4

tmds commented Nov 15, 2023

View reviewed changes

Don't call TryGetCursorPosition when stdin isn't a terminal.

a76ff28

adamsitnik approved these changes Nov 16, 2023

View reviewed changes

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 merged commit ec0251b into dotnet:main Nov 16, 2023

tmds mentioned this pull request Dec 5, 2023

Console.Unix: fix ANSI colors over redirected standard output. #95654

Merged

github-actions bot locked and limited conversation to collaborators Dec 17, 2023

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

Conversation

tmds commented Nov 6, 2023

Uh oh!

ghost commented Nov 6, 2023

Uh oh!

tmds commented Nov 6, 2023

Uh oh!

tmds commented Nov 7, 2023

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmds commented Nov 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsitnik commented Nov 16, 2023

Uh oh!

ericstj commented Nov 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants