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

The scancode returned in KEY_EVENT_RECORD don't match over ssh #5342

Closed
taviso opened this issue Apr 13, 2020 · 9 comments
Closed

The scancode returned in KEY_EVENT_RECORD don't match over ssh #5342

taviso opened this issue Apr 13, 2020 · 9 comments
Labels
Product-Conpty For console issues specifically related to conpty Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.

Comments

@taviso
Copy link

taviso commented Apr 13, 2020

Environment

Windows build number: Microsoft Windows [Version 10.0.18363.720]

C:\>ssh -V # Note, this is the ssh binary that comes with Windows 10
OpenSSH_for_Windows_7.7p1, LibreSSL 2.6.5

Steps to reproduce

I'm trying to use the console application hiew, it works perfectly in console but behaves differently over ssh, some keys are recognized incorrectly. I wrote a quick test program to dump keyboard events from ReadConsoleInput() and found that the only difference is that the wVirtualScanCode is different.

Example pressing the 'a' key locally in cmd:

Key event: 
 bKeyDown:          0
 wRepeatCount:      1
 wVirtualKeyCode:   65
 wVirtualScanCode:  30
 uChar:
  UnicodeChar:      'a' (0x61)
  AsciiChar:        'a' (0x61)
 dwControlKeyState: 0

Over ssh I get this:

Key event:  
 bKeyDown:          0
 wRepeatCount:      1
 wVirtualKeyCode:   65
 wVirtualScanCode:  79
 uChar:
  UnicodeChar:      'a' (0x61)
  AsciiChar:        'a' (0x61)
 dwControlKeyState: 0

I've verified that if I attach to the program in windbg, and patch up the scan code in the KEY_EVENT_RECORD structures after ReadConsoleInput() returns, the program works perfectly.

Expected behavior

I think the wVirtualScanCode should match locally and over ssh.

Actual behavior

The wVirtualScanCode are wildly different, and applications that examine them become confused.

If there is a setting somewhere that configured this, I wasn't able to find it, I really tried!

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Apr 13, 2020
@DHowett-MSFT DHowett-MSFT added the Product-Conpty For console issues specifically related to conpty label Apr 14, 2020
@taviso
Copy link
Author

taviso commented Apr 14, 2020

I think I've found the relevant code:

https://github.com/microsoft/terminal/blob/master/src/types/convert.cpp#L210

    const auto vk = LOBYTE(keyState);
    const WORD virtualScanCode = gsl::narrow<WORD>(MapVirtualKeyW(vk, MAPVK_VK_TO_VSC));
    KeyEvent keyEvent{ true, 1, LOBYTE(keyState), virtualScanCode, wch, 0 };

Confusingly, that seems correct to me. If I write a test program that does the same MapVirtualKey() operation, I seem to get the correct scan codes.

Is it possible that code works, but it hasn't reached a release yet? I can't see how what the code does currently in 1909 could possibly match that code 😄

I wrote a debugger script to patch the events to do MapVirtualKeyA() and it works perfectly, so this is definitely the bug.

@DHowett-MSFT
Copy link
Contributor

Great find, and thanks for the report! Yeah, we just took some changes to the PTY’s key generator; once I’m at my desk tomorrow I can confirm which Windows release it’s in or would be in (if it’s not in one yet!)

@DHowett-MSFT
Copy link
Contributor

Alternatively, this could be in InputStateMachineEngine; I’m not sure if it’s using convert.

@DHowett-MSFT
Copy link
Contributor

Disregard: I just saw the comment about the live patch. 😄

@DHowett-MSFT
Copy link
Contributor

You know, the blame for the line you pointed out takes me back to the fix for #2873, #3199. I apparently gave it the very classic and enlightening description, "It was wrong. It is now not wrong.".

Judging by the symptom, it looks like these are the same issue. It's fixed in the insider slow train (19041.x right now), and should be out when the 20H1 build of Windows is RTM.

In the mean time, if you're feeling adventurous you can replace conhost.exe with OpenConsole.exe, built from this repository, and get a slew of PTY fixes that're backed up behind the windows build train.

I should warn you, though, that this comes with no disclaimer or warranty and we have some open issues in PR that fix PTY bugs we introduced in the past, like, week. 😄

Thanks!

/dup #2873

@ghost
Copy link

ghost commented Apr 14, 2020

Hi! We've identified this issue as a duplicate of another one that already exists on this Issue Tracker. This specific instance is being closed in favor of tracking the concern over on the referenced thread. Thanks for your report!

@ghost ghost closed this as completed Apr 14, 2020
@ghost ghost added Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Apr 14, 2020
@taviso
Copy link
Author

taviso commented Apr 14, 2020

Thanks Dustin, I did build OpenConsole.exe and replaced conhost.exe on my 1909 build. I literally just cloned it, I'm at 37e62da

The behaviour does seem better, scancodes match now!

It broke some other things, and the console app I was trying to run looks garbled now. It did look correct in the 1909 conhost, it was just input that wasn't working. Oh well, I'll see if I can figure out what changed.

@DHowett-MSFT
Copy link
Contributor

I'm hoping that what you're seeing is somewhat like this:

image

If so, that'll be fixed with #5294 (bug #5291). If not, I'm mildly horrified. 😄

@taviso
Copy link
Author

taviso commented Apr 15, 2020

Ahh, that might explain it, I'll watch the bug and try it again when a fix lands.

I also get some extra INPUT_EVENTS from ReadConsoleInput() if I use ssh from within WSL. Regular keys work like 'a', 'b', etc. If I press backspace, I get the expected 0x08 (with correct scan code, yay!), then a 0x7f with LEFT_CTRL. The application doesn't handle that.

That feels terminal related (like synthetic escape sequences...?)

(Note: I didn't get any extra events with 1909 conhost.exe, just the wrong scancodes)

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product-Conpty For console issues specifically related to conpty Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.
Projects
None yet
Development

No branches or pull requests

2 participants