Add ITerminalHandoff3 in preparation for overlapped pipes

src/cascadia/CascadiaPackage/Package-Can.appxmanifest

@@ -111,8 +111,7 @@
             <com:ComInterface>
                 <com:ProxyStub Id="1D1852F4-ADAD-42B6-9A43-9437AAAD7717" DisplayName="OpenConsoleHandoffProxy" Path="OpenConsoleProxy.dll"/>
                 <com:Interface Id="E686C757-9A35-4A1C-B3CE-0BCC8B5C69F4" ProxyStubClsid="1D1852F4-ADAD-42B6-9A43-9437AAAD7717"/>
-                <com:Interface Id="59D55CCE-FC8A-48B4-ACE8-0A9286C6557F" ProxyStubClsid="1D1852F4-ADAD-42B6-9A43-9437AAAD7717"/> <!-- ITerminalHandoff -->
-                <com:Interface Id="AA6B364F-4A50-4176-9002-0AE755E7B5EF" ProxyStubClsid="1D1852F4-ADAD-42B6-9A43-9437AAAD7717"/> <!-- ITerminalHandoff2 -->
+                <com:Interface Id="6F23DA90-15C5-4203-9DB0-64E73F1B1B00" ProxyStubClsid="1D1852F4-ADAD-42B6-9A43-9437AAAD7717"/> <!-- ITerminalHandoff3 -->
                 <com:Interface Id="746E6BC0-AB05-4E38-AB14-71E86763141F" ProxyStubClsid="1D1852F4-ADAD-42B6-9A43-9437AAAD7717"/>
             </com:ComInterface>
         </com:Extension>

src/cascadia/CascadiaPackage/Package-Dev.appxmanifest

@@ -111,8 +111,7 @@
             <com:ComInterface>
                 <com:ProxyStub Id="DEC4804D-56D1-4F73-9FBE-6828E7C85C56" DisplayName="OpenConsoleHandoffProxy" Path="OpenConsoleProxy.dll"/>
                 <com:Interface Id="E686C757-9A35-4A1C-B3CE-0BCC8B5C69F4" ProxyStubClsid="DEC4804D-56D1-4F73-9FBE-6828E7C85C56"/>
-                <com:Interface Id="59D55CCE-FC8A-48B4-ACE8-0A9286C6557F" ProxyStubClsid="DEC4804D-56D1-4F73-9FBE-6828E7C85C56"/> <!-- ITerminalHandoff -->
-                <com:Interface Id="AA6B364F-4A50-4176-9002-0AE755E7B5EF" ProxyStubClsid="DEC4804D-56D1-4F73-9FBE-6828E7C85C56"/> <!-- ITerminalHandoff2 -->
+                <com:Interface Id="6F23DA90-15C5-4203-9DB0-64E73F1B1B00" ProxyStubClsid="DEC4804D-56D1-4F73-9FBE-6828E7C85C56"/> <!-- ITerminalHandoff3 -->
                 <com:Interface Id="746E6BC0-AB05-4E38-AB14-71E86763141F" ProxyStubClsid="DEC4804D-56D1-4F73-9FBE-6828E7C85C56"/>
             </com:ComInterface>
         </com:Extension>

src/cascadia/CascadiaPackage/Package-Pre.appxmanifest

@@ -200,8 +200,7 @@
             <com:ComInterface>
                 <com:ProxyStub Id="1833E661-CC81-4DD0-87C6-C2F74BD39EFA" DisplayName="OpenConsoleHandoffProxy" Path="OpenConsoleProxy.dll"/>
                 <com:Interface Id="E686C757-9A35-4A1C-B3CE-0BCC8B5C69F4" ProxyStubClsid="1833E661-CC81-4DD0-87C6-C2F74BD39EFA"/>
-                <com:Interface Id="59D55CCE-FC8A-48B4-ACE8-0A9286C6557F" ProxyStubClsid="1833E661-CC81-4DD0-87C6-C2F74BD39EFA"/> <!-- ITerminalHandoff -->
-                <com:Interface Id="AA6B364F-4A50-4176-9002-0AE755E7B5EF" ProxyStubClsid="1833E661-CC81-4DD0-87C6-C2F74BD39EFA"/> <!-- ITerminalHandoff2 -->
+                <com:Interface Id="6F23DA90-15C5-4203-9DB0-64E73F1B1B00" ProxyStubClsid="1833E661-CC81-4DD0-87C6-C2F74BD39EFA"/> <!-- ITerminalHandoff3 -->
                 <com:Interface Id="746E6BC0-AB05-4E38-AB14-71E86763141F" ProxyStubClsid="1833E661-CC81-4DD0-87C6-C2F74BD39EFA"/>
             </com:ComInterface>
         </com:Extension>

src/cascadia/CascadiaPackage/Package.appxmanifest

@@ -200,8 +200,7 @@
             <com:ComInterface>
                 <com:ProxyStub Id="3171DE52-6EFA-4AEF-8A9F-D02BD67E7A4F" DisplayName="OpenConsoleHandoffProxy" Path="OpenConsoleProxy.dll"/>
                 <com:Interface Id="E686C757-9A35-4A1C-B3CE-0BCC8B5C69F4" ProxyStubClsid="3171DE52-6EFA-4AEF-8A9F-D02BD67E7A4F"/>
-                <com:Interface Id="59D55CCE-FC8A-48B4-ACE8-0A9286C6557F" ProxyStubClsid="3171DE52-6EFA-4AEF-8A9F-D02BD67E7A4F"/> <!-- ITerminalHandoff -->
-                <com:Interface Id="AA6B364F-4A50-4176-9002-0AE755E7B5EF" ProxyStubClsid="3171DE52-6EFA-4AEF-8A9F-D02BD67E7A4F"/> <!-- ITerminalHandoff2 -->
+                <com:Interface Id="6F23DA90-15C5-4203-9DB0-64E73F1B1B00" ProxyStubClsid="3171DE52-6EFA-4AEF-8A9F-D02BD67E7A4F"/> <!-- ITerminalHandoff3 -->
                 <com:Interface Id="746E6BC0-AB05-4E38-AB14-71E86763141F" ProxyStubClsid="3171DE52-6EFA-4AEF-8A9F-D02BD67E7A4F"/>
             </com:ComInterface>
         </com:Extension>

src/cascadia/TerminalConnection/CTerminalHandoff.cpp

@@ -72,20 +72,6 @@ HRESULT CTerminalHandoff::s_StopListeningLocked()
     return S_OK;
 }
 
-// Routine Description:
-// - Helper to duplicate a handle to ourselves so we can keep holding onto it
-//   after the caller frees the original one.
-// Arguments:
-// - in - Handle to duplicate
-// - out - Where to place the duplicated value
-// Return Value:
-// - S_OK or Win32 error from `::DuplicateHandle`
-static HRESULT _duplicateHandle(const HANDLE in, HANDLE& out) noexcept
-{
-    RETURN_IF_WIN32_BOOL_FALSE(::DuplicateHandle(GetCurrentProcess(), in, GetCurrentProcess(), &out, 0, FALSE, DUPLICATE_SAME_ACCESS));
-    return S_OK;
-}
-
 // Routine Description:
 // - Receives the terminal handoff via COM from the other process,
 //   duplicates handles as COM will free those given on the way out,
@@ -102,7 +88,7 @@ static HRESULT _duplicateHandle(const HANDLE in, HANDLE& out) noexcept
 // - E_NOT_VALID_STATE if a event handler is not registered before calling. `::DuplicateHandle`
 //   error codes if we cannot manage to make our own copy of handles to retain. Or S_OK/error
 //   from the registered handler event function.
-HRESULT CTerminalHandoff::EstablishPtyHandoff(HANDLE in, HANDLE out, HANDLE signal, HANDLE ref, HANDLE server, HANDLE client, TERMINAL_STARTUP_INFO startupInfo)
+HRESULT CTerminalHandoff::EstablishPtyHandoff(HANDLE* in, HANDLE* out, HANDLE signal, HANDLE reference, HANDLE server, HANDLE client, const TERMINAL_STARTUP_INFO* startupInfo)
 {
     try
     {
@@ -120,19 +106,8 @@ HRESULT CTerminalHandoff::EstablishPtyHandoff(HANDLE in, HANDLE out, HANDLE sign
         // Report an error if no one registered a handoff function before calling this.
         THROW_HR_IF_NULL(E_NOT_VALID_STATE, localPfnHandoff);
 
-        // Duplicate the handles from what we received.
-        // The contract with COM specifies that any HANDLEs we receive from the caller belong
-        // to the caller and will be freed when we leave the scope of this method.
-        // Making our own duplicate copy ensures they hang around in our lifetime.
-        THROW_IF_FAILED(_duplicateHandle(in, in));
-        THROW_IF_FAILED(_duplicateHandle(out, out));
-        THROW_IF_FAILED(_duplicateHandle(signal, signal));
-        THROW_IF_FAILED(_duplicateHandle(ref, ref));
-        THROW_IF_FAILED(_duplicateHandle(server, server));
-        THROW_IF_FAILED(_duplicateHandle(client, client));
-
         // Call registered handler from when we started listening.
-        THROW_IF_FAILED(localPfnHandoff(in, out, signal, ref, server, client, startupInfo));
+        THROW_IF_FAILED(localPfnHandoff(in, out, signal, reference, server, client, startupInfo));
 
 #pragma warning(suppress : 26477)
         TraceLoggingWrite(

src/cascadia/TerminalConnection/CTerminalHandoff.h

@@ -28,19 +28,13 @@ Author(s):
 #define __CLSID_CTerminalHandoff "051F34EE-C1FD-4B19-AF75-9BA54648434C"
 #endif
 
-using NewHandoffFunction = HRESULT (*)(HANDLE, HANDLE, HANDLE, HANDLE, HANDLE, HANDLE, TERMINAL_STARTUP_INFO);
+using NewHandoffFunction = HRESULT (*)(HANDLE* in, HANDLE* out, HANDLE signal, HANDLE reference, HANDLE server, HANDLE client, const TERMINAL_STARTUP_INFO* startupInfo);
 
 struct __declspec(uuid(__CLSID_CTerminalHandoff))
-    CTerminalHandoff : public Microsoft::WRL::RuntimeClass<Microsoft::WRL::RuntimeClassFlags<Microsoft::WRL::RuntimeClassType::ClassicCom>, ITerminalHandoff2>
+    CTerminalHandoff : public Microsoft::WRL::RuntimeClass<Microsoft::WRL::RuntimeClassFlags<Microsoft::WRL::RuntimeClassType::ClassicCom>, ITerminalHandoff3>
 {
 #pragma region ITerminalHandoff
-    STDMETHODIMP EstablishPtyHandoff(HANDLE in,
-                                     HANDLE out,
-                                     HANDLE signal,
-                                     HANDLE ref,
-                                     HANDLE server,
-                                     HANDLE client,
-                                     TERMINAL_STARTUP_INFO startupInfo) override;
+    STDMETHODIMP EstablishPtyHandoff(HANDLE* in, HANDLE* out, HANDLE signal, HANDLE reference, HANDLE server, HANDLE client, const TERMINAL_STARTUP_INFO* startupInfo) override;
 
 #pragma endregion
 

src/cascadia/TerminalConnection/ConptyConnection.cpp

@@ -5,7 +5,6 @@
 #include "ConptyConnection.h"
 
 #include <conpty-static.h>
-#include <winternl.h>
 
 #include "CTerminalHandoff.h"
 #include "LibraryResources.h"
@@ -173,33 +172,10 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
     }
     CATCH_RETURN();
 
-    ConptyConnection::ConptyConnection(const HANDLE hSig,
-                                       const HANDLE hIn,
-                                       const HANDLE hOut,
-                                       const HANDLE hRef,
-                                       const HANDLE hServerProcess,
-                                       const HANDLE hClientProcess,
-                                       const TERMINAL_STARTUP_INFO& startupInfo) :
-        _rows{ 25 },
-        _cols{ 80 },
-        _inPipe{ hIn },
-        _outPipe{ hOut }
+    // Who decided that?
+#pragma warning(suppress : 26455) // Default constructor should not throw. Declare it 'noexcept' (f.6).
+    ConptyConnection::ConptyConnection()
     {
-        _sessionId = Utils::CreateGuid();
-
-        THROW_IF_FAILED(ConptyPackPseudoConsole(hServerProcess, hRef, hSig, &_hPC));
-        _piClient.hProcess = hClientProcess;
-
-        _startupInfo.title = winrt::hstring{ startupInfo.pszTitle, SysStringLen(startupInfo.pszTitle) };
-        _startupInfo.iconPath = winrt::hstring{ startupInfo.pszIconPath, SysStringLen(startupInfo.pszIconPath) };
-        _startupInfo.iconIndex = startupInfo.iconIndex;
-        _startupInfo.showWindow = startupInfo.wShowWindow;
-
-        try
-        {
-            _commandline = _commandlineFromProcess(hClientProcess);
-        }
-        CATCH_LOG()
     }
 
     // Function Description:
@@ -319,6 +295,53 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
         }
     }
 
+    static wil::unique_hfile duplicateHandle(const HANDLE in)
+    {
+        wil::unique_hfile h;
+        THROW_IF_WIN32_BOOL_FALSE(DuplicateHandle(GetCurrentProcess(), in, GetCurrentProcess(), h.addressof(), 0, FALSE, DUPLICATE_SAME_ACCESS));
+        return h;
+    }
+
+    // Misdiagnosis: out is being tested right in the first line.
+#pragma warning(suppress : 26430) // Symbol 'out' is not tested for nullness on all paths (f.23).
+    void ConptyConnection::InitializeFromHandoff(HANDLE* in, HANDLE* out, HANDLE signal, HANDLE reference, HANDLE server, HANDLE client, const TERMINAL_STARTUP_INFO* startupInfo)
+    {
+        THROW_HR_IF(E_UNEXPECTED, !in || !out || !startupInfo);
+
+        _sessionId = Utils::CreateGuid();
+
+        wil::unique_hfile inPipePseudoConsoleSide;
+        wil::unique_hfile outPipePseudoConsoleSide;
+        THROW_IF_WIN32_BOOL_FALSE(CreatePipe(&inPipePseudoConsoleSide, &_inPipe, nullptr, 0));
+        THROW_IF_WIN32_BOOL_FALSE(CreatePipe(&_outPipe, &outPipePseudoConsoleSide, nullptr, 0));
+
+        auto ownedSignal = duplicateHandle(signal);
+        auto ownedReference = duplicateHandle(reference);
+        auto ownedServer = duplicateHandle(server);
+        auto ownedClient = duplicateHandle(client);
+
+        THROW_IF_FAILED(ConptyPackPseudoConsole(ownedServer.get(), ownedReference.get(), ownedSignal.get(), &_hPC));
+        ownedServer.release();
+        ownedReference.release();
+        ownedSignal.release();
+
+        _piClient.hProcess = ownedClient.release();
+
+        _startupInfo.title = winrt::hstring{ startupInfo->pszTitle, SysStringLen(startupInfo->pszTitle) };
+        _startupInfo.iconPath = winrt::hstring{ startupInfo->pszIconPath, SysStringLen(startupInfo->pszIconPath) };
+        _startupInfo.iconIndex = startupInfo->iconIndex;
+        _startupInfo.showWindow = startupInfo->wShowWindow;
+
+        try
+        {
+            _commandline = _commandlineFromProcess(_piClient.hProcess);
+        }
+        CATCH_LOG()
+
+        *in = inPipePseudoConsoleSide.release();
+        *out = outPipePseudoConsoleSide.release();
+    }
+
     winrt::hstring ConptyConnection::Commandline() const
     {
         return _commandline;
@@ -618,12 +641,20 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
         // won't wait for us, and the known exit points _do_.
         auto strongThis{ get_strong() };
 
+        const auto cleanup = wil::scope_exit([this]() noexcept {
+            _LastConPtyClientDisconnected();
+        });
+
+        char buffer[128 * 1024];
+        DWORD read = 0;
+
+        til::u8state u8State;
+        std::wstring wstr;
+
         // process the data of the output pipe in a loop
         while (true)
         {
-            DWORD read{};
-
-            const auto readFail{ !ReadFile(_outPipe.get(), _buffer.data(), gsl::narrow_cast<DWORD>(_buffer.size()), &read, nullptr) };
+            const auto readFail{ !ReadFile(_outPipe.get(), &buffer[0], sizeof(buffer), &read, nullptr) };
 
             // When we call CancelSynchronousIo() in Close() this is the branch that's taken and gets us out of here.
             if (_isStateAtOrBeyond(ConnectionState::Closing))
@@ -648,7 +679,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
                 }
             }
 
-            const auto result{ til::u8u16(std::string_view{ _buffer.data(), read }, _u16Str, _u8State) };
+            const auto result{ til::u8u16(std::string_view{ &buffer[0], read }, wstr, u8State) };
             if (FAILED(result))
             {
                 // EXIT POINT
@@ -657,7 +688,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
                 return gsl::narrow_cast<DWORD>(result);
             }
 
-            if (_u16Str.empty())
+            if (wstr.empty())
             {
                 return 0;
             }
@@ -679,7 +710,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
             }
 
             // Pass the output to our registered event handlers
-            TerminalOutput.raise(_u16Str);
+            TerminalOutput.raise(wstr);
         }
 
         return 0;
@@ -695,11 +726,12 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
         ::ConptyClosePseudoConsoleTimeout(hPC, 0);
     }
 
-    HRESULT ConptyConnection::NewHandoff(HANDLE in, HANDLE out, HANDLE signal, HANDLE ref, HANDLE server, HANDLE client, TERMINAL_STARTUP_INFO startupInfo) noexcept
+    HRESULT ConptyConnection::NewHandoff(HANDLE* in, HANDLE* out, HANDLE signal, HANDLE reference, HANDLE server, HANDLE client, const TERMINAL_STARTUP_INFO* startupInfo) noexcept
     try
     {
-        _newConnectionHandlers(winrt::make<ConptyConnection>(signal, in, out, ref, server, client, startupInfo));
-
+        auto conn = winrt::make_self<ConptyConnection>();
+        conn->InitializeFromHandoff(in, out, signal, reference, server, client, startupInfo);
+        _newConnectionHandlers(*std::move(conn));
         return S_OK;
     }
     CATCH_RETURN()

src/cascadia/TerminalConnection/ConptyConnection.h

@@ -13,16 +13,9 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
 {
     struct ConptyConnection : ConptyConnectionT<ConptyConnection>, BaseTerminalConnection<ConptyConnection>
     {
-        ConptyConnection(const HANDLE hSig,
-                         const HANDLE hIn,
-                         const HANDLE hOut,
-                         const HANDLE hRef,
-                         const HANDLE hServerProcess,
-                         const HANDLE hClientProcess,
-                         const TERMINAL_STARTUP_INFO& startupInfo);
-
-        ConptyConnection() noexcept = default;
+        explicit ConptyConnection();
         void Initialize(const Windows::Foundation::Collections::ValueSet& settings);
+        void InitializeFromHandoff(HANDLE* in, HANDLE* out, HANDLE signal, HANDLE reference, HANDLE server, HANDLE client, const TERMINAL_STARTUP_INFO* startupInfo);
 
         static winrt::fire_and_forget final_release(std::unique_ptr<ConptyConnection> connection);
 
@@ -61,15 +54,15 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
 
     private:
         static void closePseudoConsoleAsync(HPCON hPC) noexcept;
-        static HRESULT NewHandoff(HANDLE in, HANDLE out, HANDLE signal, HANDLE ref, HANDLE server, HANDLE client, TERMINAL_STARTUP_INFO startupInfo) noexcept;
+        static HRESULT NewHandoff(HANDLE* in, HANDLE* out, HANDLE signal, HANDLE reference, HANDLE server, HANDLE client, const TERMINAL_STARTUP_INFO* startupInfo) noexcept;
         static winrt::hstring _commandlineFromProcess(HANDLE process);
 
         HRESULT _LaunchAttachedClient() noexcept;
         void _indicateExitWithStatus(unsigned int status) noexcept;
         void _LastConPtyClientDisconnected() noexcept;
 
-        til::CoordType _rows{};
-        til::CoordType _cols{};
+        til::CoordType _rows = 120;
+        til::CoordType _cols = 30;
         uint64_t _initialParentHwnd{ 0 };
         hstring _commandline{};
         hstring _startingDirectory{};
@@ -87,9 +80,6 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
         wil::unique_process_information _piClient;
         wil::unique_any<HPCON, decltype(closePseudoConsoleAsync), closePseudoConsoleAsync> _hPC;
 
-        til::u8state _u8State{};
-        std::wstring _u16Str{};
-        std::array<char, 4096> _buffer{};
         DWORD _flags{ 0 };
 
         til::env _initialEnv{};

src/cascadia/UnitTests_Remoting/RemotingTests.cpp

@@ -2586,7 +2586,6 @@ namespace RemotingUnitTests
             try
             {
                 const auto result = m1->ProposeCommandline(args);
-                VERIFY_IS_FALSE(true, L"This should have thrown");
             }
             catch (const winrt::hresult_error& e)
             {
@@ -2596,9 +2595,10 @@ namespace RemotingUnitTests
 
                 // This is the same check in WindowManager::_proposeToMonarch.
                 VERIFY_IS_TRUE(e.code() == RPC_SERVER_UNAVAILABLE_HR || e.code() == RPC_CALL_FAILED_HR);
+                return;
             }
-            // just don't catch other types of exceptions. They'll take out
-            // TAEF, which will count as a failure.
+
+            VERIFY_FAIL(L"This should have thrown");
         }
     }
 

src/host/proxy/IConsoleHandoff.idl

@@ -1,8 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-import "oaidl.idl";
-import "ocidl.idl";
+import "unknwn.idl";
 
 typedef struct _CONSOLE_PORTABLE_ATTACH_MSG
 {