From 6698be94a4171a1d9b6cfe84faade10466ca1410 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Sat, 5 Feb 2022 00:15:40 +0100 Subject: [PATCH] Fix profile matching for paths containing unquoted whitespace --- .../TerminalSettingsTests.cpp | 54 ++++++++++++++++ .../CascadiaSettings.cpp | 61 +++++++++++-------- .../TerminalSettingsModel/CascadiaSettings.h | 2 +- 3 files changed, 90 insertions(+), 27 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp index 100634874c81..841c43a88274 100644 --- a/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp @@ -38,6 +38,7 @@ namespace SettingsModelLocalTests TEST_METHOD(TryCreateWinRTType); TEST_METHOD(TestTerminalArgsForBinding); TEST_METHOD(CommandLineToArgvW); + TEST_METHOD(NormalizeCommandLine); TEST_METHOD(GetProfileForArgsWithCommandline); TEST_METHOD(MakeSettingsForProfile); TEST_METHOD(MakeSettingsForDefaultProfileThatDoesntExist); @@ -120,6 +121,59 @@ namespace SettingsModelLocalTests VERIFY_ARE_EQUAL(0, memcmp(beg, expectedArgv.data(), expectedArgv.size())); } + // This unit test covers GH#12345. + // * paths with more than 1 whitespace + // * paths sharing a common prefix with another directory + void TerminalSettingsTests::NormalizeCommandLine() + { + using namespace std::string_literals; + + static constexpr auto touch = [](const auto& path) { + std::ofstream file{ path }; + }; + + std::wstring guid; + { + GUID g{}; + THROW_IF_FAILED(CoCreateGuid(&g)); + guid = fmt::format( + L"{:08x}-{:04x}-{:04x}-{:02x}{:02x}-{:02x}{:02x}{:02x}{:02x}{:02x}{:02x}", + g.Data1, + g.Data2, + g.Data3, + g.Data4[0], + g.Data4[1], + g.Data4[2], + g.Data4[3], + g.Data4[4], + g.Data4[5], + g.Data4[6], + g.Data4[7]); + } + + const auto tmpdir = std::filesystem::temp_directory_path(); + const auto dir1 = tmpdir / guid; + const auto dir2 = tmpdir / (guid + L" two"); + const auto file1 = dir1 / L"file 1.exe"; + const auto file2 = dir2 / L"file 2.exe"; + + const auto cleanup = wil::scope_exit([&]() { + std::error_code ec; + remove_all(dir1, ec); + remove_all(dir2, ec); + }); + + create_directory(dir1); + create_directory(dir2); + touch(file1); + touch(file2); + + const auto commandLine = file2.native() + LR"( -foo "bar1 bar2" -baz)"s; + const auto expected = file2.native() + L"\0-foo\0bar1 bar2\0-baz"s; + const auto actual = implementation::CascadiaSettings::NormalizeCommandLine(commandLine.c_str()); + VERIFY_ARE_EQUAL(expected, actual); + } + void TerminalSettingsTests::GetProfileForArgsWithCommandline() { // I'm exclusively using cmd.exe as I know exactly where it resides at. diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index e3916eff51b3..fba878395e9e 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -593,7 +593,7 @@ Model::Profile CascadiaSettings::GetProfileForArgs(const Model::NewTerminalArgs& Model::Profile CascadiaSettings::_getProfileForCommandLine(const winrt::hstring& commandLine) const { // We're going to cache all the command lines we got, as - // _normalizeCommandLine is a relatively heavy operation. + // NormalizeCommandLine is a relatively heavy operation. std::call_once(_commandLinesCacheOnce, [this]() { _commandLinesCache.reserve(_allProfiles.Size()); @@ -612,7 +612,7 @@ Model::Profile CascadiaSettings::_getProfileForCommandLine(const winrt::hstring& try { - _commandLinesCache.emplace_back(_normalizeCommandLine(cmd.c_str()), profile); + _commandLinesCache.emplace_back(NormalizeCommandLine(cmd.c_str()), profile); } CATCH_LOG() } @@ -631,7 +631,7 @@ Model::Profile CascadiaSettings::_getProfileForCommandLine(const winrt::hstring& try { - const auto needle = _normalizeCommandLine(commandLine.c_str()); + const auto needle = NormalizeCommandLine(commandLine.c_str()); // til::starts_with(string, prefix) will always return false if prefix.size() > string.size(). // --> Using binary search we can safely skip all items in _commandLinesCache where .first.size() > needle.size(). @@ -678,7 +678,7 @@ Model::Profile CascadiaSettings::_getProfileForCommandLine(const winrt::hstring& // is considered a compatible profile with // "C:\Program Files\PowerShell\7\pwsh.exe -WorkingDirectory ~" // as it shares the same (normalized) prefix. -std::wstring CascadiaSettings::_normalizeCommandLine(LPCWSTR commandLine) +std::wstring CascadiaSettings::NormalizeCommandLine(LPCWSTR commandLine) { // Turn "%SystemRoot%\System32\cmd.exe" into "C:\WINDOWS\System32\cmd.exe". // We do this early, as environment variables might occur anywhere in the commandLine. @@ -692,6 +692,15 @@ std::wstring CascadiaSettings::_normalizeCommandLine(LPCWSTR commandLine) wil::unique_hlocal_ptr argv{ CommandLineToArgvW(normalized.c_str(), &argc) }; THROW_LAST_ERROR_IF(!argc); + // The index of the first argument in argv for our executable in argv[0]. + // Given {"C:\Program Files\PowerShell\7\pwsh.exe", "-WorkingDirectory", "~"} this will be 1. + int startOfArguments = 1; + // Returns true if the executable in argv[0] to argv[startOfArguments - 1] + // has any further arguments in argv[startOfArguments] to argv[argc - 1]. + const auto hasTrailingArguments = [&]() noexcept { + return (argc - startOfArguments) > 1; + }; + // The given commandLine should start with an executable name or path. // For instance given the following argv arrays: // * {"C:\WINDOWS\System32\cmd.exe"} @@ -713,39 +722,39 @@ std::wstring CascadiaSettings::_normalizeCommandLine(LPCWSTR commandLine) // CreateProcessW uses RtlGetExePath to get the lpPath for SearchPathW. // The difference between the behavior of SearchPathW if lpPath is nullptr and what RtlGetExePath returns // seems to be mostly whether SafeProcessSearchMode is respected and the support for relative paths. - // Windows Terminal makes the use relative paths rather impractical which is why we simply dropped the call to RtlGetExePath. + // Windows Terminal makes the use of relative paths rather impractical which is why we simply dropped the call to RtlGetExePath. const auto status = wil::SearchPathW(nullptr, argv[0], L".exe", normalized); if (status == S_OK) { - std::filesystem::path path{ std::move(normalized) }; + const auto attributes = GetFileAttributesW(normalized.c_str()); - // ExpandEnvironmentStringsW() might have returned a string that's not in the canonical capitalization. - // For instance %SystemRoot% is set to C:\WINDOWS on my system (ugh), even though the path is actually C:\Windows. - // We need to fix this as case-sensitive path comparisons will fail otherwise (Windows supports case-sensitive file systems). - // If we fail to resolve the path for whatever reason (pretty unlikely given that SearchPathW found it) - // we fall back to leaving the path as is. Better than throwing a random exception and making this unusable. + if (attributes != INVALID_FILE_ATTRIBUTES && WI_IsFlagClear(attributes, FILE_ATTRIBUTE_DIRECTORY)) { - std::error_code ec; - auto canonicalPath = std::filesystem::canonical(path, ec); - if (!ec) + std::filesystem::path path{ std::move(normalized) }; + + // canonical() will resolve symlinks, etc. for us. { - path = std::move(canonicalPath); + std::error_code ec; + auto canonicalPath = std::filesystem::canonical(path, ec); + if (!ec) + { + path = std::move(canonicalPath); + } } - } - // std::filesystem::path has no way to extract the internal path. - // So about that.... I own you, computer. Give me that path. - normalized = std::move(const_cast(path.native())); - break; + // std::filesystem::path has no way to extract the internal path. + // So about that.... I own you, computer. Give me that path. + normalized = std::move(const_cast(path.native())); + break; + } } - // If the file path couldn't be found by SearchPathW this could be the result of us being given a commandLine // like "C:\foo bar\baz.exe -arg" which is resolved to the argv array {"C:\foo", "bar\baz.exe", "-arg"}. // Just like CreateProcessW() we thus try to concatenate arguments until we successfully resolve a valid path. // Of course we can only do that if we have at least 2 remaining arguments in argv. // All other error types aren't handled at the moment. - if (argc < 2 || status != HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)) + else if (!hasTrailingArguments() || status != HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)) { break; } @@ -753,19 +762,19 @@ std::wstring CascadiaSettings::_normalizeCommandLine(LPCWSTR commandLine) // As described in the comment right above, we concatenate arguments in an attempt to resolve a valid path. // The code below turns argv from {"C:\foo", "bar\baz.exe", "-arg"} into {"C:\foo bar\baz.exe", "-arg"}. // The code abuses the fact that CommandLineToArgvW allocates all arguments back-to-back on the heap separated by '\0'. - argv[1][-1] = L' '; - --argc; + argv[startOfArguments][-1] = L' '; + ++startOfArguments; } // We've (hopefully) finished resolving the path to the executable. // We're now going to append all remaining arguments to the resulting string. // If argv is {"C:\Program Files\PowerShell\7\pwsh.exe", "-WorkingDirectory", "~"}, // then we'll get "C:\Program Files\PowerShell\7\pwsh.exe\0-WorkingDirectory\0~" - if (argc > 1) + if (hasTrailingArguments()) { // normalized contains a canonical form of argv[0] at this point. // -1 allows us to include the \0 between argv[0] and argv[1] in the call to append(). - const auto beg = argv[1] - 1; + const auto beg = argv[startOfArguments] - 1; const auto lastArg = argv[argc - 1]; const auto end = lastArg + wcslen(lastArg); normalized.append(beg, end); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h index 716ccf64d875..772f7625beba 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.h +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.h @@ -134,6 +134,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation winrt::hstring GetSerializationErrorMessage() const; // defterm + static std::wstring NormalizeCommandLine(LPCWSTR commandLine); static bool IsDefaultTerminalAvailable() noexcept; static bool IsDefaultTerminalSet() noexcept; winrt::Windows::Foundation::Collections::IObservableVector DefaultTerminals() noexcept; @@ -142,7 +143,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: static const std::filesystem::path& _settingsPath(); - static std::wstring _normalizeCommandLine(LPCWSTR commandLine); winrt::com_ptr _createNewProfile(const std::wstring_view& name) const; Model::Profile _getProfileForCommandLine(const winrt::hstring& commandLine) const;