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

Jump to bottom

Fix fail-fast due to unlocked FreeProcessData call #12599

Merged
1 commit merged into from
Mar 1, 2022
Merged

Fix fail-fast due to unlocked FreeProcessData call #12599

1 commit merged into from
Mar 1, 2022

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Feb 28, 2022 •
edited
Loading

2b202ce introduced a bug, where FreeProcessData was called without the console
lock being held. The previous code can be found in 40e3dea, on line 441-454.

PR Checklist

Validation Steps Performed

None, as this fix is purely theoretic, but it matches the stack trace
and 40e3dea clearly wasn't correctly ported to strict C++ either.

@lhecker lhecker added Product-Conhost For issues in the Console codebase Issue-Bug It either shouldn't be doing this or needs an investigation. Severity-Crash Crashes are real bad news. Priority-0 Bugs that we consider release-blocking/recall-class (P0) labels Feb 28, 2022
@lhecker lhecker requested a review from miniksa February 28, 2022 19:41
@lhecker lhecker force-pushed the dev/lhecker/fix-connection-fail-fast branch from 4c8603d to bc01b34 Compare February 28, 2022 19:43
lhecker
lhecker commented Feb 28, 2022
View reviewed changes
src/server/IoDispatchers.cpp
Comment on lines +300 to +302

// FreeProcessData() above requires the console to be locked.
UnlockConsole();
Copy link
Member Author

@lhecker lhecker Feb 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For reviewers, here's the original code again:

terminal/src/server/IoDispatchers.cpp

Lines 441 to 454 in 40e3dea

Error:
FAIL_FAST_IF(NT_SUCCESS(Status));
if (ProcessData != nullptr)
{
CommandHistory::s_Free((HANDLE)ProcessData);
gci.ProcessHandleList.FreeProcessData(ProcessData);
}
UnlockConsole();
pReceiveMsg->SetReplyStatus(Status);
return pReceiveMsg;

This is the fail-fast:

terminal/src/server/ProcessList.cpp

Lines 86 to 88 in d0d42c4

void ConsoleProcessList::FreeProcessData(_In_ ConsoleProcessHandle* const pProcessData)
{
FAIL_FAST_IF(!(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()));

(SetReplyStatus is just a primitive setter and can be called under the console lock AFAICS.)

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI the stack trace is:

0:000> kc
 # Call Site
00 KERNELBASE!RaiseFailFastException
01 conhost!wil::details::WilDynamicLoadRaiseFailFastException
02 conhost!wil::details::WilRaiseFailFastException
03 conhost!wil::details::WilFailFast
04 conhost!wil::details::ReportFailure_NoReturn<3>
05 conhost!wil::details::ReportFailure_Base<3,0>
06 conhost!wil::details::ReportFailure_Hr<3>
07 conhost!wil::details::in1diag3::_FailFast_Unexpected
08 conhost!wil::details::in1diag3::FailFast_If
09 conhost!ConsoleProcessList::FreeProcessData
0a conhost!<lambda_f86d5fd29b665fa3f430c14358e0681a>::operator()
0b conhost!wil::details::lambda_call<<lambda_f86d5fd29b665fa3f430c14358e0681a> >::reset
0c conhost!wil::details::lambda_call<<lambda_f86d5fd29b665fa3f430c14358e0681a> >::~lambda_call<<lambda_f86d5fd29b665fa3f430c14358e0681a> >
0d conhost!IoDispatchers::ConsoleHandleConnectionRequest
0e conhost!IoSorter::ServiceIoOperation
0f conhost!ConsoleIoThread
10 kernel32!BaseThreadInitThunk
11 ntdll!RtlUserThreadStart

@miniksa miniksa added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. labels Mar 1, 2022
@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Mar 1, 2022
@ghost
Copy link

ghost commented Mar 1, 2022

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 79a08ec into main Mar 1, 2022
@ghost ghost deleted the dev/lhecker/fix-connection-fail-fast branch March 1, 2022 23:02
@miniksa
Copy link
Member

miniksa commented Mar 2, 2022

I think this might not need to go to 1.12 stable as it had a conflict when I cherry-picked that seemed to show 1.12 didn't have the original bug.

@miniksa
Copy link
Member

miniksa commented Mar 2, 2022

UGH probably needs an OS side fix.

@miniksa miniksa removed the zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. label Mar 3, 2022
@miniksa
Copy link
Member

miniksa commented Mar 3, 2022

Pulled to OS. Not in 1.12, untagging.

DHowett pushed a commit that referenced this pull request Mar 10, 2022
@lhecker @DHowett
Fix fail-fast due to unlocked FreeProcessData call (#12599)
9e960ce 
2b202ce introduced a bug, where FreeProcessData was called without the console
lock being held. The previous code can be found in 40e3dea, on line 441-454.

## PR Checklist
* [x] Closes MSFT:21372705
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed
None, as this fix is purely theoretic, but it matches the stack trace
and 40e3dea clearly wasn't correctly ported to strict C++ either.

(cherry picked from commit 79a08ec)
DHowett pushed a commit that referenced this pull request Mar 24, 2022
@lhecker @miniksa
Fix fail-fast due to unlocked FreeProcessData call (#12599)
34d81b1 
2b202ce introduced a bug, where FreeProcessData was called without the console
lock being held. The previous code can be found in 40e3dea, on line 441-454.

## PR Checklist
* [x] Closes MSFT:21372705
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed
None, as this fix is purely theoretic, but it matches the stack trace
and 40e3dea clearly wasn't correctly ported to strict C++ either.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miniksa miniksa miniksa approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

Assignees
No one assigned
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Conhost For issues in the Console codebase Severity-Crash Crashes are real bad news. zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lhecker @miniksa @zadjii-msft