-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Fix overflow in Viewport::FromDimensions #12669
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Man this Viewport
class needs to go, doesn't it?
This and |
@@ -677,8 +677,7 @@ void Renderer::_PaintBufferOutput(_In_ IRenderEngine* const pEngine) | |||
|
|||
for (const auto& dirtyRect : dirtyAreas) | |||
{ | |||
// Shortcut: don't bother redrawing if the width is 0. | |||
if (dirtyRect.left == dirtyRect.right) | |||
if (!dirtyRect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW This is a bug. Some engines return dirty rects where top == bottom
, and I don't think any engine returns left == right
actually. This causes an invalid Viewport
to be created a few lines below. In either case though, this now checks both dimensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wuh oh. good catch then.
@msftbot merge this in 1 minute |
Hello @DHowett! 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:
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". |
This removes one source of potential integer overflows from the Viewport class. Other parts were left untouched, as this entire class of overflow issues gets fixed all at once, as soon as we replace COORD with til::coord (etc.). * [x] Closes #5271 * [x] I work here * [x] Tests added/passed * Call `ScrollConsoleScreenBufferW` with out of bounds coordinates * Doesn't crash ✅ (cherry picked from commit a4a6dfc)
This removes one source of potential integer overflows from the Viewport class. Other parts were left untouched, as this entire class of overflow issues gets fixed all at once, as soon as we replace COORD with til::coord (etc.). ## PR Checklist * [x] Closes #5271 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Call `ScrollConsoleScreenBufferW` with out of bounds coordinates * Doesn't crash ✅ (cherry picked from commit a4a6dfc) Signed-off-by: Dustin Howett <duhowett@microsoft.com>
This removes one source of potential integer overflows from the Viewport class. Other parts were left untouched, as this entire class of overflow issues gets fixed all at once, as soon as we replace COORD with til::coord (etc.). ## PR Checklist * [x] Closes #5271 * [x] I work here * [x] Tests added/passed ## Validation Steps Performed * Call `ScrollConsoleScreenBufferW` with out of bounds coordinates * Doesn't crash ✅ (cherry picked from commit a4a6dfc)
🎉 Handy links: |
🎉 Handy links: |
This removes one source of potential integer overflows from the Viewport class.
Other parts were left untouched, as this entire class of overflow issues gets
fixed all at once, as soon as we replace COORD with til::coord (etc.).
PR Checklist
Validation Steps Performed
ScrollConsoleScreenBufferW
with out of bounds coordinates