Repository: microsoft/terminal (language: cpp)

Issue:
Empty path to OSC 9;9; crashes Terminal window
### Windows Terminal version

1.13.10336.0

### Windows build number

10.0.19042.1466

### Other Software

_No response_

### Steps to reproduce

In WSL bash, `echo $'\e]9;9;\e\\'`

### Expected Behavior

Terminal should keep running. I don't know if the devs think an empty path should be treated as a no-op or like a well-formed path to a non-existent directory. For example, `\e]9;9;/foo\e\\\` sets the directory to /.

### Actual Behavior

Terminal window hangs immediately (tested by running  `date` in a tight loop in the background) and crashes a second later.


Discussion / hints:
This was something I was planning to look at as a followup PR #12247. In the same way that `AdaptDispatch` and `ConGetSet` are designed to throw exceptions for error handling, the equivalent Terminal classes `TerminalDispatch` and `ITerminalApi` should be doing the same thing. That way exceptions like this would be caught in the `StateMachine` and recovered gracefully.

I had initially planned to leave this till I was further along refactoring the conhost side of things, but maybe it's a good idea to get it done sooner rather than later if it'll save users from having a crashed terminal. Any objections to that?
I should add that this isn't necessarily a fix for the `OSC 9;9` handling - I'm not sure what the correct behaviour is for that. I just want to make sure the terminal isn't needlessly crashing in situations like this.
I agree. At the very least, we should fix this crash now, so we can service the fix to the in-market builds. 
Hey @j4james should this be closed by #12432? Since the crashing doesn't happen anymore? I think right now we just ignore an empty path, which sure seems reasonable to me
I'd prefer to keep it open. I'm not actually sure what the correct behaviour is meant to be - I haven't had a chance to check that in ConEmu. But even if ignoring the path is the right thing to do, I'd expect that to be handled as a deliberate condition in the path parser - not just something that happens coincidentally because of a buffer overflow.
Fine by me ☺️ I _am_ gonna move it out of the 1.14 milestone though, just for the sake of work tracking. The bug that needed immediate fixing is gone. We can double check the real behavior later. Thanks again!
> I am gonna move it out of the 1.14 milestone though, just for the sake of work tracking.

Yeah, that makes perfect sense. I've just checked ConEmu now and it looks like a blank path is simply ignored, i.e. it retains the last valid path that was set. So what we have is technically correct - it's just a code health issue now.

Fix patch:
diff --git a/src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp b/src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp
index 384be1034eb..dd6a1cd0750 100644
--- a/src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp
+++ b/src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp
@@ -435,6 +435,27 @@ void TerminalCoreUnitTests::TerminalApiTest::SetWorkingDirectory()
     stateMachine.ProcessString(L"\x1b]9;9\"C:\\\"\x1b\\");
     VERIFY_IS_TRUE(term.GetWorkingDirectory().empty());
 
+    stateMachine.ProcessString(L"\x1b"
+                               LR"(]9;9;"C:\invalid path "with" quotes")"
+                               L"\x1b\\");
+    VERIFY_IS_TRUE(term.GetWorkingDirectory().empty());
+
+    // These OSC 9;9 sequences will result in invalid CWD. It should end up empty, like above.
+    stateMachine.ProcessString(L"\x1b]9;9;\"\x1b\\");
+    VERIFY_IS_TRUE(term.GetWorkingDirectory().empty());
+
+    stateMachine.ProcessString(L"\x1b]9;9;\"\"\x1b\\");
+    VERIFY_IS_TRUE(term.GetWorkingDirectory().empty());
+
+    stateMachine.ProcessString(L"\x1b]9;9;\"\"\"\x1b\\");
+    VERIFY_IS_TRUE(term.GetWorkingDirectory().empty());
+
+    stateMachine.ProcessString(L"\x1b]9;9;\"\"\"\"\x1b\\");
+    VERIFY_IS_TRUE(term.GetWorkingDirectory().empty());
+
+    stateMachine.ProcessString(L"\x1b]9;9;No quotes \"until\" later\x1b\\");
+    VERIFY_IS_TRUE(term.GetWorkingDirectory().empty());
+
     // Valid sequences should change CWD
     stateMachine.ProcessString(L"\x1b]9;9;\"C:\\\"\x1b\\");
     VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"C:\\");
@@ -454,17 +475,4 @@ void TerminalCoreUnitTests::TerminalApiTest::SetWorkingDirectory()
 
     stateMachine.ProcessString(L"\x1b]9;9;D:\\中文\x1b\\");
     VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"D:\\中文");
-
-    // These OSC 9;9 sequences will result in invalid CWD. We shouldn't crash on these.
-    stateMachine.ProcessString(L"\x1b]9;9;\"\x1b\\");
-    VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"\"");
-
-    stateMachine.ProcessString(L"\x1b]9;9;\"\"\x1b\\");
-    VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"\"\"");
-
-    stateMachine.ProcessString(L"\x1b]9;9;\"\"\"\x1b\\");
-    VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"\"");
-
-    stateMachine.ProcessString(L"\x1b]9;9;\"\"\"\"\x1b\\");
-    VERIFY_ARE_EQUAL(term.GetWorkingDirectory(), L"\"\"");
 }
diff --git a/src/inc/til/string.h b/src/inc/til/string.h
index ac5fb2c1f4f..c6cfb28ce72 100644
--- a/src/inc/til/string.h
+++ b/src/inc/til/string.h
@@ -30,29 +30,56 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
         return visualize_control_codes(std::wstring{ str });
     }
 
-    _TIL_INLINEPREFIX std::wstring clean_filename(std::wstring str) noexcept
+    namespace details
     {
-        static constexpr std::array<bool, 128> filter{ {
+        inline constexpr uint8_t __ = 0b00;
+        inline constexpr uint8_t F_ = 0b10; // stripped in clean_filename
+        inline constexpr uint8_t _P = 0b01; // stripped in clean_path
+        inline constexpr uint8_t FP = 0b11; // stripped in clean_filename and clean_path
+        static constexpr std::array<uint8_t, 128> pathFilter{ {
             // clang-format off
-            0 /* NUL */, 0 /* SOH */, 0 /* STX */, 0 /* ETX */, 0 /* EOT */, 0 /* ENQ */, 0 /* ACK */, 0 /* BEL */, 0 /* BS  */, 0 /* HT  */, 0 /* LF  */, 0 /* VT  */, 0 /* FF  */, 0 /* CR  */, 0 /* SO  */, 0 /* SI  */,
-            0 /* DLE */, 0 /* DC1 */, 0 /* DC2 */, 0 /* DC3 */, 0 /* DC4 */, 0 /* NAK */, 0 /* SYN */, 0 /* ETB */, 0 /* CAN */, 0 /* EM  */, 0 /* SUB */, 0 /* ESC */, 0 /* FS  */, 0 /* GS  */, 0 /* RS  */, 0 /* US  */,
-            0 /* SP  */, 0 /* !   */, 1 /* "   */, 0 /* #   */, 0 /* $   */, 0 /* %   */, 0 /* &   */, 0 /* '   */, 0 /* (   */, 0 /* )   */, 1 /* *   */, 0 /* +   */, 0 /* ,   */, 0 /* -   */, 0 /* .   */, 1 /* /   */,
-            0 /* 0   */, 0 /* 1   */, 0 /* 2   */, 0 /* 3   */, 0 /* 4   */, 0 /* 5   */, 0 /* 6   */, 0 /* 7   */, 0 /* 8   */, 0 /* 9   */, 1 /* :   */, 0 /* ;   */, 1 /* <   */, 0 /* =   */, 1 /* >   */, 1 /* ?   */,
-            0 /* @   */, 0 /* A   */, 0 /* B   */, 0 /* C   */, 0 /* D   */, 0 /* E   */, 0 /* F   */, 0 /* G   */, 0 /* H   */, 0 /* I   */, 0 /* J   */, 0 /* K   */, 0 /* L   */, 0 /* M   */, 0 /* N   */, 0 /* O   */,
-            0 /* P   */, 0 /* Q   */, 0 /* R   */, 0 /* S   */, 0 /* T   */, 0 /* U   */, 0 /* V   */, 0 /* W   */, 0 /* X   */, 0 /* Y   */, 0 /* Z   */, 0 /* [   */, 1 /* \   */, 0 /* ]   */, 0 /* ^   */, 0 /* _   */,
-            0 /* `   */, 0 /* a   */, 0 /* b   */, 0 /* c   */, 0 /* d   */, 0 /* e   */, 0 /* f   */, 0 /* g   */, 0 /* h   */, 0 /* i   */, 0 /* j   */, 0 /* k   */, 0 /* l   */, 0 /* m   */, 0 /* n   */, 0 /* o   */,
-            0 /* p   */, 0 /* q   */, 0 /* r   */, 0 /* s   */, 0 /* t   */, 0 /* u   */, 0 /* v   */, 0 /* w   */, 0 /* x   */, 0 /* y   */, 0 /* z   */, 0 /* {   */, 1 /* |   */, 0 /* }   */, 0 /* ~   */, 0 /* DEL */,
+            __ /* NUL */, __ /* SOH */, __ /* STX */, __ /* ETX */, __ /* EOT */, __ /* ENQ */, __ /* ACK */, __ /* BEL */, __ /* BS  */, __ /* HT  */, __ /* LF  */, __ /* VT  */, __ /* FF  */, __ /* CR  */, __ /* SO  */, __ /* SI  */,
+            __ /* DLE */, __ /* DC1 */, __ /* DC2 */, __ /* DC3 */, __ /* DC4 */, __ /* NAK */, __ /* SYN */, __ /* ETB */, __ /* CAN */, __ /* EM  */, __ /* SUB */, __ /* ESC */, __ /* FS  */, __ /* GS  */, __ /* RS  */, __ /* US  */,
+            __ /* SP  */, __ /* !   */, FP /* "   */, __ /* #   */, __ /* $   */, __ /* %   */, __ /* &   */, __ /* '   */, __ /* (   */, __ /* )   */, FP /* *   */, __ /* +   */, __ /* ,   */, __ /* -   */, __ /* .   */, F_ /* /   */,
+            __ /* 0   */, __ /* 1   */, __ /* 2   */, __ /* 3   */, __ /* 4   */, __ /* 5   */, __ /* 6   */, __ /* 7   */, __ /* 8   */, __ /* 9   */, F_ /* :   */, __ /* ;   */, FP /* <   */, __ /* =   */, FP /* >   */, FP /* ?   */,
+            __ /* @   */, __ /* A   */, __ /* B   */, __ /* C   */, __ /* D   */, __ /* E   */, __ /* F   */, __ /* G   */, __ /* H   */, __ /* I   */, __ /* J   */, __ /* K   */, __ /* L   */, __ /* M   */, __ /* N   */, __ /* O   */,
+            __ /* P   */, __ /* Q   */, __ /* R   */, __ /* S   */, __ /* T   */, __ /* U   */, __ /* V   */, __ /* W   */, __ /* X   */, __ /* Y   */, __ /* Z   */, __ /* [   */, F_ /* \   */, __ /* ]   */, __ /* ^   */, __ /* _   */,
+            __ /* `   */, __ /* a   */, __ /* b   */, __ /* c   */, __ /* d   */, __ /* e   */, __ /* f   */, __ /* g   */, __ /* h   */, __ /* i   */, __ /* j   */, __ /* k   */, __ /* l   */, __ /* m   */, __ /* n   */, __ /* o   */,
+            __ /* p   */, __ /* q   */, __ /* r   */, __ /* s   */, __ /* t   */, __ /* u   */, __ /* v   */, __ /* w   */, __ /* x   */, __ /* y   */, __ /* z   */, __ /* {   */, FP /* |   */, __ /* }   */, __ /* ~   */, __ /* DEL */,
             // clang-format on
         } };
+    }
 
+    _TIL_INLINEPREFIX std::wstring clean_filename(std::wstring str) noexcept
+    {
+        using namespace til::details;
         std::erase_if(str, [](auto ch) {
             // This lookup is branchless: It always checks the filter, but throws
             // away the result if ch >= 128. This is faster than using `&&` (branchy).
-            return til::at(filter, ch & 127) & (ch < 128);
+            return ((til::at(details::pathFilter, ch & 127) & F_) != 0) & (ch < 128);
+        });
+        return str;
+    }
+
+    _TIL_INLINEPREFIX std::wstring clean_path(std::wstring str) noexcept
+    {
+        using namespace til::details;
+        std::erase_if(str, [](auto ch) {
+            return ((til::at(details::pathFilter, ch & 127) & _P) != 0) & (ch < 128);
         });
         return str;
     }
 
+    // is_legal_path rules on whether a path contains any non-path characters.
+    // it **DOES NOT** rule on whether a path exists.
+    _TIL_INLINEPREFIX constexpr bool is_legal_path(const std::wstring_view str) noexcept
+    {
+        using namespace til::details;
+        return !std::any_of(std::begin(str), std::end(str), [](auto&& ch) {
+            return ((til::at(details::pathFilter, ch & 127) & _P) != 0) & (ch < 128);
+        });
+    }
+
     // std::string_view::starts_with support for C++17.
     template<typename T, typename Traits>
     constexpr bool starts_with(const std::basic_string_view<T, Traits>& str, const std::basic_string_view<T, Traits>& prefix) noexcept
diff --git a/src/terminal/adapter/adaptDispatch.cpp b/src/terminal/adapter/adaptDispatch.cpp
index eb6546921a4..6807f484dee 100644
--- a/src/terminal/adapter/adaptDispatch.cpp
+++ b/src/terminal/adapter/adaptDispatch.cpp
@@ -2447,19 +2447,22 @@ bool AdaptDispatch::DoConEmuAction(const std::wstring_view string)
     {
         if (parts.size() >= 2)
         {
-            const auto path = til::at(parts, 1);
+            auto path = til::at(parts, 1);
             // The path should be surrounded with '"' according to the documentation of ConEmu.
             // An example: 9;"D:/"
-            if (path.at(0) == L'"' && path.at(path.size() - 1) == L'"' && path.size() >= 3)
+            // If we fail to find the surrounding quotation marks, we'll give the path a try anyway.
+            // ConEmu also does this.
+            if (path.size() >= 3 && path.at(0) == L'"' && path.at(path.size() - 1) == L'"')
             {
-                _api.SetWorkingDirectory(path.substr(1, path.size() - 2));
+                path = path.substr(1, path.size() - 2);
             }
-            else
+
+            if (!til::is_legal_path(path))
             {
-                // If we fail to find the surrounding quotation marks, we'll give the path a try anyway.
-                // ConEmu also does this.
-                _api.SetWorkingDirectory(path);
+                return false;
             }
+
+            _api.SetWorkingDirectory(path);
             return true;
         }
     }
diff --git a/src/til/ut_til/string.cpp b/src/til/ut_til/string.cpp
index 8e631718ae4..76bb6198b58 100644
--- a/src/til/ut_til/string.cpp
+++ b/src/til/ut_til/string.cpp
@@ -169,4 +169,16 @@ class StringTests
             VERIFY_ARE_EQUAL("", s);
         }
     }
+
+    TEST_METHOD(CleanPathAndFilename)
+    {
+        VERIFY_ARE_EQUAL(LR"(CUsersGeddyMusicAnalog Man)", til::clean_filename(LR"(C:\Users\Geddy\Music\"Analog Man")"));
+        VERIFY_ARE_EQUAL(LR"(C:\Users\Geddy\Music\Analog Man)", til::clean_path(LR"(C:\Users\Geddy\Music\"Analog Man")"));
+    }
+
+    TEST_METHOD(LegalPath)
+    {
+        VERIFY_IS_TRUE(til::is_legal_path(LR"(C:\Users\Documents and Settings\Users\;\Why not)"));
+        VERIFY_IS_FALSE(til::is_legal_path(LR"(C:\Users\Documents and Settings\"Quote-un-quote users")"));
+    }
 };


Test patch: