Repository: microsoft/terminal (language: cpp)

Issue:
Crash when toggling alt buffer in cmd shell
### Windows Terminal version

1.19.2682.0

### Windows build number

10.0.19045.3448

### Other Software

_No response_

### Steps to reproduce

1. Open a cmd shell or pwsh tab in Windows Terminal.
2. Run the python script below, pressing <kbd>Enter</kbd> a few times.

```py
import sys
while True:
  sys.stdout.write("\033[?1049h")
  sys.stdout.flush()
  sys.stdin.readline()
  sys.stdout.write("\033[?1049l")
```


### Expected Behavior

The script should be able to run forever without any ill effects.

### Actual Behavior

After pressing <kbd>Enter</kbd> three or four times, the tab crashes and disappears. The terminal itself is still fine.

I can also reproduce this in OpenConsole, so it's not a conpty thing, but you need to set the `HKCU\Console\VirtualTerminalLevel` registry entry first to enable the VT support.

Also worth noting that it doesn't seem to be triggered when executed from a WSL bash shell. I could only reproduce it with a cmd shell or pwsh.

And when run from within the debugger, I see an exception in one of two places. Most commonly here:

```
Exception thrown at 0x00007FF7F77A7FF0 in OpenConsole.exe: 0xC0000005: Access violation reading location 0x0000000000000090.
>	OpenConsole.exe!RenderData::GetViewport() Line 25	C++
 	OpenConsole.exe!Microsoft::Console::Render::Renderer::_CheckViewportAndScroll() Line 399	C++
 	OpenConsole.exe!Microsoft::Console::Render::Renderer::_PaintFrameForEngine(Microsoft::Console::Render::IRenderEngine * const pEngine) Line 122	C++
 	OpenConsole.exe!Microsoft::Console::Render::RenderThread::_ThreadProc() Line 217	C++
 	[External Code]	
```

But sometimes also here:
```
Exception thrown at 0x00007FFD7B7DC73E (ntdll.dll) in OpenConsole.exe: 0xC0000005: Access violation reading location 0x00007DF522920000.
 	ntdll.dll!LdrpDispatchUserCallTarget()	Unknown
>	OpenConsole.exe!Microsoft::Console::VirtualTerminal::StateMachine::_EnterEscape() Line 778	C++
 	OpenConsole.exe!Microsoft::Console::VirtualTerminal::StateMachine::ProcessCharacter(const wchar_t wch) Line 1864	C++
 	OpenConsole.exe!Microsoft::Console::VirtualTerminal::StateMachine::ProcessString(const std::basic_string_view<wchar_t,std::char_traits<wchar_t>> string) Line 2123	C++
 	OpenConsole.exe!DoWriteConsole(const wchar_t * pwchBuffer, unsigned __int64 * const pcbBuffer, SCREEN_INFORMATION & screenInfo, bool requiresVtQuirk, std::unique_ptr<WriteData,std::default_delete<WriteData>> & waiter) Line 321	C++
 	OpenConsole.exe!WriteConsoleWImplHelper(SCREEN_INFORMATION & context, const std::basic_string_view<wchar_t,std::char_traits<wchar_t>> buffer, unsigned __int64 & read, bool requiresVtQuirk, std::unique_ptr<WriteData,std::default_delete<WriteData>> & waiter) Line 361	C++
 	OpenConsole.exe!ApiRoutines::WriteConsoleWImpl(SCREEN_INFORMATION & context, const std::basic_string_view<wchar_t,std::char_traits<wchar_t>> buffer, unsigned __int64 & read, bool requiresVtQuirk, std::unique_ptr<IWaitRoutine,std::default_delete<IWaitRoutine>> & waiter) Line 590	C++
 	OpenConsole.exe!ApiDispatchers::ServerWriteConsole(_CONSOLE_API_MSG * const m, int * const pbReplyPending) Line 381	C++
 	OpenConsole.exe!IoSorter::ServiceIoOperation(_CONSOLE_API_MSG * const pMsg, _CONSOLE_API_MSG * * ReplyMsg) Line 33	C++
 	OpenConsole.exe!ConsoleIoThread(void * lpParameter) Line 1035	C++
 	[External Code]	
```

Git bisect suggests this was caused by commit 821ae3af2d311350fbfaa4c77af09858488681e9 (PR #15783).


Discussion / hints:

Fix patch:
diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt
index 4339b17eb04..31775457fff 100644
--- a/.github/actions/spelling/expect/expect.txt
+++ b/.github/actions/spelling/expect/expect.txt
@@ -303,6 +303,7 @@ coordnew
 COPYCOLOR
 CORESYSTEM
 cotaskmem
+countof
 CPG
 cpinfo
 CPINFOEX
diff --git a/src/host/ft_host/API_BufferTests.cpp b/src/host/ft_host/API_BufferTests.cpp
index 362b8353aef..242427e2b9a 100644
--- a/src/host/ft_host/API_BufferTests.cpp
+++ b/src/host/ft_host/API_BufferTests.cpp
@@ -3,6 +3,8 @@
 
 #include "precomp.h"
 
+#include "../../types/inc/IInputEvent.hpp"
+
 extern "C" IMAGE_DOS_HEADER __ImageBase;
 
 using namespace WEX::Logging;
@@ -17,6 +19,7 @@ class BufferTests
     END_TEST_CLASS()
 
     TEST_METHOD(TestSetConsoleActiveScreenBufferInvalid);
+    TEST_METHOD(TestCookedReadBufferReferenceCount);
 
     TEST_METHOD(TestCookedReadOnNonShareableScreenBuffer);
 
@@ -83,6 +86,40 @@ void BufferTests::TestCookedReadOnNonShareableScreenBuffer()
     VERIFY_IS_TRUE(IsConsoleStillRunning());
 }
 
+// This test ensures that COOKED_READ_DATA properly holds onto the screen buffer it is
+// reading from for the whole duration of the read. It's important that we hold a handle
+// to the main instead of the alt buffer even if this cooked read targets the latter,
+// because alt buffers are fake SCREEN_INFORMATION objects that are owned by the main buffer.
+void BufferTests::TestCookedReadBufferReferenceCount()
+{
+    static constexpr int loops = 5;
+
+    const auto in = GetStdInputHandle();
+    const auto out = GetStdOutputHandle();
+
+    DWORD inMode = 0;
+    GetConsoleMode(out, &inMode);
+    inMode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING | ENABLE_PROCESSED_OUTPUT;
+    SetConsoleMode(out, inMode);
+
+    INPUT_RECORD newlines[loops];
+    DWORD written = 0;
+    std::fill_n(&newlines[0], loops, SynthesizeKeyEvent(true, 1, L'\r', 0, L'\r', 0));
+    WriteConsoleInputW(in, &newlines[0], loops, &written);
+
+    for (int i = 0; i < loops; ++i)
+    {
+        VERIFY_SUCCEEDED(WriteConsoleW(out, L"\033[?1049h", 8, nullptr, nullptr));
+
+        wchar_t buffer[16];
+        DWORD read = 0;
+        VERIFY_SUCCEEDED(ReadConsoleW(in, &buffer[0], _countof(buffer), &read, nullptr));
+        VERIFY_ARE_EQUAL(2u, read);
+
+        VERIFY_SUCCEEDED(WriteConsoleW(out, L"\033[?1049l", 8, nullptr, nullptr));
+    }
+}
+
 void BufferTests::TestWritingInactiveScreenBuffer()
 {
     bool useVtOutput;
diff --git a/src/host/readDataCooked.cpp b/src/host/readDataCooked.cpp
index d05502dc0d0..36d54583fbb 100644
--- a/src/host/readDataCooked.cpp
+++ b/src/host/readDataCooked.cpp
@@ -62,7 +62,11 @@ COOKED_READ_DATA::COOKED_READ_DATA(_In_ InputBuffer* const pInputBuffer,
     // We need to ensure that it stays alive for the duration of the read.
     // Coincidentally this serves another important purpose: It checks whether we're allowed to read from
     // the given buffer in the first place. If it's missing the FILE_SHARE_READ flag, we can't read from it.
-    THROW_IF_FAILED(_screenInfo.AllocateIoHandle(ConsoleHandleData::HandleType::Output, GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, _tempHandle));
+    //
+    // GH#16158: It's important that we hold a handle to the main instead of the alt buffer
+    // even if this cooked read targets the latter, because alt buffers are fake
+    // SCREEN_INFORMATION objects that are owned by the main buffer.
+    THROW_IF_FAILED(_screenInfo.GetMainBuffer().AllocateIoHandle(ConsoleHandleData::HandleType::Output, GENERIC_WRITE, FILE_SHARE_READ | FILE_SHARE_WRITE, _tempHandle));
 #endif
 
     if (!initialData.empty())


Test patch: