Ignore CHAR_INFO trailers during WriteConsoleOutput

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);
+}