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.

Sign up for GitHub

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

Jump to bottom

Reset disables the ability to pass through C1 control characters #13968

Closed
j4james opened this issue Sep 11, 2022 · 5 comments · Fixed by #13969
Closed

Reset disables the ability to pass through C1 control characters #13968

j4james opened this issue Sep 11, 2022 · 5 comments · Fixed by #13969
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@j4james
Copy link
Collaborator

j4james commented Sep 11, 2022

Windows Terminal version

1.15.1862.0

Windows build number

10.0.19044.1889

Other Software

No response

Steps to reproduce

  1. Open a WSL bash shell in Windows Terminal.
  2. Execute the following statement: printf "\e 7\u009B?5h"
  3. Note that the screen attributes have reversed.
  4. Execute reset to get the screen back to normal.
  5. Execute printf "\e 7\u009B?5h" again.

Expected Behavior

Both step 2 and step 5 should cause the screen attributes to be reversed.

Actual Behavior

Only the first attempt works. The second attempt appears to output ?5h instead.

What's happening is we've got a DECAC1 sequence (ESC SP 7) which is supposed to enable C1 support, followed by DECSCNM (reverse screen mode), using a C1 control sequence (CSI ? 5 h).

That works the first time, but after the reset, the C1 support is permanently disabled on the conpty client side, so C1 control sequences can no longer be passed through.
@j4james j4james added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Sep 11, 2022
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 11, 2022
@j4james
Copy link
Collaborator Author

j4james commented Sep 11, 2022

I think this may be another regression from #13024.

When we first added support for DECAC1 in #11690, the assumption was that it would be enabled permanently on the conpty client side, and there was nothing in the terminal code that could turn that off. However, when TerminalDispatch was replaced with AdaptDispatch, it inherited the conhost SoftReset implementation, and that included turning off DECAC1.

I haven't tried this yet, but one simple fix could be to add another parser mode - something like AlwaysAcceptC1 - which is set at startup in the terminal, and never reset anywhere. Then the test in the state machine just becomes:

_parserMode.any(Mode::AcceptC1, Mode::AlwaysAcceptC1)

It's a bit hacky, but that should add no significant overhead, and we won't then have to care what AcceptC1 might be set to.

@DHowett
Copy link
Member

DHowett commented Sep 12, 2022 •
edited
Loading

Ah, this is a weird case. Great catch! Eventually we would want to turn that new "always" flag off by default, because there may not always be a guarantee that Terminal is connected to ConPTY.

I'm cool with this fix, if we file a followup/todo somewhere.

Quick question: does this mean a hard reset breaks Terminal in general? Should I crack the seal on the coming 1.15 and 1.16 releases to slip in a fix for this? 🙂

(Edit: oh, C1 isn't the common case for ConPTY. You're safe to ignore a bunch of the concerns here. Thanks!)

@j4james
Copy link
Collaborator Author

j4james commented Sep 12, 2022

Eventually we would want to turn that new "always" flag off by default, because there may not always be a guarantee that Terminal is connected to ConPTY.

Yeah. We've actually already got a comment in the code at that point.

terminal/src/cascadia/TerminalCore/Terminal.cpp

Lines 88 to 89 in d11ea72

// Until we have a true pass-through mode (GH#1173), the decision as to
// whether C1 controls are interpreted or not is made at the conhost level.

Quick question: does this mean a hard reset breaks Terminal in general?

Only if someone is actually using C1 controls, which requires actively enabling them with DECAC1. So I don't it's that big a deal, but I do know at least one application that is doing that (see microsoft/DbgShell@26a40a1).

@j4james j4james mentioned this issue Sep 12, 2022
Merged
@ghost ghost added the In-PR This issue has a related PR label Sep 12, 2022
@ghost ghost closed this as completed in #13969 Sep 12, 2022
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Sep 12, 2022
ghost pushed a commit that referenced this issue Sep 12, 2022
@j4james
Make sure Terminal state machine always accepts C1 controls (#13969)
f2b361c 
When we added support for the `DECAC1` control sequence, which
determines whether `C1` controls are accepted or not, the intention was
that conhost would be making that determination, and Windows Terminal
would always be expected to accept any passed-through `C1` controls.

However, this didn't take into account that a passed-through `RIS`
sequence could end up disabling `DECAC1`, and that would leave Windows
Terminal incapable of processing any `C1` controls. This PR attempts to
fix that oversight.

The `DECAC1` sequence was added in PR #11690, when we disabled `C1`
acceptance by default.

This is a bit of a hack, but I've added a new `AlwaysAcceptC1` mode to
the state machine, which is enabled at startup in the Terminal, and is
never disabled. The parser then just needs to check whether either
`AcceptC1` or `AlwaysAcceptC1` are set.

## Validation Steps Performed

I've manually confirmed the test case in #13968 now works as expected.

Closes #13968
@DHowett DHowett mentioned this issue Oct 10, 2022
Merged
DHowett pushed a commit that referenced this issue Dec 12, 2022
@j4james @DHowett
Make sure Terminal state machine always accepts C1 controls (#13969)
3cdd452 
When we added support for the `DECAC1` control sequence, which
determines whether `C1` controls are accepted or not, the intention was
that conhost would be making that determination, and Windows Terminal
would always be expected to accept any passed-through `C1` controls.

However, this didn't take into account that a passed-through `RIS`
sequence could end up disabling `DECAC1`, and that would leave Windows
Terminal incapable of processing any `C1` controls. This PR attempts to
fix that oversight.

The `DECAC1` sequence was added in PR #11690, when we disabled `C1`
acceptance by default.

This is a bit of a hack, but I've added a new `AlwaysAcceptC1` mode to
the state machine, which is enabled at startup in the Terminal, and is
never disabled. The parser then just needs to check whether either
`AcceptC1` or `AlwaysAcceptC1` are set.

## Validation Steps Performed

I've manually confirmed the test case in #13968 now works as expected.

Closes #13968

(cherry picked from commit f2b361c)
Service-Card-Id: 87207769
Service-Version: 1.15
DHowett pushed a commit that referenced this issue Dec 12, 2022
@j4james @DHowett
Make sure Terminal state machine always accepts C1 controls (#13969)
ee6d365 
When we added support for the `DECAC1` control sequence, which
determines whether `C1` controls are accepted or not, the intention was
that conhost would be making that determination, and Windows Terminal
would always be expected to accept any passed-through `C1` controls.

However, this didn't take into account that a passed-through `RIS`
sequence could end up disabling `DECAC1`, and that would leave Windows
Terminal incapable of processing any `C1` controls. This PR attempts to
fix that oversight.

The `DECAC1` sequence was added in PR #11690, when we disabled `C1`
acceptance by default.

This is a bit of a hack, but I've added a new `AlwaysAcceptC1` mode to
the state machine, which is enabled at startup in the Terminal, and is
never disabled. The parser then just needs to check whether either
`AcceptC1` or `AlwaysAcceptC1` are set.

## Validation Steps Performed

I've manually confirmed the test case in #13968 now works as expected.

Closes #13968

(cherry picked from commit f2b361c)
Service-Card-Id: 87207767
Service-Version: 1.16
@ghost
Copy link

ghost commented Dec 14, 2022

🎉This issue was addressed in #13969, which has now been successfully released as Windows Terminal v1.15.3465.0 and v1.15.3466.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Dec 14, 2022

🎉This issue was addressed in #13969, which has now been successfully released as Windows Terminal Preview v1.16.3463.0 and v1.16.3464.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make sure Terminal state machine always accepts C1 controls j4james/terminal
2 participants
@DHowett @j4james