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

More conhost exceptions via SetConsoleScreenBufferSize #8453

Closed
j4james opened this issue Dec 1, 2020 · 1 comment · Fixed by #8456
Closed

More conhost exceptions via SetConsoleScreenBufferSize #8453

j4james opened this issue Dec 1, 2020 · 1 comment · Fixed by #8456
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Milestone

Comments

@j4james
Copy link
Collaborator

j4james commented Dec 1, 2020

Environment

Windows build number: Version 10.0.18363.1198
Windows Terminal version (if applicable): Commit 6213172

Steps to reproduce

  1. Start a conhost cmd shell.
  2. Open the properties and make sure Wrap text output on resize is unchecked.
  3. Compile and run the program below.
#include <windows.h>
int main()
{
	HANDLE h = GetStdHandle(STD_OUTPUT_HANDLE);
	COORD coord;
	coord.X = 160;
	coord.Y = 100;
	SetConsoleScreenBufferSize(h, coord);
	SMALL_RECT r = { 80, 75, 159, 99 };
	SetConsoleWindowInfo(h, TRUE, &r);
	SetConsoleCursorPosition(h, { 159, 99 });
	coord.X = 80;
	SetConsoleScreenBufferSize(h, coord);
	return 0;
}

Expected behavior

The conhost instance shouldn't crash and there shouldn't be any exceptions logged in the debug output.

Actual behavior

On my current version of Windows the conhost instance crashes. On the latest version of OpenConsole the exception is caught, so it at least doesn't crash, but you can see the error being logged in the debug output.

This is essentially the same bug as #1976, only the overflow is horizontal rather than vertical. It should have been fixed by PR #8309, but I managed to screw that up. I mistakenly thought the Viewport::EndExclusive method returned the bottom right extent of the viewport, but it's actually returning { Left(), BottomExclusive() }, so the overflow calculation doesn't detect a horizontal overflow. See here:

// Make sure the viewport doesn't now overflow the buffer dimensions.
auto overflow = screenInfo.GetViewport().EndExclusive() - screenInfo.GetBufferSize().Dimensions();

and here:

COORD Viewport::EndExclusive() const noexcept
{
return { Left(), BottomExclusive() };
}

The fix is easy enough. I'd probably just add something like an ExclusiveExtent method to the Viewport class that returns the range I was actually excepting to get from Viewport::EndExclusive. Does that seem reasonable?

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Dec 1, 2020
@ghost ghost added the In-PR This issue has a related PR label Dec 2, 2020
@zadjii-msft
Copy link
Member

Yea that seems sensible to me. Thanks for investigating and putting the fix together!

@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Severity-Crash Crashes are real bad news. labels Dec 2, 2020
@zadjii-msft zadjii-msft added this to the Windows vNext milestone Dec 2, 2020
@DHowett DHowett added Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Dec 3, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Dec 3, 2020
@ghost ghost closed this as completed in #8456 Dec 3, 2020
@ghost ghost removed the In-PR This issue has a related PR label Dec 3, 2020
ghost pushed a commit that referenced this issue Dec 3, 2020
When resizing the buffer in the `SetConsoleScreenBufferSize` and
`SetConsoleScreenBufferInfoEx` APIs, we have tests in place to make sure
that the resize doesn't result in the viewport extending past the bottom
or right of the buffer (since that can eventually result in exceptions
being thrown). Unfortunately these tests were using the wrong X
coordinate, so they failed to detect an overflow along the horizontal
axis. This PR corrects that mistake.

PR #8309 was where the overflow detection was initially added.

The original code was using the `Viewport::EndExclusive` method to
determine the extent of the viewport, mistakenly thinking that it
returned the bottom right coordinates (it actually returns the left
coordinate). So I've now added a `BottomRightExclusive` method to the
`Viewport` class, that actually does return the coordinates we need, and
have updated the overflow tests to use that method instead.

## Validation Steps Performed
I've manually confirmed that the test case is issue #8453 is no longer
throwing an exception. 

Closes #8453
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Dec 3, 2020
DHowett pushed a commit that referenced this issue Jan 25, 2021
When resizing the buffer in the `SetConsoleScreenBufferSize` and
`SetConsoleScreenBufferInfoEx` APIs, we have tests in place to make sure
that the resize doesn't result in the viewport extending past the bottom
or right of the buffer (since that can eventually result in exceptions
being thrown). Unfortunately these tests were using the wrong X
coordinate, so they failed to detect an overflow along the horizontal
axis. This PR corrects that mistake.

PR #8309 was where the overflow detection was initially added.

The original code was using the `Viewport::EndExclusive` method to
determine the extent of the viewport, mistakenly thinking that it
returned the bottom right coordinates (it actually returns the left
coordinate). So I've now added a `BottomRightExclusive` method to the
`Viewport` class, that actually does return the coordinates we need, and
have updated the overflow tests to use that method instead.

## Validation Steps Performed
I've manually confirmed that the test case is issue #8453 is no longer
throwing an exception.

Closes #8453

(cherry picked from commit 2a2f6b3)
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conhost For issues in the Console codebase Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants