Repository: microsoft/terminal (language: cpp)

Issue:
Crash when switching back from the alt buffer
### Windows Terminal version

Commit edfa3ea0f0080eadf7f01b463ed0a8638de6ce04

### Windows build number

10.0.19045.4651

### Other Software

_No response_

### Steps to reproduce

It's not easy to reproduce, but I think all you really need to trigger the crash is to switch to the alt buffer with `printf "\e[?1049h"`, and then switch back to the main buffer with `printf "\e[?1049l"`.

This is in Windows Terminal using a build that includes the new VT passthrough (PR #17510).

### Expected Behavior

The terminal should not crash.

### Actual Behavior

I got an access violation accessing the `screenInfo` parameter in the `WriteCharsVT` function here:

https://github.com/microsoft/terminal/blob/0199ca33ddb4a02ca3549627d772ce6b330ed1ce/src/host/_stream.cpp#L353

What happens is that we first pass the `\e[?1049l` sequence through to conhost in the `stateMachine.ProcessString` call a couple of lines above. That ends up calling `SCREEN_INFORMATION::UseMainScreenBuffer`, which calls `s_RemoveScreenBuffer(psiAlt)`, which deletes the active `screenInfo` instance here:

https://github.com/microsoft/terminal/blob/0199ca33ddb4a02ca3549627d772ce6b330ed1ce/src/host/screenInfo.cpp#L231

That's the same `screenInfo` instance that was passed to our `WriteCharsVT` function, so when we later try to reference that, we're referencing a deleted object. Sometimes we get lucky and nothing bad happens, but sometimes it will crash.

I think maybe all we need to do to prevent the crash is to save the `screenInfo.OutputMode` at the start of the function, and then use that saved value instead of trying to look it up via the `screenInfo` object.


Discussion / hints:

Fix patch:
diff --git a/src/host/_stream.cpp b/src/host/_stream.cpp
index e434e413ebd..82dda40ef9b 100644
--- a/src/host/_stream.cpp
+++ b/src/host/_stream.cpp
@@ -336,6 +336,9 @@ void WriteCharsVT(SCREEN_INFORMATION& screenInfo, const std::wstring_view& str)
 {
     auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
     auto& stateMachine = screenInfo.GetStateMachine();
+    // If the given screenInfo is the alternate screen buffer, disabling the alternate screen buffer in this
+    // VT payload will cause the pointer to be invalidated. We thus need to get all the information we need now.
+    const auto disableNewlineTranslation = WI_IsFlagSet(screenInfo.OutputMode, DISABLE_NEWLINE_AUTO_RETURN);
     // When switch between the main and alt-buffer SCREEN_INFORMATION::GetActiveBuffer()
     // may change, so get the VtIo reference now, just in case.
     auto writer = gci.GetVtWriterForBuffer(&screenInfo);
@@ -350,7 +353,7 @@ void WriteCharsVT(SCREEN_INFORMATION& screenInfo, const std::wstring_view& str)
         // DISABLE_NEWLINE_AUTO_RETURN not being set is equivalent to a LF -> CRLF translation.
         const auto write = [&](size_t beg, size_t end) {
             const auto chunk = til::safe_slice_abs(str, beg, end);
-            if (WI_IsFlagSet(screenInfo.OutputMode, DISABLE_NEWLINE_AUTO_RETURN))
+            if (disableNewlineTranslation)
             {
                 writer.WriteUTF16(chunk);
             }


Test patch: