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

Fix unbalanced unlock in PtySignalInputThread::_Shutdown #12181

Merged
5 commits merged into from
Jan 24, 2022
Merged

Fix unbalanced unlock in PtySignalInputThread::_Shutdown #12181

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7dedf50
Fix unbalanced unlock in PtySignalInputThread::_Shutdown
lhecker Jan 17, 2022
169a78d
Improve robustness of CloseConsoleProcessState
lhecker Jan 18, 2022
609497f
Fix audit checks
lhecker Jan 18, 2022
6fe9843
Simplify pull request
lhecker Jan 21, 2022
c12b27a
Fix build
lhecker Jan 21, 2022
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
3 changes: 1 addition & 2 deletions src/host/PtySignalInputThread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBu
if (lastError == ERROR_BROKEN_PIPE)
{
_Shutdown();
return false;
Copy link
Member

Choose a reason for hiding this comment

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

(okay sorry to nitpick, but all this code is terribly fragile)

why are we removing these return falses in _GetData? Aren't these needed to get the input threads to break out of their while loops?

Copy link
Member

Choose a reason for hiding this comment

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

Oh right, cause we'll ExitProcess in RundownAndExit in _Shutdown, that makes sense.

Copy link
Member Author

@lhecker lhecker Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

So here's the story behind this change:
Initially I tried to add some code after ServiceLocator::RundownAndExit(ERROR_BROKEN_PIPE);. This doesn't make any sense though, as RundownAndExit never returns, but neither the regular compilation nor the AuditMode marked this as an issue. If we use the [[noreturn]] annotation however we can turn this programming error into an immediate compilation warning. This is why this PR adds [[noreturn]].

But now that any such code results in a warning I was forced to remove this "unreachable code".

Do you agree with this change? If not, would you like me to revert it?

Copy link
Member

@zadjii-msft zadjii-msft Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

no yea no, this makes sense, more sense than before. Defs keep this.

}
else
{
Expand All @@ -180,7 +179,6 @@ bool PtySignalInputThread::_GetData(_Out_writes_bytes_(cbBuffer) void* const pBu
else if (dwRead != cbBuffer)
{
_Shutdown();
return false;
}

return true;
Expand Down Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

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

Why do we not have to do this anymore? IIRC there's a very bizarre edge case bug that can occur if we don't dispatch ctrl events before tearing down

Copy link
Member Author

@lhecker lhecker Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I believe this might be a copy-paste mistake...
This function is never called while the console is locked (see _GetData in the same file), so this can't possibly happen.

However I've just pushed a change which makes the entire code a lot more robust in general in my opinion. 🙂
Instead of "jiggling the handle" in CloseConsoleProcessState() by calling LockConsole(); UnlockConsole(); one after another, I simply call LockConsole(); ProcessCtrlEvents();, which ensures we always process ctrl events immediately. (FYI: ProcessCtrlEvents() calls UnlockConsole() internally.)

Copy link
Member

Choose a reason for hiding this comment

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

I'm definitely worried about making changes here. If it's a copy/paste error, we should make sure we understand when it was introduced and when it became an error. Pull up the team OneNote and use the guidance on importing pre-github history if you need to! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I found the original comments on the PR that added this function (!2698602):

Yeah, my point was that the two functions (PtySignalInputThread::_Shutdown and VtIo::_ShutdownIfNeeded) should probably have the same signature, one way or the other, because you fixed the jiggling the handle thing. Either one of them is double-processing ctrl events or the other is not processing them at all. They should be consistent.
[...]
Derp yea, actually I believe _ShutdownIfNeeded is the right one. For both of these, if they're called under lock, we don't want to RundownAndExit before ProcessCtrlEvents is called.

This issue has now been resolved by jiggling in a different way instead. 😅


// Make sure we terminate.
Expand Down
1 change: 1 addition & 0 deletions src/host/VtIo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,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.
Expand Down
2 changes: 1 addition & 1 deletion src/host/output.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Comment on lines 522 to +523
Copy link
Member Author

@lhecker lhecker Jan 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@zadjii-msft I've drastically simplified the PR now. In fact if we want to, we can just keep the two added LockConsole() lines. 🙂

Technically we can remove these two lines from CloseConsoleProcessState now, because the only place where CloseConsoleProcessState is called apart from these two places is in window.cpp, where it's done so under lock. As such these two lines serve no purpose anymore.

}
2 changes: 0 additions & 2 deletions src/interactivity/base/HostSignalInputThread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,6 @@ bool HostSignalInputThread::_GetData(gsl::span<gsl::byte> buffer)
if (lastError == ERROR_BROKEN_PIPE)
{
_Shutdown();
return false;
}

THROW_WIN32(lastError);
Expand All @@ -172,7 +171,6 @@ bool HostSignalInputThread::_GetData(gsl::span<gsl::byte> buffer)
if (bytesRead != buffer.size())
{
_Shutdown();
return false;
}

return true;
Expand Down
4 changes: 2 additions & 2 deletions src/interactivity/base/ServiceLocator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -67,7 +67,7 @@ void ServiceLocator::RundownAndExit(const HRESULT hr)
s_inputServices.reset(nullptr);
}

TerminateProcess(GetCurrentProcess(), hr);
ExitProcess(hr);
}

#pragma region Creation Methods
Expand Down
2 changes: 1 addition & 1 deletion src/interactivity/inc/ServiceLocator.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down