Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ConPTY: Fix shutdown if killed during startup #18588

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ConPTY: Fix shutdown if killed during startup #18588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 89 additions & 100 deletions src/host/VtIo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,11 @@ using namespace Microsoft::Console::Interactivity;
const HANDLE OutHandle,
_In_opt_ const HANDLE SignalHandle)
{
FAIL_FAST_IF_MSG(_initialized, "Someone attempted to double-_Initialize VtIo");
if (_state != State::Uninitialized)
{
assert(false); // Don't call initialize twice.
return E_UNEXPECTED;
}

_hInput.reset(InHandle);
_hOutput.reset(OutHandle);
Expand All @@ -95,47 +99,33 @@ using namespace Microsoft::Console::Interactivity;
}
}

// The only way we're initialized is if the args said we're in conpty mode.
// If the args say so, then at least one of in, out, or signal was specified
_initialized = true;
return S_OK;
}

// Method Description:
// - Create the VtEngine and the VtInputThread for this console.
// MUST BE DONE AFTER CONSOLE IS INITIALIZED, to make sure we've gotten the
// buffer size from the attached client application.
// Arguments:
// - <none>
// Return Value:
// S_OK if we initialized successfully,
// S_FALSE if VtIo hasn't been initialized (or we're not in conpty mode)
// otherwise an appropriate HRESULT indicating failure.
[[nodiscard]] HRESULT VtIo::CreateIoHandlers() noexcept
{
if (!_initialized)
{
return S_FALSE;
}

// SetWindowVisibility uses the console lock to protect access to _pVtRenderEngine.
assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked());

try
// - Create and start the signal thread. The signal thread can be created
// independent of the i/o threads, and doesn't require a client first
// attaching to the console. We need to create it first and foremost,
// because it's possible that a terminal application could
// CreatePseudoConsole, then ClosePseudoConsole without ever attaching a
// client. Should that happen, we still need to exit.
if (IsValidHandle(_hSignal.get()))
{
if (IsValidHandle(_hInput.get()))
try
{
_pVtInputThread = std::make_unique<VtInputThread>(std::move(_hInput), _lookingForCursorPosition);
_pPtySignalInputThread = std::make_unique<PtySignalInputThread>(std::move(_hSignal));

// Start it if it was successfully created.
RETURN_IF_FAILED(_pPtySignalInputThread->Start());
}
CATCH_RETURN();
}
CATCH_RETURN();

// The only way we're initialized is if the args said we're in conpty mode.
// If the args say so, then at least one of in, out, or signal was specified
_state = State::Initialized;
return S_OK;
}

bool VtIo::IsUsingVt() const
{
return _initialized;
return _state != State::Uninitialized;
}

// Routine Description:
Expand All @@ -151,50 +141,64 @@ bool VtIo::IsUsingVt() const
[[nodiscard]] HRESULT VtIo::StartIfNeeded()
{
// If we haven't been set up, do nothing (because there's nothing to start)
if (!_initialized)
if (_state != State::Initialized)
{
return S_FALSE;
}

if (_pVtInputThread)
_state = State::Starting;

// SetWindowVisibility uses the console lock to protect access to _pVtRenderEngine.
assert(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked());

try
{
LOG_IF_FAILED(_pVtInputThread->Start());
if (IsValidHandle(_hInput.get()))
{
_pVtInputThread = std::make_unique<VtInputThread>(std::move(_hInput), _lookingForCursorPosition);
}
}
CATCH_RETURN();

if (_pVtInputThread)
{
Writer writer{ this };
LOG_IF_FAILED(_pVtInputThread->Start());

// MSFT: 15813316
// If the terminal application wants us to inherit the cursor position,
// we're going to emit a VT sequence to ask for the cursor position.
// If we get a response, the InteractDispatch will call SetCursorPosition,
// which will call to our VtIo::SetCursorPosition method.
//
// By sending the request before sending the DA1 one, we can simply
// wait for the DA1 response below and effectively wait for both.
if (_lookingForCursorPosition)
{
writer.WriteUTF8("\x1b[6n"); // Cursor Position Report (DSR CPR)
}

// GH#4999 - Send a sequence to the connected terminal to request
// win32-input-mode from them. This will enable the connected terminal to
// send us full INPUT_RECORDs as input. If the terminal doesn't understand
// this sequence, it'll just ignore it.
writer.WriteUTF8(
"\x1b[c" // DA1 Report (Primary Device Attributes)
"\x1b[?1004h" // Focus Event Mode
"\x1b[?9001h" // Win32 Input Mode
);
Writer writer{ this };

// MSFT: 15813316
// If the terminal application wants us to inherit the cursor position,
// we're going to emit a VT sequence to ask for the cursor position.
// If we get a response, the InteractDispatch will call SetCursorPosition,
// which will call to our VtIo::SetCursorPosition method.
//
// By sending the request before sending the DA1 one, we can simply
// wait for the DA1 response below and effectively wait for both.
if (_lookingForCursorPosition)
{
writer.WriteUTF8("\x1b[6n"); // Cursor Position Report (DSR CPR)
}

writer.Submit();
}
// GH#4999 - Send a sequence to the connected terminal to request
// win32-input-mode from them. This will enable the connected terminal to
// send us full INPUT_RECORDs as input. If the terminal doesn't understand
// this sequence, it'll just ignore it.
writer.WriteUTF8(
"\x1b[c" // DA1 Report (Primary Device Attributes)
"\x1b[?1004h" // Focus Event Mode
"\x1b[?9001h" // Win32 Input Mode
);

writer.Submit();
}

{
// Allow the input thread to momentarily gain the console lock.
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
const auto suspension = gci.SuspendLock();
_deviceAttributes = _pVtInputThread->WaitUntilDA1(3000);
{
// Allow the input thread to momentarily gain the console lock.
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
const auto suspension = gci.SuspendLock();
_deviceAttributes = _pVtInputThread->WaitUntilDA1(3000);
}
}

if (_pPtySignalInputThread)
Expand All @@ -208,6 +212,17 @@ bool VtIo::IsUsingVt() const
_pPtySignalInputThread->ConnectConsole();
}

if (_state != State::Starting)
{
// Here's where we _could_ call CloseConsoleProcessState(), but this function
// only gets get called once when the first client connects and CONSOLE_INITIALIZED
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// only gets get called once when the first client connects and CONSOLE_INITIALIZED
// only gets called once when the first client connects and CONSOLE_INITIALIZED

// is not set yet. The process list may already contain that first client,
// but since it hasn't finished connecting yet, it won't react to a CTRL_CLOSE_EVENT.
// Instead, we return an error here which will abort the connection setup.
return E_FAIL;
}

_state = State::Running;
return S_OK;
}

Expand Down Expand Up @@ -244,47 +259,21 @@ void VtIo::CreatePseudoWindow()
}
}

// Method Description:
// - Create and start the signal thread. The signal thread can be created
// independent of the i/o threads, and doesn't require a client first
// attaching to the console. We need to create it first and foremost,
// because it's possible that a terminal application could
// CreatePseudoConsole, then ClosePseudoConsole without ever attaching a
// client. Should that happen, we still need to exit.
// Arguments:
// - <none>
// Return Value:
// - S_FALSE if we're not in VtIo mode,
// S_OK if we succeeded,
// otherwise an appropriate HRESULT indicating failure.
[[nodiscard]] HRESULT VtIo::CreateAndStartSignalThread() noexcept
{
if (!_initialized)
{
return S_FALSE;
}

// If we were passed a signal handle, try to open it and make a signal reading thread.
if (IsValidHandle(_hSignal.get()))
{
try
{
_pPtySignalInputThread = std::make_unique<PtySignalInputThread>(std::move(_hSignal));

// Start it if it was successfully created.
RETURN_IF_FAILED(_pPtySignalInputThread->Start());
}
CATCH_RETURN();
}

return S_OK;
}

void VtIo::SendCloseEvent()
{
LockConsole();
const auto unlock = wil::scope_exit([] { UnlockConsole(); });

// If we're still in the process of starting up, and we're asked to shut down
// (broken pipe), `VtIo::StartIfNeeded()` will handle the cleanup for us.
// This can happen during the call to `WaitUntilDA1`, because we relinquish
// ownership of the console lock.
if (_state == State::Starting)
{
_state = State::StartupFailed;
return;
}

// This function is called when the ConPTY signal pipe is closed (PtySignalInputThread) and when the input
// pipe is closed (VtIo). Usually these two happen at about the same time. This if condition is a bit of
// a premature optimization and prevents us from sending out a CTRL_CLOSE_EVENT right after another.
Expand Down
15 changes: 11 additions & 4 deletions src/host/VtIo.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,6 @@ namespace Microsoft::Console::VirtualTerminal
static wchar_t SanitizeUCS2(wchar_t ch);

[[nodiscard]] HRESULT Initialize(const ConsoleArguments* const pArgs);
[[nodiscard]] HRESULT CreateAndStartSignalThread() noexcept;
[[nodiscard]] HRESULT CreateIoHandlers() noexcept;

bool IsUsingVt() const;
[[nodiscard]] HRESULT StartIfNeeded();
Expand All @@ -69,6 +67,15 @@ namespace Microsoft::Console::VirtualTerminal
void CreatePseudoWindow();

private:
enum class State : uint8_t
{
Uninitialized,
Initialized,
Starting,
StartupFailed,
Running,
};

[[nodiscard]] HRESULT _Initialize(const HANDLE InHandle, const HANDLE OutHandle, _In_opt_ const HANDLE SignalHandle);

void _uncork();
Expand All @@ -77,7 +84,7 @@ namespace Microsoft::Console::VirtualTerminal
// After CreateIoHandlers is called, these will be invalid.
wil::unique_hfile _hInput;
wil::unique_hfile _hOutput;
// After CreateAndStartSignalThread is called, this will be invalid.
// After Initialize is called, this will be invalid.
wil::unique_hfile _hSignal;

std::unique_ptr<Microsoft::Console::VtInputThread> _pVtInputThread;
Expand All @@ -96,7 +103,7 @@ namespace Microsoft::Console::VirtualTerminal
bool _writerRestoreCursor = false;
bool _writerTainted = false;

bool _initialized = false;
State _state = State::Uninitialized;
bool _lookingForCursorPosition = false;
bool _closeEventSent = false;
int _corked = 0;
Expand Down
27 changes: 5 additions & 22 deletions src/host/srvinit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,7 +381,6 @@ HRESULT ConsoleCreateIoThread(_In_ HANDLE Server,
// can start, so they're started below, in ConsoleAllocateConsole
auto& gci = g.getConsoleInformation();
RETURN_IF_FAILED(gci.GetVtIo()->Initialize(args));
RETURN_IF_FAILED(gci.GetVtIo()->CreateAndStartSignalThread());

return S_OK;
}
Expand Down Expand Up @@ -945,27 +944,11 @@ PWSTR TranslateConsoleTitle(_In_ PCWSTR pwszConsoleTitle, const BOOL fUnexpand,
// We'll need the size of the screen buffer in the vt i/o initialization
if (SUCCEEDED_NTSTATUS(Status))
{
auto hr = gci.GetVtIo()->CreateIoHandlers();
if (hr == S_FALSE)
{
// We're not in VT I/O mode, this is fine.
}
else if (SUCCEEDED(hr))
{
// Actually start the VT I/O threads
hr = gci.GetVtIo()->StartIfNeeded();
// Don't convert S_FALSE to an NTSTATUS - the equivalent NTSTATUS
// is treated as an error
if (hr != S_FALSE)
{
Status = NTSTATUS_FROM_HRESULT(hr);
}
else
{
Status = ERROR_SUCCESS;
}
}
else
// Actually start the VT I/O threads
auto hr = gci.GetVtIo()->StartIfNeeded();
// Don't convert S_FALSE to an NTSTATUS - the equivalent NTSTATUS
// is treated as an error
if (FAILED(hr))
{
Status = NTSTATUS_FROM_HRESULT(hr);
}
Expand Down