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

Audit FAIL_FASTs across the code #14298

Open
DHowett opened this issue Oct 27, 2022 · 3 comments
Open

Audit FAIL_FASTs across the code #14298

DHowett opened this issue Oct 27, 2022 · 3 comments
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase Severity-Crash Crashes are real bad news.

Comments

@DHowett
Copy link
Member

DHowett commented Oct 27, 2022 •
edited by lhecker
Loading

Back in 2018 we switched every NTASSERT/ASSERT/NT_ASSERT in these components to a FAIL_FAST, unequivocally, because if we cared enough to check it, we should care enough to die for it.
I think that’s good and valuable, and it has its place.

Now that these components are shared with Terminal (who seeks to host multiple copies of each of them in parallel), and now that a11y has taken a bigger bet on them performing sanely and safely,
I’m wondering if we should scale back a little bit on the take down the entire process if something lifts off into space stance.

We’re seeing a good number of crashes due to accessibility that I think should just be “oh, the API returned an error code, NVDA can recover”. We once saw reports that resizing the window when an unexpected DBCS half lands in the buffer (#4907) caused a crash.

They’re programming errors, for sure, but our users probably want things to be more reliable.

Should, or could, we surface those as exceptions and let the caller recover wherever possible?
Should we just internally recover?

Let's find out.

Generated as of 2022-10-27 ab04067

How did I generate this? 
rg -n FAIL_FAST_IF\`( --no-heading | % {
	$p=$_ -split ":";
	$pa=$p[0] -replace "\\","/";
	$ln=$p[1];
	[pscustomobject]@{File=$pa;Line=$ln}
} | group File | sort -Descending Count | % {
	"## $($_.Name)`n";
	$_.Group | % {
		"https://github.com/microsoft/terminal/blob/$(git rev-parse HEAD)/src/$($_.File)#L$($_.Line)"
	};
	"`n"
}

host/screenInfo.cpp

terminal/src/host/screenInfo.cpp

Line 193 in ab04067

FAIL_FAST_IF(!(gci.IsConsoleLocked()));

terminal/src/host/screenInfo.cpp

Lines 382 to 383 in ab04067

FAIL_FAST_IF(coordFontSize.X == 0);
FAIL_FAST_IF(coordFontSize.Y == 0);

terminal/src/host/screenInfo.cpp

Lines 402 to 403 in ab04067

FAIL_FAST_IF(coordFont.X == 0);
FAIL_FAST_IF(coordFont.Y == 0);

terminal/src/host/screenInfo.cpp

Lines 421 to 422 in ab04067

FAIL_FAST_IF(coordFontSize.X == 0);
FAIL_FAST_IF(coordFontSize.Y == 0);

terminal/src/host/screenInfo.cpp

Lines 453 to 454 in ab04067

FAIL_FAST_IF(coordFontSize.X == 0);
FAIL_FAST_IF(coordFontSize.Y == 0);

terminal/src/host/screenInfo.cpp

Lines 472 to 473 in ab04067

FAIL_FAST_IF(coordFont.X == 0);
FAIL_FAST_IF(coordFont.Y == 0);

terminal/src/host/screenInfo.cpp

Line 600 in ab04067

FAIL_FAST_IF(!(sEndX < coordScreenBufferSize.X));

terminal/src/host/screenInfo.cpp

Lines 905 to 907 in ab04067

FAIL_FAST_IF(!(_viewport.Top() >= 0));
// TODO MSFT: 17663344 - Audit call sites for this precondition. Extremely tiny offscreen windows.
//FAIL_FAST_IF(!(_viewport.IsValid()));

terminal/src/host/screenInfo.cpp

Line 1161 in ab04067

FAIL_FAST_IF(!(DeltaY <= 0));

host/cmdline.cpp

terminal/src/host/cmdline.cpp

Line 284 in ab04067

FAIL_FAST_IF(!(cookedReadData.BufferStartPtr() == cookedReadData.BufferCurrentPtr()));

terminal/src/host/cmdline.cpp

Line 448 in ab04067

FAIL_FAST_IF(!(cookedReadData.BufferStartPtr() == cookedReadData.BufferCurrentPtr()));

terminal/src/host/cmdline.cpp

Line 483 in ab04067

FAIL_FAST_IF(!(cookedReadData.BufferStartPtr() == cookedReadData.BufferCurrentPtr()));

terminal/src/host/cmdline.cpp

Line 519 in ab04067

FAIL_FAST_IF(!(cookedReadData.BufferStartPtr() == cookedReadData.BufferCurrentPtr()));

terminal/src/host/cmdline.cpp

Line 651 in ab04067

FAIL_FAST_IF(!(LastWord > cookedReadData.BufferStartPtr()));

terminal/src/host/cmdline.cpp

Line 665 in ab04067

FAIL_FAST_IF(!(LastWord > cookedReadData.BufferStartPtr()));

terminal/src/host/cmdline.cpp

Line 677 in ab04067

FAIL_FAST_IF(!(LastWord > cookedReadData.BufferStartPtr()));

terminal/src/host/cmdline.cpp

Line 685 in ab04067

FAIL_FAST_IF(!(LastWord >= cookedReadData.BufferStartPtr()));

terminal/src/host/cmdline.cpp

Line 766 in ab04067

FAIL_FAST_IF(!(NextWord < BufLast));

terminal/src/host/cmdline.cpp

Line 1000 in ab04067

FAIL_FAST_IF(!(cookedReadData.BufferStartPtr() == cookedReadData.BufferCurrentPtr()));

host/_stream.cpp

terminal/src/host/_stream.cpp

Line 252 in ab04067

FAIL_FAST_IF(!(coordCursor.Y == bufferSize.Y));

terminal/src/host/_stream.cpp

Line 429 in ab04067

FAIL_FAST_IF(!(WI_IsFlagSet(screenInfo.OutputMode, ENABLE_PROCESSED_OUTPUT)));

terminal/src/host/_stream.cpp

Line 608 in ab04067

FAIL_FAST_IF(!(WI_IsFlagSet(screenInfo.OutputMode, ENABLE_PROCESSED_OUTPUT)));

terminal/src/host/_stream.cpp

Line 664 in ab04067

FAIL_FAST_IF(!(Tmp2 >= buffer.get()));

terminal/src/host/_stream.cpp

Lines 974 to 979 in ab04067

FAIL_FAST_IF(!(WI_IsFlagSet(screenInfo.OutputMode, ENABLE_PROCESSED_OUTPUT)));
FAIL_FAST_IF(!(WI_IsFlagSet(screenInfo.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING)));
// defined down in the WriteBuffer default case hiding on the other end of the state machine. See outputStream.cpp
// This is the only mode used by DoWriteConsole.
FAIL_FAST_IF(!(WI_IsFlagSet(dwFlags, WC_LIMIT_BACKSPACE)));

terminal/src/host/_stream.cpp

Line 1187 in ab04067

FAIL_FAST_IF(wFromComplemented.size() != 1);

host/inputBuffer.cpp

terminal/src/host/inputBuffer.cpp

Line 376 in ab04067

FAIL_FAST_IF(streamRead && readCount != 1);

terminal/src/host/inputBuffer.cpp

Lines 513 to 518 in ab04067

FAIL_FAST_IF(!(unusedWaitStatus));
// write all previously existing records
size_t existingEventsWritten;
_WriteBuffer(existingStorage, existingEventsWritten, unusedWaitStatus);
FAIL_FAST_IF(!(!unusedWaitStatus));

terminal/src/host/inputBuffer.cpp

Lines 717 to 718 in ab04067

FAIL_FAST_IF(!(inEvents.size() == 1));
FAIL_FAST_IF(_storage.empty());

terminal/src/host/inputBuffer.cpp

Line 784 in ab04067

FAIL_FAST_IF(!(inEvents.size() == 1));

terminal/src/host/inputBuffer.cpp

Line 785 in ab04067

FAIL_FAST_IF(_storage.empty());

propsheet/fontdlg.cpp

terminal/src/propsheet/fontdlg.cpp

Line 704 in ab04067

FAIL_FAST_IF(!(OEMCP != 0)); // must be initialized

terminal/src/propsheet/fontdlg.cpp

Line 891 in ab04067

FAIL_FAST_IF(!(i == LB_ERR || (ULONG)i < NumberOfFonts));

terminal/src/propsheet/fontdlg.cpp

Line 1143 in ab04067

FAIL_FAST_IF(!(OEMCP != 0));

terminal/src/propsheet/fontdlg.cpp

Line 1355 in ab04067

FAIL_FAST_IF(!(FontIndex < (int)NumberOfFonts));

terminal/src/propsheet/fontdlg.cpp

Line 1498 in ab04067

FAIL_FAST_IF(!((ULONG)nFont < NumberOfFonts));

terminal/src/propsheet/fontdlg.cpp

Line 1590 in ab04067

FAIL_FAST_IF(!((ULONG)FontIndex < NumberOfFonts));

host/utils.cpp

terminal/src/host/utils.cpp

Lines 176 to 179 in ab04067

FAIL_FAST_IF(!(coordFirst.X >= 0 && coordFirst.X < cRowWidth));
FAIL_FAST_IF(!(coordSecond.X >= 0 && coordSecond.X < cRowWidth));
FAIL_FAST_IF(!(coordFirst.Y >= 0 && coordFirst.Y < cRowHeight));
FAIL_FAST_IF(!(coordSecond.Y >= 0 && coordSecond.Y < cRowHeight));

terminal/src/host/utils.cpp

Lines 231 to 232 in ab04067

FAIL_FAST_IF(!(coordCorner.X == srRectangle.Left || coordCorner.X == srRectangle.Right));
FAIL_FAST_IF(!(coordCorner.Y == srRectangle.Top || coordCorner.Y == srRectangle.Bottom));

interactivity/win32/icon.cpp

terminal/src/interactivity/win32/icon.cpp

Lines 238 to 241 in ab04067

FAIL_FAST_IF(!(&hIconRef == &_hIcon || &hIconRef == &_hSmIcon));
// expecting hDefaultIconRef to be pointing to either the regular or small default handles
FAIL_FAST_IF(!(&hDefaultIconRef == &_hDefaultIcon || &hDefaultIconRef == &_hDefaultSmIcon));

terminal/src/interactivity/win32/icon.cpp

Line 265 in ab04067

FAIL_FAST_IF(!(&hIconRef == &_hDefaultIcon || &hIconRef == &_hDefaultSmIcon));

terminal/src/interactivity/win32/icon.cpp

Line 288 in ab04067

FAIL_FAST_IF(!(&hIconRef == &_hIcon || &hIconRef == &_hSmIcon));

terminal/src/interactivity/win32/icon.cpp

Line 320 in ab04067

FAIL_FAST_IF(!(&hIconRef == &_hIcon || &hIconRef == &_hSmIcon));

host/readDataCooked.cpp

terminal/src/host/readDataCooked.cpp

Line 323 in ab04067

FAIL_FAST_IF(!gci.IsConsoleLocked());

terminal/src/host/readDataCooked.cpp

Lines 330 to 333 in ab04067

FAIL_FAST_IF(_pInputReadHandleData->IsInputPending());
// this routine should be called by a thread owning the same lock on the same console as we're reading from.
FAIL_FAST_IF(_pInputReadHandleData->GetReadCount() == 0);

terminal/src/host/readDataCooked.cpp

Line 1079 in ab04067

FAIL_FAST_IF(!(Tmp < (_backupLimit + _bytesRead)));

host/settings.cpp

terminal/src/host/settings.cpp

Lines 355 to 358 in ab04067

FAIL_FAST_IF(!(_dwWindowSize.X > 0));
FAIL_FAST_IF(!(_dwWindowSize.Y > 0));
FAIL_FAST_IF(!(_dwScreenBufferSize.X > 0));
FAIL_FAST_IF(!(_dwScreenBufferSize.Y > 0));

host/history.cpp

terminal/src/host/history.cpp

Line 41 in ab04067

FAIL_FAST_IF(WI_IsFlagClear(historyList.Flags, CLE_ALLOCATED));

terminal/src/host/history.cpp

Line 66 in ab04067

FAIL_FAST_IF(commands > SHORT_MAX);

terminal/src/host/history.cpp

Line 92 in ab04067

FAIL_FAST_IF(WI_IsFlagClear(Flags, CLE_ALLOCATED));

terminal/src/host/history.cpp

Line 197 in ab04067

FAIL_FAST_IF(!(WI_IsFlagSet(Flags, CLE_ALLOCATED)));

host/srvinit.cpp

terminal/src/host/srvinit.cpp

Lines 827 to 829 in ab04067

FAIL_FAST_IF(!(sizeof(Cac->AppName) == sizeof(Data.ApplicationName)));
FAIL_FAST_IF(!(sizeof(Cac->Title) == sizeof(Data.Title)));
FAIL_FAST_IF(!(sizeof(Cac->CurDir) == sizeof(Data.CurrentDirectory)));

host/misc.cpp

terminal/src/host/misc.cpp

Line 25 in ab04067

FAIL_FAST_IF(!(IsDBCSLeadByteConsole(*pch, &gci.OutputCPInfo) || cch == 1));

terminal/src/host/misc.cpp

Line 245 in ab04067

FAIL_FAST_IF(!(pwchSource != (LPWSTR)pchTarget));

terminal/src/host/misc.cpp

Line 272 in ab04067

FAIL_FAST_IF(!(cchTarget > 0));

interactivity/win32/window.cpp

terminal/src/interactivity/win32/window.cpp

Line 129 in ab04067

FAIL_FAST_IF(!(s_atomWindowClass == 0));

terminal/src/interactivity/win32/window.cpp

Line 531 in ab04067

FAIL_FAST_IF(!((fInKeyboardMarkMode && !fInMouseSelectMode && !fInScrollMode) ||

terminal/src/interactivity/win32/window.cpp

Line 638 in ab04067

FAIL_FAST_IF(!(rectSizeTemp.top == 0 && rectSizeTemp.left == 0));

host/readDataDirect.cpp

terminal/src/host/readDataDirect.cpp

Line 74 in ab04067

FAIL_FAST_IF(_pInputReadHandleData->GetReadCount() == 0);

terminal/src/host/readDataDirect.cpp

Line 77 in ab04067

FAIL_FAST_IF(!gci.IsConsoleLocked());

terminal/src/host/readDataDirect.cpp

Line 181 in ab04067

FAIL_FAST_IF(!(readEvents.empty()));

server/ProcessList.cpp

terminal/src/server/ProcessList.cpp

Line 36 in ab04067

FAIL_FAST_IF(!(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()));

terminal/src/server/ProcessList.cpp

Line 88 in ab04067

FAIL_FAST_IF(!(ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()));

terminal/src/server/ProcessList.cpp

Line 91 in ab04067

FAIL_FAST_IF(!(_processes.cend() != std::find(_processes.cbegin(), _processes.cend(), pProcessData)));

host/selectionInput.cpp

terminal/src/host/selectionInput.cpp

Line 25 in ab04067

FAIL_FAST_IF(!IsInSelectingState());

terminal/src/host/selectionInput.cpp

Line 271 in ab04067

FAIL_FAST_IF(!fMoveSucceeded); // we should never fail to move forward after having moved backward

terminal/src/host/selectionInput.cpp

Line 336 in ab04067

FAIL_FAST_IF(!bufferSize.IsInBounds(coordSelPoint));

tsf/TfEditSession.cpp

terminal/src/tsf/TfEditSession.cpp

Line 70 in ab04067

FAIL_FAST_IF(!(cr >= 0));

terminal/src/tsf/TfEditSession.cpp

Line 693 in ab04067

FAIL_FAST_IF(!((cchCompleted > 0) && (cchCompleted < cch)));

terminal/src/tsf/TfEditSession.cpp

Line 1030 in ab04067

FAIL_FAST_IF(!(lTextLength > 0));

server/ObjectHandle.cpp

terminal/src/server/ObjectHandle.cpp

Line 228 in ab04067

FAIL_FAST_IF(!(_IsInput()));

terminal/src/server/ObjectHandle.cpp

Line 242 in ab04067

FAIL_FAST_IF(pReadHandleData->GetReadCount() > 0);

terminal/src/server/ObjectHandle.cpp

Line 267 in ab04067

FAIL_FAST_IF(!(_IsOutput()));

host/CommandListPopup.cpp

terminal/src/host/CommandListPopup.cpp

Line 58 in ab04067

FAIL_FAST_IF(_currentCommand < 0);

terminal/src/host/CommandListPopup.cpp

Line 244 in ab04067

FAIL_FAST_IF(!(Tmp < (cookedReadData.BufferStartPtr() + cookedReadData.BytesRead())));

interactivity/win32/windowio.cpp

terminal/src/interactivity/win32/windowio.cpp

Line 69 in ab04067

FAIL_FAST_IF(!(gci.IsConsoleLocked()));

terminal/src/interactivity/win32/windowio.cpp

Line 992 in ab04067

FAIL_FAST_IF(!(ProcessData != nullptr && ProcessData->fRootProcess));

host/conimeinfo.cpp

terminal/src/host/conimeinfo.cpp

Line 338 in ab04067

FAIL_FAST_IF(size <= 0); // It's a programming error to have <= 0 cells to insert.

terminal/src/host/conimeinfo.cpp

Line 418 in ab04067

FAIL_FAST_IF(text.size() != attributes.size());

host/input.cpp

terminal/src/host/input.cpp

Line 199 in ab04067

FAIL_FAST_IF(EventsWritten != 1);

terminal/src/host/input.cpp

Line 276 in ab04067

FAIL_FAST_IF(!(!((CtrlFlags & (CONSOLE_CTRL_CLOSE_FLAG | CONSOLE_CTRL_BREAK_FLAG | CONSOLE_CTRL_C_FLAG)) && (CtrlFlags & (CONSOLE_CTRL_LOGOFF_FLAG | CONSOLE_CTRL_SHUTDOWN_FLAG)))));

host/consoleInformation.cpp

terminal/src/host/consoleInformation.cpp

Line 72 in ab04067

FAIL_FAST_IF(rc == 0);

terminal/src/host/consoleInformation.cpp

Line 84 in ab04067

FAIL_FAST_IF(rc == ULONG_MAX);

host/readDataRaw.cpp

terminal/src/host/readDataRaw.cpp

Lines 77 to 79 in ab04067

FAIL_FAST_IF(_pInputReadHandleData->GetReadCount() == 0);
FAIL_FAST_IF(!Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked());

tsf/TfDispAttr.cpp

terminal/src/tsf/TfDispAttr.cpp

Line 102 in ab04067

FAIL_FAST_IF(!(var.vt == VT_UNKNOWN));

terminal/src/tsf/TfDispAttr.cpp

Line 115 in ab04067

FAIL_FAST_IF(!(tfPropVal.varValue.vt == VT_I4)); // expecting GUIDATOMs

host/inputReadHandleData.cpp

terminal/src/host/inputReadHandleData.cpp

Line 64 in ab04067

FAIL_FAST_IF(prevCount == 0); // we just underflowed, that's a programming error.

host/directio.cpp

terminal/src/host/directio.cpp

Line 196 in ab04067

FAIL_FAST_IF(!(readEvents.empty()));

terminal/src/host/directio.cpp

Line 239 in ab04067

FAIL_FAST_IF(!(readEvents.empty()));

host/ConsoleArguments.cpp

terminal/src/host/ConsoleArguments.cpp

Line 351 in ab04067

FAIL_FAST_IF(!(CLIENT_COMMANDLINE_ARG == start->c_str()));

terminal/src/host/ConsoleArguments.cpp

Line 552 in ab04067

FAIL_FAST_IF(!args.empty());

propsheet/registry.cpp

terminal/src/propsheet/registry.cpp

Line 309 in ab04067

FAIL_FAST_IF(!(OEMCP != 0));

terminal/src/propsheet/registry.cpp

Line 781 in ab04067

FAIL_FAST_IF(!(OEMCP != 0));

server/WaitBlock.cpp

terminal/src/server/WaitBlock.cpp

Line 243 in ab04067

FAIL_FAST_IF(!(WI_IsFlagClear(TerminationReason, WaitTerminationReason::ThreadDying)));

host/writeData.cpp

terminal/src/host/writeData.cpp

Line 122 in ab04067

FAIL_FAST_IF(!(Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().getConsoleInformation().IsConsoleLocked()));

propsheet/misc.cpp

terminal/src/propsheet/misc.cpp

Line 470 in ab04067

FAIL_FAST_IF(!(ShouldAllowAllMonoTTFonts()));

interactivity/base/ServiceLocator.cpp

terminal/src/interactivity/base/ServiceLocator.cpp

Line 38 in ab04067

FAIL_FAST_IF(nullptr != s_oneCoreTeardownFunction);

host/stream.cpp

terminal/src/host/stream.cpp

Line 76 in ab04067

FAIL_FAST_IF(Wait);

host/dbcs.cpp

terminal/src/host/dbcs.cpp

Line 95 in ab04067

FAIL_FAST_IF(iDst != buffer.size());

server/IoSorter.cpp

terminal/src/server/IoSorter.cpp

Line 91 in ab04067

FAIL_FAST_IF(!(!ReplyPending));

types/sgrStack.cpp

terminal/src/types/sgrStack.cpp

Line 95 in ab04067

FAIL_FAST_IF(validParts.test(SgrSaveRestoreStackOptions::All));

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Oct 27, 2022
@DHowett DHowett added Severity-Crash Crashes are real bad news. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Oct 27, 2022
@carlos-zamora
Copy link
Member

now that a11y has taken a bigger bet on them

For the record, all of the a11y-related crashes were removed in #13250, I believe.

@carlos-zamora carlos-zamora added Product-Conhost For issues in the Console codebase and removed Needs-Tag-Fix Doesn't match tag requirements labels Oct 27, 2022
@DHowett
Copy link
Member Author

DHowett commented Nov 2, 2022

_stream.cpp:618 was removed in 86928bb.

@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 7, 2022
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Nov 7, 2022
@lhecker lhecker mentioned this issue Mar 14, 2023
Merged
@lhecker
Copy link
Member

lhecker commented Mar 14, 2023

Removed

terminal/src/host/inputReadHandleData.cpp

Line 23 in ab04067

FAIL_FAST_IF(!_isInputPending); // we shouldn't have multiline input without a pending input.
in anticipation for #14991.

@lhecker lhecker mentioned this issue Apr 12, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Needs-Tag-Fix Doesn't match tag requirements Product-Conhost For issues in the Console codebase Severity-Crash Crashes are real bad news.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@DHowett @lhecker @carlos-zamora @zadjii-msft