Ignore CHAR_INFO trailers during WriteConsoleOutput (#14840)

#13626 contains a small "regression" compared to #13321:
It now began to store trailers in the buffer wherever possible to allow
a region
of the buffer to be backed up and restored via Read/WriteConsoleOutput.
But we're unfortunately still ill-equipped to handle anything but UCS-2
via
WriteConsoleOutput, so it's best to again ignore trailers just like in
#13321.

## Validation Steps Performed
* Added unit test ✅

(cherry picked from commit 9dcdcac)
Service-Card-Id: 88719297
Service-Version: 1.17

src/buffer/out/OutputCellIterator.cpp

@@ -198,6 +198,11 @@ OutputCellIterator::operator bool() const noexcept
     CATCH_FAIL_FAST();
 }
 
+size_t OutputCellIterator::Position() const noexcept
+{
+    return _pos;
+}
+
 // Routine Description:
 // - Advances the iterator one position over the underlying data source.
 // Return Value:

src/buffer/out/OutputCellIterator.hpp

@@ -48,6 +48,7 @@ class OutputCellIterator final
 
     operator bool() const noexcept;
 
+    size_t Position() const noexcept;
     til::CoordType GetCellDistance(OutputCellIterator other) const noexcept;
     til::CoordType GetInputDistance(OutputCellIterator other) const noexcept;
     friend til::CoordType operator-(OutputCellIterator one, OutputCellIterator two) = delete;

src/buffer/out/Row.cpp

@@ -311,16 +311,20 @@ OutputCellIterator ROW::WriteCells(OutputCellIterator it, const til::CoordType c
                 }
                 break;
             case DbcsAttribute::Trailing:
-                // Handling the trailing half of wide chars ensures that we correctly restore
-                // wide characters when a user backs up and restores the viewport via CHAR_INFOs.
                 if (fillingFirstColumn)
                 {
                     // The wide char doesn't fit. Pad with whitespace.
                     // Ignore the character. There's no correct alternative way to handle this situation.
                     ClearCell(currentIndex);
                 }
-                else
+                else if (it.Position() == 0)
                 {
+                    // A common way to back up and restore the buffer is via `ReadConsoleOutputW` and
+                    // `WriteConsoleOutputW` respectively. But the area might bisect/intersect/clip wide characters and
+                    // only backup either their leading or trailing half. In general, in the rest of conhost, we're
+                    // throwing away the trailing half of all `CHAR_INFO`s (during text rendering, as well as during
+                    // `ReadConsoleOutputW`), so to make this code behave the same and prevent surprises, we need to
+                    // make sure to only look at the trailer if it's the first `CHAR_INFO` the user is trying to write.
                     ReplaceCharacters(currentIndex - 1, 2, chars);
                 }
                 ++it;

src/host/ft_host/CJK_DbcsTests.cpp

@@ -80,8 +80,7 @@ namespace DbcsWriteRead
                         const bool fReadUnicode,
                         CharInfoPattern& rgChars);
 
-    void Verify(const CharInfoPattern& rgExpected,
-                const CharInfoPattern& rgActual);
+    void Verify(std::span<CHAR_INFO> rgExpected, std::span<CHAR_INFO> rgActual);
 
     void PrepExpected(
         const WORD wAttrWritten,
@@ -158,6 +157,10 @@ class DbcsTests
     BEGIN_TEST_METHOD(TestDbcsBackupRestore)
         TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
     END_TEST_METHOD()
+
+    BEGIN_TEST_METHOD(TestInvalidTrailer)
+        TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
+    END_TEST_METHOD()
 };
 
 bool DbcsTests::DbcsTestSetup()
@@ -422,7 +425,7 @@ namespace PrepPattern
     static constexpr WORD leading = COMMON_LVB_LEADING_BYTE;
     static constexpr WORD trailing = COMMON_LVB_TRAILING_BYTE;
 
-    constexpr void replaceColorPlaceholders(CharInfoPattern& pattern, WORD attr)
+    constexpr void replaceColorPlaceholders(std::span<CHAR_INFO> pattern, WORD attr)
     {
         for (auto& info : pattern)
         {
@@ -1317,9 +1320,9 @@ void DbcsWriteRead::RetrieveOutput(const HANDLE hOut,
     }
 }
 
-void DbcsWriteRead::Verify(const CharInfoPattern& rgExpected,
-                           const CharInfoPattern& rgActual)
+void DbcsWriteRead::Verify(std::span<CHAR_INFO> rgExpected, std::span<CHAR_INFO> rgActual)
 {
+    VERIFY_ARE_EQUAL(rgExpected.size(), rgActual.size());
     // We will walk through for the number of CHAR_INFOs expected.
     for (size_t i = 0; i < rgExpected.size(); i++)
     {
@@ -2049,60 +2052,103 @@ void DbcsTests::TestDbcsStdCoutScenario()
 // In other words, writing a trailing CHAR_INFO will also automatically write a leading CHAR_INFO in the preceding cell.
 void DbcsTests::TestDbcsBackupRestore()
 {
+    static_assert(PrepPattern::DoubledW.size() == 16);
+
     const auto out = GetStdHandle(STD_OUTPUT_HANDLE);
 
-    std::array<CHAR_INFO, 16> expected = PrepPattern::DoubledW;
+    // We backup/restore 2 lines at once to ensure that it works even then. After all, an incorrect implementation
+    // might ignore all but the absolutely first CHAR_INFO instead of handling the first CHAR_INFO *on each row*.
+    std::array<CHAR_INFO, 32> expected;
+    std::ranges::copy(PrepPattern::DoubledW, expected.begin() + 0);
+    std::ranges::copy(PrepPattern::DoubledW, expected.begin() + 16);
+
     PrepPattern::replaceColorPlaceholders(expected, FOREGROUND_BLUE | FOREGROUND_INTENSITY | BACKGROUND_GREEN);
 
     // DoubledW will show up like this in the top/left corner of the terminal:
     // +----------------
     // |QいかなZYXWVUTに
+    // |QいかなZYXWVUTに
     //
     // Since those 4 Japanese characters probably aren't going to be monospace for you in your editor
     // (as they most likely aren't exactly 2 ASCII characters wide), I'll continue referring to them like this:
     // +----------------
     // |QaabbccZYXWVUTdd
+    // |QaabbccZYXWVUTdd
     {
-        SMALL_RECT region{ .Left = 0, .Right = 15 };
-        VERIFY_WIN32_BOOL_SUCCEEDED(WriteConsoleOutputW(out, expected.data(), { 16, 1 }, {}, &region));
+        SMALL_RECT region{ 0, 0, 15, 1 };
+        VERIFY_WIN32_BOOL_SUCCEEDED(WriteConsoleOutputW(out, expected.data(), { 16, 2 }, {}, &region));
     }
 
     // Make a "backup" of the viewport. The twist is that our backup region only
     // copies the trailing/leading half of the first/last glyph respectively like so:
     // +----------------
     // |  abbccZYXWVUTd
-    std::array<CHAR_INFO, 13> backup{};
-    constexpr COORD backupSize{ 13, 1 };
-    SMALL_RECT backupRegion{ .Left = 2, .Right = 14 };
+    std::array<CHAR_INFO, 26> backup{};
+    constexpr COORD backupSize{ 13, 2 };
+    SMALL_RECT backupRegion{ 2, 0, 14, 1 };
     VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputW(out, backup.data(), backupSize, {}, &backupRegion));
 
     // Destroy the text with some narrow ASCII characters, resulting in:
     // +----------------
     // |Qxxxxxxxxxxxxxxx
+    // |Qxxxxxxxxxxxxxxx
     {
         DWORD ignored;
         VERIFY_WIN32_BOOL_SUCCEEDED(FillConsoleOutputCharacterW(out, L'x', 15, { 1, 0 }, &ignored));
+        VERIFY_WIN32_BOOL_SUCCEEDED(FillConsoleOutputCharacterW(out, L'x', 15, { 1, 1 }, &ignored));
     }
 
     // Restore our "backup". The trailing half of the first wide glyph (indicated as "a" above)
     // as well as the leading half of the last wide glyph ("d"), will automatically get a
     // matching leading/trailing half respectively. In other words, this:
     // +----------------
     // |  abbccZYXWVUTd
+    // |  abbccZYXWVUTd
     //
     // turns into this:
     // +----------------
     // | aabbccZYXWVUTdd
+    // | aabbccZYXWVUTdd
     //
     // and so we restore this, overwriting all the "x" characters in the process:
     // +----------------
     // |QいかなZYXWVUTに
+    // |QいかなZYXWVUTに
     VERIFY_WIN32_BOOL_SUCCEEDED(WriteConsoleOutputW(out, backup.data(), backupSize, {}, &backupRegion));
 
-    std::array<CHAR_INFO, 16> infos{};
+    std::array<CHAR_INFO, 32> infos{};
     {
-        SMALL_RECT region{ .Left = 0, .Right = 15 };
-        VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputW(out, infos.data(), { 16, 1 }, {}, &region));
+        SMALL_RECT region{ 0, 0, 15, 1 };
+        VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputW(out, infos.data(), { 16, 2 }, {}, &region));
     }
     DbcsWriteRead::Verify(expected, infos);
 }
+
+// As tested by TestDbcsBackupRestore(), we do want to allow users to write trailers into the buffer, to allow
+// for an area of the buffer to be backed up and restored via Read/WriteConsoleOutput. But apart from that use
+// case, we'd generally do best to avoid trailers whenever possible, as conhost basically ignored them in the
+// past and only rendered leaders. Applications might now be relying on us effectively ignoring trailers.
+void DbcsTests::TestInvalidTrailer()
+{
+    auto expected = PrepPattern::DoubledW;
+    auto input = expected;
+    decltype(input) output{};
+
+    for (auto& v : input)
+    {
+        if (WI_IsFlagSet(v.Attributes, COMMON_LVB_TRAILING_BYTE))
+        {
+            v.Char.UnicodeChar = 0xfffd;
+        }
+    }
+
+    {
+        static constexpr COORD bufferSize{ 16, 1 };
+        SMALL_RECT region{ 0, 0, 15, 0 };
+        const auto out = GetStdHandle(STD_OUTPUT_HANDLE);
+        VERIFY_WIN32_BOOL_SUCCEEDED(WriteConsoleOutputW(out, input.data(), bufferSize, {}, &region));
+        VERIFY_WIN32_BOOL_SUCCEEDED(ReadConsoleOutputW(out, output.data(), bufferSize, {}, &region));
+    }
+
+    DbcsWriteRead::Verify(expected, output);
+}