vt: make sure to Flush the entire parsed sequence (#4870)

When we had to flush unknown sequences to the terminal, we were only
taking the _most recent run_ with us; therefore, if we received `\e[?12`
and `34h` in separate packets we would _only_ send out `34h`.

This change fixes that issue by ensuring that we cache partial bits of
sequences we haven't yet completed, just in case we need to flush them.

Fixes #3080.
Fixes #3081.

src/terminal/parser/stateMachine.cpp

@@ -17,6 +17,7 @@ StateMachine::StateMachine(std::unique_ptr<IStateMachineEngine> engine) :
     _intermediates{},
     _parameters{},
     _oscString{},
+    _cachedSequence{ std::nullopt },
     _processingIndividually(false)
 {
     _ActionClear();
@@ -533,6 +534,7 @@ void StateMachine::_ActionSs3Dispatch(const wchar_t wch)
 void StateMachine::_EnterGround() noexcept
 {
     _state = VTStates::Ground;
+    _cachedSequence.reset(); // entering ground means we've completed the pending sequence
     _trace.TraceStateChange(L"Ground");
 }
 
@@ -1226,11 +1228,27 @@ void StateMachine::ProcessCharacter(const wchar_t wch)
 // - true if the engine successfully handled the string.
 bool StateMachine::FlushToTerminal()
 {
-    // _pwchCurr is incremented after a call to ProcessCharacter to indicate
-    //      that pwchCurr was processed.
-    // However, if we're here, then the processing of pwchChar triggered the
-    //      engine to request the entire sequence get passed through, including pwchCurr.
-    return _engine->ActionPassThroughString(_run);
+    bool success{ true };
+
+    if (success && _cachedSequence.has_value())
+    {
+        // Flush the partial sequence to the terminal before we flush the rest of it.
+        // We always want to clear the sequence, even if we failed, so we don't accumulate bad state
+        // and dump it out elsewhere later.
+        success = _engine->ActionPassThroughString(*_cachedSequence);
+        _cachedSequence.reset();
+    }
+
+    if (success)
+    {
+        // _pwchCurr is incremented after a call to ProcessCharacter to indicate
+        //      that pwchCurr was processed.
+        // However, if we're here, then the processing of pwchChar triggered the
+        //      engine to request the entire sequence get passed through, including pwchCurr.
+        success = _engine->ActionPassThroughString(_run);
+    }
+
+    return success;
 }
 
 // Routine Description:
@@ -1365,6 +1383,13 @@ void StateMachine::ProcessString(const std::wstring_view string)
             // after dispatching the characters
             _EnterGround();
         }
+        else
+        {
+            // If the engine doesn't require flushing at the end of the string, we
+            // want to cache the partial sequence in case we have to flush the whole
+            // thing to the terminal later.
+            _cachedSequence = _cachedSequence.value_or(std::wstring{}) + std::wstring{ _run };
+        }
     }
 }
 

src/terminal/parser/stateMachine.hpp

@@ -115,6 +115,8 @@ namespace Microsoft::Console::VirtualTerminal
         std::wstring _oscString;
         size_t _oscParameter;
 
+        std::optional<std::wstring> _cachedSequence;
+
         // This is tracked per state machine instance so that separate calls to Process*
         //   can start and finish a sequence.
         bool _processingIndividually;

src/terminal/parser/ut_parser/StateMachineTest.cpp

@@ -28,18 +28,25 @@ using namespace Microsoft::Console::VirtualTerminal;
 class Microsoft::Console::VirtualTerminal::TestStateMachineEngine : public IStateMachineEngine
 {
 public:
+    void ResetTestState()
+    {
+        printed.clear();
+        passedThrough.clear();
+        csiParams.reset();
+    }
+
     bool ActionExecute(const wchar_t /* wch */) override { return true; };
     bool ActionExecuteFromEscape(const wchar_t /* wch */) override { return true; };
     bool ActionPrint(const wchar_t /* wch */) override { return true; };
     bool ActionPrintString(const std::wstring_view string) override
     {
-        printed = string;
+        printed += string;
         return true;
     };
 
     bool ActionPassThroughString(const std::wstring_view string) override
     {
-        passedThrough = string;
+        passedThrough += string;
         return true;
     };
 
@@ -52,7 +59,15 @@ public:
 
     bool ActionOscDispatch(const wchar_t /* wch */,
                            const size_t /* parameter */,
-                           const std::wstring_view /* string */) override { return true; };
+                           const std::wstring_view /* string */) override
+    {
+        if (pfnFlushToTerminal)
+        {
+            pfnFlushToTerminal();
+            return true;
+        }
+        return true;
+    };
 
     bool ActionSs3Dispatch(const wchar_t /* wch */,
                            const std::basic_string_view<size_t> /* parameters */) override { return true; };
@@ -111,6 +126,7 @@ class Microsoft::Console::VirtualTerminal::StateMachineTest
     TEST_METHOD(PassThroughUnhandled);
     TEST_METHOD(RunStorageBeforeEscape);
     TEST_METHOD(BulkTextPrint);
+    TEST_METHOD(PassThroughUnhandledSplitAcrossWrites);
 };
 
 void StateMachineTest::TwoStateMachinesDoNotInterfereWithEachother()
@@ -182,3 +198,49 @@ void StateMachineTest::BulkTextPrint()
     // Then ensure the entire buffered run was printed all at once back to us.
     VERIFY_ARE_EQUAL(String(L"12345 Hello World"), String(engine.printed.c_str()));
 }
+
+void StateMachineTest::PassThroughUnhandledSplitAcrossWrites()
+{
+    auto enginePtr{ std::make_unique<TestStateMachineEngine>() };
+    // this dance is required because StateMachine presumes to take ownership of its engine.
+    auto& engine{ *enginePtr.get() };
+    StateMachine machine{ std::move(enginePtr) };
+
+    // Hook up the passthrough function.
+    engine.pfnFlushToTerminal = std::bind(&StateMachine::FlushToTerminal, &machine);
+
+    // Broken in two pieces (test case from GH#3081)
+    machine.ProcessString(L"\x1b[?12");
+    VERIFY_ARE_EQUAL(L"", engine.passedThrough); // nothing out yet
+    VERIFY_ARE_EQUAL(L"", engine.printed);
+
+    machine.ProcessString(L"34h");
+    VERIFY_ARE_EQUAL(L"\x1b[?1234h", engine.passedThrough); // whole sequence out, no other output
+    VERIFY_ARE_EQUAL(L"", engine.printed);
+
+    engine.ResetTestState();
+
+    // Three pieces
+    machine.ProcessString(L"\x1b[?2");
+    VERIFY_ARE_EQUAL(L"", engine.passedThrough); // nothing out yet
+    VERIFY_ARE_EQUAL(L"", engine.printed);
+
+    machine.ProcessString(L"34");
+    VERIFY_ARE_EQUAL(L"", engine.passedThrough); // nothing out yet
+    VERIFY_ARE_EQUAL(L"", engine.printed);
+
+    machine.ProcessString(L"5h");
+    VERIFY_ARE_EQUAL(L"\x1b[?2345h", engine.passedThrough); // whole sequence out, no other output
+    VERIFY_ARE_EQUAL(L"", engine.printed);
+
+    engine.ResetTestState();
+
+    // Split during OSC terminator (test case from GH#3080)
+    machine.ProcessString(L"\x1b]99;foo\x1b");
+    VERIFY_ARE_EQUAL(L"", engine.passedThrough); // nothing out yet
+    VERIFY_ARE_EQUAL(L"", engine.printed);
+
+    machine.ProcessString(L"\\");
+    VERIFY_ARE_EQUAL(L"\x1b]99;foo\x1b\\", engine.passedThrough);
+    VERIFY_ARE_EQUAL(L"", engine.printed);
+}