Skip to content

Commit

Permalink
Merged PR 8072712: [Git2Git] Fix conhost crash due to cmd.exe launch …
Browse files Browse the repository at this point in the history
…race

There is a condition which causes the console host process (conhost.exe)
to crash with a `FAIL_FAST` in `WriteCharsLegacy`.

**_Repro Scenario:_** Two conditions need to be met for crash to happen:

1. The `ENABLE_PROCESSED_OUTPUT` console mode needs to be disabled. This
   condition is met through the race condition, explained in the section
   below.
2. We are printing a string where there is a full width character
   (character that requires two spaces on the screen) being printed at
   the edge of the console window. That is, we have one character space
   available, and the character requires 2 spaces.

Running following script (attached to bug) causes a crash:
`for /l %%A in (0, 1, 10000) do start /B C:\test.bat`

The script `test.bat` repeatedly prints a console-width line
with a DBCS character that doesn't fit.

_**Race:**_ Normally, we get into `WriteCharsLegacy` with
`PROCESSED_OUTPUT` enabled. However, during the initialization of a new
CMD session, `cmd!ResetConsoleMode()` is called, which first sets the
output console mode to the value of `curOutputMode` (which is a static
variable initialized to 0) by calling `SetConsoleMode()`, before then
setting it to the desired output mode with processed output enabled:

```c++
void ResetConsoleMode( void )
{
    static DWORD desOutMode = ENABLE_PROCESSED_OUTPUT | /* ... */;

    SetConsoleMode(conOut, curOutputMode); // <------ sets console mode to 0, disabling processed output
    if (GetConsoleMode(conOut, &curOutputMode)) {
        if ((curOutputMode & desOutMode) != desOutMode) {
            curOutputMode |= desOutMode;

            if (!SetConsoleMode(conOut, curOutputMode) && /* ... */) // <----- enables processed output
```

If there is another instance of CMD that is producing output in between
these two `SetConsoleMode()` calls, then we may end up in
`WriteCharsLegacy` with processed output mode disabled.

This fix removes a `FAIL_FAST_IF` that checks for `PROCESSED_OUTPUT`
mode after the initial character processing loop.

Before RS5, this was an `ASSERT()`. This FAIL_FAST was added in RS5 in
PR !1794053 (which changed all `ASSERT`-likes to `FAIL_FAST_IF`.)

We believe this assertion guarded only the "processing" of Backspace,
Tab, CR and LF, and did not expect that we would get out of the
character processing loop with unprocessed glyph characters.  The
`FAIL_FAST` is redundant in this case, as the handlers for the
Backspace, Tab, CR and LF characters we are already checking for
`PROCESSED_OUTPUT`. Therefore, it is safe to remove this `FAIL_FAST`.

It turns out that we *can* exit the loop with unprocessed glyph
characters. In these cases, we don't want to `FAIL_FAST`.

# Validation
* Basic sanity testing to confirm strings are correctly being printed.
* Repro scenario script no longer crashes conhost.exe

Retrieved from https://microsoft.visualstudio.com os.2020 OS official/rs_we_adept_e4d2 eb5d8064dc0f09d8be92efb5e6efa69983f30a3f

Related work items: MSFT-42055103
  • Loading branch information
Hamza Nauman authored and DHowett committed Nov 2, 2022
1 parent 7c8f742 commit 86928bb
Showing 1 changed file with 0 additions and 1 deletion.
1 change: 0 additions & 1 deletion src/host/_stream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,6 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
return STATUS_SUCCESS;
}

FAIL_FAST_IF(!(WI_IsFlagSet(screenInfo.OutputMode, ENABLE_PROCESSED_OUTPUT)));
switch (*lpString)
{
case UNICODE_BACKSPACE:
Expand Down

0 comments on commit 86928bb

Please sign in to comment.