Skip to content

Fix The conhost command line is not properly escaped#1815

Merged
DHowett-MSFT merged 4 commits intomicrosoft:masterfrom
fcharlie:conhost_commandline_escape
Jul 12, 2019
Merged

Fix The conhost command line is not properly escaped#1815
DHowett-MSFT merged 4 commits intomicrosoft:masterfrom
fcharlie:conhost_commandline_escape

Conversation

@fcharlie
Copy link
Contributor

@fcharlie fcharlie commented Jul 4, 2019 •
edited
Loading

Summary of the Pull Request

This PR is used to fix conhost when the process is started, and the command line is not properly escaped causing an error. For details, please see Issues: #1090

References

PR Checklist

  • Closes Bug Report: The conhost command line is not properly escaped #1090
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@fcharlie
Copy link
Contributor Author

fcharlie commented Jul 4, 2019

ConsoleArgumentsTests::ArgSplittingTests is bad..

@fcharlie fcharlie mentioned this pull request Jul 4, 2019
Closed
miniksa
miniksa requested changes Jul 5, 2019
View reviewed changes
@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 5, 2019
@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Jul 5, 2019

The core issue is that we are relying on the command line arguments coming in pre-mangled by the CRT’s argument parser.

It should be possible to, when we detect that we should switch to “client command line” parsing, just call GetCommandLineW and take the raw unmangled argument string back through CreateProcess.

Would that be a cleaner design that doesn’t require us to re-escape un-escaped strings?

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Jul 5, 2019

Oh, it looks like we aren’t relying on the CRT. Shouldn’t this be easier, then? We should just not mangle the arguments only to have to unmangle them later.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 5, 2019
@fcharlie
Copy link
Contributor Author

fcharlie commented Jul 5, 2019 •
edited
Loading

@DHowett-MSFT Using the original command line arguments is possible in this project (but the Windows SDK UCRT still needs to be escaped to fix spawnv ), but using the original command line to calculate the offset position of the parameters that conhost can handle will be cumbersome.

In addition, the 'escape' code learned the golang source. and is a mature solution.

https://github.com/golang/go/blob/master/src/syscall/exec_windows.go#L26-L95

@fcharlie
Copy link
Contributor Author

fcharlie commented Jul 10, 2019

@miniksa The PR has been updated, but the CI is not responding.

@DHowett-MSFT
Copy link
Contributor

DHowett-MSFT commented Jul 10, 2019

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Jul 10, 2019

Azure Pipelines successfully started running 1 pipeline(s).

@fcharlie
Copy link
Contributor Author

fcharlie commented Jul 10, 2019

@DHowett-MSFT Thanks.

@fcharlie fcharlie mentioned this pull request Jul 10, 2019
Closed
DHowett-MSFT
DHowett-MSFT approved these changes Jul 12, 2019
View reviewed changes
Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this!

@DHowett-MSFT DHowett-MSFT merged commit 02e8389 into microsoft:master Jul 12, 2019
@fcharlie
Copy link
Contributor Author

fcharlie commented Jul 12, 2019

@DHowett-MSFT Thanks. Windows Terminal makes me happy

@fcharlie fcharlie mentioned this pull request Jul 12, 2019
Closed
@fcharlie fcharlie deleted the conhost_commandline_escape branch July 12, 2019 04:01
mcpiroman pushed a commit to mcpiroman/terminal that referenced this pull request Jul 23, 2019
@fcharlie @mcpiroman
Fix the conhost command line not being properly escaped (microsoft#1815)
fcf4576 
This commit re-escapes the path to conhost's subprocess before it launches it.
@ghost
Copy link

ghost commented Aug 3, 2019

🎉Windows Terminal Preview v0.3.2142.0 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miniksa miniksa miniksa approved these changes

+1 more reviewer

@DHowett-MSFT DHowett-MSFT DHowett-MSFT approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Bug Report: The conhost command line is not properly escaped

3 participants

@fcharlie @DHowett-MSFT @miniksa