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.

Sign up for GitHub

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

Jump to bottom

Crash in ScrollConsoleScreenBuffer when destination coordinates overflow #5271

Closed
j4james opened this issue Apr 7, 2020 · 5 comments · Fixed by #12669
Closed

Crash in ScrollConsoleScreenBuffer when destination coordinates overflow #5271

j4james opened this issue Apr 7, 2020 · 5 comments · Fixed by #12669
Assignees
@lhecker
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Impact-Correctness It be wrong. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) 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. zInbox-Bug Ignore me!
Milestone
Terminal v1.14

Comments

@j4james
Copy link
Collaborator

j4james commented Apr 7, 2020

Environment

Windows build number: Version 10.0.18362.657

Steps to reproduce

Compile and run the following C++ code in a conhost cmd shell:

#include <windows.h>

void main() {
    SMALL_RECT rect = {0, 0, 40, 0};
    COORD dest = {32767, 0};
    CHAR_INFO fill = {L'X', 0x07};
    HANDLE output = GetStdHandle(STD_OUTPUT_HANDLE);
    ScrollConsoleScreenBufferW(output, &rect, NULL, dest, &fill);
}

Expected behavior

The first 40 characters of the first line should fill with X's.

Actual behavior

The console crashes.

When the ScrollRegion function is copying the area that is being scrolled, the _CopyRectangle function makes use of the Viewport::WalkInBoundsCircular method, and that fails when the target position goes out of bounds. See here:

terminal/src/types/viewport.cpp

Line 411 in 5de9fa9

FAIL_FAST_IF(!IsInBounds(pos, allowEndExclusive));

@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 Apr 7, 2020
@DHowett-MSFT DHowett-MSFT added Impact-Correctness It be wrong. 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 Apr 7, 2020
@DHowett-MSFT DHowett-MSFT added this to the 21H1 milestone Apr 7, 2020
@DHowett-MSFT DHowett-MSFT added the Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. label Apr 7, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Apr 7, 2020
@j4james
Copy link
Collaborator Author

j4james commented Apr 7, 2020

FYI, I've just tested with the first open source release (commit d4d59fa), and the bug was already present there. However, the legacy console doesn't have this issue.

In some ways this is could be considered a part of issue #4153, but I thought it worth raising separately because of the crash, and the fact that it's not obviously "safe math" code.

@DHowett-MSFT
Copy link
Contributor

Regressed between RS5 (17763) and 19H1 (18362). Thanks!

@DHowett-MSFT DHowett-MSFT added Priority-1 A description (P1) and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 8, 2020
@DHowett-MSFT
Copy link
Contributor

Off triage, P2, /cc @miniksa for a conhost regression from the scrolling change in 19H1

@zadjii-msft zadjii-msft modified the milestones: Windows vNext, 22H1 Jan 4, 2022
@zadjii-msft zadjii-msft modified the milestones: 22H1, Terminal v1.14 Feb 2, 2022
@zadjii-msft zadjii-msft added the zInbox-Bug Ignore me! label Mar 10, 2022
@lhecker lhecker mentioned this issue Mar 11, 2022
Merged
3 tasks
@ghost ghost added the In-PR This issue has a related PR label Mar 11, 2022
@ghost ghost closed this as completed in #12669 Mar 11, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Mar 11, 2022
ghost pushed a commit that referenced this issue Mar 11, 2022
@lhecker
Fix overflow in Viewport::FromDimensions (#12669)
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 ✅
DHowett pushed a commit that referenced this issue Mar 11, 2022
@lhecker @DHowett
Fix overflow in Viewport::FromDimensions (#12669)
fa21425 
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)
DHowett pushed a commit that referenced this issue Mar 11, 2022
@lhecker @DHowett
Fix overflow in Viewport::FromDimensions (#12669)
fda8188 
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 <[email protected]>
DHowett pushed a commit that referenced this issue Mar 24, 2022
@lhecker @DHowett
Fix overflow in Viewport::FromDimensions (#12669)
9c1fe1f 
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)
@ghost
Copy link

ghost commented Mar 25, 2022

🎉This issue was addressed in #12669, which has now been successfully released as Windows Terminal v1.12.1073.:tada:

Handy links:

@ghost
Copy link

ghost commented Mar 25, 2022

🎉This issue was addressed in #12669, which has now been successfully released as Windows Terminal Preview v1.13.1073.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@lhecker lhecker

Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Impact-Correctness It be wrong. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) 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. zInbox-Bug Ignore me!
Projects
None yet
Milestone
Terminal v1.14
Development

Successfully merging a pull request may close this issue.

Fix overflow in Viewport::FromDimensions microsoft/terminal
4 participants
@lhecker @j4james @DHowett-MSFT @zadjii-msft