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

cygwin with win32-raw-mode enabled, console consumes all available memory. #17824

Closed
adamyg opened this issue Aug 29, 2024 · 7 comments
Closed
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-External For issues that are outside this codebase

Comments

@adamyg
Copy link

adamyg commented Aug 29, 2024

Windows Terminal version

1.20.11781.0

Windows build number

10.0.19045.4780

Other Software

cygwin64, either

  • console shell running under "c:\cygwin64\bin\bash --login"; or
  • run rawtest application with cygwin.dll in path.

Steps to reproduce

Application
enables win32-input-mode (\033[?9001h), fast key input triggers condition; see attached.
memory-usage

Test application:

/*
 *  win32-input-mode key test
 *
 *  Build: gcc -o conkeytest conkeytest.c
 */

#include <termios.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

static void hex(const void *buf, unsigned len);

int
main(void)
{
        struct termios bak, cfg;
        char buf[128];
        int cnt, q;

        tcgetattr(STDIN_FILENO, &cfg);
        bak = cfg;
        cfmakeraw(&cfg);
        cfg.c_lflag &= ~ECHO;
        tcsetattr(STDIN_FILENO, TCSADRAIN, &cfg);

        printf("\033[?9001h");      // enable win32-input-mode.
//      printf("\033[?1002h");      // enable cell-motion tracking tracking.
//      printf("\033[?1006h");      // enable SGR extended mouse mode.
        printf("\n\r");
        fflush(stdout);

        for (q = 0; (cnt = read(STDIN_FILENO, buf, sizeof(buf) - 1)) > 0;) {
                buf[cnt] = 0;
                if (0 == strcmp(buf, "\x1b[81;16;113;1;0;1_")) { // q-down
                        if (++q == 2)
                                break;
                } else if (0 == strcmp(buf, "\x1b[81;16;113;0;0;1_")) { // q-up
                } else {
                        q = 0;
                }
                hex(buf, cnt);
        }

//      printf("\033[?1006l");      // disable SGR extended mouse mode.
//      printf("\033[?1002l");      // disable cell-motion tracking tracking.
        printf("\033[?9001l");      // disable win32-input-mode.
        fflush(stdout);

        tcsetattr(STDIN_FILENO, TCSADRAIN, &bak);
}


static void
hex(const void *buf, unsigned len)
{
#define HEXWIDTH 32
#define HEXCOLUMN 4
        const unsigned char *cursor = buf, *end = cursor + len;
        unsigned offset = 0;

        for (offset = 0; cursor < end; offset += HEXWIDTH) {
                const unsigned char *data = cursor;

                printf("%04x: ", offset);
                for (unsigned col = 0; col < HEXWIDTH; ++col) {
                        if ((col % HEXCOLUMN) == 0 && col)
                                printf(" |");
                        if (data == end) {
                                printf("   ");
                        } else {
                                printf(" %02x", *data++);
                        }
                }
                printf("   ");
                for (unsigned col = 0; col < HEXWIDTH; ++col) {
                        if (cursor == end) {
                                printf("%*s", HEXWIDTH - col, "");
                                break;
                        }
                        const unsigned char c = *cursor++;
                        printf("%c", (c >= ' ' && c < 0x7f ? c : '.'));
                }
                printf("\n\r");
        }
        fflush(stdout);
}

//end

Expected Behavior

Raw keys code reported without issue.
example-run

Actual Behavior

OpenConsole rapidly consumes memory and cpu, shall consume all available resources unless killed;
application becomes unresponsive.

memory-usage

@adamyg adamyg added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Aug 29, 2024
@lhecker
Copy link
Member

lhecker commented Aug 29, 2024

Using your test application, I was easily able to reproduce the issue by pasting a few lines of text (>100 characters) 5-10 times. It would then go out of control as you described.

I believe this is a bug in cygwin. When the issue occurs, something calls WriteConsoleInput. In my case with 1340 INPUT_RECORDs which we would then store and offer for reading. Then WriteConsoleInput would get called with 20100 records, 295356 records, and so on. It's definitely not a deadlock that's internal to OpenConsole/conhost. You may have to open a bug report for cygwin. At least to me it doesn't seem like there's a bug in your example code. We can't fix it either, because if someone calls WriteConsoleInput, we'll respect that request.

(The reason it may seem like a deadlock in OpenConsole is because our input processing is just extremely slow. The input buffer implementation was unfortunately "improved" by someone many years ago in the pursuit of OOP cleanliness. We're planning to fix that soon. But making it faster will not fix this bug, it'll simply make it run OOM faster. 😄)

@adamyg
Copy link
Author

adamyg commented Aug 29, 2024

Shall continue researching, might review Cygwin's console interface for any insight.
One suspect is the fhandler_console::cons_master_thread (), which attempts to mine signals within the input strem; yet it may not handle a single key being represented by multiple events.; furthermore one of the few uses of WriteConsoleInput

Note: upon disabling win32-input-mode, the problem does not not occur.

Assume there are no plans on supporting Cygwin's equiv cygwin-raw-mode, ESC[?2000h, generates ESC{Kd;Rc;Vk;Sc;Uc;CsK

@lhecker
Copy link
Member

lhecker commented Aug 29, 2024

Currently we have no plans adding support for other input protocols ourselves, but we'd gladly accept any PRs that add support for them!

@adamyg
Copy link
Author

adamyg commented Aug 31, 2024

Discussion raised within cygwin mail group.
One suspect is the fhandler_console::cons_master_thread (), which attempts to mine signals within the input stream;
yet it may not handle a single key being represented by multiple events.; furthermore one of the few uses of WriteConsoleInput. cons_master_thread peeks at the input event, removing signal events and pushes back others.

Please confirm the intended behavior of WriteConsoleInput() when win32-input-mode, is enabled?

Wondering if its behavior should be to not expand key events without Vk=0/Sc=0 values, from research, these represent the expanded pseudo key events. This behavior may also address the double encoding of mouse event under win32-input-mode,

@adamyg
Copy link
Author

adamyg commented Aug 31, 2024

@lhecker
Copy link
Member

lhecker commented Sep 3, 2024

I'm glad that you were able to fix the issue in cygwin. I'll be closing the issue then. Thank you for reporting it in their mailing list as well as fixing it! 😻

It may be worth nothing that the fix will be required even long-term. If an application requests win32-input-mode (w32im), then we'll have to respect this wish and translate any inputs to the w32im sequences, even if they come from WriteConsoleInput. This is the only way we can losslessly transmit INPUT_RECORDs across SSH for instance. As such this is an issue we cannot fix on our side unfortunately.

@lhecker lhecker closed this as completed Sep 3, 2024
@lhecker lhecker added the Resolution-External For issues that are outside this codebase label Sep 3, 2024
@lhecker
Copy link
Member

lhecker commented Sep 3, 2024

Oh, I missed your question:

Wondering if its behavior should be to not expand key events without Vk=0/Sc=0 values, from research, these represent the expanded pseudo key events. This behavior may also address the double encoding of mouse event under win32-input-mode,

The intention of the protocol is to be absolutely lossless when it comes to INPUT_RECORD. So even if wVirtualKeyCode and wVirtualScanCode are zero, members like dwControlKeyState may not be zero and we want to preserve everything perfectly.

Edit: I can confirm that mouse events are being double-encoded. I've opened #17851 to track the issue. I'll fix that ASAP and see if we can backport it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-External For issues that are outside this codebase
Projects
None yet
Development

No branches or pull requests

2 participants