Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly implement WaitForData for ReadConsoleInput #18228

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Nov 21, 2024

There were two bugs:

  • Ever since the conhost v1 -> v2 rewrite the readDataDirect.cpp
    implementation incorrectly passed false as the wait flag.
    The unintentional mistake is obvious in hindsight as the
    check for CONSOLE_STATUS_WAIT makes no sense in this case.
  • The ConPTY integration into InputBuffer was done incorrectly,
    as it would unconditionally wake up the readers/waiters without
    checking if the buffer is now actually non-empty.

Closes #15859

Validation Steps Performed

Test code:

#include <Windows.h>
#include <stdio.h>

int main() {
    HANDLE in = GetStdHandle(STD_INPUT_HANDLE);
    INPUT_RECORD buf[128];
    DWORD read;

    SetConsoleMode(
        in,
        ENABLE_PROCESSED_INPUT | ENABLE_VIRTUAL_TERMINAL_INPUT
    );

    for (int i = 0; ReadConsoleInputW(in, buf, 128, &read); ++i) {
        printf("%d read=%lu\n", i, read);
    }

    return 0;
}

Run it under Windows Terminal and type any input. >50% of all
inputs will result in read=0. This is fixed after this PR.

@lhecker lhecker force-pushed the dev/lhecker/15859-read-console-input branch from dab80c8 to 2388624 Compare November 21, 2024 00:43
@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase labels Nov 21, 2024
@@ -492,8 +492,7 @@ size_t InputBuffer::Prepend(const std::span<const INPUT_RECORD>& inEvents)
return STATUS_SUCCESS;
}

_vtInputShouldSuppress = true;
auto resetVtInputSuppress = wil::scope_exit([&]() { _vtInputShouldSuppress = false; });
const auto wakeup = _wakeupReadersOnExit();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much cleaner and safer. Also, it's more correct. We would previously call WakeUpReadersWaitingForData unconditionally but the storage could still be empty. This error has probably been in conhost since v1.

// - inEvents - Series of input records to insert into the buffer
// Return Value:
// - <none>
void InputBuffer::_HandleTerminalInputCallback(const TerminalInput::StringType& text)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second fix. It's also redundant with _writeString, especially with _vtInputShouldSuppress kicked out.

@@ -107,7 +107,7 @@ try
*pReplyStatus = _pInputBuffer->Read(_outEvents,
amountToRead,
false,
false,
true,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first fix.

Comment on lines -200 to -208
if (fIsWaitAllowed)
{
hr = ConsoleWaitQueue::s_CreateWait(m, waiter.release());
if (SUCCEEDED(hr))
{
*pbReplyPending = TRUE;
hr = CONSOLE_STATUS_WAIT;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes no sense IMO to check this here if we can just pass down fIsWaitAllowed and avoid a waiter from being created in the first place.

Comment on lines -2307 to +2314
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.ActiveCfg = Debug|Win32
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.ActiveCfg = Release|ARM64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.ActiveCfg = Release|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.ActiveCfg = Release|Win32
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.ActiveCfg = AuditMode|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.Build.0 = AuditMode|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.ActiveCfg = AuditMode|ARM64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.Build.0 = AuditMode|ARM64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.ActiveCfg = AuditMode|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.Build.0 = AuditMode|x64
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.ActiveCfg = AuditMode|Win32
{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.Build.0 = AuditMode|Win32
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Double checking) intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I was certain I reverted this...

@@ -56,6 +56,7 @@ using Microsoft::Console::Interactivity::ServiceLocator;
INPUT_READ_HANDLE_DATA& readHandleState,
const bool IsUnicode,
const bool IsPeek,
const bool IsWaitAllowed,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: update the function comment above

@@ -702,13 +674,11 @@ static bool IsPauseKey(const KEY_EVENT_RECORD& event)
// Note:
// - The console lock must be held when calling this routine.
// - will throw on failure
void InputBuffer::_WriteBuffer(const std::span<const INPUT_RECORD>& inEvents, _Out_ size_t& eventsWritten, _Out_ bool& setWaitEvent)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: setWaitEvent is still documented above. It should be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReadConsoleInput can read 0 characters and return success and it almost certainly shouldn't
2 participants