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

ConPTY pass-through sequences are being truncated #4116

Closed
j4james opened this issue Jan 5, 2020 · 2 comments · Fixed by #4125
Closed

ConPTY pass-through sequences are being truncated #4116

j4james opened this issue Jan 5, 2020 · 2 comments · Fixed by #4125
Assignees
@miniksa
Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Milestone
Terminal v0.8

Comments

@j4james
Copy link
Collaborator

j4james commented Jan 5, 2020

Environment

Windows build number: Version 10.0.18362.418
Windows Terminal version (if applicable): commit d711d73

Steps to reproduce

  1. Open a bash shell in Windows Terminal
  2. Execute the sequence: printf "\e[?999h 12345 Hello World"

Expected behavior

999 isn't a supported mode, so that should be ignored and passed through to the conpty client. Then the text 12345 Hello World should be output.

Actual behavior

The unrecognized mode sequence gets truncated, losing the final character. and then the start of the text that is output is also truncated, leaving just ello World.

What's happening is that the missing character leaves the state machine in a state where it's waiting for a final character, so it ends up eating the characters that follow, until it gets to the H in Hello World.

I believe this is a regression caused by PR #3956. I think it's the calculation of the _run variable that is off by 1, but it may be more complicated than that:

terminal/src/terminal/parser/stateMachine.cpp

Line 1252 in d711d73

_run = string.substr(start, current - start);

Also note that _run is initialized in more than one place.
@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 Jan 5, 2020
@j4james
Copy link
Collaborator Author

j4james commented Jan 5, 2020

On further review, I see the _run variable actually serves a different purpose, so changing that is not the solution. It's just not the right thing to be using for the pass-through.

To be honest I think we'd be better off just dropping this pass-through idea as a general concept. If there are specific sequences that we don't support in conhost, but we know another conpty client can handle, then we should be evaluating those on a case by case basis, and deciding whether they're safe to pass through.

The concept only really makes sense for operations that have no interaction with other parts of the system. For example, passing through a sequence to change the cursor color might be OK, but passing through a sequence that sets the left and right margins is never going to work - it'll just leave the conhost and conpty client completely out of sync.

@DHowett-MSFT DHowett-MSFT added Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Severity-Blocking We won't ship a release like this! No-siree. labels Jan 6, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 6, 2020
@DHowett-MSFT DHowett-MSFT added Needs-Tag-Fix Doesn't match tag requirements and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jan 6, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jan 6, 2020
@miniksa miniksa mentioned this issue Jan 6, 2020
Merged
7 tasks
@ghost ghost added the In-PR This issue has a related PR label Jan 6, 2020
@miniksa
Copy link
Member

miniksa commented Jan 6, 2020

@j4james, we should file another issue to further refine the reasoning behind what is and is not passthrough. I've taken this issue to generate a PR to restore the behavior I broke so as to not block the next release. Thank you for the find and apologies for the grief.

@ghost ghost closed this as completed in #4125 Jan 8, 2020
ghost pushed a commit that referenced this issue Jan 8, 2020
@miniksa
Correct passthrough sequences after refactor (#4125)
913c5ec 
## Summary of the Pull Request
When refactoring the `StateMachine::ProcessString` algorithm to use safer structures, I made an off-by-one error when attempting to simplify the loop. 

## References
- Introduced in #3956 

## PR Checklist
* [x] Closes #4116
* [x] I work here.
* [x] Tests added/passed
* [x] No documentation
* [x] I'm a core contributor.

## Detailed Description of the Pull Request / Additional comments
The algorithm in use exploited holding onto some pointers and sizes as it rotated around the loop to call back as member variables in the pass-through function `FlushToTerminal`. 

As a part of the refactor, I adjusted to persisting a `std::wstring_view` of the currently processing string instead of pointer/size. I also attempted to simplify the loop at the same time as both the individual and group branches were performing some redundant operations in respect to updating the "run" length. 

Turns out, I made a mistake here. I wrote it so it worked correctly for the bottom half where we transition from bulk printing to an escape but then I messed up the top case.

## Validation Steps Performed
- [x] Manual validation of the exact command given in the bug report.
- [x] Wrote automated tests to validate both paths through the `ProcessString` loop that work with the `_run` variable.
@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 Jan 8, 2020
@miniksa miniksa mentioned this issue Jan 8, 2020
Closed
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@miniksa miniksa

Labels
Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Milestone
Terminal v0.8
Development

Successfully merging a pull request may close this issue.

Correct passthrough sequences after refactor microsoft/terminal
3 participants
@j4james @DHowett-MSFT @miniksa