Fix out-of-bounds exceptions in Set...{Buffer,Screen}Size #8309
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
Area-Server
miniksa
approved these changes
Nov 17, 2020
There was a problem hiding this comment.
Yep, that's what I was expecting. I briefly thought about "deduplicate those fixes into a helper" but that would be ugly and confusing and meh.
Good catch on the inclusive/exclusive thing. It looks like it was just someone (probably me) getting confused on that point again.
DHowett
changed the title
This was referenced Dec 1, 2020
ghost
pushed a commit
that referenced
this pull request
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
DHowett
pushed a commit
that referenced
this pull request
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)
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)
|
🎉 Handy links: |
ghost
mentioned this pull request
|
🎉 Handy links: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
References
Technically this is a fix for issue #256, although that has been closed
as "needs-repro".
PR Checklist
Detailed Description of the Pull Request / Additional comments
The main fix was to add checks in the
SetConsoleScreenBufferSizeImpland
SetConsoleScreenBufferInfoExImplmethods, to make sure theviewport 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::SetViewportwas also a source of viewportoverflow 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
IsCursorDoubleWidthmethod was incorrectly marked asnoexcept, which was preventing its exceptions from being caught.Ideally it shouldn't be throwing exceptions at all any more, but I've
removed the
noexceptspecifier, 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