From e06e1314a8c3d34a8220c6cb671b46044d9156df Mon Sep 17 00:00:00 2001
From: Leonard Hecker <lhecker@microsoft.com>
Date: Mon, 14 Feb 2022 20:46:46 +0100
Subject: [PATCH] Fix off-by-one bug in NormalizeCommandLine (#12484)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

#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 ✅
---
 .../LocalTests_SettingsModel/TerminalSettingsTests.cpp   | 9 ++++++++-
 src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp  | 9 ++-------
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp b/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
index 841c43a8827..4f1ce422c14 100644
--- a/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
+++ b/src/cascadia/LocalTests_SettingsModel/TerminalSettingsTests.cpp
@@ -199,6 +199,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",
                     }
                 ]
             }
@@ -217,7 +221,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,
@@ -227,6 +231,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 },
         };
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
index fba878395e9..4935cee4b9c 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
+++ b/src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp
@@ -695,11 +695,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:
@@ -754,7 +749,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;
         }
@@ -770,7 +765,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().