Windows Terminal proves to be terminal?

What do you need to control your OS using only the keyboard? Every developer has their own take on the answer—Microsoft's was to release Windows Terminal. But what's going on under its hood? It might be time to take a closer look and see whether this terminal is showing signs of a terminal condition.

Intro

Starting with Windows 11 Sun Valley 2 (version 22H2), Windows Terminal replaced Windows Console Host. However, Windows Terminal is also available for earlier versions of Windows—just not anything older than Windows 10 May 2020 Update (version 2004). The first thing that stands out compared to the traditional command-line shell is that it now comes with tabs and a new default font, Cascadia Mono. You'll also find this font included in the latest Visual Studio Code and Visual Studio 2022.

Claiming that Windows Terminal fully replaces conhost.exe was, admittedly, a bit of a bold move. Windows Terminal "upgrades" it much like Cybermen do—or assimilates it like the Borg, if you prefer—and in the end, this sleek modern-looking app with Direct3D and Quake mode turns out to be a well-disguised, rock-solid piece of legacy. If you're a ReactOS contributor and hesitate to click on the link, you can rest assured. The source code it leads to is safe for your eyes. Windows Console Host is released under the permissive MIT license, and there's even a proof of that.

Get cozy at your desk with your favorite virtual terminal (who knows, maybe you're reading this in a text-based browser). Either way, PVS-Studio is about to steal the show.

PVS-Studio installation

You can download the distribution kit from this page. You'll need a license to run the analyzer, but the process of getting a trial version won't tie up your hands on the keyboard and mouse for long. Installing the analyzer won't take much time either. You can also use PVS-Studio as a plugin in Visual Studio 2022. At the time of writing, the latest version of the analyzer is 7.37.

Since we're talking about a virtual terminal today, I better mention that PVS-Studio can be deployed in unattended mode with extra command-line parameters.

Build and analysis configuration

I analyzed the fc0a06c commit in the Release configuration. After checking CPython this way, I was eager to keep running more checks on typical end-user software. Or, as Fenris from Steel Hunters might put it: "I yet hunger... feed me another."

And once again, since we're talking about the virtual terminal, I'll show you two ways to run code analysis: from the command line using PVS-Studio_Cmd.exe and through GUI in Visual Studio 2022.

If you're using the command line, all necessary dependencies can be installed via the WinGet configuration file located in the .config folder. If you're on the Community edition, just use the command listed in README.md:

winget configure .config\configuration.winget

If you open a solution via Visual Studio, you'll see a window prompting you to install any workloads missing from your IDE. However, non-Visual Studio related components still need to be installed manually, and PowerShell 7 being one of them.

Issues found

When Windows Terminal is built, NuGet packages are restored, several vcpkg packages get downloaded, and auto-generated code is produced. All that noise needs to be kept out of the analyzer's touch to avoid any distractions from the actual component code. To do it, exclude from the analysis all files with paths matching the following patterns:

\terminal\oss\
\terminal\obj\*\vcpkg\
\terminal\packages\Microsoft.Windows.ImplementationLibrary*\
\Generated Files\

To exclude these paths when using PVS-Studio_Cmd, create a .pvsconfig configuration file and list them using the //V_EXCLUDE_PATH parameter. From here on, I'll refer to the created file as wt.pvsconfig. You can find it in the terminal folder, located in the root directory of the Windows Terminal source code.

To exclude paths in the Visual Studio 2022 plugin, navigate to Extensions > PVS-Studio > Options > Don't Check Files. There, you can either add the paths directly to the PathMasks section, or simply add the wt.pvsconfig file to the solution (the default shortcut is Shift+Alt+A)—that way, you won't need to modify the analyzer settings manually. Now, I wouldn't take up your time listing every possible way to use the config file—the docs have it covered :)

Let's run the analysis. Running analysis on MSBuild projects using the PVS-Studio_Cmd.exe command-line utility looks like this:

PVS-Studio_Cmd.exe -t D:\Git\terminal\OpenConsole.sln ^
                   -c Release -p x64 ^
                   -C D:\Git\terminal\wt.pvsconfig

In Visual Studio:

Upon completion, PVS-Studio_Cmd.exe saves the report file in .plog format to the same directory as the target project or solution specified by the -t parameter. To change the report file location and name, use the -o parameter. The report can also be save in .plog and .json formats. For details on how to work with the report, please refer to our documentation. Once the analysis is done, the Visual Studio plugin automatically shows the results in a table at the bottom of the window.

Surprise! C\

I didn't mention that this project uses two languages, and, quite by accident, there's some managed code in the mix. WinUI 3 is the culprit here—everything beyond the terminal emulator's black veil is built on it. Now that I've warned you—no guilt on my side. We won't stay here long and will be back to C++ soon enough. Analyzer warnings come in pairs, and there aren't many. Promise!

So, here's the first pair of PVS-Studio warnings, the case N1:

V3061 Parameter 'sbiexOriginal' is always rewritten in method body before being used. WinEventTests.cs 404

V3061 Parameter 'sbiex' is always rewritten in method body before being used. WinEventTests.cs 409

private void TestScrollByOverflowImpl(
    CmdApp app, ViewportArea area, IntPtr hConsole,
    WinCon.CONSOLE_SCREEN_BUFFER_INFO_EX sbiex,
    Queue<EventData> expected,
    WinCon.CONSOLE_SCREEN_BUFFER_INFO_EX sbiexOriginal
)
{
    // Get original screen information
    sbiexOriginal = app.GetScreenBufferInfo();                  // <=
    short promptLineEnd = sbiexOriginal.dwCursorPosition.X;
    promptLineEnd--; // prompt line ended one position left of cursor

    // Resize the window to only have two lines left at the bottom
    // to test overflow when we echo some text
    sbiex = sbiexOriginal;                                      // <=
    ....
}

The issue popped up in a unit test that was disabled. This one's already marked by an issue in the repository. Its author mentions: "I've turned off the ones that are failing. They need to be fixed and restored." Maybe they got my little nudge, but that's not for sure.

Now, a classic one by PVS-Studio, the case N2:

V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. WinEventTests.cs 65

public override bool Equals(object obj)
{
    if (typeof(EventData) == obj.GetType())
    {
        return Equals((EventData)obj);
    }
    else
    {
        return base.Equals(obj);
    }
}

Here, obj can be anything—even null. And if it happens to be null, we won't hit the else branch. Instead, we'll get a pie NullReferenceException in the face, and quite deservedly. A minor tweak to the method helps prevent the surprise.

if (obj == null)
    return false;

if (typeof(EventData) == obj.GetType())
{
    return Equals((EventData)obj);
}

The case N3:

V3137 The 'fSuccess' variable is assigned but is not used by the end of the function. Program2.cs 80

public static void enableVT()
{
    IntPtr hCon = Pinvoke.GetStdHandle(Pinvoke.STD_OUTPUT_HANDLE);

    int mode;
    bool fSuccess = Pinvoke.GetConsoleMode(hCon, out mode);

    if (fSuccess)
    {
        mode |= Pinvoke.ENABLE_VIRTUAL_TERMINAL_PROCESSING;
        fSuccess = Pinvoke.SetConsoleMode(hCon, mode);           // <=
    }
}

There's also the disableVT() method, triggering the same warning, which is clearly a case of copy-paste:

• V3137 The 'fSuccess' variable is assigned but is not used by the end of the function. Program2.cs 92

The SetConsoleMode function from the Windows API does return a boolean indicating whether the console settings were applied successfully. But here, no one bothered to check why it might fail or... it's just a copy-paste from a GetConsoleMode call. After all, both functions return bool and share the same parameter list and order. I'd guess this was copy-pasted because there was no longer any need to support older versions of Windows (like Windows 10 Threshold 1 (version 1507) which didn't support VT100 or similar console virtual terminal sequences.

Iterators turning into a bUBy trap

Managed code's done. So, as promised, let's dive into the unmanaged one. Buckle up and meet C++!

The PVS-Studio warning:

V783 [CERT-CTR51-CPP] Dereferencing of the invalid iterator 'shades.end()' might take place. ColorHelper.cpp 194

winrt::Windows::UI::Color ColorHelper::GetAccentColor(
    const winrt::Windows::UI::Color& color
)
{
    ....
    auto shades = std::map<float, HSL>();
    ....

    // 3f is quite nice if the whole non-client area is painted
    constexpr auto readability = 1.75f;
    for (auto shade : shades)
    {
        if (shade.first >= readability)
        {
            return HslToRgb(shade.second);
        }
    }
    return HslToRgb(shades.end()->second);    // <=
}

Is it possible that none of the shades will meet the readability criteria? We can't say for sure, but it's quite likely. This is a textbook case of undefined behavior—and no need to flash any psychic paper to prove it, since dereferencing std::map::end() causes exactly that, as this iterator points just past the last element in a std::map.

Telepaths take a break

The PVS-Studio warning:

V1004 [CERT-EXP34-C] The 'pSettings' pointer was used unsafely after it was verified against nullptr. Check lines: 199, 211. window.cpp 211

[[nodiscard]] NTSTATUS Window::_MakeWindow(
    _In_ Settings* const pSettings,
    _In_ SCREEN_INFORMATION* const pScreen
)
{
    auto& g = ServiceLocator::LocateGlobals();
    auto& gci = g.getConsoleInformation();
    auto status = STATUS_SUCCESS;

    if (pSettings == nullptr)                   <=
    {
        status = STATUS_INVALID_PARAMETER_1;
    }
    ....

    const auto useDx = pSettings->GetUseDx();   <=
    try
    {
#if TIL_FEATURE_CONHOSTATLASENGINE_ENABLED
        if (useDx)
        {
            pAtlasEngine = new AtlasEngine();
            g.pRender->AddRenderEngine(pAtlasEngine);
        }
        else
#endif
        ....
    }    
    catch (...)
    {
       status = NTSTATUS_FROM_HRESULT(wil::ResultFromCaughtException());
    }
    ....
}

Here we have the classic case of dereferencing a null pointer. Why is it here? Let's break it down. We have the pSettings parameter that may be nullptr. We check this and set the status accordingly.

if (pSettings == nullptr)
{
    status = STATUS_INVALID_PARAMETER_1;
}

Here's where it gets interesting: we dereference a nullptr and then attempt to use the data obtained from it. I can hear the kettle of tension starting to whistle, as this is a textbook ERROR_ACCESS_VIOLATION. The crash happens right on this line:

const auto useDx = pSettings->GetUseDx();

No, wrapping this operation in a try-catch block won't help—dereferencing a null pointer can't be handled by exceptions. There's no standard NullPointerException in C++. Someone might say there's the /EHa flag and you can catch all exceptions with catch(...), but even Microsoft advises against using it:

Specifying /EHa and trying to handle all exceptions by using catch(...) can be dangerous. In most cases, asynchronous exceptions are unrecoverable and should be considered fatal. Catching them and proceeding can cause process corruption and lead to bugs that are hard to find and fix.

Even though Windows and Visual C++ support SEH, we strongly recommend that you use ISO-standard C++ exception handling (/EHsc or /EHs). It makes your code more portable and flexible.

Further in the code, the exception is "handled" by updating status and skipping operations that would have run if the code had succeeded.

catch (...)
{
    status = NTSTATUS_FROM_HRESULT(wil::ResultFromCaughtException());
}

if (SUCCEEDED_NTSTATUS(status))
{
    ....
}
return status;

Too bad it doesn't work like that... or rather, it doesn't work at all.

Nearly a COMa

The PVS-Studio warning:

V1114 Suspicious use of 'static_cast' when working with COM interfaces. Consider using the 'QueryInterface' member function. uiaTextRange.cpp 84

namespace Microsoft::Console::Types
{
    class ScreenInfoUiaProviderBase :
        public WRL::RuntimeClass<
            WRL::RuntimeClassFlags<WRL::ClassicCom | WRL::InhibitFtmBase>,
            IRawElementProviderSimple,
            IRawElementProviderFragment,
            ITextProvider
        >,
        public IUiaTraceable
    ....
}

namespace Microsoft::Console::Interactivity::Win32
{
    class ScreenInfoUiaProvider final :
        public Microsoft::Console::Types::ScreenInfoUiaProviderBase
    {
        ....
    }
    ....
}

HWND UiaTextRange::_getWindowHandle() const
{
    const auto provider = static_cast<ScreenInfoUiaProvider*>(_pProvider); // <=
    return provider->GetWindowHandle();
}

The Windows Runtime C++ Template Library (WRL) includes functionality for working with COM interfaces. Before working with an interface, you need to check whether it's accessible and whether the correct interface will actually be returned. Raw pointer casting to access them provides neither of these guarantees, nor does it handle reference counting for the object with that interface. That's why you better use QueryInterface(), and it'll stop COM from being the monster under the bed that haunts you at night. But here's the catch—ScreenInfoUiaProvider doesn't have an assigned IID, so there's no way to properly call QueryInterface() to get the interface.

If you know COM better than I do and can make sense of what's going on here, welcome to the comments. To me, it's just a mess :)

Pretending to be productive

The PVS-Studio warning:

V519 [CERT-MSC13-C] The 'delayedLineBreak' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2938, 2941. textBuffer.cpp 2941

void TextBuffer::_SerializeRow(
    const ROW& row, const til::CoordType startX,
    const til::CoordType endX, const bool addLineBreak,
    const bool isLastRow, std::wstring& buffer,
    std::optional<TextAttribute>& previousTextAttr, bool& delayedLineBreak
) const
{
    ....
    // Handle empty rows (with no runs).
    // See above for more details about `delayedLineBreak`.
    if (delayedLineBreak)
    {
        buffer.append(L"\r\n");
        delayedLineBreak = false;
    }

    delayedLineBreak = !row.WasWrapForced() && addLineBreak;
    ....
}

Changing the value of delayedLineBreak is pointless here—it gets immediately overwritten. This slipped through code review. Even the sharpest eye of the most experienced developer can overlook a tricky detail—but a static analyzer never gets tired. PVS-Studio can analyze code in pull requests, and as for Windows Terminal, this can be done seamlessly—right within GitHub Actions!

You can't write Code without C

The PVS-Studio warning:

V668 [CERT-MEM52-CPP] There is no sense in testing the 'pszTranslatedConsoleTitle' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. srvinit.cpp 657

PWSTR TranslateConsoleTitle(_In_ PCWSTR pwszConsoleTitle,
                            const BOOL fUnexpand,
                            const BOOL fSubstitute)
{
  ....
  LPWSTR pszTranslatedConsoleTitle;
  const auto cbTranslatedConsoleTitle = cbSystemRoot + cbConsoleTitle;
  Tmp = pszTranslatedConsoleTitle = (PWSTR)new BYTE[cbTranslatedConsoleTitle];
  if (pszTranslatedConsoleTitle == nullptr)
  {
      return nullptr;
  }
  ....
}

Looks like the code of a C programmer who's just started writing in C++. Old habits took over, and the fingers automatically typed a null pointer check—just in case memory wasn't allocated. If operator new[] comes into play, this step isn't needed, since failed memory allocation should be caught using try-catch.

try
{
    Tmp = pszTranslatedConsoleTitle = (PWSTR)new BYTE[cbTranslatedConsoleTitle];
}
catch (std::bad_alloc)
{
    return nullptr;
}

Since there's no error handling here, it's exitus letalis for the program if there's no memory left to allocate.

Will the patient make it?

It will. It'll stick around, and probably for a good long while. The title just adds a touch of drama. If the prognosis matched the name, Windows Terminal would've been shut down, and Microsoft would've pulled the plug already. Not only did the patient make it—it quickly took over as the default terminal app, shipping right along with Windows 11.

Some are thrilled by the gradual open-sourcing of Windows components, others are cautious, and some simply curious. While I'm just here watching the secret life of bugs. PVS-Studio makes that easier, it cuts bug lifespans dramatically. By the way, if you have an open-source project, you can use PVS-Studio static analyzer too, with a free license available for OSS projects.