From 722aafadeb5601002464515252d5e8f190f7dee0 Mon Sep 17 00:00:00 2001 From: Ian O'Neill Date: Tue, 22 Feb 2022 15:20:05 +0000 Subject: [PATCH] Don't crash trying to parse a command line that's a directory (#12538) ## Summary of the Pull Request Prevents a crash that could occur when invoking `wt C:\` ## PR Checklist * [x] Closes #12535 * [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA * [x] Tests added/passed * [ ] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx * [ ] Schema updated. * [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx ## Detailed Description of the Pull Request / Additional comments Updates `CascadiaSettings::NormalizeCommandLine()` to check that there are a suitable number of command line arguments to be concatenated together, to prevent accessing an array index in `argv` that doesn't exist. Also prevents a test flake that could occur in `TerminalSettingsTests::CommandLineToArgvW()`, due to generating an empty command line argument. ## Validation Steps Performed Added a test, and checked that invoking each of the command lines below behaved as expected: ``` wtd C:\ # Window pops up with [error 2147942405 (0x80070005) when launching `C:\'] wtd C:\Program Files # Window pops up with [error 2147942402 (0x80070002) when launching `C:\Program Files'] wtd cmd # cmd profile pops up wtd C:\Program Files\Powershell\7\pwsh -WorkingDirectory C:\ # PowerShell profile pops up in C:\ wtd "C:\Program Files\Powershell\7\pwsh" -WorkingDirectory C:\ # PowerShell profile pops up in C:\ wtd . # Window pops up with [error 2147942405 (0x80070005) when launching `.'] ``` --- .../TerminalSettingsTests.cpp | 20 ++++++++++++++----- .../CascadiaSettings.cpp | 11 +++++++--- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp index 4f1ce422c14..ad2463bbb70 100644 --- a/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp +++ b/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp @@ -85,7 +85,8 @@ namespace SettingsModelLocalTests for (int i = 0; i < expectedArgc; ++i) { const bool useQuotes = static_cast(rng(2)); - const auto count = static_cast(rng(64)); + // We need to ensure there is at least one character + const auto count = static_cast(rng(64) + 1); const auto ch = static_cast(rng('z' - 'a' + 1) + 'a'); if (i != 0) @@ -107,6 +108,7 @@ namespace SettingsModelLocalTests input.push_back(L'"'); } } + Log::Comment(NoThrowString().Format(input.c_str())); int argc; wil::unique_hlocal_ptr argv{ ::CommandLineToArgvW(input.c_str(), &argc) }; @@ -168,10 +170,18 @@ namespace SettingsModelLocalTests 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); + { + 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); + } + { + const auto commandLine = L"C:\\"; + const auto expected = L"C:\\"; + const auto actual = implementation::CascadiaSettings::NormalizeCommandLine(commandLine); + VERIFY_ARE_EQUAL(expected, actual); + } } void TerminalSettingsTests::GetProfileForArgsWithCommandline() diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp index 4935cee4b9c..b3f49688a26 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp @@ -744,12 +744,17 @@ std::wstring CascadiaSettings::NormalizeCommandLine(LPCWSTR commandLine) break; } } + // All other error types aren't handled at the moment. + else if (status != HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)) + { + 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"}. + // like "C:\foo bar\baz.exe -arg" which is resolved to the argv array {"C:\foo", "bar\baz.exe", "-arg"}, + // or we were erroneously given a directory to execute (e.g. someone ran `wt .`). // 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. - else if ((argc - startOfArguments) < 2 || status != HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND)) + if ((argc - startOfArguments) < 2) { break; }