Repository: microsoft/terminal (language: cpp)

Issue:
Terminal crash on high unicode chars: out of range exception reading UnicodeStorage during UpdatePatternLocations
1.12.10908

Uncertain as to repro steps.

Watson failure hash 326e3a1b-889b-f24e-582d-0e18d3c853b4

<html>
<body>
<!--StartFragment-->

Frame | Name | | offset | | | | file || line
-- | -- | -- | -- | -- | -- | -- | -- | -- | --
0 | KERNELBASE!RaiseException |   | 0x000000000000006C |   |   |   | xcpt.c |   | 936
1 | VCRUNTIME140!_CxxThrowException |   | 0x0000000000000090 |   |   |   | throw.cpp |   | 75
2 | msvcp140!std::_Xout_of_range |   | 0x0000000000000022 |   |   |   | xthrow.cpp |   | 25
3 | Microsoft_Terminal_Control!CharRow::GetText |   | 0x000000000004414C |   |   |   | CharRow.cpp |   | 217
4 | Microsoft_Terminal_Control!TextBuffer::GetPatterns |   | 0x00000000000000C0 |   |   |   | textBuffer.cpp |   | 2665
5 | Microsoft_Terminal_Control!Microsoft::Terminal::Core::Terminal::UpdatePatternsUnderLock |   | 0x000000000000008E |   |   |   | Terminal.cpp |   | 1255
6 | Microsoft_Terminal_Control!winrt::Microsoft::Terminal::Control::implementation::ControlCore::UpdatePatternLocations |   | 0x0000000000000044 |   |   |   | ControlCore.cpp |   | 494

<!--EndFragment-->
</body>
</html>


Discussion / hints:
IT'S SUPER BAD
Wait, this _isn't_ the UnicodeStorage one

Fix patch:
diff --git a/src/buffer/out/OutputCellIterator.cpp b/src/buffer/out/OutputCellIterator.cpp
index d32087fe9e6..b6658f2900a 100644
--- a/src/buffer/out/OutputCellIterator.cpp
+++ b/src/buffer/out/OutputCellIterator.cpp
@@ -97,14 +97,14 @@ OutputCellIterator::OutputCellIterator(const std::wstring_view utf16Text) :
 // Arguments:
 // - utf16Text - UTF-16 text range
 // - attribute - Color to apply over the entire range
-OutputCellIterator::OutputCellIterator(const std::wstring_view utf16Text, const TextAttribute attribute) :
+OutputCellIterator::OutputCellIterator(const std::wstring_view utf16Text, const TextAttribute& attribute, const size_t fillLimit) :
     _mode(Mode::Loose),
     _currentView(s_GenerateView(utf16Text, attribute)),
     _run(utf16Text),
     _attr(attribute),
     _distance(0),
     _pos(0),
-    _fillLimit(0)
+    _fillLimit(fillLimit)
 {
 }
 
diff --git a/src/buffer/out/OutputCellIterator.hpp b/src/buffer/out/OutputCellIterator.hpp
index 59c6007aa3a..6f05bb792cc 100644
--- a/src/buffer/out/OutputCellIterator.hpp
+++ b/src/buffer/out/OutputCellIterator.hpp
@@ -38,7 +38,7 @@ class OutputCellIterator final
     OutputCellIterator(const wchar_t& wch, const TextAttribute& attr, const size_t fillLimit = 0) noexcept;
     OutputCellIterator(const CHAR_INFO& charInfo, const size_t fillLimit = 0) noexcept;
     OutputCellIterator(const std::wstring_view utf16Text);
-    OutputCellIterator(const std::wstring_view utf16Text, const TextAttribute attribute);
+    OutputCellIterator(const std::wstring_view utf16Text, const TextAttribute& attribute, const size_t fillLimit = 0);
     OutputCellIterator(const gsl::span<const WORD> legacyAttributes) noexcept;
     OutputCellIterator(const gsl::span<const CHAR_INFO> charInfos) noexcept;
     OutputCellIterator(const gsl::span<const OutputCell> cells);
diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp
index c59a7308ead..274bde52ebf 100644
--- a/src/buffer/out/textBuffer.cpp
+++ b/src/buffer/out/textBuffer.cpp
@@ -2297,8 +2297,6 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
         // the "right" boundary, which is one past the final valid
         // character)
         short iOldCol = 0;
-        auto chars{ row.GetCharRow().cbegin() };
-        auto attrs{ row.GetAttrRow().begin() };
         const auto copyRight = iRight;
         for (; iOldCol < copyRight; iOldCol++)
         {
@@ -2311,9 +2309,9 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
             try
             {
                 // TODO: MSFT: 19446208 - this should just use an iterator and the inserter...
-                const auto glyph = chars->Char();
-                const auto dbcsAttr = chars->DbcsAttr();
-                const auto textAttr = *attrs;
+                const auto glyph = row.GetCharRow().GlyphAt(iOldCol);
+                const auto dbcsAttr = row.GetCharRow().DbcsAttrAt(iOldCol);
+                const auto textAttr = row.GetAttrRow().GetAttrByColumn(iOldCol);
 
                 if (!newBuffer.InsertCharacter(glyph, dbcsAttr, textAttr))
                 {
@@ -2322,9 +2320,6 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
                 }
             }
             CATCH_RETURN();
-
-            ++chars;
-            ++attrs;
         }
 
         // GH#32: Copy the attributes from the rest of the row into this new buffer.
@@ -2355,21 +2350,18 @@ HRESULT TextBuffer::Reflow(TextBuffer& oldBuffer,
         // for inserting an attr would be past the right of the new buffer.
         for (short copyAttrCol = iOldCol;
              copyAttrCol < cOldColsTotal && newAttrColumn < newWidth;
-             copyAttrCol++)
+             copyAttrCol++, newAttrColumn++)
         {
             try
             {
                 // TODO: MSFT: 19446208 - this should just use an iterator and the inserter...
-                const auto textAttr = *attrs;
+                const auto textAttr = row.GetAttrRow().GetAttrByColumn(copyAttrCol);
                 if (!newRow.GetAttrRow().SetAttrToEnd(newAttrColumn, textAttr))
                 {
                     break;
                 }
             }
             CATCH_LOG(); // Not worth dying over.
-
-            ++newAttrColumn;
-            ++attrs;
         }
 
         // If we found the old row that the caller was interested in, set the
diff --git a/src/host/ut_host/ScreenBufferTests.cpp b/src/host/ut_host/ScreenBufferTests.cpp
index fa9cf131963..68aec670704 100644
--- a/src/host/ut_host/ScreenBufferTests.cpp
+++ b/src/host/ut_host/ScreenBufferTests.cpp
@@ -6315,6 +6315,7 @@ void ScreenBufferTests::TestReflowEndOfLineColor()
     stateMachine.ProcessString(L"\x1b[44m"); // Blue BG
     stateMachine.ProcessString(L" CCC \n"); // " abc " (with spaces on either side)
     stateMachine.ProcessString(L"\x1b[43m"); // yellow BG
+    stateMachine.ProcessString(L"\U0001F643\n"); // GH#12837: Support for surrogate characters during reflow
     stateMachine.ProcessString(L"\x1b[K"); // Erase line
     stateMachine.ProcessString(L"\x1b[2;6H"); // move the cursor to the end of the BBBBB's
 
@@ -6336,7 +6337,11 @@ void ScreenBufferTests::TestReflowEndOfLineColor()
         TestUtils::VerifyLineContains(iter2, L' ', defaultAttrs, static_cast<size_t>(width - 5));
 
         auto iter3 = tb.GetCellLineDataAt({ 0, 3 });
-        TestUtils::VerifyLineContains(iter3, L' ', yellow, static_cast<size_t>(width));
+        TestUtils::VerifyLineContains(iter3, L"\U0001F643", yellow, 2u);
+        TestUtils::VerifyLineContains(iter3, L' ', defaultAttrs, static_cast<size_t>(width - 2));
+
+        auto iter4 = tb.GetCellLineDataAt({ 0, 4 });
+        TestUtils::VerifyLineContains(iter4, L' ', yellow, static_cast<size_t>(width));
     };
 
     Log::Comment(L"========== Checking the buffer state (before) ==========");


Test patch: