From d6776f51f62486bb3abe3920de8be0daf4de2c3c Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 8 Jan 2020 20:10:23 +0100 Subject: [PATCH 1/3] Fixed a deadlock when printing surrogate pairs Fixes GH#4145. --- src/cascadia/TerminalCore/Terminal.cpp | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 65038eb9ba1..f6f030b6195 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -456,21 +456,17 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView) { // TODO: MSFT 21006766 // This is not great but I need it demoable. Fix by making a buffer stream writer. - if (wch >= 0xD800 && wch <= 0xDFFF) - { - const OutputCellIterator it{ stringView.substr(i, 2), _buffer->GetCurrentAttributes() }; - const auto end = _buffer->Write(it); - const auto cellDistance = end.GetCellDistance(it); - i += cellDistance - 1; - proposedCursorPosition.X += gsl::narrow(cellDistance); - } - else - { - const OutputCellIterator it{ stringView.substr(i, 1), _buffer->GetCurrentAttributes() }; - const auto end = _buffer->Write(it); - const auto cellDistance = end.GetCellDistance(it); - proposedCursorPosition.X += gsl::narrow(cellDistance); - } + // + // If wch is a surrogate character we need to read 2 code units + // from the stringView to form a single code point. + const auto isSurrogate = wch >= 0xD800 && wch <= 0xDFFF; + const size_t codePointLength = isSurrogate ? 2 : 1; + const auto view = stringView.substr(i, codePointLength); + const OutputCellIterator it{ view, _buffer->GetCurrentAttributes() }; + const auto end = _buffer->Write(it); + const auto cellDistance = end.GetCellDistance(it); + proposedCursorPosition.X += gsl::narrow(cellDistance); + i += codePointLength - 1; } // If we're about to scroll past the bottom of the buffer, instead cycle the buffer. From ca65a41e8ce4b85f5fdeb741d789e6b3be1e6567 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 8 Jan 2020 22:44:52 +0100 Subject: [PATCH 2/3] Addressed review comments --- src/cascadia/TerminalCore/Terminal.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index f6f030b6195..77f3afd7f0c 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -460,13 +460,11 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView) // If wch is a surrogate character we need to read 2 code units // from the stringView to form a single code point. const auto isSurrogate = wch >= 0xD800 && wch <= 0xDFFF; - const size_t codePointLength = isSurrogate ? 2 : 1; - const auto view = stringView.substr(i, codePointLength); + const auto view = stringView.substr(i, isSurrogate ? 2 : 1); const OutputCellIterator it{ view, _buffer->GetCurrentAttributes() }; const auto end = _buffer->Write(it); - const auto cellDistance = end.GetCellDistance(it); - proposedCursorPosition.X += gsl::narrow(cellDistance); - i += codePointLength - 1; + proposedCursorPosition.X += gsl::narrow(end.GetCellDistance(it)); + i += end.GetInputDistance(it) - 1; } // If we're about to scroll past the bottom of the buffer, instead cycle the buffer. From c4c07be0242bde3c9b69ab9ee46f8a540d96ee92 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Fri, 10 Jan 2020 00:53:43 +0100 Subject: [PATCH 3/3] Added a surrogate printing test case --- src/cascadia/TerminalCore/Terminal.cpp | 23 +++++- .../TerminalApiTest.cpp | 72 +++++++++++++++++++ 2 files changed, 93 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp index 77f3afd7f0c..f71ffb76472 100644 --- a/src/cascadia/TerminalCore/Terminal.cpp +++ b/src/cascadia/TerminalCore/Terminal.cpp @@ -463,8 +463,27 @@ void Terminal::_WriteBuffer(const std::wstring_view& stringView) const auto view = stringView.substr(i, isSurrogate ? 2 : 1); const OutputCellIterator it{ view, _buffer->GetCurrentAttributes() }; const auto end = _buffer->Write(it); - proposedCursorPosition.X += gsl::narrow(end.GetCellDistance(it)); - i += end.GetInputDistance(it) - 1; + const auto cellDistance = end.GetCellDistance(it); + const auto inputDistance = end.GetInputDistance(it); + + if (inputDistance > 0) + { + // If "wch" was a surrogate character, we just consumed 2 code units above. + // -> Increment "i" by 1 in that case and thus by 2 in total in this iteration. + proposedCursorPosition.X += gsl::narrow(cellDistance); + i += inputDistance - 1; + } + else + { + // If _WriteBuffer() is called with a consecutive string longer than the viewport/buffer width + // the call to _buffer->Write() will refuse to write anything on the current line. + // GetInputDistance() thus returns 0, which would in turn cause i to be + // decremented by 1 below and force the outer loop to loop forever. + // This if() basically behaves as if "\r\n" had been encountered above and retries the write. + // With well behaving shells during normal operation this safeguard should normally not be encountered. + proposedCursorPosition.X = 0; + proposedCursorPosition.Y++; + } } // If we're about to scroll past the bottom of the buffer, instead cycle the buffer. diff --git a/src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp b/src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp index a4c08504b71..45a64e0f1c5 100644 --- a/src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp +++ b/src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp @@ -12,6 +12,9 @@ using namespace winrt::Microsoft::Terminal::Settings; using namespace Microsoft::Terminal::Core; +using namespace WEX::Logging; +using namespace WEX::TestExecution; + namespace TerminalCoreUnitTests { #define WCS(x) WCSHELPER(x) @@ -37,5 +40,74 @@ namespace TerminalCoreUnitTests VERIFY_IS_FALSE(term.SetColorTableEntry(256, 100)); VERIFY_IS_FALSE(term.SetColorTableEntry(512, 100)); } + + // Terminal::_WriteBuffer used to enter infinite loops under certain conditions. + // This test ensures that Terminal::_WriteBuffer doesn't get stuck when + // PrintString() is called with more code units than the buffer width. + TEST_METHOD(PrintStringOfSurrogatePairs) + { + DummyRenderTarget renderTarget; + Terminal term; + term.Create({ 100, 100 }, 3, renderTarget); + + std::wstring text; + text.reserve(600); + + for (size_t i = 0; i < 100; ++i) + { + text.append(L"𐐌𐐜𐐬"); + } + + struct Baton + { + HANDLE done; + std::wstring text; + Terminal* pTerm; + } baton{ + CreateEventW(nullptr, TRUE, FALSE, L"done signal"), + text, + &term, + }; + + Log::Comment(L"Launching thread to write data."); + const auto thread = CreateThread( + nullptr, + 0, + [](LPVOID data) -> DWORD { + const Baton& baton = *reinterpret_cast(data); + Log::Comment(L"Writing data."); + baton.pTerm->PrintString(baton.text); + Log::Comment(L"Setting event."); + SetEvent(baton.done); + return 0; + }, + (LPVOID)&baton, + 0, + nullptr); + + Log::Comment(L"Waiting for the write."); + switch (WaitForSingleObject(baton.done, 2000)) + { + case WAIT_OBJECT_0: + Log::Comment(L"Didn't get stuck. Success."); + break; + case WAIT_TIMEOUT: + Log::Comment(L"Wait timed out. It got stuck."); + Log::Result(WEX::Logging::TestResults::Failed); + break; + case WAIT_FAILED: + Log::Comment(L"Wait failed for some reason. We didn't expect this."); + Log::Result(WEX::Logging::TestResults::Failed); + break; + default: + Log::Comment(L"Wait return code that no one expected. Fail."); + Log::Result(WEX::Logging::TestResults::Failed); + break; + } + + TerminateThread(thread, 0); + CloseHandle(baton.done); + return; + } }; }