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

v2 conhost crash with SetConsoleScreenBufferInfoEx calls #256

Closed
rprichard opened this issue Sep 17, 2018 · 3 comments
Closed

v2 conhost crash with SetConsoleScreenBufferInfoEx calls #256

rprichard opened this issue Sep 17, 2018 · 3 comments
Assignees
Labels
Needs-Repro We can't figure out how to make this happen. Please help find a simplified repro. Product-Conhost For issues in the Console codebase Work-Item It's being tracked by an actual work item internally. (to be removed soon)

Comments

@rprichard
Copy link

  • Your Windows build number: [Version 10.0.17760.1]

What you're doing and what's happening: (Copy & paste specific commands and their output, or include screen shots)

Using SetConsoleScreenBufferInfoEx to make the console buffer and window smaller, simultaneously, can crash the v2 conhost with a recent Insiders build.

Steps:

  1. Open a cmd/powershell console window.
  2. Open the console properties and uncheck the "Layout > Wrap text output on resize" option.
  3. Run the test program I included.
  4. The console window disappears.

What's wrong / what should be happening instead:

SetConsoleScreenBufferInfoEx shouldn't crash. The text Hello? should appear, and the console should return control to cmd/powershell.

Aside 1: I don't think this affects winpty at all.
Aside 2: SetConsoleScreenBufferInfoEx seems to handle srWindow differently than both SetConsoleWindowInfo and GetConsoleScreenBufferInfoEx. It tends to set the window size to 1 less width/height than the requested size, so a pair of calls to GetConsoleScreenBufferInfoEx and SetConsoleScreenBufferInfoEx shrinks the console window.

Test program

#include <windows.h>
#include <stdio.h>

int main(int argc, char *argv[]) {

    HANDLE conout = GetStdHandle(STD_OUTPUT_HANDLE);
    CONSOLE_SCREEN_BUFFER_INFOEX info = { sizeof(info) };
    GetConsoleScreenBufferInfoEx(conout, &info);

    info.dwSize.X = 120;
    info.dwSize.Y = 5000;
    info.srWindow.Left = info.srWindow.Top = 0;
    info.srWindow.Right = 121;
    info.srWindow.Bottom = 26;
    SetConsoleScreenBufferInfoEx(conout, &info);

    SetConsoleCursorPosition(conout, COORD { 0, 4000 });

    info.dwSize.X = 80;
    info.dwSize.Y = 20;
    info.srWindow.Right = 41;
    info.srWindow.Bottom = 11;
    SetConsoleScreenBufferInfoEx(conout, &info);

    printf("Hello?\n");

    return 0;
}
@zadjii-msft zadjii-msft added Work-Item It's being tracked by an actual work item internally. (to be removed soon) Product-Conhost For issues in the Console codebase labels Sep 17, 2018
@zadjii-msft zadjii-msft added this to the 19H1 milestone Sep 17, 2018
@zadjii-msft
Copy link
Member

Thanks for the bug report, I've filed msft:19013486 to make sure this gets fixed ASAP :)

@miniksa
Copy link
Member

miniksa commented Jan 4, 2019

I'm not able to reproduce this issue anymore. Perhaps we fixed it?

If it's still crashing for you, can you try to find the report ID so I can maybe correlate it?
It should be in the Event Viewer under Windows Logs > Application.

I believe it's Event ID 1000 for Application Error (and should contain conhost.exe as the faulting application path with a report ID GUID that you can give to me).

@miniksa miniksa self-assigned this Jan 18, 2019
@miniksa miniksa added the Needs-Repro We can't figure out how to make this happen. Please help find a simplified repro. label Jan 18, 2019
@miniksa miniksa removed this from the 19H1 milestone Jan 18, 2019
@DHowett-MSFT
Copy link
Contributor

Closing this for now. If you can get the report ID from a recent repro of this crash, we'd love to take a look. Thanks!

DHowett pushed a commit that referenced this issue Nov 17, 2020
This fixes a number of exceptions that can cause conhost to crash when
the buffer is resized in such a way that the viewport or cursor position
end up out of bounds.

Technically this is a fix for issue #256, although that has been closed
as "needs-repro".

The main fix was to add checks in the `SetConsoleScreenBufferSizeImpl`
and `SetConsoleScreenBufferInfoExImpl` methods, to make sure the
viewport doesn't extend past the bottom or right of the buffer after a
resize. If it has overflowed, we move the viewport back up or left until
it's back within the buffer boundaries. We also check if the cursor
position has ended up out of bounds, and if so, clamp it back inside the
buffer.

The `SCREEN_INFORMATION::SetViewport` was also a source of viewport
overflow problems, because it was mistakenly using inclusive coordinates
in its range checks, which resulted in them being off by one. That has
now been corrected to use exclusive coordinates.

Finally, the `IsCursorDoubleWidth` method was incorrectly marked as
`noexcept`, which was preventing its exceptions from being caught.
Ideally it shouldn't be throwing exceptions at all any more, but I've
removed the `noexcept` specifier, so if it does throw an exception,
it'll at least have more chance of recovering without a crash.

## Validation Steps Performed

I put together a few test cases (based on the reports in issues #276 and
#1976) which consistently caused conhost to crash, or to generate an
exception visible in the debug output. With this PR applied, those test
cases are no longer crashing or triggering exceptions.

Closes #1976
DHowett pushed a commit that referenced this issue Jan 25, 2021
This fixes a number of exceptions that can cause conhost to crash when
the buffer is resized in such a way that the viewport or cursor position
end up out of bounds.

Technically this is a fix for issue #256, although that has been closed
as "needs-repro".

The main fix was to add checks in the `SetConsoleScreenBufferSizeImpl`
and `SetConsoleScreenBufferInfoExImpl` methods, to make sure the
viewport doesn't extend past the bottom or right of the buffer after a
resize. If it has overflowed, we move the viewport back up or left until
it's back within the buffer boundaries. We also check if the cursor
position has ended up out of bounds, and if so, clamp it back inside the
buffer.

The `SCREEN_INFORMATION::SetViewport` was also a source of viewport
overflow problems, because it was mistakenly using inclusive coordinates
in its range checks, which resulted in them being off by one. That has
now been corrected to use exclusive coordinates.

Finally, the `IsCursorDoubleWidth` method was incorrectly marked as
`noexcept`, which was preventing its exceptions from being caught.
Ideally it shouldn't be throwing exceptions at all any more, but I've
removed the `noexcept` specifier, so if it does throw an exception,
it'll at least have more chance of recovering without a crash.

## Validation Steps Performed

I put together a few test cases (based on the reports in issues #276 and
#1976) which consistently caused conhost to crash, or to generate an
exception visible in the debug output. With this PR applied, those test
cases are no longer crashing or triggering exceptions.

Closes #1976

(cherry picked from commit 9a07049)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs-Repro We can't figure out how to make this happen. Please help find a simplified repro. Product-Conhost For issues in the Console codebase Work-Item It's being tracked by an actual work item internally. (to be removed soon)
Projects
None yet
Development

No branches or pull requests

4 participants