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

Ctrl-C handling is inadequate for interactive cmd and PowerShell #116

Open
unshare opened this issue May 16, 2017 · 23 comments
Open

Ctrl-C handling is inadequate for interactive cmd and PowerShell #116

unshare opened this issue May 16, 2017 · 23 comments

Comments

@unshare
Copy link

unshare commented May 16, 2017

If I use a combination of Cygwin + OpenSSH + WinPTY to run interactive cmd or PowerShell they don't treat Ctrl-C as a command input cancellation.

Ctrl-C works for command abort, so this is not a pressing issue. Just an annoyance due to my keyboard skills.

I've reviewed Ctrl-C handling (the one with GenerateConsoleCtrlEvent(CTRL_C_EVENT, 0)) and I have no idea what is wrong. But I'm not a Win32 API specialist.

@rprichard
Copy link
Owner

rprichard commented May 17, 2017

Yeah, I've noticed this before. I think synthesizing Ctrl-C requires more than just calling GenerateConsoleCtrlEvent. My impression was that the official API was insufficient. Maybe it had something to do with interrupting an in-progress ReadConsole API?

I wonder if winpty should synthesize Ctrl-C GUI events, like it does for WSL navigation keys.

@rprichard rprichard added the bug label May 17, 2017
@rprichard
Copy link
Owner

It looks like ConEmu also takes the send-a-GUI-message route.

// (wParam == 'C' || wParam == VK_PAUSE/*Break*/)
// Ctrl+C & Ctrl+Break are very special signals in Windows
// After many tries I decided to send 'C'/Break to console window
// via simple `SendMessage(ghConWnd, WM_KEYDOWN, r.Event.KeyEvent.wVirtualKeyCode, 0)`
// That is done inside ../ConEmuCD/Queue.cpp::ProcessInputMessage
// Some restriction applies to it also, look inside `ProcessInputMessage`
//
// Yep, there is another nice function - GenerateConsoleCtrlEvent
// But it has different restrictions and it doesn't break ReadConsole
// because that function loops deep inside Windows kernel...
// However, user can call it via GuiMacro:
// Break() for Ctrl+C or Break(1) for Ctrl+Break
// if simple Keys("^c") does not work in your case.

https://github.com/Maximus5/ConEmu/blob/694d71c27f83628e65d2fbbb54c4f2c94547933f/src/ConEmu/RealConsole.cpp#L6798

if (lbProcessEvent)
{
    // Issue 590: GenerateConsoleCtrlEvent does not break ReadConsole[A|W] function!
    SetLastError(0);
    LRESULT lSendRc =
    SendMessage(ghConWnd, WM_KEYDOWN, r.Event.KeyEvent.wVirtualKeyCode, 0);
    DWORD nErrCode = GetLastError();
    msprintf(szLog, countof(szLog), L"  ---  CtrlC/CtrlBreak sent (%u,%u)", LODWORD(lSendRc), nErrCode);
    LogString(szLog);
}

https://github.com/Maximus5/ConEmu/blob/694d71c27f83628e65d2fbbb54c4f2c94547933f/src/ConEmuCD/Queue.cpp#L106

@rprichard
Copy link
Owner

I noticed a related issue: #117

@unshare
Copy link
Author

unshare commented May 18, 2017

Thanks a lot, really, but it's only partial success.
cmd works great, but it's not fixed for PowerShell (just tested)
Should I report a new issue?

@rprichard
Copy link
Owner

I can just reopen this issue. Can you provide more details? It's working with both CMD and PowerShell for me.

e.g. I type some text into at the PowerShell prompt, then press Ctrl-C, and it moves to the next line. I'm testing on Windows 7, though -- maybe I should look at Windows 10.

@rprichard rprichard reopened this May 18, 2017
@rprichard
Copy link
Owner

OK, it's broken on Windows 10 v15063. It just adds a ^C to the end of the line.

@unshare
Copy link
Author

unshare commented May 18, 2017

Correct. I can test against a Windows Server 2016 in a couple of hours. Or there's no need?

@rprichard
Copy link
Owner

Probably not necessary, I've found something that's obviously wrong, but I'm wondering why I got it wrong. I think I'll have it fixed in a few hours / a day or two.

i.e For Ctrl-C, winpty is generating:

$ ./build/winpty ./build/winpty-agent.exe  --show-input

Press any keys -- Ctrl-D exits

buffer-resized: dwSize=(132,3000)
key: dn rpt=1 scn=0x46 CANCEL ch=0x3
key: up rpt=1 scn=0x46 CANCEL ch=0x3

The normal console generates:

C:\rprichard\proj\winpty>build\winpty-agent.exe --show-input

Press any keys -- Ctrl-D exits

key: dn rpt=1 scn=0x1d LCtrl-CONTROL ch=0
key: dn rpt=1 scn=0x2e LCtrl-C ch=0x3
key: up rpt=1 scn=0x2e LCtrl-C ch=0x3
key: up rpt=1 scn=0x1d CONTROL ch=0

I bet that fixing the discrepancy will fix the PowerShell problem.

@rprichard
Copy link
Owner

i.e. The issue is that winpty isn't matching what Windows would use in a KEY_EVENT_RECORD object in the console input buffer. winpty is using VK_CANCEL with no modifiers, but the real Windows console uses a Ctrl modifier and a virtualkey of 'C'. We both use 0x3 for the character.

@rprichard
Copy link
Owner

The exact same situations happens in WinXP, so it's not a new bug. I probably didn't notice it, because most programs use SetConsoleCtrlHandler.

Here's one way to fix the bug:

$ git diff
diff --git a/src/agent/DefaultInputMap.cc b/src/agent/DefaultInputMap.cc
index b4dd7b4..e3a4dba 100644
--- a/src/agent/DefaultInputMap.cc
+++ b/src/agent/DefaultInputMap.cc
@@ -235,6 +235,7 @@ static void addSimpleEntries(InputMap &inputMap) {

         {   "\x7F",         { VK_BACK,  '\x08', 0,                                  } },
         {   ESC "\x7F",     { VK_BACK,  '\x08', LEFT_ALT_PRESSED,                   } },
+        {   "\x03",         { 'C',      '\x03', LEFT_CTRL_PRESSED,                  } },

         // Handle special F1-F5 for TERM=linux and TERM=cygwin.
         {   ESC "[[A",      { VK_F1,    '\0',   0                                   } },

Maybe I'll remove the (inputConsoleMode() & ENABLE_PROCESSED_INPUT) restriction on the GUI-message code path, though.

@rprichard
Copy link
Owner

I tried reusing the GUI-message code path for unprocessed Ctrl-C, but the character code was always NUL, and PowerShell didn't work. The diff I posted should work, though.

@rprichard
Copy link
Owner

Let me know if my change fixed it. It worked for me on Windows 10.

@unshare
Copy link
Author

unshare commented May 18, 2017

Well, I've tested the current HEAD (0c86e4f) against

  • Windows 10 1703 64-bit locally
  • Windows Server 2016 Core and Windows Hyper-V Server 2016 through Cygwin + OpenSSH
    The former works great, thank you.
    The later not quite.
    This change has broken something.
  • In PowerShell, Ctrl-C does not interrupt the current command (e.g. sleep 30)
  • In cmd, on the contrary, command interrupt works, but Ctrl-C doesn't cancel command input

rprichard added a commit that referenced this issue May 19, 2017
This change reverts the first part of the GH-116 fix.

See #116
@rprichard
Copy link
Owner

I reverted the first part of this issue's fix -- winpty uses GenerateConsoleCtrlEvent again. I reopened this issue, but I don't know how to fix it, so maybe it's just a limitation. Maybe a compromise is possible...

What I expect now:

  • Ctrl-C will always interrupt a command.
  • Ctrl-C will cancel command input in new versions of PowerShell (e.g Win10), but not in cmd or in old versions of PowerShell (e.g. Win7).

I don't really know how Windows' input handling works -- I'll describe what I think happens. When an event like WM_KEYDOWN is added to a message queue, Windows reads the entire keyboard state (256 bytes) and associates it with the event. When the event is dispatched, the event's keyboard state is made available through GetKeyboardState. When the console sees a 'C' key-down message, it checks GetKeyboardState for modifiers keys. The SSH server uses a background window station/desktop, where the keyboard state is always "no keys pressed." Therefore, it's impossible to send a Ctrl-C WM_KEYDOWN message to a backgrounded console window.

I tried using SendInput and keybd_event to hold down Ctrl while sending just WM_KEY{DOWN,UP} for a 'C' character. This worked in a foreground desktop, but the SendInput call failed with an "Access Denied" error in the background desktop. Even if it worked, a window station can have multiple desktops -- window stations have independent keyboard state, but what about desktops? I don't want winpty to interfere with the keyboard state for other programs (including other winpty instances).

I noticed that the KEY_EVENT_RECORD had a NUL UnicodeChar with SendMessage, but a non-NUL value if I used PostMessage. On the other hand, when I tried sending four message with PostMessage (Ctrl-down, C-down, C-up, Ctrl-up), Windows reordered the messages to (Ctrl-down, Ctrl-up, C-down, C-up). A Sleep(1) call "fixes" the issue, but might be introducing a race. (Maybe not, though? I see that SendInput uses KEYBDINPUT, which has a time timestamp in milliseconds. Maybe a Sleep(1) would guarantee unique timestamps, preventing reordering.)

The existing use of WM_KEYDOWN for WSL navigation keys isn't quite right, as I commented in the code. Example: If I run a foreground SSH server somehow, then hold down Ctrl, while a remote user logs in and runs WSL mc, I expect the navigation keys to break. I think it can be fixed by enabling winpty's desktop backgrounding for all Windows versions, not just XP and Vista.

As a compromise, maybe a winpty user can indicate that they're using winpty in the foreground, and then winpty can use WM_KEYDOWN for Ctrl-C. It could work for IDEs, I think. It's a race, because winpty's WM_KEYDOWN will be misinterpreted if the user lifts Ctrl too quickly. (If winpty uses PostMessage, then it will be misinterpreted as an ordinary 'C' keypress. If it uses SendMessage, then it'll produce an unusual key event where wVirtualKeyCode is 'C' but UnicodeChar is NUL.)

@rprichard rprichard reopened this May 19, 2017
@unshare
Copy link
Author

unshare commented May 19, 2017

Whoa, that's a lot to process. Will do during spare time.
One more point: there is Server Core. There's a lot of desktop components missing there.
Actually, I use winpty on my Windows 10 developer desktop and sometimes launch something on headless Windows Server 2016 Core servers using Cygwin + OpenSSH.
I'm aware of SSH transport for PowerShell, but it's not quite usable for me now, that's why I rely on winpty.

@johnsonc
Copy link

Hi,

I hope this helps..

I tried to winpty use in GitSCM (bash on MINGW32) and it works well.
winpty emacs -nw
Except that Ctrl-C produces a instead of Ctrl-C (Any help to make it recognizable as Ctrl-C? )

$ winpty --version
winpty version 0.2.2-dev

$ winpty winpty-agent.exe --show-input

Press any keys -- Ctrl-D exits
focus: gained
key: dn rpt=1 scn=70 CANCEL ch=0x3
key: up rpt=1 scn=70 CANCEL ch=0x3
key: dn rpt=1 scn=29 LCtrl-CONTROL ch=0
key: dn rpt=1 scn=44 LCtrl-Z ch=0x1a
key: up rpt=1 scn=44 LCtrl-Z ch=0x1a
key: up rpt=1 scn=29 CONTROL ch=0

@parkovski
Copy link

I fixed this here - my understanding of it is unprocessed mode needs both the GenerateConsoleCtrlEvent and the keypress to work. I've only tested this in the VSCode integrated terminal on Windows 10 though, I'm not sure about any other cases.

@parkovski
Copy link

I think MS is going to have to solve this one, hopefully whatever they do will work here too - PowerShell/Win32-OpenSSH#914

@parkovski
Copy link

I think I figured this one out. In processed mode, GenerateConsoleCtrlEvent should be enough. In unprocessed mode, you just need to write ^C. The thing that's been messing everyone up I think is that there is a cmd.exe bug - if input is a standard console handle, it calls GetKeyboardState to decide whether to process the event. This only works if its thread has keyboard focus - e.g. the conhost window is focused - otherwise it ignores the event. What really makes this frustrating, because I want to say "just use PowerShell", but lots of programs use a batch file as kind of a pseudo-symlink, and cmd displays the 'Stop batch job' prompt where this issue shows up.

I consider this a cmd.exe bug and not a bug in winpty, node-pty, or any dependent project, and I'm not sure if there's a feasible workaround in all cases, but if there is I think it's worth implementing. I'm not familiar with the restrictions on background desktops - does anyone know if it's possible to AttachThreadInput from the agent to the conhost process, and set the appropriate state when generating the event?

@glego
Copy link

glego commented Jan 29, 2018

As @parkovski mentioned it seems to be fixed in the PowerShell Integrated Console but not in powershell.

If you're unsure what terminal is used you can see it in the terminal drop-down menu.

2018-01-29_110921

You can launch the integrated console without opening a ps1 file from the command panel. Press CTRL+P and enter: PowerShell: Show Integrated Console

vscode_powershell_show_integrated

vscode_powershell_show_integrated_terminal

Unfortunately, the problem still persists in the powershell terminal interface...

vscode_powershell_ctrl_c

PowerShell Version

PS C:\Users\glenn\OneDrive\Projects\Learning\docker> $PSVersionTable

Name                           Value
----                           -----
PSVersion                      5.1.16299.98
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.16299.98
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

vscode version

Version 1.19.3
Commit 7c4205b5c6e52a53b81c69d2b2dc8a627abaa0ba
Date 2018-01-25T10:36:34.867Z
Shell 1.7.9
Renderer 58.0.3029.110
Node 7.9.0
Architecture x64

Cheers,
Glenn

@parkovski
Copy link

Quickly skimming over the PowerShell editor services libraries, it looks like that's because the integrated console handles ^C processing differently.

@mikemaccana
Copy link

Hey @rprichard! Is 0.44 far off? There's a bunch of us hanging for the fix to be included in our favorite win-pty projects 🙂

@SeanCurtis-Roblox
Copy link

SeanCurtis-Roblox commented Jan 20, 2022

I'd like to add one passing note -- I think it's an additional symptom of the problem being discussed here.

I'm running git bash under Windows 10 via Mingw64 terminal window. I want the git editor to be emacs -nw. However, I can't run it directly (as it reports that "standard input is not tty"). The partial solution is to preface it with winpty. However, emacs command including Ctrl-C don't work (and as exit is C-x, C-c that can become irritating quickly). Instead, emacs reports it received the command C-x, <pause>. So, clearly C-x isn't propagating to emacs in a form that emacs recognizes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants