Skip to content

Handle exception if ConsoleHost tries to set cursor out of bounds because screen buffer changed#15380

Merged
anmenaga merged 4 commits into
PowerShell:masterfrom
SteveL-MSFT:cursor-y-bounds
May 14, 2021
Merged

Handle exception if ConsoleHost tries to set cursor out of bounds because screen buffer changed#15380
anmenaga merged 4 commits into
PowerShell:masterfrom
SteveL-MSFT:cursor-y-bounds

Conversation

@SteveL-MSFT
Copy link
Copy Markdown
Member

@SteveL-MSFT SteveL-MSFT commented May 11, 2021

PR Summary

There's a race condition between the call to set the console cursor position and the screen buffer size has changed that puts the coordinates outside of the screen buffer area and causes pwsh to crash with an unhandled exception. Also, it seems that the console host is calling Win32 APIs instead of the .NET APIs and I don't see any specific reason to do that other than consistency with other native APIs that need to be used.

Fix is to switch to using Console.SetCursorPosition and remove all use of the Win32 API. Catch the ArgumentOutOfRangeException and ignore it. There is really no way to avoid the race condition here as any validation of the screen buffer size can be wrong by the time we set the cursor position. The user may have resized the window or when tabs show up on Windows Terminal, the screen buffer size automatically shrinks so that the window size doesn't change.

PR Context

Fix #15254

PR Checklist

@ghost ghost assigned anmenaga May 11, 2021
Comment thread src/System.Management.Automation/utils/PInvokeDllNames.cs
@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented May 13, 2021

Iwonder that tests fail.
.Net GetBufferInfo() implementation https://github.com/dotnet/runtime/blob/1b09a384f29eafd98ec6bcb2d9e6fc820c9db801/src/libraries/System.Console/src/System/ConsolePal.Windows.cs#L1066-L1077
PowerShell implementation looks the same"

private static
ConsoleHandle
GetBufferInfo(out ConsoleControl.CONSOLE_SCREEN_BUFFER_INFO bufferInfo)
{
ConsoleHandle result = ConsoleControl.GetActiveScreenBufferHandle();
bufferInfo = ConsoleControl.GetConsoleScreenBufferInfo(result);
return result;

GitHub
.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps. - dotnet/runtime

@SteveL-MSFT
Copy link
Copy Markdown
Member Author

@iSazonov it appears that the difference in the .NET code and the previous PS code is that PS is only using the stdout handle while .NET is checking stdin, stdout, and stderr and if any of them fail, an exception is thrown. In this case, I suspect stdin is not a valid handle on CI.

Given that the .NET code is working differently here, I think we should revert the GetCursorPosition code back.

Copy link
Copy Markdown

@anmenaga anmenaga left a comment

Choose a reason for hiding this comment

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

LGTM

@anmenaga anmenaga added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label May 14, 2021
@anmenaga anmenaga merged commit 38d582f into PowerShell:master May 14, 2021
@SteveL-MSFT SteveL-MSFT deleted the cursor-y-bounds branch May 14, 2021 19:20
@ghost
Copy link
Copy Markdown

ghost commented May 27, 2021

🎉v7.2.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PowerShell process occasionally terminates due to Win32 internal error at SetConsoleCursorPosition

3 participants