Win32 input mode breaks WSL .exe interop #20852

@zadjii-msft commented on GitHub (Nov 20, 2023):

@craigloewen-msft as an FYI. We may have broken windows interop for WSL for insiders. We're looking into it.

@zadjii-msft commented on GitHub (Nov 20, 2023): @craigloewen-msft as an FYI. We may have broken windows interop for WSL for insiders. We're looking into it.

claunia commented

@zadjii-msft commented on GitHub (Nov 20, 2023):

See also:

@zadjii-msft commented on GitHub (Nov 20, 2023): See also: * #15083 * #15296

claunia commented

@lhecker commented on GitHub (Nov 20, 2023):

Probably also causes #16307

@lhecker commented on GitHub (Nov 20, 2023): Probably also causes #16307

claunia commented

@j4james commented on GitHub (Nov 20, 2023):

From what @DHowett said in https://github.com/microsoft/terminal/pull/15476#discussion_r1210897525, my understanding was that the conpty DLL was tightly coupled to the openconsole version. So I'm wondering whether it's possible that you're getting a version mismatch by replacing the system conhost, if you aren't also using the corresponding conpty DLL?

If it's not something like that, I have no idea what the issue could be. But I haven't had a chance to look at the code in detail (and probably won't have much time again until the weekend).

@j4james commented on GitHub (Nov 20, 2023): From what @DHowett said in https://github.com/microsoft/terminal/pull/15476#discussion_r1210897525, my understanding was that the conpty DLL was tightly coupled to the openconsole version. So I'm wondering whether it's possible that you're getting a version mismatch by replacing the system conhost, if you aren't also using the corresponding conpty DLL? If it's not something like that, I have no idea what the issue could be. But I haven't had a chance to look at the code in detail (and probably won't have much time again until the weekend).

claunia commented

@j4james commented on GitHub (Nov 20, 2023):

I have an idea what the problem might be, but I don't really know how WSL works behind the scenes so my understanding may be incorrect.

I'm assuming when you launch a Windows executable from within WSL it acts like a third-party conpty client, and creates a conpty connection.
I'm assuming it would do this without the PSEUDOCONSOLE_WIN32_INPUT_MODE flag, but since that flag was removed in PR 15475, we now act as if it's always set.
Without that flag set, conhost would not previously have sent DECSET sequences to enable win32 input mode and focus event mode, but now it does that regardless.
It was assumed that change would be harmless, because a client that didn't understand those modes would ignore them, but I suspect WSL is actually passing them up to the parent terminal.
This would result in the parent terminal enabling win32 input mode, and from then on any keystrokes would be sent to WSL in a format that it doesn't understand (possibly also affecting VT query responses).

If the above assumptions are correct, we're probably going to need to revert most if not all of PR #15476 in order to fix this, and come up with another solution for #15461.

@j4james commented on GitHub (Nov 20, 2023): I have an idea what the problem might be, but I don't really know how WSL works behind the scenes so my understanding may be incorrect. 1. I'm assuming when you launch a Windows executable from within WSL it acts like a third-party conpty client, and creates a conpty connection. 2. I'm assuming it would do this *without* the `PSEUDOCONSOLE_WIN32_INPUT_MODE` flag, but since that flag was removed in PR 15475, we now act as if it's always set. 3. Without that flag set, conhost would not previously have sent `DECSET` sequences to enable win32 input mode and focus event mode, but now it does that regardless. 4. It was assumed that change would be harmless, because a client that didn't understand those modes would ignore them, but I suspect WSL is actually passing them up to the parent terminal. 5. This would result in the parent terminal enabling win32 input mode, and from then on any keystrokes would be sent to WSL in a format that it doesn't understand (possibly also affecting VT query responses). If the above assumptions are correct, we're probably going to need to revert most if not all of PR #15476 in order to fix this, and come up with another solution for #15461.

claunia commented

@lhecker commented on GitHub (Nov 21, 2023):

The way I see it, the fundamental problem is that the same Mode::Win32 flag is being used for both "input" into and "output" from the InputBuffer and indiscriminately of the purpose of the read.

By default it should only take affect when INPUT_RECORDs are being read for communication between a terminal and ConPTY but since we only have a single InputBuffer::Read() and a single InputBuffer::Write() function it taints even all of the public APIs that don't get those win32-input-sequences. This is especially weird for internal APIs like the aforementioned ConhostInternalGetSet::ReturnResponse which is already is text-based and hence the double-encoding. So, if I don't misunderstand the purpose of the Win32 DECSET sequence I think it's actually fine that it's being passed up through WSL. It just shouldn't be immediately visible to console clients.

Do we support console clients requesting win32-input-sequences? If so, then that doesn't change the above IMO, because a client requesting it shouldn't change the encoding for any other attached client.

Fixing the double-encoding is trivial with just a couple lines changed, but fixing the Win32 sequences being visible to console clients is difficult since it requires us to delay encoding to the time of Read() and make it dependent on the requester. I've started writing a modernized InputBuffer but it's fairly messy given our current architecture and the vast compatibility requirements. 🤦

I suspect we'll end up having to revert most of #15476 after all... 😢

@lhecker commented on GitHub (Nov 21, 2023): The way I see it, the fundamental problem is that the same `Mode::Win32` flag is being used for both "input" into and "output" from the `InputBuffer` and indiscriminately of the purpose of the read. By default it should only take affect when `INPUT_RECORD`s are being read for communication between a terminal and ConPTY but since we only have a single `InputBuffer::Read()` and a single `InputBuffer::Write()` function it taints even all of the public APIs that don't get those win32-input-sequences. This is especially weird for internal APIs like the aforementioned `ConhostInternalGetSet::ReturnResponse` which is already is text-based and hence the double-encoding. So, if I don't misunderstand the purpose of the Win32 `DECSET` sequence I think it's actually fine that it's being passed up through WSL. It just shouldn't be immediately visible to console clients. Do we support console clients requesting win32-input-sequences? If so, then that doesn't change the above IMO, because a client requesting it shouldn't change the encoding for any other attached client. Fixing the double-encoding is trivial with just a couple lines changed, but fixing the Win32 sequences being visible to console clients is difficult since it requires us to delay encoding to the time of `Read()` and make it dependent on the requester. I've started writing a modernized `InputBuffer` but it's fairly messy given our current architecture and the vast compatibility requirements. 🤦 I suspect we'll end up having to revert most of #15476 after all... 😢

claunia commented

@DHowett commented on GitHub (Nov 21, 2023):

Do we support console clients requesting win32-input-sequences?

I don't see why we shouldn't! It's not different from enabling mouse mode with DECSET 1003.

If so, then that doesn't change the above IMO, because a client requesting it shouldn't change the encoding for any other attached client.

As above, it's not different from enabling mouse mode with DECSET 1003. Unfortunately, we can't know who's requesting it and on whose behalf it is being requested. Even if we track the client application, how do we know that ssh.exe isn't passing input to two different processes--one of which supports win32 input, and one of which does not?

@DHowett commented on GitHub (Nov 21, 2023): > Do we support console clients requesting win32-input-sequences? I don't see why we shouldn't! It's not different from enabling mouse mode with `DECSET` `1003`. > If so, then that doesn't change the above IMO, because a client requesting it shouldn't change the encoding for any other attached client. As above, it's not different from enabling mouse mode with `DECSET` `1003`. Unfortunately, we can't _know_ who's requesting it and on whose behalf it is being requested. Even if we track the client application, how do we know that `ssh.exe` isn't passing input to two different processes--one of which supports win32 input, and one of which does not?

claunia commented

@lhecker commented on GitHub (Nov 21, 2023):

Even if we track the client application, how do we know that ssh.exe isn't passing input to two different processes--one of which supports win32 input, and one of which does not?

In my vision, win32-input-sequences would be requested automatically by ConPTY at all times, just like it works right now. It's just that console clients (= in case of SSH on the server side) should not see those in form of 20 INPUT_RECORDs per actual input character if they didn't ask for it. That part is easy to do as far as I can see.

@lhecker commented on GitHub (Nov 21, 2023): > Even if we track the client application, how do we know that `ssh.exe` isn't passing input to two different processes--one of which supports win32 input, and one of which does not? In my vision, win32-input-sequences would be requested automatically by ConPTY at all times, just like it works right now. It's just that console clients (= in case of SSH on the server side) should not see those in form of 20 `INPUT_RECORD`s per actual input character if they didn't ask for it. That part is easy to do as far as I can see.

claunia commented

@lhecker commented on GitHub (Nov 21, 2023):

See... I'm actually an idiot. The reason it ACTUALLY happens is because of this:
12318d97d0/src/terminal/parser/stateMachine.cpp (L2139-L2142)

Unfortunately, that statement is entirely wrong. The most basic example for this is that our input buffer is a basic

char buffer[256];

and if your input pipe got more characters than that, the comment's assumption will be broken, because the read will be split up into multiple chunks. Second of all, however, WSL doesn't use anonymous pipes and so they can split up inputs into arbitrary chunks down to individual bytes. That's why we see broken win32 input sequences randomly in WSL.

@lhecker commented on GitHub (Nov 21, 2023): See... I'm actually an idiot. The reason it _ACTUALLY_ happens is because of this: https://github.com/microsoft/terminal/blob/12318d97d0d9df09e05b5dd64622ff4f5b09382b/src/terminal/parser/stateMachine.cpp#L2139-L2142 ![confused meme](https://media.tenor.com/VKtUy6VLhXIAAAAC/ni%C3%B1a-asco.gif) _Unfortunately_, that statement is entirely wrong. The most basic example for this is that our input buffer is a basic ```cpp char buffer[256]; ``` and if your input pipe got more characters than that, the comment's assumption will be broken, because the read will be split up into multiple chunks. Second of all, however, WSL doesn't use anonymous pipes and so they can split up inputs into arbitrary chunks down to individual bytes. That's why we see broken win32 input sequences randomly in WSL.

claunia commented

@DHowett commented on GitHub (Nov 21, 2023):

That part is easy to do as far as I can see.

Yyeehehhhhh.. eh. Eaugh.

When an application requests ENABLE_VIRTUAL_TERMINAL_INPUT under ConPTY, we pass the input received through ConPTY directly through to the application. It allows the terminal and the client application to support input modalities that conhost doesn't know about.

@DHowett commented on GitHub (Nov 21, 2023): > That part is easy to do as far as I can see. Yyeehehhhhh.. eh. Eaugh. When an application requests `ENABLE_VIRTUAL_TERMINAL_INPUT` under ConPTY, we pass the input received through ConPTY _directly through_ to the application. It allows the terminal and the client application to support input modalities that conhost doesn't know about.

claunia commented

@j4james commented on GitHub (Nov 26, 2023):

I did a little hack experiment with fixing the ConhostInternalGetSet::ReturnResponse implementation, so it sends the response directly to InputBuffer::_HandleTerminalInputCallback (instead of creating input events, which get converted into VT sequences, which then converted back into input events).

This fixes the problem with VT query responses not working correctly in conhost when win32-input mode is set (I'm almost sure we already had an issue for that, but I couldn't find it). It also fixes the conpty deadlock on startup that you're getting when running an exe from within WSL.

I can't reproduce the second problem though. Running your ReadConsoleInputW test app, I only see two events per key press, i.e. the up and down events. The keypresses are not showing up as a bunch of undecoded win32-input sequences.

That said, you still have a problem when you exit the exe and return to WSL, because the terminal is still in win32-input mode at that point. Since your typical WSL shell is not going to be expecting that, you get a lot of garbage output at the prompt when you type.

I can work around that by adding a win32-input reset after the exe call, e.g. testinput.exe; printf "\e[?9001l", but that's obviously not a practical solution for users. Someone needs to be responsible for restoring that mode, and I'm not sure where that should happen.

@j4james commented on GitHub (Nov 26, 2023): I did a little hack experiment with fixing the `ConhostInternalGetSet::ReturnResponse` implementation, so it sends the response directly to `InputBuffer::_HandleTerminalInputCallback` (instead of creating input events, which get converted into VT sequences, which then converted back into input events). This fixes the problem with VT query responses not working correctly in conhost when win32-input mode is set (I'm almost sure we already had an issue for that, but I couldn't find it). It also fixes the conpty deadlock on startup that you're getting when running an exe from within WSL. I can't reproduce the second problem though. Running your `ReadConsoleInputW` test app, I only see two events per key press, i.e. the up and down events. The keypresses are not showing up as a bunch of undecoded win32-input sequences. That said, you still have a problem when you exit the exe and return to WSL, because the terminal is still in win32-input mode at that point. Since your typical WSL shell is not going to be expecting that, you get a lot of garbage output at the prompt when you type. I can work around that by adding a win32-input reset after the exe call, e.g. `testinput.exe; printf "\e[?9001l"`, but that's obviously not a practical solution for users. Someone needs to be responsible for restoring that mode, and I'm not sure where that should happen.

claunia commented

@zadjii-msft commented on GitHub (Jan 31, 2024):

We're pretty sure this was fixed, just not actually marked as closed.

@zadjii-msft commented on GitHub (Jan 31, 2024): We're pretty sure this was fixed, just not actually marked as closed.

claunia commented