Repository: microsoft/terminal (language: cpp)

Issue:
`FAST_FAIL_FATAL_APP_EXIT` in `UnlockConsole()`
### Windows Terminal version

main 7061c54

### Windows build number

10.0.22000.434

### Other Software

_No response_

### Steps to reproduce

1. Open the terminal
2. Close the terminal

### Expected Behavior

The instance of `OpenConsole.exe` shouldn't crash.

### Actual Behavior

The instance of `OpenConsole.exe` crashes. The `FAIL_FAST_IF` was added in 7061c54 (#11890), here: https://github.com/microsoft/terminal/blob/2bf10007ffc368e8a0ccf31f47de9fb045d38f40/src/host/consoleInformation.cpp#L81-L90

cc @lhecker 

```
0:001> !analyze -v
*******************************************************************************
*                                                                             *
*                        Exception Analysis                                   *
*                                                                             *
*******************************************************************************

DEBUG_FLR_EXCEPTION_CODE(8000ffff) and the ".exr -1" ExceptionCode(c0000409) don't match

KEY_VALUES_STRING: 1

    Key  : Analysis.CPU.mSec
    Value: 1734

    Key  : Analysis.DebugAnalysisManager
    Value: Create

    Key  : Analysis.Elapsed.mSec
    Value: 26653

    Key  : Analysis.Init.CPU.mSec
    Value: 296

    Key  : Analysis.Init.Elapsed.mSec
    Value: 237998

    Key  : Analysis.Memory.CommitPeak.Mb
    Value: 122

    Key  : FailFast.Name
    Value: FATAL_APP_EXIT

    Key  : FailFast.Type
    Value: 7

    Key  : Timeline.Process.Start.DeltaSec
    Value: 7

    Key  : WER.OS.Branch
    Value: co_release

    Key  : WER.OS.Timestamp
    Value: 2021-06-04T16:28:00Z

    Key  : WER.OS.Version
    Value: 10.0.22000.1


FILE_IN_CAB:  OpenConsole.exe.16980.dmp

NTGLOBALFLAG:  0

PROCESS_BAM_CURRENT_THROTTLED: 0

PROCESS_BAM_PREVIOUS_THROTTLED: 0

APPLICATION_VERIFIER_FLAGS:  0

CONTEXT:  (.ecxr)
rax=000000e8760fdf80 rbx=000000e8760fe500 rcx=000000e8760fdf80
rdx=000000e8760fe430 rsi=000000e8760fe500 rdi=000000e8760fdf80
rip=00007ff88b042492 rsp=000000e8760fdea0 rbp=0000000000000001
 r8=0000000000000000  r9=000000e8760fe3e0 r10=00000fff11608469
r11=0000020000020008 r12=00007ff79ebcb2ee r13=0000000000000001
r14=0000000000000000 r15=00007ff79ec73f98
iopl=0         nv up ei pl nz na po nc
cs=0033  ss=002b  ds=002b  es=002b  fs=0053  gs=002b             efl=00000204
KERNELBASE!RaiseFailFastException+0x152:
00007ff8`8b042492 0f1f440000      nop     dword ptr [rax+rax]
Resetting default scope

EXCEPTION_RECORD:  (.exr -1)
ExceptionAddress: 00007ff79ebcb2ee (OpenConsole!CONSOLE_INFORMATION::UnlockConsole+0x000000000000005e)
   ExceptionCode: c0000409 (Security check failure or stack buffer overrun)
  ExceptionFlags: 00000001
NumberParameters: 3
   Parameter[0]: 0000000000000007
   Parameter[1]: ffffffff8000ffff
   Parameter[2]: 0000000000000054
Subcode: 0x7 FAST_FAIL_FATAL_APP_EXIT 

PROCESS_NAME:  OpenConsole.exe

EXCEPTION_CODE_STR:  8000ffff

STACK_TEXT:  
000000e8`760fdea0 00007ff7`9eb84239     : 00000000`00000000 00007ff8`8b042340 00000000`00000000 000000e8`760fe6b0 : KERNELBASE!RaiseFailFastException+0x152
000000e8`760fe480 00007ff7`9eb84038     : 000000e8`760fe5d0 00007ff7`9ebcc092 00000000`00000000 00000000`00000000 : OpenConsole!wil::details::WilDynamicLoadRaiseFailFastException+0x49
000000e8`760fe4b0 00007ff7`9eb8510d     : 00000000`00000000 00000000`00000000 00000000`00000000 00000000`00000000 : OpenConsole!wil::details::WilRaiseFailFastException+0x18
000000e8`760fe4e0 00007ff7`9eb86ca8     : 000000e8`760ffb60 000000e8`760fe6b0 00007ff7`9ebcc092 00000000`00000000 : OpenConsole!wil::details::WilFailFast+0xdd
000000e8`760fe5b0 00007ff7`9eb85140     : 00000000`00000000 00000000`00000000 00000000`00000000 00007ff7`9ebcc092 : OpenConsole!wil::details::ReportFailure_NoReturn<3>+0x248
000000e8`760ffab0 00007ff7`9eb851af     : 00000000`00000000 00000000`00000000 00000000`00000000 00000000`00000000 : OpenConsole!wil::details::ReportFailure_Base<3,0>+0x30
000000e8`760ffb10 00007ff7`9eb8ee8b     : 00000000`00000002 00000000`00000000 00000000`00000000 00007ff8`8a8e218b : OpenConsole!wil::details::ReportFailure_Hr<3>+0x5f
000000e8`760ffb90 00007ff7`9ebcb2ee     : 00000223`b1238dd0 00000000`00000000 00000000`00000002 00000000`00000000 : OpenConsole!wil::details::in1diag3::_FailFast_Unexpected+0x1b
000000e8`760ffbe0 00007ff7`9ebcc092     : 00000000`00000002 00000000`00000000 00000000`00000000 000000e8`760ffc40 : OpenConsole!CONSOLE_INFORMATION::UnlockConsole+0x5e
000000e8`760ffc10 00007ff7`9ebcbf01     : 00000000`00000000 00000000`00000000 00000000`00000000 00000223`b1232230 : OpenConsole!Microsoft::Console::PtySignalInputThread::_GetData+0x52
000000e8`760ffc60 00007ff8`8c1b54e0     : 00000000`00000000 00000000`00000000 00000000`00000000 00000000`00000000 : OpenConsole!Microsoft::Console::PtySignalInputThread::_InputThread+0x31
000000e8`760ffcb0 00007ff8`8d3e485b     : 00000000`00000000 00000000`00000000 00000000`00000000 00000000`00000000 : kernel32!BaseThreadInitThunk+0x10
000000e8`760ffce0 00000000`00000000     : 00000000`00000000 00000000`00000000 00000000`00000000 00000000`00000000 : ntdll!RtlUserThreadStart+0x2b


FAULTING_SOURCE_LINE:  F:\windows-terminal\src\host\consoleInformation.cpp

FAULTING_SOURCE_FILE:  F:\windows-terminal\src\host\consoleInformation.cpp

FAULTING_SOURCE_LINE_NUMBER:  84

FAULTING_SOURCE_CODE:  
    80: void CONSOLE_INFORMATION::UnlockConsole()
    81: {
    82:     // See description of recursionCount a few lines above.
    83:     const auto rc = --recursionCount;
>   84:     FAIL_FAST_IF(rc == ULONG_MAX);
    85:     if (rc == 0)
    86:     {
    87:         lock.unlock();
    88:     }
    89: }


SYMBOL_NAME:  OpenConsole!CONSOLE_INFORMATION::UnlockConsole+5e

MODULE_NAME: OpenConsole

IMAGE_NAME:  OpenConsole.exe

STACK_COMMAND:  ~1s ; .ecxr ; kb

FAILURE_BUCKET_ID:  FAIL_FAST_FATAL_APP_EXIT_8000ffff_OpenConsole.exe!CONSOLE_INFORMATION::UnlockConsole

OS_VERSION:  10.0.22000.1

BUILDLAB_STR:  co_release

OSPLATFORM_TYPE:  x64

OSNAME:  Windows 10

FAILURE_ID_HASH:  {71c04470-b2ad-a39c-f89f-e94a80ecb962}

Followup:     MachineOwner
---------
```


Discussion / hints:
A double unlock appears to be happening in `PtySignalInputThread::_Shutdown()`.

https://github.com/microsoft/terminal/blob/7061c54ac59c8469a00a6ce36334ba379ff704e9/src/host/PtySignalInputThread.cpp#L225-L240

The call to `Output::CloseConsoleProcessState()` sets the `CONSOLE_CTRL_CLOSE_FLAG` flag on the console information and then locks the console via `Handle::LockConsole()`. It then immediately unlocks it again via `Handle::UnlockConsole()`, which finds the lock count to be 1, so delegates to `Input::ProcessCtrlEvents()`, which clears the `CONSOLE_CTRL_CLOSE_FLAG` control flag and calls `CONSOLE_INFORMATION::UnlockConsole()` to actually unlock the console.

Control returns to `PtySignalInputThread::_Shutdown()`, so `Input::ProcessCtrlEvents()` is then called, which finds the control flag to be 0, so calls `CONSOLE_INFORMATION::UnlockConsole()`, leading to the fail fast.
I'm sorry for not catching this issue. I focused my testing entirely on OpenConsole.exe and didn't consider testing ConPTY specifically (with Windows Terminal).

Calling `LeaveCriticalSection()` too often is a fairly nasty mistake, because the only way to ever notice it is if you call `EnterCriticalSection()` in the future, which we don't do here (we're exiting the application after all):
> If a thread calls LeaveCriticalSection when it does not have ownership of the specified critical section object, an error occurs that may cause another thread using EnterCriticalSection to wait indefinitely. ([source](https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-leavecriticalsection))

I suspect the call to `ProcessCtrlEvents();` is invalid. `_Shutdown()` is called by `_GetData()`, whose only two places of invocation are outside of the console lock. I'll submit a PR which removes the call to `ProcessCtrlEvents();` and adds an assertion ensuring that the lock isn't acquired when `_Shutdown()` is called.

Fix patch:
diff --git a/src/host/PtySignalInputThread.cpp b/src/host/PtySignalInputThread.cpp
index 3855a467ab5..16b330085e6 100644
--- a/src/host/PtySignalInputThread.cpp
+++ b/src/host/PtySignalInputThread.cpp
@@ -170,7 +170,6 @@ bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBu
         if (lastError == ERROR_BROKEN_PIPE)
         {
             _Shutdown();
-            return false;
         }
         else
         {
@@ -180,7 +179,6 @@ bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBu
     else if (dwRead != cbBuffer)
     {
         _Shutdown();
-        return false;
     }
 
     return true;
@@ -233,6 +231,7 @@ void PtySignalInputThread::_Shutdown()
     //      happens if this method is called outside of lock, but if we're
     //      currently locked, we want to make sure ctrl events are handled
     //      _before_ we RundownAndExit.
+    LockConsole();
     ProcessCtrlEvents();
 
     // Make sure we terminate.
diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp
index f7ac039bc5c..5b482ecd185 100644
--- a/src/host/VtIo.cpp
+++ b/src/host/VtIo.cpp
@@ -10,6 +10,7 @@
 
 #include "../renderer/base/renderer.hpp"
 #include "../types/inc/utils.hpp"
+#include "handle.h" // LockConsole
 #include "input.h" // ProcessCtrlEvents
 #include "output.h" // CloseConsoleProcessState
 
@@ -383,6 +384,7 @@ void VtIo::_ShutdownIfNeeded()
         //      happens if this method is called outside of lock, but if we're
         //      currently locked, we want to make sure ctrl events are handled
         //      _before_ we RundownAndExit.
+        LockConsole();
         ProcessCtrlEvents();
 
         // Make sure we terminate.
diff --git a/src/host/output.cpp b/src/host/output.cpp
index c07cb566e10..a5f539ccc77 100644
--- a/src/host/output.cpp
+++ b/src/host/output.cpp
@@ -520,5 +520,5 @@ void CloseConsoleProcessState()
     //      ctrl event will never actually get dispatched.
     // So, lock and unlock here, to make sure the ctrl event gets handled.
     LockConsole();
-    auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
+    UnlockConsole();
 }
diff --git a/src/interactivity/base/HostSignalInputThread.cpp b/src/interactivity/base/HostSignalInputThread.cpp
index 2dc71ccca20..1c2abeb471a 100644
--- a/src/interactivity/base/HostSignalInputThread.cpp
+++ b/src/interactivity/base/HostSignalInputThread.cpp
@@ -163,7 +163,6 @@ bool HostSignalInputThread::_GetData(gsl::span<gsl::byte> buffer)
         if (lastError == ERROR_BROKEN_PIPE)
         {
             _Shutdown();
-            return false;
         }
 
         THROW_WIN32(lastError);
@@ -172,7 +171,6 @@ bool HostSignalInputThread::_GetData(gsl::span<gsl::byte> buffer)
     if (bytesRead != buffer.size())
     {
         _Shutdown();
-        return false;
     }
 
     return true;
diff --git a/src/interactivity/base/ServiceLocator.cpp b/src/interactivity/base/ServiceLocator.cpp
index 577caad3617..3bc691abdc6 100644
--- a/src/interactivity/base/ServiceLocator.cpp
+++ b/src/interactivity/base/ServiceLocator.cpp
@@ -34,7 +34,7 @@ wil::unique_hwnd ServiceLocator::s_pseudoWindow = nullptr;
 
 #pragma region Public Methods
 
-void ServiceLocator::RundownAndExit(const HRESULT hr)
+[[noreturn]] void ServiceLocator::RundownAndExit(const HRESULT hr)
 {
     // MSFT:15506250
     // In VT I/O Mode, a client application might die before we've rendered
@@ -67,7 +67,7 @@ void ServiceLocator::RundownAndExit(const HRESULT hr)
         s_inputServices.reset(nullptr);
     }
 
-    TerminateProcess(GetCurrentProcess(), hr);
+    ExitProcess(hr);
 }
 
 #pragma region Creation Methods
diff --git a/src/interactivity/inc/ServiceLocator.hpp b/src/interactivity/inc/ServiceLocator.hpp
index 583efb2540c..41e27bcdf9f 100644
--- a/src/interactivity/inc/ServiceLocator.hpp
+++ b/src/interactivity/inc/ServiceLocator.hpp
@@ -30,7 +30,7 @@ namespace Microsoft::Console::Interactivity
     class ServiceLocator final
     {
     public:
-        static void RundownAndExit(const HRESULT hr);
+        [[noreturn]] static void RundownAndExit(const HRESULT hr);
 
         // N.B.: Location methods without corresponding creation methods
         //       automatically create the singleton object on demand.


Test patch: