Implement basic profile matching #11390

lhecker · 2021-10-01T17:03:30Z

This implements command line matching for CascadiaSettings::GetProfileForArgs.
The command lines for all user profiles are resolved to absolute file paths,
argument quotes are standardized ("canonicalized") and the results are cached.

When GetProfileForArgs is called with a Commandline() value, we "canonicalize"
the argument as well and find the profile that is the longest prefix.
If none could be found the default profile is returned.

PR Checklist

Closes [DefApp] Pass LNK/EXE data from OpenCon to ITerminalHandoff #9458
Closes Make it possible to use an existing profile if an app is launched via defterm #10952
I work here
Tests added/passed

Validation Steps Performed

Open a cmd.exe tab in the store-version of WT
Run start cmd
--> A tab with the cmd.exe profile opens
Run start pwsh.exe
--> A tab with the PowerShell 7 profile opens
Run PowerShell 7 from the start menu
--> A tab with the PowerShell 7 profile opens
Create a symlink for PowerShell 7 and launch pwsh.exe from there
--> A tab with the PowerShell 7 profile opens

src/cascadia/TerminalConnection/ConptyConnection.cpp

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

carlos-zamora · 2021-10-06T19:01:20Z

src/host/srvinit.cpp

-    wil::unique_handle clientProcess{ OpenProcess(PROCESS_QUERY_INFORMATION | SYNCHRONIZE, TRUE, static_cast<DWORD>(connectMessage->Descriptor.Process)) };
+    wil::unique_handle clientProcess{ OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ | SYNCHRONIZE, TRUE, static_cast<DWORD>(connectMessage->Descriptor.Process)) };


This seems unrelated

Does it? 😉

You need the VM_READ permission to read memory from the process... which is how we get the command line

this brings fear - do we always have permission to do that? This is conhost code so I don't want conhost to start magically failing to open handles.......

...... nevermind this is defterm code so if we fail to open the client with these permissions then we'll just fail the delegation entirely. That's fine.

...... nevermind this is defterm code

That's what got me hahaha

If we don't get PROCESS_VM_READ with a non-protected process at the same IL as us... someone's really changed how the process model on Windows works and this likely won't be the only problem we experience.

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

…atching

* Added unit tests * Fixed inverted ConnectionType comparison * Standardized PROCESS_BASIC_INFORMATION throughout the project * Use wil resource classes for safety * Give canonical() a namespace for clarity * Added exception handling for command line matching as command line normalization may throw * Finally introduced PCG into the project For now it's only used in a unit test though.

lhecker · 2021-10-07T01:58:47Z

@miniksa If you can, it'd be awesome if you review the latest commit tomorrow, titled "An assortment of changes". It addresses a view points you made.

zadjii-msft

I'm at 17/20 but the kid just woke up so figured I'd get you some feedback before I disappear this morning

zadjii-msft · 2021-10-07T12:03:19Z

src/inc/til/rand.h

+        static const auto RtlGenRandom = []() {
+            // The documentation states to use advapi32.dll, but technically
+            // SystemFunction036 lives in cryptbase.dll since Windows 7.
+            const auto cryptbase = LoadLibraryExW(L"cryptbase.dll", nullptr, 0);


As always, there's wil helpers for this kind of thing:

wil::unique_hmodule hDComp{ LoadLibraryEx(L"Dcomp.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32) }; RETURN_LAST_ERROR_IF(hDComp.get() == nullptr); auto fn = GetProcAddressByFunctionDeclaration(hDComp.get(), DCompositionCreateSurfaceHandle); RETURN_LAST_ERROR_IF(fn == nullptr); return fn(GENERIC_ALL, nullptr, &_swapChainHandle);

wil::unique_hmodule will unload the library when it exits the scope. Can I really do that while I still hold the proc address for later use? (I return the proc address and assign them to the static variable.)

Furthermore I didn't use GetProcAddressByFunctionDeclaration because there's no function declaration available for RtlGenRandom. In order to use GetProcAddressByFunctionDeclaration then I'd have to separately declare the type first. Would you prefer that anyways?

No you cannot let go of it if you're going to use it again later. You have leaked the count though by not holding the module also as a static. Probably not a huge deal, but it is a thing.

Oh yeah DLL unloading should continue working shouldn’t it?

src/cascadia/TerminalApp/TerminalPage.cpp

zadjii-msft · 2021-10-07T12:16:12Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

        _inPipe{ hIn },
        _outPipe{ hOut }
    {
        THROW_IF_FAILED(ConptyPackPseudoConsole(hServerProcess, hRef, hSig, &_hPC));
-        if (_guid == guid{})


I suppose this means we won't be assigning defterms guids anymore, but they already wouldn't be able to have WT_SESSION in their env block so that doesn't really matter.

Ah I shouldn't have committed that... I don't want to change unrelated code like that.
I'll revert this change. Although I'll drop the if condition, as the _guid can't possibly be anything but guid{}.

zadjii-msft · 2021-10-07T12:33:59Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

+    // Requires PROCESS_BASIC_INFORMATION | PROCESS_VM_READ privileges.
+    winrt::hstring ConptyConnection::_commandlineFromProcess(HANDLE process)
+    {
+        struct PROCESS_BASIC_INFORMATION


don't love documenting undocumented bits of the OS. We should try file deliverables internally to document these (since I can't imagine them changing)

I've opened an internal PR.

zadjii-msft · 2021-10-07T12:35:26Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

+
+        // Yeah I know... Don't use "impl" stuff... But why do you make something _that_ useful private? :(
+        // The hstring_builder allows us to create a hstring without intermediate copies. Neat!
+        winrt::impl::hstring_builder commandline{ params.CommandLine.Length / 2u };


similarly, we should maybe file a issue on cppwinrt to move at least the builder out of impl, because it does have value.

I've opened an issue: microsoft/cppwinrt#1035

zadjii-msft · 2021-10-07T12:38:11Z

src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp

+    // This test ensures CommandLineToArgvW doesn't change just to be sure.
+    void TerminalSettingsTests::CommandLineToArgvW()
+    {
+        pcg_engines::oneseq_dxsm_64_32 rng{ til::gen_random<uint64_t>() };


huh, so we're bringing in that whole random header just for this one test. This might make more sense as a fuzzed test, but ain't nobody got time for that in 1.12

I had written a fuzzer for til::rle in the past that makes use of this RNG. With the header in place I can finally import that project. 🙂 It's also only a very minimalistic 45 LOC version of the library that I wrote. I wanted to just inline that into the til/rand.h header, but this wouldn't have allowed me to properly credit the author with a cgmanifest.

miniksa · 2021-10-07T21:16:47Z

src/inc/til/rand.h

+        static const auto RtlGenRandom = []() {
+            // The documentation states to use advapi32.dll, but technically
+            // SystemFunction036 lives in cryptbase.dll since Windows 7.
+            const auto cryptbase = LoadLibraryExW(L"cryptbase.dll", nullptr, 0);


No you cannot let go of it if you're going to use it again later. You have leaked the count though by not holding the module also as a static. Probably not a huge deal, but it is a thing.

miniksa · 2021-10-07T21:19:22Z

src/inc/til/rand.h

+
+namespace til
+{
+    void gen_random(void* data, uint32_t length)


Did I miss where you actually use gen_random and not use this just to import pcg_random.hpp on your behalf?

This function is used in one of the tests to initialize the PCG RNG.

ghost · 2021-10-08T00:10:18Z

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.

DHowett

big comments!

DHowett · 2021-10-08T17:32:24Z

src/inc/til/rand.h

+        {
+            RtlGenRandomLoader() noexcept :
+                // The documentation states to use advapi32.dll, but technically
+                // SystemFunction036 lives in cryptbase.dll since Windows 7.


ideally we would have used the apiset 😉

(Going back and forth between these two threads -- SystemFunction036 is hosted in api-ms-win-security-systemfunctions-l1-1-0... but that apiset is only documented as containing SystemFunction036 after contract version win8. I bet they could back-version that and be fine, but that's not likely to happen.)

Since the apiset doesn't exist on win8, why shouldn't we have used advapi32.dll itself? The loader will take care of finding the secret place where it actually lives.

Also, why did we have to delay load this? I have so many questions.

I'm going to open a PR documenting the reasons behind this better.

DHowett · 2021-10-08T17:33:20Z

src/inc/til/rand.h

+                // The documentation states to use advapi32.dll, but technically
+                // SystemFunction036 lives in cryptbase.dll since Windows 7.
+                module{ LoadLibraryExW(L"cryptbase.dll", nullptr, LOAD_LIBRARY_SEARCH_SYSTEM32) },
+                proc{ reinterpret_cast<decltype(proc)>(GetProcAddress(module.get(), "SystemFunction036")) }


What on earth? RtlGenRandom is fully documented, but we're.. looking it up with a weird name in a private DLL? Why?

Interesting. It's a macro.

It does have a prototype in NTSecAPI.h, under this name. Could we really not have used it with the wil helper? It would resolve to SystemFunction036, sure, but at least we wouldn't leak that name into our own code.

If not, could we not have stringified the content of the RtlGenRandom macro instead?

DHowett · 2021-10-08T17:42:26Z

src/host/ntprivapi.cpp

@@ -31,9 +31,7 @@
        return Status;
    }

-    // This is the actual field name, but in the public SDK, it's named Reserved3. We need to pursue publishing the real name.


Please file an internal bug on documenting this.

I feel weird that we just threw our hands up and dumped a bunch of Reserved fields into the public eye 😄

The documentation is now public: https://docs.microsoft.com/en-us/windows/win32/procthread/zwqueryinformationprocess#PROCESS_BASIC_INFORMATION
I'm going to copy the documentation over to the Nt variant soon.

DHowett · 2021-10-08T17:43:04Z

src/cascadia/TerminalSettingsModel/WslDistroGenerator.cpp

@@ -265,14 +265,12 @@ static bool getWslNames(const wil::unique_hkey& wslRootKey,

        std::wstring buffer;
        auto result = wil::AdaptFixedSizeToAllocatedResult<std::wstring, 256>(buffer, [&](PWSTR value, size_t valueLength, size_t* valueLengthNeededWithNull) -> HRESULT {
-            auto length = static_cast<DWORD>(valueLength);
+            auto length = gsl::narrow<DWORD>(valueLength * sizeof(wchar_t));


unrelated? but generally fine.

ghost · 2021-10-20T17:07:34Z

🎉Windows Terminal Preview v1.12.2922.0 has been released which incorporates this pull request.:tada:

Handy links:

Implement basic profile matching

4642ccf

ghost added Area-DefApp Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Oct 1, 2021

lhecker added 2 commits October 2, 2021 00:04

Added fallback to _normalizeCommandLine for UWP

91f1cf1

Satisfy linter

4176523

lhecker added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. zBugBash-Consider and removed zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. labels Oct 4, 2021

zadjii-msft mentioned this pull request Oct 4, 2021

The "Default Terminal" option is only available for Preview builds of the Terminal #11415

Closed

3 tasks

Ignore Azure profiles

2f297f6

zadjii-msft added this to the Terminal v1.12 milestone Oct 5, 2021

lhecker marked this pull request as ready for review October 6, 2021 02:15