From 7ac2d6d9719ef5715a27f39121bb1ca69eb337e4 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Wed, 2 Feb 2022 17:05:40 -0600 Subject: [PATCH] I think this is a valid test for #8663 --- src/host/directio.cpp | 10 +++++ src/host/inputBuffer.cpp | 9 ++-- src/host/readDataDirect.cpp | 9 ++++ src/host/ut_host/InputBufferTests.cpp | 64 +++++++++++++++++++++++++-- 4 files changed, 86 insertions(+), 6 deletions(-) diff --git a/src/host/directio.cpp b/src/host/directio.cpp index 760a7ecf759..232995e7f07 100644 --- a/src/host/directio.cpp +++ b/src/host/directio.cpp @@ -211,6 +211,16 @@ void EventsToUnicode(_Inout_ std::deque>& inEvents, try { SplitToOem(readEvents); + + // After SplitToOem returns, readEvents might be bigger than it + // was before. It might be more than 2x the size it was before, + // because a single codepoint might have been expanded into more + // that a single char. + // + // As of GH #8663, InputBuffer::Read will have pre-emptively + // checked how much space each key is about to take up, and will + // only return as many as will fit in readBuffer _after_ a call + // to SplitToOem. } CATCH_LOG(); } diff --git a/src/host/inputBuffer.cpp b/src/host/inputBuffer.cpp index 8893e3987d5..8dfd3853a4c 100644 --- a/src/host/inputBuffer.cpp +++ b/src/host/inputBuffer.cpp @@ -434,10 +434,13 @@ void InputBuffer::_ReadBuffer(_Out_ std::deque>& ou } } } - // TODO!: If we do this, then readers who come asking for 4 events might + // When we do this, then readers who come asking for 4 events might // only get 3 back, under the implication that they know they need to - // expand the events themselves. Who are all the InputBuffer::Read - // callers? Are they all expanding (in the case of !unicode) in post? + // expand the events themselves. There are only two callers: + // * DirectReadData::Notify + // * _DoGetConsoleInput + // + // and they both handle !unicode the same way. if (performNormalRead) { diff --git a/src/host/readDataDirect.cpp b/src/host/readDataDirect.cpp index d6a160d5026..d79032c95a0 100644 --- a/src/host/readDataDirect.cpp +++ b/src/host/readDataDirect.cpp @@ -153,6 +153,15 @@ bool DirectReadData::Notify(const WaitTerminationReason TerminationReason, try { SplitToOem(readEvents); + // After SplitToOem returns, readEvents might be bigger than it + // was before. It might be more than 2x the size it was before, + // because a single codepoint might have been expanded into more + // that a single char. + // + // As of GH #8663, InputBuffer::Read will have pre-emptively + // checked how much space each key is about to take up, and will + // only return as many as will fit in readBuffer _after_ a call + // to SplitToOem. } CATCH_LOG(); } diff --git a/src/host/ut_host/InputBufferTests.cpp b/src/host/ut_host/InputBufferTests.cpp index 141f982240d..25349a9f766 100644 --- a/src/host/ut_host/InputBufferTests.cpp +++ b/src/host/ut_host/InputBufferTests.cpp @@ -434,6 +434,17 @@ class InputBufferTests std::deque> outEvents; size_t eventsRead = 0; bool resetWaitEvent = false; + + // GH #8663: We only insert 4 events. but we need to ask for 6 here. + // The Raised Hand emoji is U+270B in utf16, but it's 0xE2 0x9C 0x8B in utf-8. + Log::Comment(fmt::format(L"Codepage: {}", ServiceLocator::LocateGlobals().getConsoleInformation().CP).c_str()); + ServiceLocator::LocateGlobals().getConsoleInformation().CP = 932; + Log::Comment(fmt::format(L"Changed to: {}", ServiceLocator::LocateGlobals().getConsoleInformation().CP).c_str()); + + // GH #8663: We only insert 4 events. When we ask for 4 here., we'll only get 3. + // InpuBuffer::_ReadBuffer, when called with !unicode, will only return + // as many events as _will_ fit into readCount, assuming the caller + // expands them with SplitToOem. inputBuffer._ReadBuffer(outEvents, recordInsertCount, eventsRead, @@ -442,12 +453,59 @@ class InputBufferTests false, false); - // TODO! I broke this test. This test expects 4 events to get read back, regardless of the fact that one would get expanded by ConvertToA. - // This might be okay, the test probably shouldn't be calling the private method directly. + // the dbcs record should have counted for two elements in + // the array, making it so that we get less events read + VERIFY_ARE_EQUAL(recordInsertCount - 1, eventsRead); + VERIFY_ARE_EQUAL(eventsRead, outEvents.size()); + for (size_t i = 0; i < eventsRead; ++i) + { + VERIFY_ARE_EQUAL(outEvents[i]->ToInputRecord(), inRecords[i]); + } + } + + TEST_METHOD(ReadingDbcsCharsPadsOutputArrayForEmoji) + { + Log::Comment(L"During a utf-8 read, make sure the input buffer leaves " + L"enough room for keys that could be expanded into more than two chars."); + + // write a mouse event, key event, dbcs key event, mouse event + InputBuffer inputBuffer; + const unsigned int recordInsertCount = 4; + INPUT_RECORD inRecords[recordInsertCount]; + inRecords[0].EventType = MOUSE_EVENT; + inRecords[1] = MakeKeyEvent(TRUE, 1, L'A', 0, L'A', 0); + inRecords[2] = MakeKeyEvent(TRUE, 1, 0x270B, 0, 0x270B, 0); // U+270B RAISED HAND + inRecords[3].EventType = MOUSE_EVENT; + + std::deque> inEvents; + for (size_t i = 0; i < recordInsertCount; ++i) + { + inEvents.push_back(IInputEvent::Create(inRecords[i])); + } + + inputBuffer.Flush(); + VERIFY_IS_GREATER_THAN(inputBuffer.Write(inEvents), 0u); + + // read them out non-unicode style and compare + std::deque> outEvents; + size_t eventsRead = 0; + bool resetWaitEvent = false; + + // GH #8663: We only insert 4 events. but we need to ask for 6 here. + // The Raised Hand emoji is U+270B in utf16, but it's 0xE2 0x9C 0x8B in utf-8. + ServiceLocator::LocateGlobals().getConsoleInformation().CP = 65001; + + inputBuffer._ReadBuffer(outEvents, + 5, + eventsRead, + false, + resetWaitEvent, + false, + false); // the dbcs record should have counted for two elements in // the array, making it so that we get less events read - VERIFY_ARE_EQUAL(eventsRead, recordInsertCount - 1); + VERIFY_ARE_EQUAL(3, eventsRead); VERIFY_ARE_EQUAL(eventsRead, outEvents.size()); for (size_t i = 0; i < eventsRead; ++i) {