-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Fix The conhost command line is not properly escaped #1815
Fix The conhost command line is not properly escaped #1815
Conversation
|
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? |
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. |
@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 In addition, the 'escape' code learned the https://github.com/golang/go/blob/master/src/syscall/exec_windows.go#L26-L95 |
@miniksa The PR has been updated, but the CI is not responding. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@DHowett-MSFT Thanks. |
There was a problem hiding this 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 Thanks. Windows Terminal makes me happy |
This commit re-escapes the path to conhost's subprocess before it launches it.
🎉 Handy links: |
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
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed