Skip to content

Commit

Permalink
Don't crash trying to parse a command line that's a directory (#12538)
Browse files Browse the repository at this point in the history
## 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 `.']
```
  • Loading branch information
ianjoneill authored Feb 22, 2022
1 parent 788d33c commit 722aafa
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 8 deletions.
20 changes: 15 additions & 5 deletions src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,8 @@ namespace SettingsModelLocalTests
for (int i = 0; i < expectedArgc; ++i)
{
const bool useQuotes = static_cast<bool>(rng(2));
const auto count = static_cast<size_t>(rng(64));
// We need to ensure there is at least one character
const auto count = static_cast<size_t>(rng(64) + 1);
const auto ch = static_cast<wchar_t>(rng('z' - 'a' + 1) + 'a');

if (i != 0)
Expand All @@ -107,6 +108,7 @@ namespace SettingsModelLocalTests
input.push_back(L'"');
}
}
Log::Comment(NoThrowString().Format(input.c_str()));

int argc;
wil::unique_hlocal_ptr<PWSTR[]> argv{ ::CommandLineToArgvW(input.c_str(), &argc) };
Expand Down Expand Up @@ -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()
Expand Down
11 changes: 8 additions & 3 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down

0 comments on commit 722aafa

Please sign in to comment.