Repository: microsoft/terminal
File: src/cascadia/TerminalConnection/CTerminalHandoff.cpp (C++)

Code before:
// - ref - Client reference handle for console session so it stays alive until we let go
// - server - PTY process handle to track for lifetime/cleanup
// - client - Process handle to client so we can track its lifetime and exit appropriately
// Return Value:
// - 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)
{
    try
    {
        // Stash a local copy of _pfnHandoff before we stop listening.
        auto localPfnHandoff = _pfnHandoff;

        // Because we are REGCLS_SINGLEUSE... we need to `CoRevokeClassObject` after we handle this ONE call.
        // COM does not automatically clean that up for us. We must do it.
        s_StopListening();

        std::unique_lock lock{ _mtx };

        // 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));

#pragma warning(suppress : 26477)
        TraceLoggingWrite(
            g_hTerminalConnectionProvider,
            "ReceiveTerminalHandoff_Success",
            TraceLoggingDescription("successfully received a terminal handoff"),
            TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
            TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));

        return S_OK;
    }
    catch (...)
    {
        const auto hr = wil::ResultFromCaughtException();


Review comment (suggestion):
FYI this change is correct (i.e. holding the lock while calling `s_StopListening`).
Previously, `s_StopListening` would acquire the lock itself, so nothing has effectively changed here.

Code after:
//   a connection is on its way in.
// Arguments:
// - in - PTY input handle that we will read from
// - out - PTY output handle that we will write to
// - signal - PTY signal handle for out of band messaging
// - ref - Client reference handle for console session so it stays alive until we let go
// - server - PTY process handle to track for lifetime/cleanup
// - client - Process handle to client so we can track its lifetime and exit appropriately
// Return Value:
// - 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)
{
    try
    {
        std::unique_lock lock{ _mtx };

        // s_StopListeningLocked sets _pfnHandoff to nullptr.
        // localPfnHandoff is tested for nullness below.
#pragma warning(suppress : 26429) // Symbol '...' is never tested for nullness, it can be marked as not_null (f.23).
        auto localPfnHandoff = _pfnHandoff;

        // Because we are REGCLS_SINGLEUSE... we need to `CoRevokeClassObject` after we handle this ONE call.
        // COM does not automatically clean that up for us. We must do it.
        LOG_IF_FAILED(s_StopListeningLocked());

        // 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));

#pragma warning(suppress : 26477)
        TraceLoggingWrite(
            g_hTerminalConnectionProvider,
            "ReceiveTerminalHandoff_Success",
            TraceLoggingDescription("successfully received a terminal handoff"),
            TraceLoggingKeyword(MICROSOFT_KEYWORD_MEASURES),
            TelemetryPrivacyDataTag(PDT_ProductAndServiceUsage));