-
-
Notifications
You must be signed in to change notification settings - Fork 31.2k
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
Python Launcher for Windows (py.exe
) breaks on non-python shebang line
#94399
Comments
The launcher implements the virtual shebang "#!/usr/bin/env <name>" to search the directories in the Chances are that a developer has "%SystemRoot%\System32\bash.exe" from WSL. However, the launcher is distributed as a 32-bit application, in which case accessing "%SystemRoot%\System32" gets redirected to the 32-bit system directory "%SystemRoot%\SysWOW64", in which "bash.exe" likely doesn't exist. If the system happens to have bash from an MSYS2 or Cygwin installation in |
And if no Footnotes
|
There is no difference between a py.exe process that's executed manually versus a file association, so any behavior change for the For the new implementation of the launcher (3.11+), I think it would be okay to use the default Python when For the old launcher, it at least shouldn't try to pass the |
I glanced at the implementation; Lines 900 to 921 in 68fb032
Which is actually part of my argument that Even on a unixy platform where a shebang is respected for
It's documented as "the extension is added only if the specified file name does not end with an extension," so yeah, if the base name contains a
I just checked the code, and I think this line is the offending one that puts the full shebang line into the |
It's a bad idea to change the basic behavior of the old launcher, which has been in place for a long time. But it seems reasonable to clean it up to not call
The launcher wasn't implemented to take a private parameter when it's invoked for a file association, as opposed to explicitly invoked on the command line, so there's no way to reliably distinguish these cases for shebang handling. That said, if you specify a version, the shebang is ignored, e.g. run
|
Not to hurt my own case, but
This could be read as documenting that |
I don't think it was the intention to document support for anything but a "python" executable, with some supported extension from |
While I should have gathered this from when I skimmed the code: the path lookup behavior isn't limited to |
If I'm reading this right, I accidentally fixed everything in the new launcher and we don't need this issue anymore? (That's my favourite kind of fix.)
Yep 😄 Might be worth catching that specific case, but |
Yes. As the new (This is imho the correct behavior. Calling |
The new launcher doesn't implement the search semantics of "/usr/bin/env python". The behavior is documented as follows:
In the old launcher, the virtual "env" command is implemented to work like a |
Yeah, from a security POV I'm happier not letting the launcher launch any application just because someone double-clicked on a .py file (though of course that file may well launch it, and likely more reliably, but easier to detect). At the same time I don't want to exclude non-CPython runtimes from being detected, and only allowing (effectively) I think on balance, I'm coming down on the "run anything" path, and we recommend against installing or using the launcher in secure contexts. Which I normally recommend against anyway, just to remove a few potential hijacks, so the advice won't change. To be clear about the semantics:
|
The old launcher doesn't try |
@eryksun Would appreciate your review of my PR. Interestingly, SearchPath also checks the application directory like CreateProcess (which makes sense, though surprised me a little), which needed the env var so that tests wouldn't break in the build directory. Forcing safe search paths seems like an easy decision, but it would be nice to have all the options that LoadLibraryEx has... |
If
In the above, "C:\Program Files\Python311" is the application directory. A command that includes one or more path separators (e.g. "dir\command") is resolved against each directory in the search path, not just against the current working directory, and the application directory has precedence.
An application can also pass an explicit
The current directory must remain in a
|
Okay, so I could just load up PATH and pass it in directly to avoid the extra searches. I'll have to think about whether that's preferable to the default behaviour or not, since it would only affect cases that we don't construct ourselves. But it might be helpful if e.g. you copy py.exe into a venv Scripts directory to have it prefer the venv even if it's not activated. |
Decided to go with the PATH-only search, which means I had to revise the tests. Also discovered a few tweaks were needed to properly handle converting lines like |
…'/usr/bin/env' shebang lines (pythonGH-95582) (cherry picked from commit 67840ed) Co-authored-by: Steve Dower <[email protected]>
…bin/env' shebang lines (GH-95582)
I still personally think As a compromise: would it be reasonable to print a warning if launching an interpreter that is not |
…bin/env' shebang lines (GH-95582) (cherry picked from commit 67840ed) Co-authored-by: Steve Dower <[email protected]>
Also not If someone creates a |
For completeness sake, the reason rust-lang/rust's
It'd be nice to call out in the docs that a |
That would be a nice little tweak (edit as a new issue). Though it should be noted that it definitely assumes it's a Python interpreter, and will launch and pass arguments to it as if it is a Python interpreter. It just doesn't have any way to verify whether it's actually a Python runtime or not (at least until it fails when it doesn't know how to launch a .py file). |
Hm, could there be a simple way for Python interpreters to register with the py launcher on install? This wouldn't be a security feature but would act as a sanity check. |
Yes, it's PEP 514, and it already works. I can't see us extending venv to use it though, as those are usually meant to be transient, and will just be deleted by users (which won't clean up the registry). |
Sorry if this isn't the best place to post. It appears I ran into some behavior that was changed and is closely related to the discussion here.
I think there is mention of this exact use case in this issue but I don't quite understand it:
Should this behavior be restored? In either case, what are some alternative (perhaps better or more correct?) ways to achieve this? |
A new issue would be best, yeah. I'm surprised that I missed this, to be honest, it's something that I've definitely used before. I don't think there's any harm in adding it back though. |
A shebang with a fully-qualified executable should be supported by the launcher. You should be able to associate a script with a particular virtual environment. However, that's a separate issue from the discussion about supporting the "/usr/bin/env" virtual command. We need a new issue for this. |
Bug report
Given a non-python shebang line, e.g.
#!/usr/bin/env bash
,py.exe
falls over withpy.exe
should not be trying to interpret non-python shebang lines on Windows. Ifpy
has been invoked (especially if manually invoked rather than implicitly by ftype association), the intent is to interpret the file as a python script.I originally hit this with rust-lang/rust's x.py script, which is currently using
/usr/bin/env bash
to attempt to launch Python 3 across all OSes, whether they provide apython
executable or justpython3
. (MSYS makes this more fun, as it does directly interpret and use the shebang line, rather than calling theftype
registered handler.)Your environment
Microsoft Windows
Version 21H2 (OS Build 22000.739)
The text was updated successfully, but these errors were encountered: