Clean up some misuses of INVALID_HANDLE_VALUE (fixes #427) (#1105)

Almost all functions in the Windows API that open or create objects and return HANDLEs to them return null on failure; only a few (mostly to do with the file system) return INVALID_HANDLE_VALUE on failure. This PR scrubs the repo of a few, but not necessarily all, cases where INVALID_HANDLE_VALUE was mistakenly used or tested against instead of null. In particular, it fixes 2 cases reported in issue #427 where the return value of CreateThread() was compared against INVALID_HANDLE_VALUE against null, causing the error handling code to run at the wrong time.

There are a lot of other uses of INVALID_HANDLE_VALUE I found that looked questionable, but which I left alone. Most of these were used to initialize HANDLE-typed variables and as a sentinel to see if those variables remained unset to a "real" value.

Fixes #427

src/cascadia/TerminalConnection/ConhostConnection.h

@@ -36,7 +36,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
         HANDLE _outPipe{ INVALID_HANDLE_VALUE }; // The pipe for reading output from
         HANDLE _signalPipe{ INVALID_HANDLE_VALUE };
         DWORD _outputThreadId{};
-        HANDLE _hOutputThread{ INVALID_HANDLE_VALUE };
+        HANDLE _hOutputThread{ nullptr };
         PROCESS_INFORMATION _piConhost{};
         guid _guid{}; // A "unique" session identifier for connected client
         bool _closing{};

src/host/PtySignalInputThread.cpp

@@ -181,7 +181,7 @@ HRESULT PtySignalInputThread::Start() noexcept
                            0,
                            &dwThreadId);
 
-    RETURN_LAST_ERROR_IF(hThread == INVALID_HANDLE_VALUE);
+    RETURN_LAST_ERROR_IF_NULL(hThread);
     _hThread.reset(hThread);
     _dwThreadId = dwThreadId;
 

src/host/VtInputThread.cpp

@@ -173,7 +173,7 @@ HRESULT VtInputThread::Start()
                            0,
                            &dwThreadId);
 
-    RETURN_LAST_ERROR_IF(hThread == INVALID_HANDLE_VALUE);
+    RETURN_LAST_ERROR_IF_NULL(hThread);
     _hThread.reset(hThread);
     _dwThreadId = dwThreadId;
 

src/host/ft_host/CJK_DbcsTests.cpp

@@ -278,8 +278,6 @@ class DbcsTests
     END_TEST_METHOD()
 };
 
-HANDLE hScreen = INVALID_HANDLE_VALUE;
-
 bool DbcsTests::DbcsTestSetup()
 {
     return true;

src/interactivity/onecore/ConIoSrvComm.cpp

@@ -24,7 +24,7 @@ extern void UnlockConsole();
 using namespace Microsoft::Console::Interactivity::OneCore;
 
 ConIoSrvComm::ConIoSrvComm() :
-    _inputPipeThreadHandle(INVALID_HANDLE_VALUE),
+    _inputPipeThreadHandle(nullptr),
     _pipeReadHandle(INVALID_HANDLE_VALUE),
     _pipeWriteHandle(INVALID_HANDLE_VALUE),
     _alpcClientCommunicationPort(INVALID_HANDLE_VALUE),
@@ -40,11 +40,11 @@ ConIoSrvComm::ConIoSrvComm() :
 ConIoSrvComm::~ConIoSrvComm()
 {
     // Cancel pending IOs on the input thread that might get us stuck.
-    if (INVALID_HANDLE_VALUE != _inputPipeThreadHandle)
+    if (_inputPipeThreadHandle)
     {
         LOG_IF_WIN32_BOOL_FALSE(CancelSynchronousIo(_inputPipeThreadHandle));
         CloseHandle(_inputPipeThreadHandle);
-        _inputPipeThreadHandle = INVALID_HANDLE_VALUE;
+        _inputPipeThreadHandle = nullptr;
     }
     
     // Free any handles we might have open.
@@ -242,7 +242,7 @@ NTSTATUS ConIoSrvComm::EnsureConnection()
 VOID ConIoSrvComm::ServiceInputPipe()
 {
     // Save off a handle to the thread that is coming in here in case it gets blocked and we need to tear down.
-    THROW_HR_IF(E_NOT_VALID_STATE, INVALID_HANDLE_VALUE != _inputPipeThreadHandle); // We can't store two of them, so it's invalid if there are two.
+    THROW_HR_IF(E_NOT_VALID_STATE, _inputPipeThreadHandle); // We can't store two of them, so it's invalid if there are two.
     THROW_IF_WIN32_BOOL_FALSE(DuplicateHandle(GetCurrentProcess(),
                                               GetCurrentThread(),
                                               GetCurrentProcess(),

src/renderer/gdi/paint.cpp

@@ -22,7 +22,7 @@ using namespace Microsoft::Console::Render;
 HRESULT GdiEngine::StartPaint() noexcept
 {
     // If we have no handle, we don't need to paint. Return quickly.
-    RETURN_HR_IF(S_FALSE, INVALID_HANDLE_VALUE == _hwndTargetWindow);
+    RETURN_HR_IF(S_FALSE, !_IsWindowValid());
 
     // If we're already painting, we don't need to paint. Return quickly.
     RETURN_HR_IF(S_FALSE, _fPaintStarted);

src/tools/vtpipeterm/VtConsole.cpp

@@ -20,16 +20,12 @@ VtConsole::VtConsole(PipeReadCallback const pfnReadCallback,
                      bool const fHeadless,
                      bool const fUseConpty,
                      COORD const initialSize) :
-    _signalPipe(INVALID_HANDLE_VALUE),
-    _outPipe(INVALID_HANDLE_VALUE),
-    _inPipe(INVALID_HANDLE_VALUE),
-    _dwOutputThreadId(0)
+    _pfnReadCallback(pfnReadCallback),
+    _fHeadless(fHeadless),
+    _fUseConPty(fUseConpty),
+    _lastDimensions(initialSize)
 {
-    _pfnReadCallback = pfnReadCallback;
-    _fHeadless = fHeadless;
-    _fUseConPty = fUseConpty;
-    _lastDimensions = initialSize;
-
+    THROW_IF_NULL_ALLOC(pfnReadCallback);
 }
 
 void VtConsole::spawn()

src/tools/vtpipeterm/VtConsole.hpp

@@ -57,9 +57,9 @@ private:
     PROCESS_INFORMATION _piPty;
     PROCESS_INFORMATION _piClient;
 
-    HANDLE _outPipe;
-    HANDLE _inPipe;
-    HANDLE _signalPipe;
+    HANDLE _outPipe = INVALID_HANDLE_VALUE;
+    HANDLE _inPipe = INVALID_HANDLE_VALUE;
+    HANDLE _signalPipe = INVALID_HANDLE_VALUE;
 
     HPCON _hPC;
 
@@ -70,8 +70,8 @@ private:
 
     PipeReadCallback _pfnReadCallback;
 
-    DWORD _dwOutputThreadId;
-    HANDLE _hOutputThread = INVALID_HANDLE_VALUE;
+    DWORD _dwOutputThreadId = 0;
+    HANDLE _hOutputThread = nullptr;
 
     void _createPseudoConsole(const std::wstring& command);
     void _createConptyManually(const std::wstring& command);