Skip to content

Commit

Permalink
Fixed a deadlock when printing surrogate pairs (#4150)
Browse files Browse the repository at this point in the history 
## Summary of the Pull Request

See [my code comment](#4150 (comment)) below for technical details of the issue that caused #4145.

## PR Checklist
* [x] Closes #1360, Closes #4145.
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

## Detailed Description of the Pull Request / Additional comments

TBH I kinda hope this project could migrate to an internal use of UTF-8 in the future. 😶

## Validation Steps Performed

Followed the "Steps to reproduce" in #4145 and ensured the "Expected behavior" happens.
  • Loading branch information
@lhecker
lhecker authored and msftbot[bot] committed Jan 15, 2020
1 parent cc9d2ca commit 3e6b4b5
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 9 deletions.
31 changes: 22 additions & 9 deletions src/cascadia/TerminalCore/Terminal.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -456,20 +456,33 @@ 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)
//
// 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 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);
const auto inputDistance = end.GetInputDistance(it);

if (inputDistance > 0)
{
const OutputCellIterator it{ stringView.substr(i, 2), _buffer->GetCurrentAttributes() };
const auto end = _buffer->Write(it);
const auto cellDistance = end.GetCellDistance(it);
i += cellDistance - 1;
// 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<SHORT>(cellDistance);
i += inputDistance - 1;
}
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<SHORT>(cellDistance);
// 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++;
}
}

Expand Down
72 changes: 72 additions & 0 deletions src/cascadia/UnitTests_TerminalCore/TerminalApiTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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<Baton*>(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;
}
};
}

0 comments on commit 3e6b4b5

Please sign in to comment.