From cd57f2c24e2852cd6094acf2159ff2a867936c59 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 8 Apr 2022 19:26:16 +0200 Subject: [PATCH] Fix DBCS attribute corruption during reflow (#12853) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 855e136 contains a regression which breaks buffer reflow if wide surrogate characters are present. This happens because we made use of the `TextBufferCellIterator` whose increment operator skips 2 cells for wide characters. This created a "misalignment" in the reflow logic which was written for cell-wise iteration. This commit fixes the issue, by reverting back to the previous algorithm without iterators. Closes #12837 Closes MSFT-38904421 ## Validation Steps Performed * Run ``pwsh -noprofile -command echo "`u{D83D}`u{DE43}"`` * Resizing conhost preserves all contents ✅ * Resizing Windows Terminal doesn't crash it ✅ * Added a test covering this issue ✅ (cherry picked from commit 10b9044120785735adda548a951329557116a6ab) Service-Card-Id: 80340089 Service-Version: 1.12 --- src/buffer/out/OutputCellIterator.cpp | 4 ++-- src/buffer/out/OutputCellIterator.hpp | 2 +- src/buffer/out/textBuffer.cpp | 18 +++++------------- src/host/ut_host/ScreenBufferTests.cpp | 7 ++++++- 4 files changed, 14 insertions(+), 17 deletions(-) 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 legacyAttributes) noexcept; OutputCellIterator(const gsl::span charInfos) noexcept; OutputCellIterator(const gsl::span cells); diff --git a/src/buffer/out/textBuffer.cpp b/src/buffer/out/textBuffer.cpp index 1939fafb506..6a86f089b60 100644 --- a/src/buffer/out/textBuffer.cpp +++ b/src/buffer/out/textBuffer.cpp @@ -2245,8 +2245,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++) { @@ -2259,9 +2257,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)) { @@ -2270,9 +2268,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. @@ -2303,21 +2298,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 8d1178efeb7..2bb11c6cced 100644 --- a/src/host/ut_host/ScreenBufferTests.cpp +++ b/src/host/ut_host/ScreenBufferTests.cpp @@ -6296,6 +6296,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 @@ -6317,7 +6318,11 @@ void ScreenBufferTests::TestReflowEndOfLineColor() TestUtils::VerifyLineContains(iter2, L' ', defaultAttrs, static_cast(width - 5)); auto iter3 = tb.GetCellLineDataAt({ 0, 3 }); - TestUtils::VerifyLineContains(iter3, L' ', yellow, static_cast(width)); + TestUtils::VerifyLineContains(iter3, L"\U0001F643", yellow, 2u); + TestUtils::VerifyLineContains(iter3, L' ', defaultAttrs, static_cast(width - 2)); + + auto iter4 = tb.GetCellLineDataAt({ 0, 4 }); + TestUtils::VerifyLineContains(iter4, L' ', yellow, static_cast(width)); }; Log::Comment(L"========== Checking the buffer state (before) ==========");