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.

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

Incorrect space sanitization for shell creation #4322

Closed
sqlrob opened this issue Jan 21, 2020 · 6 comments
Closed

Incorrect space sanitization for shell creation #4322

sqlrob opened this issue Jan 21, 2020 · 6 comments
Labels
Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.

Comments

@sqlrob
Copy link

sqlrob commented Jan 21, 2020

Environment

> [Environment]::OSVersion

Platform ServicePack Version      VersionString
-------- ----------- -------      -------------
 Win32NT             10.0.18363.0 Microsoft Windows NT 10.0.18363.0

Terminal version 0.8.10091.0

Steps to reproduce

  1. Create a file C:\Program, contents irrelevant; this applies to en-us, other languages will have different names.
  2. Open a powershell tab (other shells may have the issue, didn't check)

Expected behavior

Tab opens with a usable Powershell

Actual behavior

Window opens with the message

[error 0x800700c1 when launching `powershell.exe']
@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 21, 2020
@DHowett-MSFT
Copy link
Contributor

(For the shell,)
This is, and always will be, how CreateProcess with search path expansion works. Sorry.

  • We're relying on the system to do PATH resolution
  • If we implement our own heuristics here, we have to expand PATH and PATHEXT to properly populate the first arg to CreateProcess.
  • Even if we fix this, other applications will fail in the same way.
  • You need to be an admin to make a file there; there are far easier ways to break Terminal

(For the console host,)
This was fixed by #4172, which will be in the next servicing update... which may make this a /dup of #4061

@ghost
Copy link

ghost commented Jan 21, 2020

Hi! We've identified this issue as a duplicate of another one that already exists on this Issue Tracker. This specific instance is being closed in favor of tracking the concern over on the referenced thread. Thanks for your report!

@ghost ghost closed this as completed Jan 21, 2020
@ghost ghost added Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. and removed 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 21, 2020
@sqlrob
Copy link
Author

sqlrob commented Jan 21, 2020

Shells are in well defined locations. Why is PATH even being used?

@DHowett-MSFT
Copy link
Contributor

I'm not sure I agree that shells are in well-defined locations- A user's entitled to reorganize PATH such that their preferred instance of pwsh.exe comes first, or so that they can set version priorities for OpenSSH. Shells are wherever the user wants to put them!

@eryksun
Copy link

eryksun commented Jan 22, 2020

* If we implement our own heuristics here, we have to expand `PATH` _and_ `PATHEXT` to properly populate the first arg to `CreateProcess`.

CreateProcessW doesn't use PATHEXT. It calls SearchPathW with an optional ".EXE" extension.

@DHowett-MSFT
Copy link
Contributor

Fair enough.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing.
Projects
None yet
Development

No branches or pull requests

3 participants