Skip to content

Commit

Permalink
Fix off-by-one bug in NormalizeCommandLine (#12484)
Browse files Browse the repository at this point in the history
#12348 introduced an off-by-one bug. While the `NormalizeCommandLine` loop
should exit early when there aren't at least _two_ arguments to be joined,
the final argument-append needs to happen even if just _one_ argument exists.

This commit fixes the issue and introduces changes to additionally monitor
the early loop exit, as well as the call to `ExpandEnvironmentStringsW`.

## PR Checklist
* [x] Closes #12461
* [x] I work here
* [x] Tests added/passed

## Validation Steps Performed
* All `TerminalSettingsTests` tests pass ✅

(cherry picked from commit e06e131)
  • Loading branch information
lhecker authored and DHowett committed Feb 16, 2022
1 parent 6419ac1 commit 0892a13
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,10 @@ namespace SettingsModelLocalTests
"guid": "{6239a42c-3333-49a3-80bd-e8fdd045185c}",
"commandline": "cmd.exe /A /C",
"connectionType": "{9a9977a7-1fe0-49c0-b6c0-13a0cd1c98a1}"
},
{
"guid": "{6239a42c-4444-49a3-80bd-e8fdd045185c}",
"commandline": "C:\\invalid.exe",
}
]
}
Expand All @@ -213,7 +217,7 @@ namespace SettingsModelLocalTests
TestCase{ L"cmd.exe", 0 },
// SearchPathW() normalization + case insensitive matching.
TestCase{ L"cmd.exe /a", 1 },
TestCase{ L"C:\\Windows\\System32\\cmd.exe /A", 1 },
TestCase{ L"%SystemRoot%\\System32\\cmd.exe /A", 1 },
// Test that we don't pick the equally long but different "/A /B" variant.
TestCase{ L"C:\\Windows\\System32\\cmd.exe /A /C", 1 },
// Test that we don't pick the shorter "/A" variant,
Expand All @@ -223,6 +227,9 @@ namespace SettingsModelLocalTests
// Ignore profiles with a connection type, like the Azure cloud shell.
// Instead it should pick any other prefix.
TestCase{ L"C:\\Windows\\System32\\cmd.exe /A /C", 1 },
// Failure to normalize a path (e.g. because the path doesn't exist)
// should yield the unmodified input string (see NormalizeCommandLine).
TestCase{ L"C:\\invalid.exe /A /B", 4 },
// Return base layer profile for missing profiles.
TestCase{ L"C:\\Windows\\regedit.exe", -1 },
};
Expand Down
9 changes: 2 additions & 7 deletions src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -711,11 +711,6 @@ std::wstring CascadiaSettings::NormalizeCommandLine(LPCWSTR commandLine)
// 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:
Expand Down Expand Up @@ -770,7 +765,7 @@ std::wstring CascadiaSettings::NormalizeCommandLine(LPCWSTR commandLine)
// 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 (!hasTrailingArguments() || status != HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
else if ((argc - startOfArguments) < 2 || status != HRESULT_FROM_WIN32(ERROR_FILE_NOT_FOUND))
{
break;
}
Expand All @@ -786,7 +781,7 @@ std::wstring CascadiaSettings::NormalizeCommandLine(LPCWSTR commandLine)
// 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 (hasTrailingArguments())
if (startOfArguments < argc)
{
// 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().
Expand Down

0 comments on commit 0892a13

Please sign in to comment.