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

Correct horizontal coordinates in viewport overflow test #8456

Merged
1 commit merged into from
Dec 3, 2020

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Dec 2, 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

@zadjii-msft
Copy link
Member

@msftbot make sure @miniksa signs off on this

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 2, 2020
@ghost
Copy link

ghost commented Dec 2, 2020

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@ghost ghost added 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 Severity-Crash Crashes are real bad news. labels Dec 3, 2020
@ghost ghost merged commit 2a2f6b3 into microsoft:main Dec 3, 2020
@j4james j4james deleted the fix-oob-exceptions-2 branch December 5, 2020 00:23
DHowett pushed a commit that referenced this pull request 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 pull request 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. AutoMerge Marked for automatic merge by the bot when requirements are met 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More conhost exceptions via SetConsoleScreenBufferSize
3 participants