microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
95.5k stars 8.31k forks source link

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

Closed adamyg closed 1 month ago

adamyg commented 2 months ago

Windows Terminal version

1.20.11781.0

Windows build number

10.0.19045.4780

Other Software

cygwin64, either

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

lhecker commented 2 months ago

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 commented 2 months ago

Shall continue researching, might review Cygwin's console interface for any insight. _One suspect is the fhandler_console::cons_masterthread (), 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 commented 2 months ago

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 commented 2 months ago

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_masterthread 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 commented 2 months ago

Update: https://sourceware.org/pipermail/cygwin-patches/2024q3/012748.html

lhecker commented 1 month ago

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 commented 1 month ago

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.